Re: [RESEND PATCH] git-checkout: fix argument parsing to detect ambiguous arguments.

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 23, 2008, 10:10 UTC
Message-ID: <alpine.DEB.1.00.0807231158390.2830@eeepc-johanness>
In-Reply-To: <20080723012751.GI11831@artemis.madism.org>

Hi,

On Wed, 23 Jul 2008, Pierre Habouzit wrote:

> Note that it also fix a bug, git checkout -- <path> would be badly

s/fix/fixes/

Show 5 quoted lines

> understood as git checkout <branch> if <path> is ambiguous with a branch.
> 
> Testcases included.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>

Instead of the "Testcases included", I would have expected something like

	When calling "git checkout <name>" and <name> is both a path and a 
	branch, Git would silently assume that you meant the branch.  
	Worse, "git checkout -- <name>" would behave the same.

probably even as first paragraph.

>   This is a resend with proper patches that made me realize that my

s/patches/tests/

Show 30 quoted lines

> previous patch had silly mistakes and wasn't testing thigns properly.
> 
>  builtin-checkout.c            |   23 +++++++++++++++++------
>  t/t2010-checkout-ambiguous.sh |   35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+), 6 deletions(-)
>  create mode 100755 t/t2010-checkout-ambiguous.sh
> 
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index fbd5105..97321e6 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -438,12 +438,17 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  
>  	opts.track = git_branch_track;
>  
> -	argc = parse_options(argc, argv, options, checkout_usage, 0);
> -	if (argc) {
> +	argc = parse_options(argc, argv, options, checkout_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (argc && strcmp(argv[0], "--")) {
> +		int may_be_ambiguous = argc == 1 || strcmp(argv[1], "--");
> +
>  		arg = argv[0];
> -		if (get_sha1(arg, rev))
> -			;
> -		else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
> +		if (get_sha1(arg, rev)) {
> +			if (may_be_ambiguous)
> +				verify_filename(NULL, arg);

Still you allow "git checkout <path> --" to succeed, right? It should not.

IMHO you need "must_be_branch" and "must_not_be_path" instead of "may_be_ambiguous".

I.e. something like

		int must_be_branch = argc > 1 && !strcmp(argv[1], "--");
		int must_not_be_path = argc > 1 && strcmp(argv[1], "--");

		arg = argv[0];
		if (get_sha1(arg, rev)) {
			if (must_be_branch)
				die ("Not a branch: '%s'", arg);
		}
		else {
			if (must_not_be_path)
				verify_non_filename(NULL, arg);
			if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
				[...]

And maybe you want to add a test for "git checkout <path> --" to fail, too.

Ciao, Dscho