[PATCH] branch: make --set-upstream saner without an explicit starting point

[PATCH] branch: make --set-upstream saner without an explicit starting point

[Carlos Martín Nieto]

The branch command assumes HEAD as the starting point if none is
specified. This causes --set-upstream to behave unexpectedly if the
user types

    git branch --set-upstream origin/master

git-branch will assume a second argument of HEAD and create config
entries for a local branch origin/master to track the current
branch. This is rarely, if ever, what the user wants to do.

Catch invocations with --set-upstream and only one branch so the
command above sets up the current branch to track origin's master
branch.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---

I got tired of --set-upstream biting me in the arse so I (presumably)
fixed it. I've only run the t3200 test for now. I'll check the rest of
the suite when I'm in front of a computer that's got some power, but I
don't expect other tests to be affected.

 builtin/branch.c  | 16 ++++++++++++++--
 t/t3200-branch.sh | 23 +++++++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..6bbabda 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -853,10 +853,22 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			usage_with_options(builtin_branch_usage, options);
 	} else if (argc > 0 && argc <= 2) {
+		const char *branch, *upstream;
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
-		create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
-			      force_create, reflog, 0, quiet, track);
+
+		/* The usual way, make the branch point be HEAD of none is specified */
+		branch = argv[0];
+		upstream = (argc == 2) ? argv[1] : head;
+
+		/* If the command was 'git branch --set-upstream origin/master',
+		   make HEAD track origin/master, not the other way around */
+		if (track == BRANCH_TRACK_OVERRIDE && argc == 1) {
+			branch = head;
+			upstream = argv[0];
+		}
+
+		create_branch(head, branch, upstream, force_create, reflog, 0, quiet, track);
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a17f8b2..e06d642 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -369,6 +369,29 @@ test_expect_success \
     'git tag foobar &&
      test_must_fail git branch --track my11 foobar'
 
+test_expect_success 'set upstream with both branches explicit' \
+    'git config remote.local.url . &&
+     git config remote.local.fetch refs/heads/master:refs/remotes/local/master &&
+     (git show-ref -q refs/remotes/local/master || git fetch local) &&
+     git branch --no-track my12 &&
+     git branch --set-upstream my12 local/master &&
+     test $(git config branch.my12.remote) = local &&
+     test $(git config branch.my12.merge) = refs/heads/master'
+
+# The unsets at the end is to leave the master config as we found it,
+# so later tests don't get confused
+
+test_expect_success 'set upstream with implicit HEAD as branch to modify' \
+    'git config remote.local.url . &&
+     git config remote.local.fetch refs/heads/master:refs/remotes/local/master &&
+     (git show-ref -q refs/remotes/local/master || git fetch local) &&
+     git branch --set-upstream local/master &&
+     test $(git config branch.master.remote) = local &&
+     test $(git config branch.master.merge) = refs/heads/master
+     git config --unset branch.master.remote &&
+     git config --unset branch.master.merge
+'
+
 # Keep this test last, as it changes the current branch
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
-- 
1.7.11.1.104.ge7b44f1

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Jeff King]

On Thu, Jul 05, 2012 at 11:29:49AM +0200, Carlos Martín Nieto wrote:

> The branch command assumes HEAD as the starting point if none is
> specified. This causes --set-upstream to behave unexpectedly if the
> user types
> 
>     git branch --set-upstream origin/master
> 
> git-branch will assume a second argument of HEAD and create config
> entries for a local branch origin/master to track the current
> branch. This is rarely, if ever, what the user wants to do.
> 
> Catch invocations with --set-upstream and only one branch so the
> command above sets up the current branch to track origin's master
> branch.

I have been tempted to write this patch several times but was afraid
that somebody was relying on the existing behavior. I think the behavior
you propose is much saner.

> +# The unsets at the end is to leave the master config as we found it,
> +# so later tests don't get confused
> +
> +test_expect_success 'set upstream with implicit HEAD as branch to modify' \
> +    'git config remote.local.url . &&
> +     git config remote.local.fetch refs/heads/master:refs/remotes/local/master &&
> +     (git show-ref -q refs/remotes/local/master || git fetch local) &&
> +     git branch --set-upstream local/master &&
> +     test $(git config branch.master.remote) = local &&
> +     test $(git config branch.master.merge) = refs/heads/master
> +     git config --unset branch.master.remote &&
> +     git config --unset branch.master.merge
> +'

