[PATCHv2 0/3] Improve branch UI for setting upstream information

[PATCHv2 0/3] Improve branch UI for setting upstream information

[Carlos Martín Nieto]

Hi all,

As a result of making --unset-upstream fail if the given branch
doesn't exist, I discovered a copy-paste error in on the the tests in
the patch after it, so I'm resending the whole thing.

The changes from the last reroll are the tightening of the situations
where git will show an error message (not it's just if the branch is
new and exists as remote-tracking) which I already sent as a reply in
the other thread; and making --unset-upstream error out on bad input,
which I already mentioned above.

   cmn

Carlos Martín Nieto (3):
  branch: introduce --set-upstream-to
  branch: add --unset-upstream option
  branch: deprecate --set-upstream and show help if we detect possible
    mistaken use

 Documentation/git-branch.txt | 14 ++++++++-
 builtin/branch.c             | 60 +++++++++++++++++++++++++++++++++++++--
 t/t3200-branch.sh            | 67 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 4 deletions(-)

-- 
1.7.12.3.g0dd8ef6

[PATCH 1/3] branch: introduce --set-upstream-to

[Carlos Martín Nieto]

The existing --set-uptream option can cause confusion, as it uses the
usual branch convention of assuming a starting point of HEAD if none
is specified, causing

    git branch --set-upstream origin/master

to create a new local branch 'origin/master' that tracks the current
branch. As --set-upstream already exists, we can't simply change its
behaviour. To work around this, introduce --set-upstream-to which
accepts a compulsory argument indicating what the new upstream branch
should be and one optinal argument indicating which branch to change,
defaulting to HEAD.

The new options allows us to type

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

to set the current branch's upstream to be origin's master.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 Documentation/git-branch.txt |  9 ++++++++-
 builtin/branch.c             | 17 +++++++++++++++--
 t/t3200-branch.sh            | 14 ++++++++++++++
 3 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 47235be..e41c4b5 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 	[--column[=<options>] | --no-column]
 	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
+'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
 'git branch' (-d | -D) [-r] <branchname>...
 'git branch' --edit-description [<branchname>]
@@ -48,7 +49,7 @@ branch so that 'git pull' will appropriately merge from
 the remote-tracking branch. This behavior may be changed via the global
 `branch.autosetupmerge` configuration flag. That setting can be
 overridden by using the `--track` and `--no-track` options, and
-changed later using `git branch --set-upstream`.
+changed later using `git branch --set-upstream-to`.
 
 With a `-m` or `-M` option, <oldbranch> will be renamed to <newbranch>.
 If <oldbranch> had a corresponding reflog, it is renamed to match
@@ -173,6 +174,12 @@ start-point is either a local or remote-tracking branch.
 	like `--track` would when creating the branch, except that where
 	branch points to is not changed.
 
+-u <upstream>::
+--set-upstream-to=<upstream>::
+	Set up <branchname>'s tracking information so <upstream> is
+	considered <branchname>'s upstream branch. If no <branchname>
+	is specified, then it defaults to the current branch.
+
 --edit-description::
 	Open an editor and edit the text to explain what the branch is
 	for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 0e060f2..3c978eb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -713,6 +713,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
 	int quiet = 0;
+	const char *new_upstream = NULL;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
 	struct commit_list *with_commit = NULL;
@@ -726,6 +727,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			BRANCH_TRACK_EXPLICIT),
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
+		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
 		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
@@ -794,10 +796,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 
-	if (!delete && !rename && !edit_description && argc == 0)
+	if (!delete && !rename && !edit_description && !new_upstream && argc == 0)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list > 1)
+	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)
@@ -852,6 +854,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			rename_branch(argv[0], argv[1], rename > 1);
 		else
 			usage_with_options(builtin_branch_usage, options);
