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

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

[Pierre Habouzit]

Note that it also fix a bug, git checkout -- <path> would be badly
understood as git checkout <branch> if <path> is ambiguous with a branch.

Testcases included.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  A user on #git happened to have issues that made me realize that
  builtin-checkout is badly broken wrt argument parseing.

  This clearly needs to be applied to master, probably to maint too.

  The patch is against next though, but should probably apply to other
  branches just fine.


 builtin-checkout.c            |    9 +++++++--
 t/t2010-checkout-ambiguous.sh |   27 +++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)
 create mode 100755 t/t2010-checkout-ambiguous.sh

diff --git a/builtin-checkout.c b/builtin-checkout.c
index fbd5105..1490e8e 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -438,9 +438,14 @@ 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], "--")) {
 		arg = argv[0];
+
+		if (argc == 1 || strcmp(argv[1], "--"))
+			verify_non_filename(NULL, arg);
 		if (get_sha1(arg, rev))
 			;
 		else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
new file mode 100755
index 0000000..c1a86a2
--- /dev/null
+++ b/t/t2010-checkout-ambiguous.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+test_description='checkout and pathspecs/refspecs ambiguities'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hello >world &&
+	git add world &&
+	git commit -m initial &&
+	git branch world
+'
+
+test_expect_success 'branch switching' '
+	git checkout world --
+'
+
+test_expect_success 'checkout world from the index' '
+	git checkout -- world
+'
+
+test_expect_success 'ambiguous call' '
+	test_must_fail git checkout world
+'
+
+test_done
-- 
1.6.0.rc0.154.g60644

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2557 bytes --]

On Wed, Jul 23, 2008 at 01:02:20AM +0000, Pierre Habouzit wrote:
> Note that it also fix a bug, git checkout -- <path> would be badly
> understood as git checkout <branch> if <path> is ambiguous with a branch.
> 
> Testcases included.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> 
>   A user on #git happened to have issues that made me realize that
>   builtin-checkout is badly broken wrt argument parseing.
> 
>   This clearly needs to be applied to master, probably to maint too.
> 
>   The patch is against next though, but should probably apply to other
>   branches just fine.
> 
> 
>  builtin-checkout.c            |    9 +++++++--
>  t/t2010-checkout-ambiguous.sh |   27 +++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 2 deletions(-)
>  create mode 100755 t/t2010-checkout-ambiguous.sh
> 
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index fbd5105..1490e8e 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -438,9 +438,14 @@ 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], "--")) {
>  		arg = argv[0];
> +
> +		if (argc == 1 || strcmp(argv[1], "--"))
> +			verify_non_filename(NULL, arg);
>  		if (get_sha1(arg, rev))
>  			;
>  		else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
> diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
> new file mode 100755
> index 0000000..c1a86a2
> --- /dev/null
> +++ b/t/t2010-checkout-ambiguous.sh
> @@ -0,0 +1,26 @@
> +#!/bin/sh
> +
> +test_description='checkout and pathspecs/refspecs ambiguities'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	echo hello >world &&
> +	git add world &&
> +	git commit -m initial &&
> +	git branch world
> +'
> +
> +test_expect_success 'branch switching' '
> +	git checkout world --
> +'
> +
> +test_expect_success 'checkout world from the index' '
> +	git checkout -- world
> +'

  Okay those two tests are stupid in the sense that they don't check
git-checkout does what it's supposed to do. One should check the first
one outputs 'Switched to branch "world"'

  and the second should rather be:

'
  echo "bye bye" > world &&
  git checkout -- world &&
  git diff --quiet --exit-code
'


[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Johannes Schindelin]

Hi,

On Wed, 23 Jul 2008, Pierre Habouzit wrote:

> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index fbd5105..1490e8e 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -438,9 +438,14 @@ 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], "--")) {
>  		arg = argv[0];
> +
> +		if (argc == 1 || strcmp(argv[1], "--"))
> +			verify_non_filename(NULL, arg);

Why "argc == 1"?  Should "git checkout <path>" really fail?  That would be 
a change in behavior that _would_ hit me.

However, you may want to verify_non_filename() when argc == 1 _and_ 
get_sha1() succeeded.  Because then, <path> is ambiguous.

Ciao,
Dscho

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

On Wed, Jul 23, 2008 at 01:17:52AM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 23 Jul 2008, Pierre Habouzit wrote:
> 
> > diff --git a/builtin-checkout.c b/builtin-checkout.c
> > index fbd5105..1490e8e 100644
> > --- a/builtin-checkout.c
> > +++ b/builtin-checkout.c
> > @@ -438,9 +438,14 @@ 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], "--")) {
> >  		arg = argv[0];
> > +
> > +		if (argc == 1 || strcmp(argv[1], "--"))
> > +			verify_non_filename(NULL, arg);
> 
> Why "argc == 1"?  Should "git checkout <path>" really fail?  That would be 
> a change in behavior that _would_ hit me.

  No I was mistaken about what verify_non_filename did, actually I
should not code when I'm so obviously tired, and I wanted
verify_non_filename to do what I meant instead of checking what it does
;P

  I believe my resent patch is better.

> However, you may want to verify_non_filename() when argc == 1 _and_ 
> get_sha1() succeeded.  Because then, <path> is ambiguous.

  Yes and the reverse when we have sucessfully parsed something that
looks like a path as a path. Anyways, someone should carefully proofread
my resent patch, it's likely that errors slipped through given my sleep
deprivation atm.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 3041 bytes --]