The unsets will not run if the test fails. Use test_when_finished to
insert cleanup, or better yet use test_config which handles this case
automagically (you are not setting them initially, but perhaps you
should set them to some known value initially to make sure that your
command changes them as expected).

I don't understand the point of the show-ref call, though. Isn't the
fetch idempotent, and you can just run it always?

-Peff

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Carlos Martín Nieto]

On Thu, 2012-07-05 at 05:42 -0400, Jeff King wrote:
> On Thu, Jul 05, 2012 at 11:29:49AM +0200, Carlos Martín Nieto wrote:
> 
> > The branch command assumes HEAD as the starting point if none is
> > specified. This causes --set-upstream to behave unexpectedly if the
> > user types
> > 
> >     git branch --set-upstream origin/master
> > 
> > git-branch will assume a second argument of HEAD and create config
> > entries for a local branch origin/master to track the current
> > branch. This is rarely, if ever, what the user wants to do.
> > 
> > Catch invocations with --set-upstream and only one branch so the
> > command above sets up the current branch to track origin's master
> > branch.
> 
> I have been tempted to write this patch several times but was afraid
> that somebody was relying on the existing behavior. I think the behavior
> you propose is much saner.

Those two people who rely on the current behaviour will just have to
make a sacrifice for the good of the rest of the user community. I guess
we could introduce it in steps by first warning, but I doubt it would be
worth the effort.

> 
> > +# The unsets at the end is to leave the master config as we found it,
> > +# so later tests don't get confused
> > +
> > +test_expect_success 'set upstream with implicit HEAD as branch to modify' \
> > +    'git config remote.local.url . &&
> > +     git config remote.local.fetch refs/heads/master:refs/remotes/local/master &&
> > +     (git show-ref -q refs/remotes/local/master || git fetch local) &&
> > +     git branch --set-upstream local/master &&
> > +     test $(git config branch.master.remote) = local &&
> > +     test $(git config branch.master.merge) = refs/heads/master
> > +     git config --unset branch.master.remote &&
> > +     git config --unset branch.master.merge
> > +'
> 
> The unsets will not run if the test fails. Use test_when_finished to
> insert cleanup, or better yet use test_config which handles this case
> automagically (you are not setting them initially, but perhaps you
> should set them to some known value initially to make sure that your
> command changes them as expected).

Considering that the unset is there only because a later test does 'git
fetch' instead of specifying which remote we should fetch from, and this
setting confuses it (expecting to fetch from origin, but instead
fetching from local), I wonder if it wouldn't be better to simply make
the fetch explicit in line 712 so it reads 'git fetch origin'. This way
we can forget about undoing the configuration, because we're overriding
it anyway.
> 
> I don't understand the point of the show-ref call, though. Isn't the
> fetch idempotent, and you can just run it always?

That is a good point. I just copied what the --track tests are doing a
few tests up. Looking at more tests, it seems to be what most do. Maybe
something like this:

---8<---

Subject: branch: make --set-upstream saner without an explicit
 starting point

The branch command assumes HEAD as the starting point if none is
specified. This causes --set-upstream to behave unexpectedly if the
user types

    git branch --set-upstream origin/master

git-branch will assume a second argument of HEAD and create config
entries for a local branch origin/master to track the current
branch. This is rarely, if ever, what the user wants to do.