+	} else if (new_upstream) {
+		struct branch *branch = branch_get(argv[0]);
+
+		if (!ref_exists(branch->refname))
+			die(_("branch '%s' does not exist"), branch->name);
+
+		/*
+		 * create_branch takes care of setting up the tracking
+		 * info and making sure new_upstream is correct
+		 */
+		create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
 	} else if (argc > 0 && argc <= 2) {
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a17f8b2..e9019ac 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -369,6 +369,20 @@ test_expect_success \
     'git tag foobar &&
      test_must_fail git branch --track my11 foobar'
 
+test_expect_success 'use --set-upstream-to modify HEAD' \
+    'test_config branch.master.remote foo &&
+     test_config branch.master.merge foo &&
+     git branch my12
+     git branch --set-upstream-to my12 &&
+     test "$(git config branch.master.remote)" = "." &&
+     test "$(git config branch.master.merge)" = "refs/heads/my12"'
+
+test_expect_success 'use --set-upstream-to modify a particular branch' \
+    'git branch my13
+     git branch --set-upstream-to master my13 &&
+     test "$(git config branch.my13.remote)" = "." &&
+     test "$(git config branch.my13.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
-- 
1.7.12.3.g0dd8ef6

[PATCH 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

We have ways of setting the upstream information, but if we want to
unset it, we need to resort to modifying the configuration manually.

Teach branch an --unset-upstream option that unsets this information.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 Documentation/git-branch.txt |  5 +++++
 builtin/branch.c             | 21 ++++++++++++++++++---
 t/t3200-branch.sh            | 19 +++++++++++++++++++
 3 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e41c4b5..9c1d2f1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
+'git branch' --unset-upstream [<branchname>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
 'git branch' (-d | -D) [-r] <branchname>...
 'git branch' --edit-description [<branchname>]
@@ -180,6 +181,10 @@ start-point is either a local or remote-tracking branch.
 	considered <branchname>'s upstream branch. If no <branchname>
 	is specified, then it defaults to the current branch.
 
+--unset-upstream::
+	Remove the upstream information for <branchname>. If no branch
+	is specified it defaults to the current branch.
+
 --edit-description::
 	Open an editor and edit the text to explain what the branch is
 	for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 3c978eb..557995d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -712,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, rename = 0, force_create = 0, list = 0;
 	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
-	int quiet = 0;
+	int quiet = 0, unset_upstream = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
@@ -728,6 +728,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
 		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
+		OPT_BOOLEAN(0, "unset-upstream", &unset_upstream, "Unset the upstream info"),
 		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
@@ -796,10 +797,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 
-	if (!delete && !rename && !edit_description && !new_upstream && argc == 0)
+	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)
+	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)
@@ -865,6 +866,20 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		 * info and making sure new_upstream is correct
 		 */
 		create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+	} else if (unset_upstream) {
+		struct branch *branch = branch_get(argv[0]);
+		struct strbuf buf = STRBUF_INIT;
+
+		if (!branch_has_merge_config(branch)) {
+			die(_("Branch '%s' has no upstream information"), branch->name);
+		}
+
+		strbuf_addf(&buf, "branch.%s.remote", branch->name);
+		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "branch.%s.merge", branch->name);
+		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..1018e8b 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,25 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
      test "$(git config branch.my13.remote)" = "." &&
      test "$(git config branch.my13.merge)" = "refs/heads/master"'
 
+test_expect_success '--unset-upstream should fail if given a non-existent branch' \
+    'test_must_fail git branch --unset-upstream i-dont-exist'
+
+test_expect_success 'test --unset-upstream on HEAD' \
+    'git branch my14
+     test_config branch.master.remote foo &&
+     test_config branch.master.merge foo &&
+     git branch --set-upstream-to my14 &&
+     git branch --unset-upstream &&
+     test_must_fail git config branch.master.remote &&
+     test_must_fail git config branch.master.merge'
+
+test_expect_success 'test --unset-upstream on a particular branch' \
+    'git branch my15
+     git branch --set-upstream-to master my14 &&
+     git branch --unset-upstream my14 &&
+     test_must_fail git config branch.my14.remote &&
+     test_must_fail git config branch.my14.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.12.3.g0dd8ef6

[PATCH 3/3] branch: deprecate --set-upstream and show help if we detect possible mistaken use

[Carlos Martín Nieto]

This interface is error prone, and a better one (--set-upstream-to)
exists. Add a message listing the alternatives and suggest how to fix
a --set-upstream invocation in case the user only gives one argument
which causes a local branch with the same name as a remote-tracking
one to be created. The typical case is

    git branch --set-upstream origin/master

when the user meant

    git branch --set-upstream master origin/master

assuming that the current branch is master. Show a message telling the
user how to undo their action and get what they wanted. For the
command above, the message would be

The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to
Branch origin/master set up to track local branch master.

If you wanted to make 'master' track 'origin/master', do this:

    git branch -d origin/master
    git branch --set-upstream-to origin/master

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 557995d..5e95e35 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -881,10 +881,36 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		git_config_set_multivar(buf.buf, NULL, NULL, 1);
 		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