Note that it also fix a bug, git checkout -- <path> would be badly
understood as git checkout <branch> if <path> is ambiguous with a branch.

Testcases included.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  This is a resend with proper patches that made me realize that my
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);
+		} else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
 			new.name = arg;
 			setup_branch_path(&new);
 			if (resolve_ref(new.path, rev, 1, NULL))
@@ -454,10 +459,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			source_tree = new.commit->tree;
 			argv++;
 			argc--;
+			if (may_be_ambiguous)
+				verify_non_filename(NULL, arg);
 		} else if ((source_tree = parse_tree_indirect(rev))) {
 			argv++;
 			argc--;
-		}
+			if (may_be_ambiguous)
+				verify_non_filename(NULL, arg);
+		} else
+			if (may_be_ambiguous)
+				verify_filename(NULL, arg);
 	}
 
 	if (argc && !strcmp(argv[0], "--")) {
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
new file mode 100755
index 0000000..389ba8c
--- /dev/null
+++ b/t/t2010-checkout-ambiguous.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+test_description='checkout and pathspecs/refspecs ambiguities'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hello >world &&
+	echo hello >all &&
+	git add all world &&
+	git commit -m initial &&
+	git branch world
+'
+
+test_expect_success 'branch switching' '
+	test "refs/heads/master" = "$(git symbolic-ref HEAD)" &&
+	git checkout world -- &&
+	test "refs/heads/world" = "$(git symbolic-ref HEAD)"
+'
+
+test_expect_success 'checkout world from the index' '
+	echo bye > world &&
+	git checkout -- world &&
+	git diff --exit-code --quiet
+'
+
+test_expect_success 'non ambiguous call' '
+	git checkout all
+'
+
+test_expect_success 'ambiguous call' '
+	test_must_fail git checkout world
+'
+
+test_done
-- 
1.6.0.rc0.154.ge2a39.dirty


[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 3747 bytes --]

On Wed, Jul 23, 2008 at 01:27:51AM +0000, Pierre Habouzit wrote:
> Note that it also fix a bug, git checkout -- <path> would be badly
> understood as git checkout <branch> if <path> is ambiguous with a branch.
> 
> Testcases included.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
> 
>   This is a resend with proper patches that made me realize that my
> 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);

  Dscho mentionned on irc that this bit is superfluous, we already know
