Jonathan Nieder <jrnieder@xxxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html