+		struct branch *branch = branch_get(argv[0]);
+		int branch_existed = 0, remote_tracking = 0;
+		struct strbuf buf = STRBUF_INIT;
+
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
+
+		if (track == BRANCH_TRACK_OVERRIDE)
+			fprintf(stderr, _("The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n"));
+
+		strbuf_addf(&buf, "refs/remotes/%s", branch->name);
+		remote_tracking = ref_exists(buf.buf);
+		strbuf_release(&buf);
+
+		branch_existed = ref_exists(branch->refname);
 		create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
 			      force_create, reflog, 0, quiet, track);
+
+		/*
+		 * We only show the instructions if the user gave us
+		 * one branch which doesn't exist locally, but is the
+		 * name of a remote-tracking branch.
+		 */
+		if (argc == 1 && track == BRANCH_TRACK_OVERRIDE &&
+		    !branch_existed && remote_tracking) {
+			fprintf(stderr, _("\nIf you wanted to make '%s' track '%s', do this:\n\n"), head, branch->name);
+			fprintf(stderr, _("    git branch -d %s\n"), branch->name);
+			fprintf(stderr, _("    git branch --set-upstream-to %s\n"), branch->name);
+		}
+
 	} else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 1018e8b..f2a076c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -402,6 +402,40 @@ test_expect_success 'test --unset-upstream on a particular branch' \
      test_must_fail git config branch.my14.remote &&
      test_must_fail git config branch.my14.merge'
 
+test_expect_success '--set-upstream shows message when creating a new branch that exists as remote-tracking' \
+    'git update-ref refs/remotes/origin/master HEAD &&
+     git branch --set-upstream origin/master 2>actual &&
+     test_when_finished git update-ref -d refs/remotes/origin/master &&
+     test_when_finished git branch -d origin/master &&
+     cat >expected <<EOF &&
+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to
+
+If you wanted to make '"'master'"' track '"'origin/master'"', do this:
+
+    git branch -d origin/master
+    git branch --set-upstream-to origin/master
+EOF
+     test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with two args only shows the deprecation message' \
+    'git branch --set-upstream master my13 2>actual &&
+     test_when_finished git branch --unset-upstream master &&
+     cat >expected <<EOF &&
+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to
+EOF
+     test_cmp expected actual
+'
+
+test_expect_success '--set-upstream with one arg only shows the deprecation message if the branch existed' \
+    'git branch --set-upstream my13 2>actual &&
+     test_when_finished git branch --unset-upstream my13 &&
+     cat >expected <<EOF &&
+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to
+EOF
+     test_cmp expected actual
+'
+
 # 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.12.3.g0dd8ef6

Re: [PATCHv2 0/3] Improve branch UI for setting upstream information

[Junio C Hamano]

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

> As a result of making --unset-upstream fail if the given branch
> doesn't exist, I discovered a copy-paste error in on the the tests in
> the patch after it, so I'm resending the whole thing.
>
> The changes from the last reroll are the tightening of the situations
> where git will show an error message (not it's just if the branch is
> new and exists as remote-tracking) which I already sent as a reply in
> the other thread; and making --unset-upstream error out on bad input,
> which I already mentioned above.

Thanks.

In addition to "--unset-upstream must fail on i-dont-exist branch"
in [2/3], I am wondering if we would want to also make sure the
command fails when the upstream information is not set for the
branch, i.e. something like the following on top.

What do you think?

 t/t3200-branch.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git i/t/t3200-branch.sh w/t/t3200-branch.sh
index 1018e8b..a0aaedd 100755
--- i/t/t3200-branch.sh
+++ w/t/t3200-branch.sh
@@ -393,7 +393,9 @@ test_expect_success 'test --unset-upstream on HEAD' \
      git branch --set-upstream-to my14 &&
      git branch --unset-upstream &&
      test_must_fail git config branch.master.remote &&
-     test_must_fail git config branch.master.merge'
+     test_must_fail git config branch.master.merge &&
+     test_must_fail git branch --unset-upstream
+'
 
 test_expect_success 'test --unset-upstream on a particular branch' \
     'git branch my15

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Ralf Thielow]

On Thu, Aug 30, 2012 at 7:23 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
> behaviour. To work around this, introduce --set-upstream-to which
> accepts a compulsory argument indicating what the new upstream branch
> should be and one optinal argument indicating which branch to change,
> defaulting to HEAD.
>