Catch invocations with --set-upstream and only one branch so the
command above sets up the current branch to track origin's master
branch.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/branch.c  | 16 ++++++++++++++--
 t/t3200-branch.sh | 20 +++++++++++++++++++-
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..6bbabda 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -853,10 +853,22 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		else
 			usage_with_options(builtin_branch_usage, options);
 	} else if (argc > 0 && argc <= 2) {
+		const char *branch, *upstream;
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
-		create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
-			      force_create, reflog, 0, quiet, track);
+
+		/* The usual way, make the branch point be HEAD of none is specified */
+		branch = argv[0];
+		upstream = (argc == 2) ? argv[1] : head;
+
+		/* If the command was 'git branch --set-upstream origin/master',
+		   make HEAD track origin/master, not the other way around */
+		if (track == BRANCH_TRACK_OVERRIDE && argc == 1) {
+			branch = head;
+			upstream = argv[0];
+		}
+
+		create_branch(head, branch, upstream, force_create, reflog, 0, quiet, track);
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a17f8b2..1b0a73c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -369,6 +369,24 @@ test_expect_success \
     'git tag foobar &&
      test_must_fail git branch --track my11 foobar'
 
+test_expect_success 'set upstream with both branches explicit' \
+    'git config remote.local.url . &&
+     git config remote.local.fetch refs/heads/master:refs/remotes/local/master &&
+     git fetch local &&
+     git branch --no-track my12 &&
+     git branch --set-upstream my12 local/master &&
+     test $(git config branch.my12.remote) = local &&
+     test $(git config branch.my12.merge) = refs/heads/master'
+
+test_expect_success 'set upstream with implicit HEAD as branch to modify' \
+    'git config remote.local.url . &&
+     git config remote.local.fetch refs/heads/master:refs/remotes/local/master &&
+     git fetch local &&
+     git branch --set-upstream local/master &&
+     test $(git config branch.master.remote) = local &&
+     test $(git config branch.master.merge) = refs/heads/master
+'
+
 # Keep this test last, as it changes the current branch
 cat >expect <<EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	branch: Created from master
@@ -686,7 +704,7 @@ test_expect_success 'use set-upstream on the current branch' '
 	git --bare init myupstream.git &&
 	git push myupstream.git master:refs/heads/frotz &&
 	git remote add origin myupstream.git &&
-	git fetch &&
+	git fetch origin &&
 	git branch --set-upstream master origin/frotz &&
 
 	test "z$(git config branch.master.remote)" = "zorigin" &&
-- 
1.7.11.1.104.ge7b44f1

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> The branch command assumes HEAD as the starting point if none is
> specified. This causes --set-upstream to behave unexpectedly if the
> user types
>
>     git branch --set-upstream origin/master
>
> git-branch will assume a second argument of HEAD and create config
> entries for a local branch origin/master to track the current
> branch. This is rarely, if ever, what the user wants to do.
>
> Catch invocations with --set-upstream and only one branch so the
> command above sets up the current branch to track origin's master
> branch.

If you look at the set of management operations "git branch"
(i.e. other than "listing" [*1*]) allows you to do, the first name
on the command line always is the branch that is manipulated for
everything other than the "set upstream" operation.  In that sense,
the current implementation consistently handles command line
arguments with other options, and your patch breaks the consistency
in the UI.

I think it was a mistake that nobody noticed that it is likely that
the operation most often will be done for the current branch and the
usual "give me one branch name to operate on, or I'll operate on the
current branch" command line convention of "git branch" commannd is
not a good fit for it, when "set upstream" feature was added, and
suggested an alternative syntax that avoids the mistake you quoted
above, perhaps something like:

	git branch --set-upstream-to=origin/master [HEAD]

which would have been very clear whose upstream is set to what (with
or without the name of the other branch).  In other words, make the
name "origin/master" *NOT* the first branch name on the command line
in the usual sense, but a parameter to the --set-upstream option, so
that "give me one branch name to operate on, or I'll operate on the
current branch" convention is still kept.

You also broke people who corrected another kind of mistake in this
workflow:

    git checkout frotz
    hack hack
    # ok, shared infrastructure between two branches are
    # sound, and I can build the other topic on top of this
    # state
    git branch nitfol
    # oops, forgot to mark that nitfol is derived on frotz with --track
    git branch --set-upstream nitfol

where the last one meant "git branch --set-upstream nitfol frotz",
to retroactively mark the upstream of the named branch, no?

Even though my instinct tends to agree with your "is rarely, if
ever", I do not think it is sane to change the behaviour of a
command that produced one result without failing to produce
something entirely different like your patch does (it would have
been a different story if an operation that everybody got failure
and did not produce a useful result were updated to produce a useful
result).

