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

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 0f57397..9edd9c3 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -863,6 +863,13 @@ 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);
> +	return argcount;
> +}

This is somewhat unfortunate; it pretends to be a reusable helper by
being a separate function, but it is not very reusable (see below).

> @@ -917,19 +934,32 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		arg = "@{-1}";
>  
>  	if (get_sha1_mb(arg, rev)) {
> +		/*
> +		 * Either case (3) or (4), with <something> not being
> +		 * a commit, or an attempt to use case (1) with an
> +		 * invalid ref.
> +		 */
> +		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);

Up to this point, the updated code makes very good sense.

>  			if (!remote)
> -				return argcount;
> +				return error_invalid_ref(arg, has_dash_dash, argcount);

The original that returned "argcount" from here were unnecessarily
misleading in the first place. It saw "git checkout foo" where "foo"
does not refer to an object nor a filesystem entity and there was no
unique "refs/remotes/*/foo"; it wanted to return 0 to tell the
caller that it consumed zero arguments as branch names.

And the updated code is even more obscure.  This calling site makes
it look as if it is an error to have no unique "refs/remotes/*/foo"
at this point of the code by naming the helper function "error_*()",
but it is an error in some case and not in others.

                if (!remote) {
                        if (has_dash_dash)
                                die(_("..."));
                        return 0;
                }

would be a lot more understandable.
		
The only reason you have conditional die() here (and on the "else"
side of this "if" statement) is because you delayed the die that was
at a much earlier point in the original.  And the only reason you
created the unfortunate helper function is because you need to deal
with that delayed decision to die now in two places.

So it may be even cleaner to read if you did it this way:

	if (get_sha1_mb(...)) {
		/*
		 * The first token is not a valid rev; we should
		 * ordinarily error out if "git checkout foo --"
		 * if foo is not a valid rev, but first see if
		 * we can auto-create foo to continue...
		 */
		int recover_with_dwim = dwim_new_local_branch_ok;

		... decide if we want to recover_with_dwim ...

		if (recover_with_dwim) {
			const char *remote = unique_tracking_name(arg, rev);
			if (remote) {
				*new_branch = arg;
				arg = remote;
			} else {
				/* no; arg cannot be salvaged */
				recover_with_dwim = 0;
			}
		}

		if (!recover_with_dwim) {
			if (has_dash_dash)
				die(_("invalid ref %s", arg);
			return 0; /* we saw no branch/commit */
		}
		/* otherwise we made a successful recovery */
	}