Could you please also add this new option to the
"contrib/completion/git-completion.bash"
script?

Thanks!

Re: [PATCHv2 0/3] Improve branch UI for setting upstream information

[Carlos Martín Nieto]

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

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> As a result of making --unset-upstream fail if the given branch
>> doesn't exist, I discovered a copy-paste error in on the the tests in
>> the patch after it, so I'm resending the whole thing.
>>
>> The changes from the last reroll are the tightening of the situations
>> where git will show an error message (not it's just if the branch is
>> new and exists as remote-tracking) which I already sent as a reply in
>> the other thread; and making --unset-upstream error out on bad input,
>> which I already mentioned above.
>
> Thanks.
>
> In addition to "--unset-upstream must fail on i-dont-exist branch"
> in [2/3], I am wondering if we would want to also make sure the
> command fails when the upstream information is not set for the
> branch, i.e. something like the following on top.
>
> What do you think?
>
>  t/t3200-branch.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git i/t/t3200-branch.sh w/t/t3200-branch.sh
> index 1018e8b..a0aaedd 100755
> --- i/t/t3200-branch.sh
> +++ w/t/t3200-branch.sh
> @@ -393,7 +393,9 @@ test_expect_success 'test --unset-upstream on HEAD' \
>       git branch --set-upstream-to my14 &&
>       git branch --unset-upstream &&
>       test_must_fail git config branch.master.remote &&
> -     test_must_fail git config branch.master.merge'
> +     test_must_fail git config branch.master.merge &&
> +     test_must_fail git branch --unset-upstream
> +'

Yeah, this looks good, makes sure that it will still behave correctly
even if the code path for these two situations diverges.

   cmn

Re: [PATCHv2 0/3] Improve branch UI for setting upstream information

[Junio C Hamano]

carlos@cmartin.tk (Carlos Martín Nieto) writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Carlos Martín Nieto <cmn@elego.de> writes:
>>
>>> As a result of making --unset-upstream fail if the given branch
>>> doesn't exist, I discovered a copy-paste error in on the the tests in
>>> the patch after it, so I'm resending the whole thing.
>>>
>>> The changes from the last reroll are the tightening of the situations
>>> where git will show an error message (not it's just if the branch is
>>> new and exists as remote-tracking) which I already sent as a reply in
>>> the other thread; and making --unset-upstream error out on bad input,
>>> which I already mentioned above.
>>
>> Thanks.
>>
>> In addition to "--unset-upstream must fail on i-dont-exist branch"
>> in [2/3], I am wondering if we would want to also make sure the
>> command fails when the upstream information is not set for the
>> branch, i.e. something like the following on top.
>>
>> What do you think?
>>
>>  t/t3200-branch.sh | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git i/t/t3200-branch.sh w/t/t3200-branch.sh
>> index 1018e8b..a0aaedd 100755
>> --- i/t/t3200-branch.sh
>> +++ w/t/t3200-branch.sh
>> @@ -393,7 +393,9 @@ test_expect_success 'test --unset-upstream on HEAD' \
>>       git branch --set-upstream-to my14 &&
>>       git branch --unset-upstream &&
>>       test_must_fail git config branch.master.remote &&
>> -     test_must_fail git config branch.master.merge'
>> +     test_must_fail git config branch.master.merge &&
>> +     test_must_fail git branch --unset-upstream
>> +'
>
> Yeah, this looks good, makes sure that it will still behave correctly
> even if the code path for these two situations diverges.

Alright; will squash.

Thanks.

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Carlos Martín Nieto]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> On Thu, Aug 30, 2012 at 7:23 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
>> behaviour. To work around this, introduce --set-upstream-to which
>> accepts a compulsory argument indicating what the new upstream branch
>> should be and one optinal argument indicating which branch to change,
>> defaulting to HEAD.
>>
>
> Could you please also add this new option to the
> "contrib/completion/git-completion.bash"
> script?

If I knew how those things work... Is this enough?

   cmn

--8<--
Subject: [PATCH] completion: add --set-upstream-to and --unset-upstream

---
 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ffedce7..4f46357 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -880,6 +880,7 @@ _git_branch ()
 			--color --no-color --verbose --abbrev= --no-abbrev
 			--track --no-track --contains --merged --no-merged
 			--set-upstream --edit-description --list
+			--unset-upstream --set-upstream-to=
 			"
 		;;
 	*)