that 'arg' cannot be a refspec here, so in the worse case, if it's not a
file at all, this will generate a curious error message about
"ambiguous" arguments, whereas it should fail with an "unknown file"
error or sth similar. This two lines should probably remain ';' like it
previously was.

  The rest looks correct to me, but it's late...

> +		} else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
>  			new.name = arg;
>  			setup_branch_path(&new);
>  			if (resolve_ref(new.path, rev, 1, NULL))
> @@ -454,10 +459,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  			source_tree = new.commit->tree;
>  			argv++;
>  			argc--;
> +			if (may_be_ambiguous)
> +				verify_non_filename(NULL, arg);
>  		} else if ((source_tree = parse_tree_indirect(rev))) {
>  			argv++;
>  			argc--;
> -		}
> +			if (may_be_ambiguous)
> +				verify_non_filename(NULL, arg);
> +		} else
> +			if (may_be_ambiguous)
> +				verify_filename(NULL, arg);
>  	}
>  
>  	if (argc && !strcmp(argv[0], "--")) {
> diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
> new file mode 100755
> index 0000000..389ba8c
> --- /dev/null
> +++ b/t/t2010-checkout-ambiguous.sh
> @@ -0,0 +1,35 @@
> +#!/bin/sh
> +
> +test_description='checkout and pathspecs/refspecs ambiguities'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	echo hello >world &&
> +	echo hello >all &&
> +	git add all world &&
> +	git commit -m initial &&
> +	git branch world
> +'
> +
> +test_expect_success 'branch switching' '
> +	test "refs/heads/master" = "$(git symbolic-ref HEAD)" &&
> +	git checkout world -- &&
> +	test "refs/heads/world" = "$(git symbolic-ref HEAD)"
> +'
> +
> +test_expect_success 'checkout world from the index' '
> +	echo bye > world &&
> +	git checkout -- world &&
> +	git diff --exit-code --quiet
> +'
> +
> +test_expect_success 'non ambiguous call' '
> +	git checkout all
> +'
> +
> +test_expect_success 'ambiguous call' '
> +	test_must_fail git checkout world
> +'
> +
> +test_done
> -- 
> 1.6.0.rc0.154.ge2a39.dirty
> 



[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Johannes Schindelin]

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/

> 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/

> 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

Resubmit after a night of sleep

[Pierre Habouzit]

Okay, so after a (too short) night sleep, here is a way better series.
The first patch corrects a real bug, and should be applied to maint.
It's a one liner, the comment is self explanatory.

The second one is a twofold UI improvement wrt error messages that
git-checkout (if we abstract the issues from patch 1) already caught as
errors but with dreadful errors. And with respect to the case where
there is no '--' and that there is an ambiguity. We used to always favor
the interpretation where the first argument is understood as a
path (which is really horrible because it's the destructive choice: at
least if we favored the reference, user would never loose local
modifications), it now barfs.

I reckon I've not checked what git-checkout did when it wasn't a
builtin, so I don't know if the second part of this UI improvement comes
as fixing a regression (in which case the patch could be considered for
maint) or if it's an ambiguity that was here like forever, in which case
it's less urgent, but should IMHO be addressed because git-checkout is
porcelain: see http://gist.github.com/1402 for the user issue that made
me discover that, you'll see we want a desambiguation one way or the
other.

[PATCH 1/2] git-checkout: fix command line parsing.

[Pierre Habouzit]

This fixes an issue when you use:

    $ git checkout -- <path1> [<paths>...]

and that <path1> can also be understood as a reference. git-checkout
mistakenly understands this as the same as:

    $ git checkout <path1> -- [<paths>...]

because parse-options was eating the '--' and the argument parser thought
he was parsing:

    $ git checkout <path1> [<paths>...]

Where there indeed is an ambiguity

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-checkout.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index fbd5105..9cadf9c 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -438,7 +438,8 @@ 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);
+	argc = parse_options(argc, argv, options, checkout_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
 	if (argc) {
 		arg = argv[0];
 		if (get_sha1(arg, rev))
-- 
1.6.0.rc0.155.g8e50b

[PATCH 2/2] git-checkout: improve error messages, detect ambiguities.

[Pierre Habouzit]

The patch is twofold: it moves the option consistency checks just under
the parse_options call so that it doesn't get in the way of the tree
reference vs. pathspecs desambiguation.

The other part rewrites the way to understand arguments so that when
git-checkout fails it does with an understandable message. Compared to the
previous behavior we now have:

  - a better error message when doing:
        git checkout <blob reference> --
    now complains about the reference not pointing to a tree, instead of
    things like:
        error: pathspec <blob reference> did not match any file(s) known to git.
        error: pathspec '--' did not match any file(s) known to git.

    you what it spitted before.

  - a better error message when doing:
        git checkout <path> --
    It now complains about <path> not being a reference instead of the
    completely obscure:
        error: pathspec '--' did not match any file(s) known to git.

  - an error when -- wasn't used and the first argument is ambiguous.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-checkout.c            |   71 +++++++++++++++++++++++++++++++----------
 t/t2010-checkout-ambiguous.sh |   39 ++++++++++++++++++++++
 2 files changed, 93 insertions(+), 17 deletions(-)
 create mode 100755 t/t2010-checkout-ambiguous.sh

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 9cadf9c..e9ae834 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -430,6 +430,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('m', NULL, &opts.merge, "merge"),
 		OPT_END(),
 	};
+	int has_dash_dash;
 
 	memset(&opts, 0, sizeof(opts));
 	memset(&new, 0, sizeof(new));
@@ -440,11 +441,52 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, options, checkout_usage,
 			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (!opts.new_branch && (opts.track != git_branch_track))
+		die("git checkout: --track and --no-track require -b");
+
+	if (opts.force && opts.merge)
+		die("git checkout: -f and -m are incompatible");
+
+	/*
+	 * case 1: git checkout <ref> -- [<paths>]
+	 *
+	 *   <ref> must be a valid tree, everything after the '--' must be
+	 *   a path.
+	 *
+	 * case 2: git checkout -- [<paths>]
+	 *
+	 *   everything after the '--' must be paths.
+	 *
+	 * case 3: git checkout <something> [<paths>]
+	 *
+	 *   <something> shall not be ambiguous.
+	 *   - If it's *only* a reference, treat it like case (1).
+	 *   - If it's only a path, treat it like case (2).
+	 *   - else: fail.
+	 *
+	 */
 	if (argc) {
+		if (!strcmp(argv[0], "--")) {       /* case (2) */
+			argv++;
+			argc--;
+			goto no_reference;
+		}
+		
 		arg = argv[0];
-		if (get_sha1(arg, rev))
-			;
-		else if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
+		has_dash_dash = argc > 1 && !strcmp(argv[1], "--");
+
+		if (get_sha1(arg, rev)) {
+			if (has_dash_dash)          /* case (1) */
+				die("invalid reference: %s", arg);
+			goto no_reference;          /* case (3 -> 2) */
+		}
+
+		/* we can't end up being in (2) anymore, eat the argument */
+		argv++;
+		argc--;
+
+		if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
 			new.name = arg;
 			setup_branch_path(&new);
 			if (resolve_ref(new.path, rev, 1, NULL))
@@ -453,25 +495,20 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 				new.path = NULL;
 			parse_commit(new.commit);
 			source_tree = new.commit->tree;
-			argv++;
-			argc--;
-		} else if ((source_tree = parse_tree_indirect(rev))) {
+		} else
+			source_tree = parse_tree_indirect(rev);
+
+		if (!source_tree)                   /* case (1): want a tree */
+			die("reference is not a tree: %s", arg);
+		if (!has_dash_dash)                 /* case (3 -> 1) */
+			verify_non_filename(NULL, arg);
+		else {
 			argv++;
 			argc--;
 		}
 	}
 
