Re: [PATCH v2 1/2] checkout: allow dwim for branch creation for "git checkout $branch --"

[Jonathan Nieder <jrnieder@gmail.com>]

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

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.

[...]
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -863,6 +863,14 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
>  	return NULL;
>  }
>  
> +static int error_invalid_ref(const char *arg, int has_dash_dash, int argcount)
> +{
> +	if (has_dash_dash)
> +		die(_("invalid reference: %s"), arg);
> +	else
> +		return argcount;
> +}

Style: I'd leave out the 'else'

	if (has_dash_dash)
		...
	return argcount;

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.

[...]
> @@ -881,6 +889,12 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	 *   <ref> must be a valid tree, everything after the '--' must be
>  	 *   a path.
>  	 *
> +	 *   A sub-case of (1) is "git checkout <ref> --". In this
> +	 *   case, checkout behaves like case (3), except that it does
> +	 *   not attempt to understand <ref> as a file (hence, the
> +	 *   short-hand to create branch <ref> works even if <ref>
> +	 *   exists as a filename).

Maybe simpler to explain as a separate case?

	case 1: git checkout <ref> -- <paths>
	case 2: git checkout -- [<paths>]
	case 3: git checkout <something> [--]

	  If <something> is a commit, [...]

	  If <something> is _not_ a commit, either "--" is present or
	  <something> is not a path, no -t nor -b was given, and [...]

	  Otherwise, if "--" is present, treat it like case (1).

	  Otherwise behave like case (4).

	case 4: git checkout <something> <paths>

	  The first argument must not be ambiguous.
	  - If it's *only* a reference, [...]


[...]
> @@ -916,20 +930,28 @@ static int parse_branchname_arg(int argc, const char **argv,
>  	if (!strcmp(arg, "-"))
>  		arg = "@{-1}";
>  
> -	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?

> -		if (has_dash_dash)          /* case (1) */
> -			die(_("invalid reference: %s"), arg);
> -		if (dwim_new_local_branch_ok &&
> -		    !check_filename(NULL, arg) &&
> -		    argc == 1) {
> -			const char *remote = unique_tracking_name(arg, rev);
> -			if (!remote)
> -				return argcount;
> +		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))) {
			...

Then can come the "invalid reference" check for case (1):

		} else if (has_dash_dash)	/* case (1) */
			die(...);

Then case (4).

		else	/* case (4) */
			return argcount;

[...]
> --- 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?

> +	test_must_fail git rev-parse --verify refs/heads/spam &&
> +	test_branch master
> +'
> +
> +test_expect_success 'checkout <branch> -- succeeds, even if a file with the same name exists' '
> +	git checkout -B master &&
> +	test_might_fail git branch -D spam &&
> +
> +	>spam &&
> +	git checkout spam -- &&
> +	test_branch spam &&
> +	test_cmp_rev refs/remotes/extra_dir/repo_c/extra_dir/spam HEAD &&
> +	test_branch_upstream spam repo_c spam

Nice.

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

Thanks, and hope that helps,
Jonathan