-- 
1.7.12.3.g0dd8ef6

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Ralf Thielow]

On Fri, Aug 31, 2012 at 5:22 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
> Ralf Thielow <ralf.thielow@gmail.com> writes:
>
>> On Thu, Aug 30, 2012 at 7:23 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
>>> behaviour. To work around this, introduce --set-upstream-to which
>>> accepts a compulsory argument indicating what the new upstream branch
>>> should be and one optinal argument indicating which branch to change,
>>> defaulting to HEAD.
>>>
>>
>> Could you please also add this new option to the
>> "contrib/completion/git-completion.bash"
>> script?
>
> If I knew how those things work... Is this enough?
>

Yes, Thanks.

>    cmn
>
> --8<--
> Subject: [PATCH] completion: add --set-upstream-to and --unset-upstream
>
> ---
>  contrib/completion/git-completion.bash | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index ffedce7..4f46357 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -880,6 +880,7 @@ _git_branch ()
>                         --color --no-color --verbose --abbrev= --no-abbrev
>                         --track --no-track --contains --merged --no-merged
>                         --set-upstream --edit-description --list
> +                       --unset-upstream --set-upstream-to=
>                         "
>                 ;;
>         *)
> --
> 1.7.12.3.g0dd8ef6

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> On Fri, Aug 31, 2012 at 5:22 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
>> Ralf Thielow <ralf.thielow@gmail.com> writes:
>>
>>> On Thu, Aug 30, 2012 at 7:23 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
>>>> behaviour. To work around this, introduce --set-upstream-to which
>>>> accepts a compulsory argument indicating what the new upstream branch
>>>> should be and one optinal argument indicating which branch to change,
>>>> defaulting to HEAD.
>>>>
>>>
>>> Could you please also add this new option to the
>>> "contrib/completion/git-completion.bash"
>>> script?
>>
>> If I knew how those things work... Is this enough?
>>
>
> Yes, Thanks.

While you are at it, perhaps you may want to unadvertise --set-upstream?

>
>>    cmn
>>
>> --8<--
>> Subject: [PATCH] completion: add --set-upstream-to and --unset-upstream
>>
>> ---
>>  contrib/completion/git-completion.bash | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index ffedce7..4f46357 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -880,6 +880,7 @@ _git_branch ()
>>                         --color --no-color --verbose --abbrev= --no-abbrev
>>                         --track --no-track --contains --merged --no-merged
>>                         --set-upstream --edit-description --list
>> +                       --unset-upstream --set-upstream-to=
>>                         "
>>                 ;;
>>         *)
>> --
>> 1.7.12.3.g0dd8ef6

Re: [PATCH 1/3] branch: introduce --set-upstream-to

[Carlos Martín Nieto]

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

> Ralf Thielow <ralf.thielow@gmail.com> writes:
>
>> On Fri, Aug 31, 2012 at 5:22 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
>>> Ralf Thielow <ralf.thielow@gmail.com> writes:
>>>
>>>> On Thu, Aug 30, 2012 at 7:23 PM, Carlos Martín Nieto <cmn@elego.de> wrote:
>>>>> behaviour. To work around this, introduce --set-upstream-to which
>>>>> accepts a compulsory argument indicating what the new upstream branch
>>>>> should be and one optinal argument indicating which branch to change,
>>>>> defaulting to HEAD.
>>>>>
>>>>
>>>> Could you please also add this new option to the
>>>> "contrib/completion/git-completion.bash"
>>>> script?
>>>
>>> If I knew how those things work... Is this enough?
>>>
>>
>> Yes, Thanks.
>
> While you are at it, perhaps you may want to unadvertise --set-upstream?
>

Yeah, that'd be good.

   cmn

--8<--
Subject: [PATCH] completion: add --set-upstream-to and --unset-upstream

Remove --set-upstream as it's deprecated now.
---
 contrib/completion/git-completion.bash | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ffedce7..4a4d30a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -879,7 +879,8 @@ _git_branch ()
 		__gitcomp "
 			--color --no-color --verbose --abbrev= --no-abbrev
 			--track --no-track --contains --merged --no-merged
-			--set-upstream --edit-description --list
+			--set-upstream-to= --edit-description --list
+			--unset-upstream
 			"
 		;;
 	*)
-- 
1.7.12.3.g0dd8ef6

