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

From: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Date: Aug 15, 2017, 10:56 UTC
Message-ID: <772aaebf-81ea-ac22-9d2f-35d0778f502f@gmail.com>
In-Reply-To: <xmqqy3qluck4.fsf@gitster.mtv.corp.google.com>

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.

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 9 quoted lines

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

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

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?