Re: [PATCH v3 1/2 / RFC] builtin/branch: stop supporting the use of --set-upstream option

Kaartic Sivaraam [off-list ref] writes:

The '--set-upstream' option of branch was deprecated in,

    b347d06bf branch: deprecate --set-upstream and show help if we
    detect possible mistaken use (Thu, 30 Aug 2012 19:23:13 +0200)

It was deprecated for the reasons specified in the commit message of the
referenced commit.

I wonder if these two lines add any value here.  Those who know the
reason would not be helped, and those who don't know have to view
"git show b347d06bf" anyway.

Make 'branch' die with an appropraite error message when the '--set-upstream'
option is used.

OK.

Note that there's a reason behind "dying with an error message" instead of
"not accepting the option". 'git branch' would *accept* '--set-upstream'
even after it's removal as a consequence of,

        Unique prefix can be abbrievated in option names

                          AND

    '--set-upstream' is a unique prefix of '--set-upstream-to'
       (when the '--set-upstream' option has been removed)

In order to smooth the transition for users and to avoid them being affected
by the "prefix issue" it was decided to make branch die when seeing the
'--set-upstream' flag for a few years and let the users know that it would be
removed some time in the future.

I somehow think the above wastes bits a bit too much.  Wouldn't it
be sufficient to say

    In order to prevent "--set-upstream" on a command line from
    being taken as an abbreviated form of "--set-upstream-to",
    explicitly catch "--set-upstream" option and die, instead of
    just removing it from the list of options.

    $ git branch --set-upstream origin/master
    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.

After,

    $ git branch
    * master

    $ git branch --set-upstream origin/master
    fatal: the '--set-upstream' flag is no longer supported and will be removed. Consider using '--track' or '--set-upstream-to'

Because from the end-user's point of view, it has already been
removed, I'd phrase it more like

	The --set-upstream option has been removed.  Use --track or ...

and make sure we do not list "--set-upstream" in the list of
supported options in

	git branch -h

output.

 A query,

    I see the following code in the code path a little above the die statement
    added in this change,

            if (!strcmp(argv[0], "HEAD"))
    		    	die(_("it does not make sense to create 'HEAD' manually"));

    It does seem to be doing quite a nice job of avoiding an ambiguity that could
    have bad consequences but it's still possible to create a branch named 'HEAD'
    using the '-b' option of 'checkout'. Should 'git checkout -b HEAD' actually
    fail(it does not currently) for the same reason 'git branch HEAD' fails?

    My guess is that people would use 'git checkout -b <new_branch_name> <starting_point>'
    more than it's 'git branch' counterpart.    

Thanks for noticing.  I offhand see no reason not to do what you
suggest above.