Re: [PATCH 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> cmn@elego.de (Carlos Martín Nieto) writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Carlos Martín Nieto <cmn@elego.de> writes:
>>>>
>>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>>>> index e9019ac..93e5d6e 100755
>>>>> --- a/t/t3200-branch.sh
>>>>> +++ b/t/t3200-branch.sh
>>>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
>>>>>       test "$(git config branch.my13.remote)" = "." &&
>>>>>       test "$(git config branch.my13.merge)" = "refs/heads/master"'
>>>>>  
>>>>> +test_expect_success 'test --unset-upstream on HEAD' \
>>>>> +    'git branch my14
>>>>> +     test_config branch.master.remote foo &&
>>>>> +     test_config branch.master.merge foo &&
>>>>> +     git branch --set-upstream-to my14 &&
>>>>> +     git branch --unset-upstream &&
>>>>> +     test_must_fail git config branch.master.remote &&
>>>>> +     test_must_fail git config branch.master.merge'
>>>>> +
>>>>> +test_expect_success 'test --unset-upstream on a particular branch' \
>>>>> +    'git branch my15
>>>>> +     git branch --set-upstream-to master my14 &&
>>>>> +     git branch --unset-upstream my14 &&
>>>>> +     test_must_fail git config branch.my14.remote &&
>>>>> +     test_must_fail git config branch.my14.merge'
>>>>> +
>>>>
>>>> What should happen when you say "--unset-upstream" on a branch B
>>>> that does not have B@{upstream}?  Should it fail?  Should it be
>>>> silently ignored?  Is it undefined that we do not want to test?
>>>
>>> I'd say it should be ignored, as the end result we want is for there to
>>> be no upstream information. What we do underneath is remove a couple of
>>> config options, wich doesn't fail if they didn't insist in the first
>>> place.
>>
>> I am not sure about that reasoning.
>>
>>     $ git config --unset core.nosuchvariable ; echo $?
>>     5
>>
>> looks like a failure to me.
>>
>> More importantly, wouldn't we want to catch typo in
>>
>> 	git branch --unset-upstream mext
>>
>> which admittedly should come from a different codepath (I do not
>> think your patch checks if "mext" branch exists before going ahead
>> to the config--unset dance)?
>
> Sorry, the last paragraph was incomplete.
>
> In general, we should detect errors and allow the user to choose to
> ignore.
>
> A script that wants to make sure that B does not have an upstream,
> without knowing if it already has one, can say "--unset-upstream B"
> and choose to ignore the error if B does not have an upstream.  
>
> If the script is carefully written, it would try to check if B has
> one and call "--unset-upstream B" only when it doesn't.  A casually
> written loose script would say "--unset-upstream B 2>/dev/null"
> and keep going (it would not notice other kinds of errors, but that
> is what makes it "casual and loose").

OK, that's a good point, I'll update the patch.

   cmn

Re: [PATCH 2/3] branch: add --unset-upstream option

[Junio C Hamano]

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

> cmn@elego.de (Carlos Martín Nieto) writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Carlos Martín Nieto <cmn@elego.de> writes:
>>>
>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>>> index e9019ac..93e5d6e 100755
>>>> --- a/t/t3200-branch.sh
>>>> +++ b/t/t3200-branch.sh
>>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
>>>>       test "$(git config branch.my13.remote)" = "." &&
>>>>       test "$(git config branch.my13.merge)" = "refs/heads/master"'
>>>>  
>>>> +test_expect_success 'test --unset-upstream on HEAD' \
>>>> +    'git branch my14
>>>> +     test_config branch.master.remote foo &&
>>>> +     test_config branch.master.merge foo &&
>>>> +     git branch --set-upstream-to my14 &&
>>>> +     git branch --unset-upstream &&
>>>> +     test_must_fail git config branch.master.remote &&
>>>> +     test_must_fail git config branch.master.merge'
>>>> +
>>>> +test_expect_success 'test --unset-upstream on a particular branch' \
>>>> +    'git branch my15
>>>> +     git branch --set-upstream-to master my14 &&
>>>> +     git branch --unset-upstream my14 &&
>>>> +     test_must_fail git config branch.my14.remote &&
>>>> +     test_must_fail git config branch.my14.merge'
>>>> +
>>>
>>> What should happen when you say "--unset-upstream" on a branch B
>>> that does not have B@{upstream}?  Should it fail?  Should it be
>>> silently ignored?  Is it undefined that we do not want to test?
>>
>> I'd say it should be ignored, as the end result we want is for there to
>> be no upstream information. What we do underneath is remove a couple of
>> config options, wich doesn't fail if they didn't insist in the first
>> place.
>
> I am not sure about that reasoning.
>
>     $ git config --unset core.nosuchvariable ; echo $?
>     5
>
> looks like a failure to me.
>
> More importantly, wouldn't we want to catch typo in
>
> 	git branch --unset-upstream mext
>
> which admittedly should come from a different codepath (I do not
> think your patch checks if "mext" branch exists before going ahead
> to the config--unset dance)?

Sorry, the last paragraph was incomplete.

In general, we should detect errors and allow the user to choose to
ignore.

A script that wants to make sure that B does not have an upstream,
without knowing if it already has one, can say "--unset-upstream B"
and choose to ignore the error if B does not have an upstream.  

If the script is carefully written, it would try to check if B has
one and call "--unset-upstream B" only when it doesn't.  A casually
written loose script would say "--unset-upstream B 2>/dev/null"
and keep going (it would not notice other kinds of errors, but that
is what makes it "casual and loose").

Re: [PATCH 2/3] branch: add --unset-upstream option

[Junio C Hamano]

cmn@elego.de (Carlos Martín Nieto) writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Carlos Martín Nieto <cmn@elego.de> writes:
>>
>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>> index e9019ac..93e5d6e 100755
>>> --- a/t/t3200-branch.sh
>>> +++ b/t/t3200-branch.sh
>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
>>>       test "$(git config branch.my13.remote)" = "." &&
>>>       test "$(git config branch.my13.merge)" = "refs/heads/master"'
>>>  
>>> +test_expect_success 'test --unset-upstream on HEAD' \
>>> +    'git branch my14
>>> +     test_config branch.master.remote foo &&
>>> +     test_config branch.master.merge foo &&
>>> +     git branch --set-upstream-to my14 &&
>>> +     git branch --unset-upstream &&
>>> +     test_must_fail git config branch.master.remote &&
>>> +     test_must_fail git config branch.master.merge'
>>> +
>>> +test_expect_success 'test --unset-upstream on a particular branch' \
>>> +    'git branch my15
>>> +     git branch --set-upstream-to master my14 &&
>>> +     git branch --unset-upstream my14 &&
>>> +     test_must_fail git config branch.my14.remote &&
>>> +     test_must_fail git config branch.my14.merge'
>>> +
>>
>> What should happen when you say "--unset-upstream" on a branch B
>> that does not have B@{upstream}?  Should it fail?  Should it be
>> silently ignored?  Is it undefined that we do not want to test?
>
> I'd say it should be ignored, as the end result we want is for there to
> be no upstream information. What we do underneath is remove a couple of
> config options, wich doesn't fail if they didn't insist in the first
> place.

I am not sure about that reasoning.

    $ git config --unset core.nosuchvariable ; echo $?
    5

looks like a failure to me.

More importantly, wouldn't we want to catch typo in

	git branch --unset-upstream mext

which admittedly should come from a different codepath (I do not
think your patch checks if "mext" branch exists before going ahead
to the config--unset dance)?

Re: [PATCH 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

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

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index e9019ac..93e5d6e 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
>>       test "$(git config branch.my13.remote)" = "." &&
>>       test "$(git config branch.my13.merge)" = "refs/heads/master"'
>>  
>> +test_expect_success 'test --unset-upstream on HEAD' \
>> +    'git branch my14
>> +     test_config branch.master.remote foo &&
>> +     test_config branch.master.merge foo &&
>> +     git branch --set-upstream-to my14 &&
>> +     git branch --unset-upstream &&
>> +     test_must_fail git config branch.master.remote &&
>> +     test_must_fail git config branch.master.merge'
>> +
>> +test_expect_success 'test --unset-upstream on a particular branch' \
>> +    'git branch my15
>> +     git branch --set-upstream-to master my14 &&
>> +     git branch --unset-upstream my14 &&
>> +     test_must_fail git config branch.my14.remote &&
>> +     test_must_fail git config branch.my14.merge'
>> +
>
> What should happen when you say "--unset-upstream" on a branch B
> that does not have B@{upstream}?  Should it fail?  Should it be
> silently ignored?  Is it undefined that we do not want to test?

I'd say it should be ignored, as the end result we want is for there to
be no upstream information. What we do underneath is remove a couple of
config options, wich doesn't fail if they didn't insist in the first
place.

Would you like a test that makes sure two --unset-upstream in a row
don't cause any errors?

   cmn

Re: [PATCH 2/3] branch: add --unset-upstream option

[Junio C Hamano]

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

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index e9019ac..93e5d6e 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
>       test "$(git config branch.my13.remote)" = "." &&
>       test "$(git config branch.my13.merge)" = "refs/heads/master"'
>  
> +test_expect_success 'test --unset-upstream on HEAD' \
> +    'git branch my14
> +     test_config branch.master.remote foo &&
> +     test_config branch.master.merge foo &&
> +     git branch --set-upstream-to my14 &&
> +     git branch --unset-upstream &&
> +     test_must_fail git config branch.master.remote &&
> +     test_must_fail git config branch.master.merge'
> +
> +test_expect_success 'test --unset-upstream on a particular branch' \
> +    'git branch my15
> +     git branch --set-upstream-to master my14 &&
> +     git branch --unset-upstream my14 &&
> +     test_must_fail git config branch.my14.remote &&
> +     test_must_fail git config branch.my14.merge'
> +

What should happen when you say "--unset-upstream" on a branch B
that does not have B@{upstream}?  Should it fail?  Should it be
silently ignored?  Is it undefined that we do not want to test?

[PATCH 2/3] branch: add --unset-upstream option

[Carlos Martín Nieto]

We have ways of setting the upstream information, but if we want to
unset it, we need to resort to modifying the configuration manually.

Teach branch an --unset-upstream option that unsets this information.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 Documentation/git-branch.txt |  5 +++++
 builtin/branch.c             | 17 ++++++++++++++---
 t/t3200-branch.sh            | 16 ++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e41c4b5..9c1d2f1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
+'git branch' --unset-upstream [<branchname>]
 'git branch' (-m | -M) [<oldbranch>] <newbranch>
 'git branch' (-d | -D) [-r] <branchname>...
 'git branch' --edit-description [<branchname>]
@@ -180,6 +181,10 @@ start-point is either a local or remote-tracking branch.
 	considered <branchname>'s upstream branch. If no <branchname>
 	is specified, then it defaults to the current branch.
 
+--unset-upstream::
+	Remove the upstream information for <branchname>. If no branch
+	is specified it defaults to the current branch.
+
 --edit-description::
 	Open an editor and edit the text to explain what the branch is
 	for, to be used by various other commands (e.g. `request-pull`).
diff --git a/builtin/branch.c b/builtin/branch.c
index 3c978eb..08068f7 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -712,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	int delete = 0, rename = 0, force_create = 0, list = 0;
 	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
-	int quiet = 0;
+	int quiet = 0, unset_upstream = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
@@ -728,6 +728,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT( 0, "set-upstream",  &track, "change upstream info",
 			BRANCH_TRACK_OVERRIDE),
 		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
+		OPT_BOOLEAN(0, "unset-upstream", &unset_upstream, "Unset the upstream info"),
 		OPT__COLOR(&branch_use_color, "use colored output"),
 		OPT_SET_INT('r', "remotes",     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
@@ -796,10 +797,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
 
-	if (!delete && !rename && !edit_description && !new_upstream && argc == 0)
+	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)
+	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
 	if (abbrev == -1)
@@ -865,6 +866,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		 * info and making sure new_upstream is correct
 		 */
 		create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+	} else if (unset_upstream) {
+		struct branch *branch = branch_get(argv[0]);
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_addf(&buf, "branch.%s.remote", branch->name);
+		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "branch.%s.merge", branch->name);
+		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
 		if (kinds != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e9019ac..93e5d6e 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \
      test "$(git config branch.my13.remote)" = "." &&
      test "$(git config branch.my13.merge)" = "refs/heads/master"'
 
+test_expect_success 'test --unset-upstream on HEAD' \
+    'git branch my14
+     test_config branch.master.remote foo &&
+     test_config branch.master.merge foo &&
+     git branch --set-upstream-to my14 &&
+     git branch --unset-upstream &&
+     test_must_fail git config branch.master.remote &&
+     test_must_fail git config branch.master.merge'
+
+test_expect_success 'test --unset-upstream on a particular branch' \
+    'git branch my15
+     git branch --set-upstream-to master my14 &&
+     git branch --unset-upstream my14 &&
+     test_must_fail git config branch.my14.remote &&
+     test_must_fail git config branch.my14.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