Coming from the above observation, while I am sympathetic to your
cause and agree that we would want to do something about it, I am
having a hard time to convince myself that your patch is the best
way to go.

I am not entirely happy with the hypothetical "set-upstream-to"
myself, either.


[Footnote]

*1* The point of "listing" is you do not know the names and asking
the command to produce them, so it is OK to be different.  The "set
upstream" operation in question does not share the excuse to be
different.

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think it was a mistake that nobody noticed that it is likely that
> the operation most often will be done for the current branch and the
> usual "give me one branch name to operate on, or I'll operate on the
> current branch" command line convention of "git branch" commannd is
> not a good fit for it, when "set upstream" feature was added, and
> suggested an alternative syntax that avoids the mistake you quoted
> above, perhaps something like:
>
> 	git branch --set-upstream-to=origin/master [HEAD]
>
> which would have been very clear whose upstream is set to what (with
> or without the name of the other branch).  In other words, make the
> name "origin/master" *NOT* the first branch name on the command line
> in the usual sense, but a parameter to the --set-upstream option, so
> that "give me one branch name to operate on, or I'll operate on the
> current branch" convention is still kept.
> 
> You also broke people who corrected another kind of mistake in this
> workflow:
> ...
> Coming from the above observation, while I am sympathetic to your
> cause and agree that we would want to do something about it, I am
> having a hard time to convince myself that your patch is the best
> way to go.
>
> I am not entirely happy with the hypothetical "set-upstream-to"
> myself, either.

Thinking about it a bit more, I am starting to think that something
based on the "set upstream to" could be a sane way forward:

 * add "git branch [--set-upstream-to=<name>]" that does what your
   patch does.  The synopsis must make it clear that <name> is not
   the usual first <name> like other "branch" command line arguments
   that specify the branch being operated on, but is an argument to
   the --set-upstream option [*1*].

 * when "git branch --set-upstream <name>" without <start point>
   is given, you first see if <name> exists and find out the
   upstream of <name>, do what the user told you to do (i.e. reset
   the upstream of the <name>d branch to the current branch), and
   give hints to recover.  Two possibilities:

     $ git checkout frotz
     $ git branch --set-upstream xyzzy
     Branch xyzzy set up to track local branch frotz.
     If you wanted to make frotz track xyzzy, do this:
       $ git branch --set-upstream xyzzy <original>
       $ git branch --set-upstream-to xyzzy

     $ git checkout frotz
     $ git branch --set-upstream origin/xyzzy
     Branch origin/xyzzy set up to track local branch frotz.
     If you wanted to make frotz track xyzzy, do this:
       $ git branch -d origin/xyzzy
       $ git branch --set-upstream-to origin/xyzzy

 * possibly, deprecate --set-upstream as a historical wart that had
   misdesigned UI, and when it is used, give deprecation warning and
   nudge the user to use --set-upstream-to instead.


[Footnote]

*1* The parseopt parser will allow both of:

    $ git branch --set-upstream-to=origin/master
    $ git branch --set-upstream-to origin/master

    but the braket around the option name "--set-upstream-to" and
    its argument <name> should make it clear, i.e.

	git branch [--set-upstream-to <name>] [<branch>]

    or

    	git branch [--set-upstream-to=<name>] [<branch>]

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Carlos Martín Nieto]

On Thu, 2012-07-05 at 10:44 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think it was a mistake that nobody noticed that it is likely that
> > the operation most often will be done for the current branch and the
> > usual "give me one branch name to operate on, or I'll operate on the
> > current branch" command line convention of "git branch" commannd is
> > not a good fit for it, when "set upstream" feature was added, and
> > suggested an alternative syntax that avoids the mistake you quoted
> > above, perhaps something like:
> >
> > 	git branch --set-upstream-to=origin/master [HEAD]
> >
> > which would have been very clear whose upstream is set to what (with
> > or without the name of the other branch).  In other words, make the
> > name "origin/master" *NOT* the first branch name on the command line
> > in the usual sense, but a parameter to the --set-upstream option, so
> > that "give me one branch name to operate on, or I'll operate on the
> > current branch" convention is still kept.
> > 
> > You also broke people who corrected another kind of mistake in this
> > workflow:
> > ...
> > Coming from the above observation, while I am sympathetic to your
> > cause and agree that we would want to do something about it, I am
> > having a hard time to convince myself that your patch is the best
> > way to go.
> >
> > I am not entirely happy with the hypothetical "set-upstream-to"
> > myself, either.
>
> Thinking about it a bit more, I am starting to think that something
> based on the "set upstream to" could be a sane way forward:
> 
>  * add "git branch [--set-upstream-to=<name>]" that does what your
>    patch does.  The synopsis must make it clear that <name> is not
>    the usual first <name> like other "branch" command line arguments
>    that specify the branch being operated on, but is an argument to
>    the --set-upstream option [*1*].

