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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2017, 20:19 UTC
Message-ID: <xmqqy3qluck4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170814085442.31174-1-kaarticsivaraam91196@gmail.com>

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Show 7 quoted lines

> 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.

Show 15 quoted lines

> 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.

Show 7 quoted lines

> 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.

Show 15 quoted lines

>  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.