-		OPT_SET_INT( 0, "set-upstream",  &track, N_("change upstream info"),
+		OPT_SET_INT( 0, "set-upstream",  &track, N_("no longer supported"),
 			BRANCH_TRACK_OVERRIDE),

Here we would want to use something like

	{ OPTION_SET_INT, 0, "set-upstream", &track, NULL, N_("do not use"),
	  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, BRANCH_TRACK_OVERRIDE },

in order to hide the option from "git branch -h" output.

All review comments from Martin were also good ones, and I won't
repeat them here.

Thanks.

On Tuesday 15 August 2017 01:49 AM, Junio C Hamano wrote:

I wonder if these two lines add any value here. Those who know the
reason would not be helped, and those who don't know have to view
"git show b347d06bf" anyway.

That's right.

I somehow think the above wastes bits a bit too much.  Wouldn't it
be sufficient to say

     In order to prevent "--set-upstream" on a command line from
     being taken as an abbreviated form of "--set-upstream-to",
     explicitly catch "--set-upstream" option and die, instead of
     just removing it from the list of options.

Thanks for the shorter version. I'll use this :)

Because from the end-user's point of view, it has already been
removed, I'd phrase it more like

	The --set-upstream option has been removed.  Use --track or ...

I thought I changed it. It seems to have gone missing. Thanks for 
noticing this.

and make sure we do not list "--set-upstream" in the list of
supported options in

	git branch -h

output.

I guess the instructions given below are enough. They do seem to be 
doing hiding it
from 'git branch -h'

Here we would want to use something like
	{ OPTION_SET_INT, 0, "set-upstream", &track, NULL, N_("do not use"),
	  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, BRANCH_TRACK_OVERRIDE },

in order to hide the option from "git branch -h" output.

All review comments from Martin were also good ones, and I won't
repeat them here.

quoted

  A query,

     I see the following code in the code path a little above the die statement
     added in this change,

             if (!strcmp(argv[0], "HEAD"))
     		    	die(_("it does not make sense to create 'HEAD' manually"));

     It does seem to be doing quite a nice job of avoiding an ambiguity that could
     have bad consequences but it's still possible to create a branch named 'HEAD'
     using the '-b' option of 'checkout'. Should 'git checkout -b HEAD' actually
     fail(it does not currently) for the same reason 'git branch HEAD' fails?

     My guess is that people would use 'git checkout -b <new_branch_name> <starting_point>'
     more than it's 'git branch' counterpart.

Thanks for noticing.  I offhand see no reason not to do what you
suggest above.

There are two ways in which this could be done.

1. Duplicate the check done in 'builtin/branch.c' in 
'builtin/checkout.c'. This doesn't
     sound good to me.

2. Do the check in 'branch.c::validate_new_branchname' to ensure there's 
no way to
     create a branch with the name of 'HEAD'.

Which one is preferred?

Avoiding the clean up step of tests may help in some cases but in other
cases they cause the other unrelated tests to fail for unobvious reasons.
It's better to cleanup a few things to keep other tests from failing
as a result of it.

So, cleanup a cruft left behind by an old test in order for the changes that
are to be introduced to be independent of it.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 This was part of [PATCH v3 1/2] and has been separated as it seemed to be
 a "logically separate" change.

 t/t3200-branch.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd37ac47c..b54b3ebf3 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -560,6 +560,7 @@ test_expect_success 'use --set-upstream-to modify HEAD' '
 test_expect_success 'use --set-upstream-to modify a particular branch' '
 	git branch my13 &&
 	git branch --set-upstream-to master my13 &&
+	test_when_finished "git branch --unset-upstream my13" &&
 	test "$(git config branch.my13.remote)" = "." &&
 	test "$(git config branch.my13.merge)" = "refs/heads/master"
 '

-- 
2.14.1.534.g641031ecb

The '--set-upstream' option of branch was deprecated in,

    b347d06bf branch: deprecate --set-upstream and show help if we
    detect possible mistaken use (Thu, 30 Aug 2012 19:23:13 +0200)

In order to prevent "--set-upstream" on a command line from being taken as
an abbreviated form of "--set-upstream-to", explicitly catch "--set-upstream"
option and die, instead of just removing it from the list of options.

The option is planned to be removed after this change has been around for a few
years.

The before/after behaviour for a simple case follows,

    $ git remote
    origin

Before,

    $ git branch
    * master

    $ git branch --set-upstream origin/master
    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.

    $ echo $?
    0

    $ git branch
    * master
      origin/master

After,

    $ git branch
    * master

    $ git branch --set-upstream origin/master
    fatal: the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.

    $ echo $?
    128

    $ git branch
    * master

Helped-by: Martin Ågren [off-list ref],  Junio C Hamano [off-list ref]
Signed-off-by: Kaartic Sivaraam <redacted>
---
 Changes in v4:

    - made a few changes suggested by Martin
    - hid the '--set-upstream' option from 'git branch -h' as suggested by Junio 
    - updated commit message as suggested by Junio
    - updated error message (this should have been in v3 but somehow got skipped)

 Note: I'm not using the word 'removed' in the error messages or in the documentation as
 I feel it counter-intuitive from the end user's perspective because 

    * 'git branch' still accepts the option in the command line

    *  The option has not yet been removed from the synopsis of the documentation and I think
       we can't remove it from the 'Synopsis' porion of the documentation as it doesn't make
       sense (at least to me) to give a description of an option not listed in the synopsis.
       Moreover, we have to state the reason for not supporting it in some place.

 I guess the phrase 'no longer supported' is equally communicative. Let me know if that was not
 a right decision.

 Documentation/git-branch.txt |  8 ++++----
 builtin/branch.c             | 25 +++--------------------
 t/t3200-branch.sh            | 47 ++------------------------------------------
 t/t6040-tracking-info.sh     | 20 +++++++------------
 4 files changed, 16 insertions(+), 84 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 81bd0a7b7..948d9c9ef 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt

@@ -195,10 +195,10 @@ start-point is either a local or remote-tracking branch.
 	branch.autoSetupMerge configuration variable is true.
 
 --set-upstream::
-	If specified branch does not exist yet or if `--force` has been
-	given, acts exactly like `--track`. Otherwise sets up configuration
-	like `--track` would when creating the branch, except that where
-	branch points to is not changed.
+	As this option had confusing syntax it's no longer supported. Please use
+	--track or --set-upstream-to instead.
++
+Note: This could possibly become an alias of --set-upstream-to in the future.
 
 -u <upstream>::
 --set-upstream-to=<upstream>::

diff --git a/builtin/branch.c b/builtin/branch.c
index a3bd2262b..6e3ea5787 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -557,8 +557,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT__QUIET(&quiet, N_("suppress informational messages")),
 		OPT_SET_INT('t', "track",  &track, N_("set up tracking mode (see git-pull(1))"),
 			BRANCH_TRACK_EXPLICIT),
-		OPT_SET_INT( 0, "set-upstream",  &track, N_("change upstream info"),
-			BRANCH_TRACK_OVERRIDE),
+		{ OPTION_SET_INT, 0, "set-upstream", &track, NULL, N_("do not use"),
+			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, BRANCH_TRACK_OVERRIDE },
 		OPT_STRING('u', "set-upstream-to", &new_upstream, N_("upstream"), N_("change the upstream info")),
 		OPT_BOOL(0, "unset-upstream", &unset_upstream, N_("Unset the upstream info")),
 		OPT__COLOR(&branch_use_color, N_("use colored output")),

@@ -755,8 +755,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		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 (!strcmp(argv[0], "HEAD"))
 			die(_("it does not make sense to create 'HEAD' manually"));

@@ -768,28 +766,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			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"));
+			die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
 
-		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(argv[0], (argc == 2) ? argv[1] : head,
 			      force, 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 b54b3ebf3..34f556998 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh

@@ -606,38 +606,8 @@ test_expect_success 'test --unset-upstream on a particular branch' '
 	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_i18ncmp 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_i18ncmp 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_i18ncmp expected actual
+test_expect_success '--set-upstream fails' '
+    test_must_fail git branch --set-upstream origin/master
 '
 
 test_expect_success '--set-upstream-to notices an error to set branch as own upstream' '

@@ -962,19 +932,6 @@ test_expect_success 'attempt to delete a branch merged to its base' '
 	test_must_fail git branch -d my10
 '
 
-test_expect_success 'use set-upstream on the current branch' '
-	git checkout master &&
-	git --bare init myupstream.git &&
-	git push myupstream.git master:refs/heads/frotz &&
-	git remote add origin myupstream.git &&
-	git fetch &&
-	git branch --set-upstream master origin/frotz &&
-
-	test "z$(git config branch.master.remote)" = "zorigin" &&
-	test "z$(git config branch.master.merge)" = "zrefs/heads/frotz"
-
-'
-
 test_expect_success 'use --edit-description' '
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 97a07655a..be78cc4fa 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh

@@ -188,35 +188,29 @@ test_expect_success 'fail to track annotated tags' '
 	test_must_fail git checkout heavytrack
 '
 
-test_expect_success 'setup tracking with branch --set-upstream on existing branch' '
+test_expect_success '--set-upstream-to does not change branch' '
 	git branch from-master master &&
-	test_must_fail git config branch.from-master.merge > actual &&
-	git branch --set-upstream from-master master &&
-	git config branch.from-master.merge > actual &&
-	grep -q "^refs/heads/master$" actual
-'
-
-test_expect_success '--set-upstream does not change branch' '
+	git branch --set-upstream-to master from-master &&
 	git branch from-master2 master &&
 	test_must_fail git config branch.from-master2.merge > actual &&
 	git rev-list from-master2 &&
 	git update-ref refs/heads/from-master2 from-master2^ &&
 	git rev-parse from-master2 >expect2 &&
-	git branch --set-upstream from-master2 master &&
+	git branch --set-upstream-to master from-master2 &&
 	git config branch.from-master.merge > actual &&
 	git rev-parse from-master2 >actual2 &&
 	grep -q "^refs/heads/master$" actual &&
 	cmp expect2 actual2
 '
 
-test_expect_success '--set-upstream @{-1}' '
-	git checkout from-master &&
+test_expect_success '--set-upstream-to @{-1}' '
+	git checkout follower &&
 	git checkout from-master2 &&
 	git config branch.from-master2.merge > expect2 &&
-	git branch --set-upstream @{-1} follower &&
+	git branch --set-upstream-to @{-1} from-master &&
 	git config branch.from-master.merge > actual &&
 	git config branch.from-master2.merge > actual2 &&
-	git branch --set-upstream from-master follower &&
+	git branch --set-upstream-to follower from-master &&
 	git config branch.from-master.merge > expect &&
 	test_cmp expect2 actual2 &&
 	test_cmp expect actual

-- 
2.14.0.rc1.434.g6eded367a

Signed-off-by: Kaartic Sivaraam <redacted>
---
 No changes in this one. Sending this just because of the change in the total number
 of commits.

 branch.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index ad5a2299b..a40721f3c 100644
--- a/branch.c
+++ b/branch.c

@@ -90,24 +90,24 @@ int install_branch_config(int flag, const char *local, const char *origin, const
 		if (shortname) {
 			if (origin)
 				printf_ln(rebasing ?
-					  _("Branch %s set up to track remote branch %s from %s by rebasing.") :
-					  _("Branch %s set up to track remote branch %s from %s."),
+					  _("Branch '%s' set up to track remote branch '%s' from '%s' by rebasing.") :
+					  _("Branch '%s' set up to track remote branch '%s' from '%s'."),
 					  local, shortname, origin);
 			else
 				printf_ln(rebasing ?
-					  _("Branch %s set up to track local branch %s by rebasing.") :
-					  _("Branch %s set up to track local branch %s."),
+					  _("Branch '%s' set up to track local branch '%s' by rebasing.") :
+					  _("Branch '%s' set up to track local branch '%s'."),
 					  local, shortname);
 		} else {
 			if (origin)
 				printf_ln(rebasing ?
-					  _("Branch %s set up to track remote ref %s by rebasing.") :
-					  _("Branch %s set up to track remote ref %s."),
+					  _("Branch '%s' set up to track remote ref '%s' by rebasing.") :
+					  _("Branch '%s' set up to track remote ref '%s'."),
 					  local, remote);
 			else
 				printf_ln(rebasing ?
-					  _("Branch %s set up to track local ref %s by rebasing.") :
-					  _("Branch %s set up to track local ref %s."),
+					  _("Branch '%s' set up to track local ref '%s' by rebasing.") :
+					  _("Branch '%s' set up to track local ref '%s'."),
 					  local, remote);
 		}
 	}

-- 
2.14.1.534.g641031ecb

On 17 August 2017 at 04:54, Kaartic Sivaraam
[off-list ref] wrote:

Helped-by: Martin Ågren [off-list ref],  Junio C Hamano [off-list ref]
Signed-off-by: Kaartic Sivaraam <redacted>

I didn't expect a "Helped-by", all I did was to give some random
comments. :-) I'm not so sure about the comma-separation, that seems to
be a first in the project.

    *  The option has not yet been removed from the synopsis of the documentation and I think
       we can't remove it from the 'Synopsis' porion of the documentation as it doesn't make
       sense (at least to me) to give a description of an option not listed in the synopsis.

The "git interpret-trailers --parse" thread nearby is adding some
options without mentioning them in the synopsis [1], and those options
can actually be useful, whereas "--set-upstream" only results in a fatal
error. So I don't know.

       Moreover, we have to state the reason for not supporting it in some place.

 I guess the phrase 'no longer supported' is equally communicative. Let me know if that was not
 a right decision.

I think it's ok. Of course, I know exactly what you want to say, and
why, so I'm biased. :-)

[1] https://public-inbox.org/git/20170815102334.qc4w7akl44bti44x@sigill.intra.peff.net/