git-push branch confusion caused by user mistake

git-push branch confusion caused by user mistake

[Phil Hord]

This week a user accidentally did this:

    $ git push origin origin/master
    Total 0 (delta 0), reused 0 (delta 0)
    To parent.git
     * [new branch]      origin/master -> origin/master

He saw his mistake when the "new branch" message appeared, but he was
confused about how to fix it and worried he broke something.

It seems reasonable that git expanded the original args into this one:

    git push origin refs/remotes/origin/master

However, since the dest ref was not provided, it was assumed to be the
same as the source ref, so it worked as if he typed this:

    git push origin refs/remotes/origin/master:refs/remotes/origin/master

Indeed, git ls-remote origin shows the result:

    $ git ls-remote origin
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e HEAD
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/remotes/origin/master

Also, I verified that this (otherwise valid) command has similar
unexpected results:
    $ git remote add other foo.git && git fetch other && git push
origin other/topic
    $ git ls-remote origin
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e HEAD
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/remotes/origin/master
    d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/remotes/other/topic

I think git should be smarter about deducing the dest ref from the
source ref if the source ref is in refs/remotes, but I'm not sure how
far to take it.  It feels like we should translate refspecs something
like this for push:

    origin/master
        => refs/remotes/origin/master:refs/heads/master

    refs/remotes/origin/master
         => refs/remotes/origin/master:refs/heads/master

    origin/master:origin/master
         => refs/remotes/origin/master:refs/heads/origin/master

    master:refs/remotes/origin/master
         => refs/heads/master:refs/remotes/origin/master

That is, we should not infer a remote refspec of "refs/remotes/*"; we
should only get there if "refs/remotes" was given explicitly by the
user.

Does this seem reasonable?  I can try to work up a patch if so.

Re: git-push branch confusion caused by user mistake

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> I think git should be smarter about deducing the dest ref from the
> source ref if the source ref is in refs/remotes, but I'm not sure how
> far to take it.

My knee-jerk reaction is "Don't take it anywhere".  

Giving a refspec from the command line is an established way to
defeat the default behaviour when you do not give any and only the
remote, and making it do things behind user's back, you would be
robbing the escape hatch from people.

It often is useful in real-life workflow when "git push $dest
origin/master" does exactly the way it works now, which I actually
use myself.  Imagine that you have two repositories, use one of them
primarily to interact with the outside world and do your work, but
you then occasionally push from that primary repository to the other
one, instead of logging into the host that has the other one and
running a fetch on that host from the outside world.  Your "trying
to be clever when given a colon-less refspec" will force people to
type "git push $dest origin/master:origin/master" in such a case.

Re: git-push branch confusion caused by user mistake

[Jakub Narębski]

W dniu 10.03.2017 o 22:44, Phil Hord pisze:
> This week a user accidentally did this:
> 
>     $ git push origin origin/master
>     Total 0 (delta 0), reused 0 (delta 0)
>     To parent.git
>      * [new branch]      origin/master -> origin/master
> 
> He saw his mistake when the "new branch" message appeared, but he was
> confused about how to fix it and worried he broke something.

It is nowadays very easy to delete accidentally created remote branch
with

      $ git push origin --delete origin/master
 
> It seems reasonable that git expanded the original args into this one:
> 
>     git push origin refs/remotes/origin/master
> 
> However, since the dest ref was not provided, it was assumed to be the
> same as the source ref, so it worked as if he typed this:
> 
>     git push origin refs/remotes/origin/master:refs/remotes/origin/master

This rule depends on push.default setting, but it is a very simple
rule.  Simple is good.  DWIM is usually not worth it, unless program
can guess what you meant, and what you meant is always the same.

> I think git should be smarter about deducing the dest ref from the
> source ref if the source ref is in refs/remotes, but I'm not sure how
> far to take it.  It feels like we should translate refspecs something
> like this for push:
> 
>     origin/master
>         => refs/remotes/origin/master:refs/heads/master
[...]

Such push doesn't make sense (unless you have a quite unusual situation).

Note that 'origin/master', that is 'refs/remotes/origin/master' is a
remote-tracking branch, that is a ref that is meant to track position
of the 'master' branch ('refs/heads/master') in the 'origin' remote.
Thus it should always be the same as 'master' in 'origin', or be behind
if you didn't fetch.

> Does this seem reasonable?  I can try to work up a patch if so.

Thus I don't think such complication is reasonable.

-- 
Jakub Narębski
 

Re: git-push branch confusion caused by user mistake

[Jacob Keller]

On Fri, Mar 10, 2017 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> I think git should be smarter about deducing the dest ref from the
>> source ref if the source ref is in refs/remotes, but I'm not sure how
>> far to take it.
>
> My knee-jerk reaction is "Don't take it anywhere".
>
> Giving a refspec from the command line is an established way to
> defeat the default behaviour when you do not give any and only the
> remote, and making it do things behind user's back, you would be
> robbing the escape hatch from people.
>
> It often is useful in real-life workflow when "git push $dest
> origin/master" does exactly the way it works now, which I actually
> use myself.  Imagine that you have two repositories, use one of them
> primarily to interact with the outside world and do your work, but
> you then occasionally push from that primary repository to the other
> one, instead of logging into the host that has the other one and
> running a fetch on that host from the outside world.  Your "trying
> to be clever when given a colon-less refspec" will force people to
> type "git push $dest origin/master:origin/master" in such a case.
>
>

It might be worth having some warning or something happen here? I've
had several  co-workers at $DAYJOB get confused by this sort of thing.

Thanks,
Jake

Re: git-push branch confusion caused by user mistake

[Phil Hord]

On Mon, Mar 13, 2017 at 1:55 AM Jacob Keller <jacob.keller@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Phil Hord <phil.hord@gmail.com> writes:
> >> I think git should be smarter about deducing the dest ref from the
> >> source ref if the source ref is in refs/remotes, but I'm not sure how
> >> far to take it.
> >
> > My knee-jerk reaction is "Don't take it anywhere".
> >
> > Giving a refspec from the command line is an established way to
> > defeat the default behaviour when you do not give any and only the
> > remote, and making it do things behind user's back, you would be
> > robbing the escape hatch from people.
>
> It might be worth having some warning or something happen here? I've
> had several  co-workers at $DAYJOB get confused by this sort of thing.

On one very active project at $work, we have 380,000 commits, 4600
branches in refs/heads and 96 branches in refs/remotes.  About half of
the refs/remotes (43) are obviously user errors.  The other half it's
not possible for me to know.

I suggested to our admins to block attempts to push to
'refs/remotes/*' so in the future users don't lose track of commits
they think they pushed.  But I don't know if that will really happen.

Thanks for the counterexample feedback.