Proposal: streamlined git push behavior

[Shawn Pearce]

Lets call the current `git push origin HEAD:refs/for/master` ... awkward.

Here is an alternative for discussion.

  $ git checkout -b r/master/frob origin/master

  ...

  $ git commit -a -m "Frob the frobinator"

  $ git push origin HEAD

  ...

  remote: New Changes:

  remote:   https://gerrit/42 ...

What? How does that work?  :)

Recall that `git push origin HEAD` is shorthand to push the current branch, which in our case was r/master/frob. So the last command is actually:

  git push origin r/master/frob

Which git push assumes is:

  git push origin \

    refs/heads/r/master/frob:refs/heads/r/master/frob

Ok... assuming r/master/frob does not exist as a branch at the server, Git will try to create this branch. Attempting to create is part of the magic of why refs/for/master works in Gerrit; the refs/for/ namespace always appears empty to clients so clients can always perform a fast-forward update.

The proposal is refs/heads/r/ behaves like refs/for/.

We keep refs/for/ for backwards compatibility.

A drawback is branch names cannot start with this r/ prefix.

I also considered other prefixes like review/ and even @, e.g.:

  git checkout -b @master/frob origin/master

  ...

  git push origin @master/frob

And I can't decide. :)

I'm getting motivated to look at this because of push options coming soon. With Git 2.10 we can start to write things like:

  git push origin -o r=dbor...@google.com -o label=Verified+1 r/master/frob

[Brad Larson]

As a mere user, I like both r/master and @master.  I don't think either one is significantly better than the other.  Sorry this isn't helpful :)

I can't tell for certain - does this method require naming your branch the same as your push destination?  That is a bit of a bummer, a lot of docs have examples with more descriptive names (bug183, newFeature, etc).  Maybe not a big deal though.

The new push options do sound cool.

[Shawn Pearce]

Instead of:

  git checkout -b bug183

You would do:

  git checkout -b @master/bug183

So encode the destination into the prefix for your local branch name.

[Matthias Sohn]

I don't yet see how your proposal is related to push options ?

How could I push for review to a branch foo/bar ? Like this ?

git checkout -b @foo/bar/frob origin/foo/bar

...

git push origin @foo/bar

-Matthias

[Martin Fick]

On Monday, August 29, 2016 11:33:32 AM 'Shawn Pearce' via

Repo and Gerrit Discussion wrote:
> > I can't tell for certain - does this method require
> > naming your branch the same as your push destination?
> > That is a bit of a bummer, a lot of docs have examples
> > with more descriptive names (bug183, newFeature, etc).
> > Maybe not a big deal though.
>
> Instead of:
>
> git checkout -b bug183
>
> You would do:
>
> git checkout -b @master/bug183
>
> So encode the destination into the prefix for your local
> branch name.

I am probably not the person to optimize a workflow for, but
here is my perspective:

Personally, I would not find this useful. I work on my local
branches much more than I do on my remote branches, and I
would not want the extra burden of encoding the remote
branch name in my local branches.

When developing something new, I often create many copies of
a local branch, many named silly things like feature,
feature.1 feature.2 feature.3 feature3.rebased ... (all for
the same feature as it evolves). I am effectively
manipulating many copies of the same local branch and
keeping backups of my old designs. In my case, this would
mean having to either name all those backup copies above
with the destination @master/feature @master/feature1
@master/feature2 @master/feature3 @master/feature.rebased
... or rename the final "ready" branch from feature to
@master/feature right before pushing. I see little gain in
that.

Generally speaking, pushing for review is a command I run
rarely compared to the amount of times I manipulate local
branch names. Local branch operations are thus what I want
my workflow optimized for. If I have to think a little
before pushing for review (to add refs/for/master) first,
that is good. It will force me to pause before pushing and
making sure I am really ready to push, and that I am pushing
to the right destination (internal vs external...),


-Martin


--
The Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, hosted by The Linux Foundation

[Shawn Pearce]

That is really useful feedback, thanks Martin.

I was thinking more about the average user coming from GitHub. They
struggle to contribute to a project that uses Gerrit Code Review
because we didn't ask them to:

1) Fork a repository on GitHub.
2) Clone from their fork.
3) Push a random branch back to their fork using `git push origin
my-messy-feature6`
4) Visit https://github.com/...something... and Create Pull Request

I was looking at this as can we get the Gerrit workflow to be:

1) Clone from the project's repository.
2) Push using `git push origin r/master/my-feature`

Which seems a few steps shorter and maybe still explainable to people
who are used to the GH PR workflow above.

[Kenny Ho]

The struggle is real.  I have users who are new to git asking why it can't be more like github and also want the plain "git push" to work.

What if, instead of returning a generic "prohibited by gerrit" when there's no permission to push to refs/heads/*, the push get redirected to the corresponding refs/for/* automatically (potentially with addition notices on the redirect)?

[Phil Hord]

I really like the idea of a shortcut for 'refs/for/' which is named '@'.  It would be nice to say

    git push origin HEAD:@master/frob

instead of

    git push origin HEAD:refs/for/master/frob

