From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Thu, 26 Sep 2013 08:03:49 GMT
Subject: Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"
Message-ID: <vpq38osm2l6.fsf@anie.imag.fr>
In-Reply-To: <20130925223334.GB9464@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi,
>
> Matthieu Moy wrote:
>
>> The "--" notation disambiguates files and branches, but as a side-effect
>> of the previous implementation, also disabled the branch auto-creation
>> when $branch does not exist.
>
> Hm.  I am not sure that was just an implementation side-effect.
>
> Normally 'git checkout <branch> --' means "Check out that branch,
> and I mean it!".  'git checkout -- <pattern>' means "Check out
> these paths from the index, and I mean it!"  'git checkout <blah>'
> means "Do what I mean".

I'm not sure what was the intent, but I to me 'git checkout <branch> --'
means "I know <branch> is a ref, so don't try to interpret it
otherwise".

> On the other hand, if I want to do 'git checkout <branch> --'
> while disabling the "set up master to track origin/master" magic,
> I can use 'git checkout --no-track <branch> --'.  So I think this
> is a good change.

There's also --no-guess for that (purposely undocumented according to
the commit message of 46148dd7ea).

> More importantly, what's the contract behind this function?  Is there
> a simpler explanation than "If argument #2 is true, print a certain
> message depending on argument #1; otherwise, return argument #3?".
> If not, it might be clearer to inline it.

I made a function just because I needed the same pattern twice, and
having a function avoided overly nested if statements.

>> -	if (get_sha1_mb(arg, rev)) {
>> +	if (get_sha1_mb(arg, rev)) { /* case (1)? */
>
> The check means that we are most likely not in case (1), since arg isn't
> a commit name, right?

Not really. We're likely in an incorrect use of case 1, and want to give
an accurate error message (like "invalid reference").

>> +		int try_dwim = dwim_new_local_branch_ok;
>> +
>> +		if (check_filename(NULL, arg) && !has_dash_dash)
>> +			try_dwim = 0;
>> +		/*
>> +		 * Accept "git checkout foo" and "git checkout foo --"
>> +		 * as candidates for dwim.
>> +		 */
>> +		if (!(argc == 1 && !has_dash_dash) &&
>> +		    !(argc == 2 && has_dash_dash))
>> +			try_dwim = 0;
>> +
>> +		if (try_dwim) {
>> +			const char *remote = unique_tracking_name(arg, rev);
>> +			if (!remote)
>> +				return error_invalid_ref(arg, has_dash_dash, argcount);
>
> This could be simplified by eliminating try_dwim local.
>
> We are trying case (3) first:
>
> 		if (dwim_new_local_branch_ok &&
> 		    (argc == 1 || (argc == 2 && has_dash_dash)) &&
> 		    (has_dash_dash || !check_filename(NULL, arg))) {

I disagree that this is simpler. I introduced try_dwim precisely to
avoid this kind of monster boolean expression, and to leave room for
comments in the code.

You'd also need a "if (has_dash_dash)" inside this branch of the if to
give an accurate error message for "git checkout <non-branch> --".

> [...]
>> --- a/t/t2024-checkout-dwim.sh
>> +++ b/t/t2024-checkout-dwim.sh
>> @@ -164,4 +164,26 @@ test_expect_success 'checkout of branch from a single remote succeeds #4' '
>>  	test_branch_upstream eggs repo_d eggs
>>  '
>>  
>> +test_expect_success 'checkout of branch with a file having the same name fails' '
>> +	git checkout -B master &&
>> +	test_might_fail git branch -D spam &&
>> +
>> +	>spam &&
>> +	test_must_fail git checkout spam &&
>> +	test_must_fail git checkout spam &&
>
> Why twice?

Oops, fixed.

> Do we check that "git checkout --no-track spam --" avoids Dscho's
> DWIM?

Could be done in addition, but I have no time for this, sorry.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/