-	if (argc && !strcmp(argv[0], "--")) {
-		argv++;
-		argc--;
-	}
-
-	if (!opts.new_branch && (opts.track != git_branch_track))
-		die("git checkout: --track and --no-track require -b");
-
-	if (opts.force && opts.merge)
-		die("git checkout: -f and -m are incompatible");
-
+no_reference:
 	if (argc) {
 		const char **pathspec = get_pathspec(prefix, argv);
 
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
new file mode 100755
index 0000000..50d1f43
--- /dev/null
+++ b/t/t2010-checkout-ambiguous.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='checkout and pathspecs/refspecs ambiguities'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hello >world &&
+	echo hello >all &&
+	git add all world &&
+	git commit -m initial &&
+	git branch world
+'
+
+test_expect_success 'reference must be a tree' '
+	test_must_fail git checkout $(git hash-object ./all) --
+'
+
+test_expect_success 'branch switching' '
+	test "refs/heads/master" = "$(git symbolic-ref HEAD)" &&
+	git checkout world -- &&
+	test "refs/heads/world" = "$(git symbolic-ref HEAD)"
+'
+
+test_expect_success 'checkout world from the index' '
+	echo bye > world &&
+	git checkout -- world &&
+	git diff --exit-code --quiet
+'
+
+test_expect_success 'non ambiguous call' '
+	git checkout all
+'
+
+test_expect_success 'ambiguous call' '
+	test_must_fail git checkout world
+'
+
+test_done
-- 
1.6.0.rc0.155.g8e50b

Re: [PATCH 2/2] git-checkout: improve error messages, detect ambiguities.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> The patch is twofold: it moves the option consistency checks just under
> the parse_options call so that it doesn't get in the way of the tree
> reference vs. pathspecs desambiguation.

I think this goes a bit too far.

Even if you have a file called 'master' tracked in your project, when you
say:

    $ git checkout master

that's almost always branch switching.  Forcing "git checkout master --"
disambiguation for such a common case is simply a wrong thing to do from
the usability point of view.

So how about (obviously we are interested only in the case without
disambiguating '--' here):

    (3-1) if there is only one token left and if it is a rev, that's the
          branch to check out or commit to detach to.

    (3-2) otherwise the user might have mistyped one of the paths, so help
          avoiding by making sure the first token is unambiguously either
          a rev or a path (but not both).

Re: [PATCH 2/2] git-checkout: improve error messages, detect ambiguities.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Pierre Habouzit <madcoder@debian.org> writes:
>
>> The patch is twofold: it moves the option consistency checks just under
>> the parse_options call so that it doesn't get in the way of the tree
>> reference vs. pathspecs desambiguation.
>
> I think this goes a bit too far.
>
> Even if you have a file called 'master' tracked in your project, when you
> say:
>
>     $ git checkout master
>
> that's almost always branch switching.  Forcing "git checkout master --"
> disambiguation for such a common case is simply a wrong thing to do from
> the usability point of view.

So something like this on top of your patch.

 builtin-checkout.c            |   19 +++++++++++++++----
 t/t2010-checkout-ambiguous.sh |   15 +++++++++++++--
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index d99c1c0..411cc51 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -460,7 +460,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	 *
 	 * case 3: git checkout <something> [<paths>]
 	 *
-	 *   <something> shall not be ambiguous.
+	 *   With no paths, if <something> is a commit, that is to
+	 *   switch to the branch or detach HEAD at it.
+	 *
+	 *   Otherwise <something> shall not be ambiguous.
 	 *   - If it's *only* a reference, treat it like case (1).
 	 *   - If it's only a path, treat it like case (2).
 	 *   - else: fail.
@@ -474,7 +477,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		}
 
 		arg = argv[0];