Let's do this then. Disregard my earlier patch making -u a synonym of
--set-upstream so we can make it a synonym of --set-upstream-to instead.
This way we can use -u and then it's not so bad if the long name is a
bit ugly.

> 
>  * when "git branch --set-upstream <name>" without <start point>
>    is given, you first see if <name> exists and find out the
>    upstream of <name>, do what the user told you to do (i.e. reset
>    the upstream of the <name>d branch to the current branch), and
>    give hints to recover.  Two possibilities:
> 
>      $ git checkout frotz
>      $ git branch --set-upstream xyzzy
>      Branch xyzzy set up to track local branch frotz.
>      If you wanted to make frotz track xyzzy, do this:
>        $ git branch --set-upstream xyzzy <original>
>        $ git branch --set-upstream-to xyzzy
> 
>      $ git checkout frotz
>      $ git branch --set-upstream origin/xyzzy
>      Branch origin/xyzzy set up to track local branch frotz.
>      If you wanted to make frotz track xyzzy, do this:
>        $ git branch -d origin/xyzzy
>        $ git branch --set-upstream-to origin/xyzzy

Yep, this seems good. Now that you mention the <name> existing, I wonder
if letting --set-upstream create the branch as well wasn't another bad
decision, as the name suggests it's for setting that information after
the branch has already been created.

> 
>  * possibly, deprecate --set-upstream as a historical wart that had
>    misdesigned UI, and when it is used, give deprecation warning and
>    nudge the user to use --set-upstream-to instead.

I'd definitely like to deprecate the current behaviour. It's a common
source of irritation (not just for me personally, it shows up in #git
every once in a while).

I'll probably have some patches to send at the end of the weekend.

   cmn

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> Yep, this seems good. Now that you mention the <name> existing, I wonder
> if letting --set-upstream create the branch as well wasn't another bad
> decision, as the name suggests it's for setting that information after
> the branch has already been created.

You should be able to correct that for --set-upstream-to=<upstream>.
It is clearly about setting upstream for an existing branch, right?

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Carlos Martín Nieto]

On Fri, 2012-07-06 at 00:29 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > Yep, this seems good. Now that you mention the <name> existing, I wonder
> > if letting --set-upstream create the branch as well wasn't another bad
> > decision, as the name suggests it's for setting that information after
> > the branch has already been created.
> 
> You should be able to correct that for --set-upstream-to=<upstream>.
> It is clearly about setting upstream for an existing branch, right?

Yeah, it's for changing the tracking information and should refuse to do
so if the branch doesn't exist yet.

   cmn

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Junio C Hamano]

Ping on a seemingly stalled discussion (no need to rush but just
checking).

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Carlos Martín Nieto]

On Tue, 2012-07-17 at 22:56 -0700, Junio C Hamano wrote:
> Ping on a seemingly stalled discussion (no need to rush but just
> checking).

I've implemented the feedback, but been slacking on writing the tests
which is what's stopped me from re-sending the series.


   cmn

Re: [PATCH] branch: make --set-upstream saner without an explicit starting point

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> On Tue, 2012-07-17 at 22:56 -0700, Junio C Hamano wrote:
>> Ping on a seemingly stalled discussion (no need to rush but just
>> checking).
>
> I've implemented the feedback, but been slacking on writing the tests
> which is what's stopped me from re-sending the series.

Another mild ping.