It doesn't impose any different workflow on Martin if he wants to keep spelling out "refs/for/" and keep all his local branch names, either. 

Moreover Gerrit could be smart and reject any push to @nonexistent-branch.  Early on I had a clumsy admin freak out that his changes were "lost" because he had pushed to "rfes/for/master".  Git turned that into refs/heads/rfes/for/master, and  Gerrit accepted it since he had create-branch permission. It took me a week to figure out what happened there.

But it would be harder for the admin to misspell @, though if he left the @ off, of course, he would push directly to master. 

Some lessons are only learned the hard way.  Most, in fact.

[Martin Fick]

On Tuesday, August 30, 2016 01:45:04 PM Kenny Ho wrote:
>
> What if, instead of returning a generic "prohibited by
> gerrit" when there's no permission to push to
> refs/heads/*, the push get redirected to the
> corresponding refs/for/* automatically (potentially with
> addition notices on the redirect)?

I thought of this too. The downside is that you would be
training users to do something that could eventually be
really bad. i.e. the day they are given permissions to push
directly to refs/heads they will suddenly no longer be
creating changes, but rather pushing directly to their
destination branch. It generally is not a good idea to have
the specific permissions of the user affect what their push
does,

[Shawn Pearce]

On Tue, Aug 30, 2016 at 3:16 PM, Martin Fick <mf...@codeaurora.org> wrote:
>
> On Tuesday, August 30, 2016 01:45:04 PM Kenny Ho wrote:
> >
> > What if, instead of returning a generic "prohibited by
> > gerrit" when there's no permission to push to
> > refs/heads/*, the push get redirected to the
> > corresponding refs/for/* automatically (potentially with
> > addition notices on the redirect)?
>
> I thought of this too. The downside is that you would be
> training users to do something that could eventually be
> really bad. i.e. the day they are given permissions to push
> directly to refs/heads they will suddenly no longer be
> creating changes, but rather pushing directly to their
> destination branch. It generally is not a good idea to have
> the specific permissions of the user affect what their push
> does,

Yes, that is an excellent reason not to accept new code reviews at refs/heads/*.

Another reason is the branch (e.g. master) may be moving faster than
the developer codes:

D> git clone URL
S> submit change updating master
D> git push origin master

Because of the submit git push will error out telling the user its a
non-fast-forward update. So the user has to fetch rebase and push
again. But if they aren't touching the same files, or if the project's
submit strategy is Rebase If Necessary, there is no reason for the
user to go through a fetch+rebase+push dance just to upload a code
review.

A "magic prefix" that doesn't exist lets us get around both of these
issues. Especially if we can refuse to create branches that use that
magic prefix.

[Matthias Sohn]

On Wed, Aug 31, 2016 at 12:16 AM, Martin Fick <mf...@codeaurora.org> wrote:

On Tuesday, August 30, 2016 01:45:04 PM Kenny Ho wrote:
>
> What if, instead of returning a generic "prohibited by
> gerrit" when there's no permission to push to
> refs/heads/*, the push get redirected to the
> corresponding refs/for/* automatically (potentially with
> addition notices on the redirect)?

I thought of this too.  The downside is that you would be
training users to do something that could eventually be
really bad.  i.e. the day they are given permissions to push
directly to refs/heads they will suddenly no longer be
creating changes, but rather pushing directly to their
destination branch.  It generally is not a good idea to have
the specific permissions of the user affect what their push
does,

+1, push behaviour shouldn't change just because permission settings changed

-Matthias

[Saša Živkov]

IMHO, there is some value in "connecting" a newly created local branch to its destination remote branch:

it makes it harder to accidentally push to a wrong remote branch, like for example: checkout form origin/master and then push to stable. 

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Dave Borowitz]

I'm not sure this actually solves the problem. GitHub users are used to typing "git push origin <whatever they want>". With Gerrit today, they have to type "git push origin <something specific>". Your proposal is just replacing <specific thing A> with <specific thing B>. If they are confused by A, I'm not convinced they won't be confused by B.

Maybe the easiest thing to explain is if just "git push" results in a code review. You get this behavior today if you do "git config remote.origin.push HEAD:refs/for/master". Can we work a default push refspec into the protocol somehow, so this happens automatically on clone? Or maybe just include that git config command in the example command to set up your commit-msg hook?

Another possibility (which admittedly sounds like <specific thing C>): what about just "git push -o review", which implies any refs/heads/* commands are actually refs/for/*.

(Elsewhere on the thread someone suggested better error messages, possibly with a cut-and-pasteable command. We should definitely do that, independent of any new features.)

 

Which seems a few steps shorter and maybe still explainable to people
who are used to the GH PR workflow above.

[Dave Borowitz]

Git already stores upstream information if you do "git branch foo origin/foo". My git-push-review script (and I think git-review also) uses this to find the right branch to push to. This also raises the potential issue of conflicting "upstream" information between what "git branch" thinks and what you encode in the refname.

[Saša Živkov]

Therefore I wrote just "connecting" without intention to imply that encoding remote branch

in the refname is the right way to do the "connecting".

[Sven Selberg]

On Wednesday, August 31, 2016 at 5:14:01 PM UTC+2, Dave Borowitz wrote:

With Gerrit today, they have to type "git push origin <something specific>". Your proposal is just replacing <specific thing A> with <specific thing B>. If they are confused by A, I'm not convinced they won't be confused by B.

That was my immediate reaction also.

I believe we will always end up with <specific thing X>. Is it not better to suggest a simple alias for the user and let them keep track of which remote branch to push to?

~/.gitconfig

----------------------8X---------------------

[alias]

        gr = !sh -c 'git push origin HEAD:refs/for/$1' -

----------------------X8---------------------

Alternatively suggest an alias that would handle the magic on the client side.

/Sven

[Stefan Beller]

On Tue, Aug 30, 2016 at 1:45 PM, Kenny Ho <y2k...@gmail.com> wrote:
> The struggle is real. I have users who are new to git asking why it can't
> be more like github and also want the plain "git push" to work.
>
> What if, instead of returning a generic "prohibited by gerrit" when there's
> no permission to push to refs/heads/*, the push get redirected to the
> corresponding refs/for/* automatically (potentially with addition notices on
> the redirect)?
>

I wonder if we should introduce a review flag, e.g.

git push origin -o as-review refs/heads/master:refs/heads/master

The ref spec is spelled out to point out there is no magic involved, e.g.

git push origin -o as-review HEAD

would also just work.

We can also introduce a new flag that disallows direct pushing
if not given the direct option:

git push -o I-want-to-push-to-a-branch HEAD:refs/heads/master

works, but just pushing HEAD:refs/heads/master doesn't.

[Saša Živkov]

On Fri, Sep 2, 2016 at 6:08 PM, 'Stefan Beller' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

On Tue, Aug 30, 2016 at 1:45 PM, Kenny Ho <y2k...@gmail.com> wrote:
> The struggle is real.  I have users who are new to git asking why it can't
> be more like github and also want the plain "git push" to work.
>
> What if, instead of returning a generic "prohibited by gerrit" when there's
> no permission to push to refs/heads/*, the push get redirected to the
> corresponding refs/for/* automatically (potentially with addition notices on
> the redirect)?
>

I wonder if we should introduce a review flag, e.g.

    git push origin -o as-review refs/heads/master:refs/heads/master

The ref spec is spelled out to point out there is no magic involved, e.g.

    git push origin -o as-review HEAD

would also just work.

Would it work if my current local branch is not master but something else like fixing-bug-123

and I want to push from fixing-bug-123 for review for master?

 

We can also introduce a new flag that disallows direct pushing
if not given the direct option:

    git push -o I-want-to-push-to-a-branch HEAD:refs/heads/master

works, but just pushing HEAD:refs/heads/master doesn't.

[Stefan Beller]

On Fri, Sep 2, 2016 at 9:19 AM, Saša Živkov <ziv...@gmail.com> wrote:
>
>
> On Fri, Sep 2, 2016 at 6:08 PM, 'Stefan Beller' via Repo and Gerrit
> Discussion <repo-d...@googlegroups.com> wrote:
>>
>> On Tue, Aug 30, 2016 at 1:45 PM, Kenny Ho <y2k...@gmail.com> wrote:
>> > The struggle is real. I have users who are new to git asking why it
>> > can't
>> > be more like github and also want the plain "git push" to work.
>> >
>> > What if, instead of returning a generic "prohibited by gerrit" when
>> > there's
>> > no permission to push to refs/heads/*, the push get redirected to the
>> > corresponding refs/for/* automatically (potentially with addition
>> > notices on
>> > the redirect)?
>> >
>>
>> I wonder if we should introduce a review flag, e.g.
>>
>> git push origin -o as-review refs/heads/master:refs/heads/master
>>
>> The ref spec is spelled out to point out there is no magic involved, e.g.
>>
>> git push origin -o as-review HEAD
>>
>> would also just work.
>
>
> Would it work if my current local branch is not master but something else
> like fixing-bug-123
> and I want to push from fixing-bug-123 for review for master?
>

Sure, if we extend that proposal. ;)

By default Gerrit will not accept any pushes to refs/heads/*, but
instead you
need to specify what should happen with each pushed ref:

# for ease of use:
git config remote.origin.push HEAD

git push -o review-for=master origin

This would push whatever you have at HEAD to a review for master,
as you explicitly told Gerrit this is for master. This would be interesting
when pushing many changes though:

# get back to 'matching' branches behavior:
git config --unset remote.origin.push

git push -o matching-reviews origin :

So if you have two matching branches "master" and "stable" you create a
codereview for each. What about fixing-bug-123? Gerrit could error out
as that branch doesn't exist on the server, but more options to the rescue:

git push -o matching-reviews -o review-for-if-no-match=master origin :

By transferring the complexity to the push options (which are documented to
be server/backend specific), a user should have no expectations on how things
work, but rather want to read up on that. And when the options are feature
rich enough, the whole refspec doesn't matter any more.