Kaartic Sivaraam <kaarticsivaraam91196@xxxxxxxxx> 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.