-		has_dash_dash = argc > 1 && !strcmp(argv[1], "--");
+		has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
 
 		if (get_sha1(arg, rev)) {
 			if (has_dash_dash)          /* case (1) */
@@ -500,8 +503,16 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 
 		if (!source_tree)                   /* case (1): want a tree */
 			die("reference is not a tree: %s", arg);
-		if (!has_dash_dash)                 /* case (3 -> 1) */
-			verify_non_filename(NULL, arg);
+		if (!has_dash_dash) {/* case (3 -> 1) */
+			/*
+			 * Do not complain the most common case
+			 *	git checkout branch
+			 * even if there happen to be a file called 'branch';
+			 * it would be extremely annoying.
+			 */
+			if (argc)
+				verify_non_filename(NULL, arg);
+		}
 		else {
 			argv++;
 			argc--;
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 50d1f43..7cc0a35 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -32,8 +32,19 @@ test_expect_success 'non ambiguous call' '
 	git checkout all
 '
 
-test_expect_success 'ambiguous call' '
-	test_must_fail git checkout world
+test_expect_success 'allow the most common case' '
+	git checkout world &&
+	test "refs/heads/world" = "$(git symbolic-ref HEAD)"
+'
+
+test_expect_success 'check ambiguity' '
+	test_must_fail git checkout world all
+'
+
+test_expect_success 'disambiguate checking out from a tree-ish' '
+	echo bye > world &&
+	git checkout world -- world &&
+	git diff --exit-code --quiet
 '
 
 test_done

Re: [PATCH 2/2] git-checkout: improve error messages, detect ambiguities.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]

On Wed, Jul 23, 2008 at 11:04:08PM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > The patch is twofold: it moves the option consistency checks just under
> > the parse_options call so that it doesn't get in the way of the tree
> > reference vs. pathspecs desambiguation.
> 
> I think this goes a bit too far.
> 
> Even if you have a file called 'master' tracked in your project, when you
> say:
> 
>     $ git checkout master
> 
> that's almost always branch switching.  Forcing "git checkout master --"
> disambiguation for such a common case is simply a wrong thing to do from
> the usability point of view.
> 
> So how about (obviously we are interested only in the case without
> disambiguating '--' here):
> 
>     (3-1) if there is only one token left and if it is a rev, that's the
>           branch to check out or commit to detach to.
> 
>     (3-2) otherwise the user might have mistyped one of the paths, so help
>           avoiding by making sure the first token is unambiguously either
>           a rev or a path (but not both).

  It sounds really reasonable, and your patch seems really fine.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] checkout: mention '--' in the docs

[SZEDER Gábor]

'git checkout' uses '--' to separate options from paths, but it was not
mentioned in the documentation

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
And here are two small changes related to 'git checkout --'.

 Documentation/git-checkout.txt |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 2abfbda..5aa69c0 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git checkout' [-q] [-f] [[--track | --no-track] -b <new_branch> [-l]] [-m] [<branch>]
-'git checkout' [<tree-ish>] <paths>...
+'git checkout' [<tree-ish>] [--] <paths>...
 
 DESCRIPTION
 -----------
-- 
1.6.0.rc0.35.gb83a7

[PATCH] bash: offer only paths after '--' for 'git checkout'

[SZEDER Gábor]

Commit d773c631 (bash: offer only paths after '--', 2008-07-08) did the
same for several other git commands, but 'git checkout' went unnoticed.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2edb341..9b51fda 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -626,6 +626,8 @@ _git_bundle ()
 
 _git_checkout ()
 {
+	__git_has_doubledash && return
+
 	__gitcomp "$(__git_refs)"
 }
 
-- 
1.6.0.rc0.35.gb83a7

Re: [PATCH] bash: offer only paths after '--' for 'git checkout'

[Shawn O. Pearce]

SZEDER GGGbor <szeder@ira.uka.de> wrote:
> Commit d773c631 (bash: offer only paths after '--', 2008-07-08) did the
> same for several other git commands, but 'git checkout' went unnoticed.
> 
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 2edb341..9b51fda 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -626,6 +626,8 @@ _git_bundle ()
>  
>  _git_checkout ()
>  {
> +	__git_has_doubledash && return
> +
>  	__gitcomp "$(__git_refs)"
>  }
>  

-- 
Shawn.