Inconsistent behavior of the path disambiguator

Inconsistent behavior of the path disambiguator

[Dun Peal]

Hi.

When I clone a remote that has a branch `foo`, then `git checkout foo
--`, the path disamgiuator makes the operation fail. `git checkout
foo` without the disambiguator works. Following that, when branch
`foo` already exists, `git checkout foo --` works even with the
disambiguator that caused it to fail previously.

Here's a log demonstrating a full reproduction of the bug with git
1.7.2 on Linux:

=== LOG START ===

$ mkdir origin && cd origin && git init && echo 1 >> test.txt && git
add test.txt && git commit -am "First." && git branch foo
Initialized empty Git repository in /home/afine/lab/gitexp/origin/origin/.git/
[master (root-commit) 6042525] First.
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 test.txt
$ cd .. && git clone origin local && cd local
Cloning into local...
done.
$ git checkout foo --
fatal: invalid reference: foo
$ git checkout foo
Branch foo set up to track remote branch foo from origin.
Switched to a new branch 'foo'
$ git checkout master
Switched to branch 'master'
$ git checkout foo --
Switched to branch 'foo'

=== LOG END ===

This looks like a bug.

Thanks, D

Re: Inconsistent behavior of the path disambiguator

[Junio C Hamano]

Dun Peal <dunpealer@gmail.com> writes:

> When I clone a remote that has a branch `foo`, then `git checkout foo
> --`, the path disamgiuator makes the operation fail. `git checkout
> foo` without the disambiguator works. Following that, when branch
> `foo` already exists, `git checkout foo --` works even with the
> disambiguator that caused it to fail previously.

I do not think there is any bug.  You were being bitten by folks who tried
to be helpful for newbies by introducing a(n arguably confusing) special
case; I can see why this is confusing, though.

> $ git checkout foo --
> fatal: invalid reference: foo

Immediately after a clone you would have

    refs/heads/master
    refs/HEAD -> refs/heads/master
    refs/remotes/origin/foo
    refs/remotes/origin/whatever-else-you-have
    ...

and there is no commit that you can name with "foo" when asking git to
check out some paths out of, nor there is no branch that you can name with
"foo" when asking git to check out to work on it.

You can say 'origin/foo', though, in general.

> $ git checkout foo
> Branch foo set up to track remote branch foo from origin.

Asking to check out a branch "foo" in order to work on extending the
history of that branch, when there is _no_ "foo", has a special magic
invented by some folks to "help usability", if there is only one $remote
that has "foo" in it.  In this case, you have refs/remotes/origin/foo but
no other refs/remotes/$frotz/foo, and this special magic kicks in.

git behaves as if you meant "git checkout -t -b foo origin/foo" but were
too lazy to type that yourself in this case.

> Switched to a new branch 'foo'
> $ git checkout master
> Switched to branch 'master'
> $ git checkout foo --
> Switched to branch 'foo'

This is referring to local branch "foo", there is nothing magic.

Re: Inconsistent behavior of the path disambiguator

[Dun Peal]

On Fri, Oct 29, 2010 at 7:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I do not think there is any bug.  You were being bitten by folks who tried
> to be helpful for newbies by introducing a(n arguably confusing) special
> case; I can see why this is confusing, though.

Thanks.

We just switched to Git, and our developers got used to being able to
just `git checkout branch` to create a local remote-tracking one for
that remote branch.

Then we were bitten by a branch `foo` when path `./foo` existed.

I think the syntactic sugar needs to either be removed, or
(preferably) be made consistent.

The current behavior is that if I try to checkout a branch that
doesn't exist, but does exist on the remote, git creates a local
remote tracking branch for the one I tried to check out.

It makes no sense IMHO for the disambiguating syntax to break that
behavior, especially since in some cases, the disambiguating syntax is
necessary.

Thanks, D

Re: Inconsistent behavior of the path disambiguator

[Jonathan Nieder]

Junio C Hamano wrote:

>       I can see why this is confusing, though.
>
> Dun Peal <dunpealer@gmail.com> writes:
>> $ git checkout foo --
>> fatal: invalid reference: foo
>
> Immediately after a clone you would have
>
>     refs/heads/master
>     refs/HEAD -> refs/heads/master
>     refs/remotes/origin/foo
>     refs/remotes/origin/whatever-else-you-have
>     ...
>
> and there is no commit that you can name with "foo" when asking git to
> check out some paths out of, nor there is no branch that you can name with
> "foo" when asking git to check out to work on it.

How about something like this?
-- 8< --
Subject: checkout: apply Dscho's dwim even with "--" present

git reset and similar commands use -- to disambiguate between
revisions and paths on the command line.  The same syntax is not
necessary to specify a revision to git checkout (for convenience
and historical reasons, revisions are preferred over paths), but
for consistency it is accepted:

	git checkout master --; # check out master branch, not "master" file.

The autovivification of branches introduced by 70c9ac2f1 (DWIM "git
checkout frotz" to "git checkout -b frotz origin/frotz", 2009-10-18)
is currently disabled by that syntax, for no good reason.  Paranoid
scripts can still use

	git checkout --no-guess master

or even better,

	old=$(git rev-parse --verify HEAD)
	new=$(git rev-parse --verify refs/heads/master^0)
	git read-tree -m -u --exclude-standard $old $new
	git symbolic-ref -m "$me: switching branches" HEAD refs/heads/master

Requested-by: Dun Peal <dunpealer@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9240faf..1dc3640 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -771,6 +771,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	 *   <ref> must be a valid tree, everything after the '--' must be
 	 *   a path.
 	 *
+	 *   Except: with no paths, if <something> does not resolve as
+	 *   an object, no -t nor -b was given, and there is a tracking
+	 *   branch whose name is <something> in one and only one remote,
+	 *   then this is a short-hand to fork local <something> from
+	 *   that remote-tracking branch.
+	 *
 	 * case 2: git checkout -- [<paths>]
 	 *
 	 *   everything after the '--' must be paths.
@@ -808,13 +814,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			arg = "@{-1}";
 
 		if (get_sha1_mb(arg, rev)) {
-			if (has_dash_dash)          /* case (1) */
-				die("invalid reference: %s", arg);
 			if (!patch_mode &&
 			    dwim_new_local_branch &&
 			    opts.track == BRANCH_TRACK_UNSPECIFIED &&
 			    !opts.new_branch &&
-			    !check_filename(NULL, arg) &&
+			    (has_dash_dash || !check_filename(NULL, arg)) &&
 			    argc == 1) {
 				const char *remote = unique_tracking_name(arg);
 				if (!remote || get_sha1(remote, rev))
@@ -822,9 +826,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 				opts.new_branch = arg;
 				arg = remote;
 				/* DWIMmed to create local branch */
-			}
-			else
+			} else if (has_dash_dash) {	/* case (1) */
+				die("invalid reference: %s", arg);
+			} else {
 				goto no_reference;
+			}
 		}
 
 		/* we can't end up being in (2) anymore, eat the argument */

Re* Inconsistent behavior of the path disambiguator

[Junio C Hamano]

Dun Peal <dunpealer@gmail.com> writes:

> It makes no sense IMHO for the disambiguating syntax to break that
> behavior,...

Ahh, Ok, and sorry, I think I misread your original.

I agree that there shouldn't be any difference between "git co foo" and
"git co foo --" when you do not have local "foo" (i.e. immediately after
cloning from somebody who has "foo").

Perhaps something like this...

 builtin/checkout.c |   52 +++++++++++++++++++++++++++++++++-------------------
 1 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9240faf..ceaf479 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -804,17 +804,30 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		arg = argv[0];
 		has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
 
+		if (has_dash_dash) {
+			argv++;
+			argc--;
+			/*
+			 * Copy arg so that argv[] ends up being all
+			 * paths if arg turns out to be a non-ref.
+			 */
+			argv[0] = arg;
+		}
+
+		/*
+		 * arg may be a ref; or it may be the first of the
+		 * paths, if we did not see dash-dash.
+		 */
 		if (!strcmp(arg, "-"))
 			arg = "@{-1}";
 
 		if (get_sha1_mb(arg, rev)) {
-			if (has_dash_dash)          /* case (1) */
-				die("invalid reference: %s", arg);
+			/* Not a ref */
 			if (!patch_mode &&
 			    dwim_new_local_branch &&
 			    opts.track == BRANCH_TRACK_UNSPECIFIED &&
 			    !opts.new_branch &&
-			    !check_filename(NULL, arg) &&
+			    (has_dash_dash || !check_filename(NULL, arg)) &&
 			    argc == 1) {
 				const char *remote = unique_tracking_name(arg);
 				if (!remote || get_sha1(remote, rev))
@@ -823,11 +836,26 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 				arg = remote;
 				/* DWIMmed to create local branch */
 			}
-			else
+			else {
 				goto no_reference;
+			}
+		} else if (!has_dash_dash && argc > 1) {
+			/*
+			 * We saw "checkout foo bar" without dashdash,
+			 * and "foo" can name an object.  It shouldn't
+			 * be a filename. Note that by checking argc,
+			 * we do not complain in the most common case
+			 *	git checkout branch
+			 * even if there happen to be a file called 'branch';
+			 * it would be extremely annoying.
+			 */
+			verify_non_filename(NULL, arg);
 		}
 
-		/* we can't end up being in (2) anymore, eat the argument */
+		/*
+		 * It turns out that we had a ref, so argv[1:] are the
+		 * paths (we stripped dash-dash already).
+		 */
 		argv++;
 		argc--;
 
@@ -847,20 +875,6 @@ 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) */
-			/*
-			 * 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--;
-		}
 	}
 
 no_reference:

Re: Inconsistent behavior of the path disambiguator

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>>       I can see why this is confusing, though.
>>
>> Dun Peal <dunpealer@gmail.com> writes:
>>> $ git checkout foo --
>>> fatal: invalid reference: foo
>>
>> Immediately after a clone you would have
>>
>>     refs/heads/master
>>     refs/HEAD -> refs/heads/master
>>     refs/remotes/origin/foo
>>     refs/remotes/origin/whatever-else-you-have
>>     ...
>>
>> and there is no commit that you can name with "foo" when asking git to
>> check out some paths out of, nor there is no branch that you can name with
>> "foo" when asking git to check out to work on it.
>
> How about something like this?

That looks simpler than what I just did.  Can we have a few tests, too?

> -- 8< --
> Subject: checkout: apply Dscho's dwim even with "--" present
>
> git reset and similar commands use -- to disambiguate between
> revisions and paths on the command line.  The same syntax is not
> necessary to specify a revision to git checkout (for convenience
> and historical reasons, revisions are preferred over paths), but
> for consistency it is accepted:
>
> 	git checkout master --; # check out master branch, not "master" file.
>
> The autovivification of branches introduced by 70c9ac2f1 (DWIM "git
> checkout frotz" to "git checkout -b frotz origin/frotz", 2009-10-18)
> is currently disabled by that syntax, for no good reason.  Paranoid
> scripts can still use
>
> 	git checkout --no-guess master
>
> or even better,
>
> 	old=$(git rev-parse --verify HEAD)
> 	new=$(git rev-parse --verify refs/heads/master^0)
> 	git read-tree -m -u --exclude-standard $old $new
> 	git symbolic-ref -m "$me: switching branches" HEAD refs/heads/master
>
> Requested-by: Dun Peal <dunpealer@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 9240faf..1dc3640 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -771,6 +771,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  	 *   <ref> must be a valid tree, everything after the '--' must be
>  	 *   a path.
>  	 *
> +	 *   Except: with no paths, if <something> does not resolve as
> +	 *   an object, no -t nor -b was given, and there is a tracking
> +	 *   branch whose name is <something> in one and only one remote,
> +	 *   then this is a short-hand to fork local <something> from
> +	 *   that remote-tracking branch.
> +	 *
>  	 * case 2: git checkout -- [<paths>]
>  	 *
>  	 *   everything after the '--' must be paths.
> @@ -808,13 +814,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  			arg = "@{-1}";
>  
>  		if (get_sha1_mb(arg, rev)) {
> -			if (has_dash_dash)          /* case (1) */
> -				die("invalid reference: %s", arg);
>  			if (!patch_mode &&
>  			    dwim_new_local_branch &&
>  			    opts.track == BRANCH_TRACK_UNSPECIFIED &&
>  			    !opts.new_branch &&
> -			    !check_filename(NULL, arg) &&
> +			    (has_dash_dash || !check_filename(NULL, arg)) &&
>  			    argc == 1) {
>  				const char *remote = unique_tracking_name(arg);
>  				if (!remote || get_sha1(remote, rev))
> @@ -822,9 +826,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  				opts.new_branch = arg;
>  				arg = remote;
>  				/* DWIMmed to create local branch */
> -			}
> -			else
> +			} else if (has_dash_dash) {	/* case (1) */
> +				die("invalid reference: %s", arg);
> +			} else {
>  				goto no_reference;
> +			}
>  		}
>  
>  		/* we can't end up being in (2) anymore, eat the argument */

[PATCH v2] checkout: apply Dscho's dwim even with "--" present

[Jonathan Nieder]

git reset and similar commands use -- to disambiguate between
revisions and paths on the command line.  The same syntax is not
necessary to specify a revision to git checkout (for convenience
and historical reasons, revisions are preferred over paths), but
for consistency it is accepted:

	git checkout master --; # check out master branch, not "master" file.

The autovivification of branches introduced by 70c9ac2f1 (DWIM "git
checkout frotz" to "git checkout -b frotz origin/frotz", 2009-10-18)
is currently disabled by that syntax, for no good reason.  Paranoid
scripts can still use

	git checkout --no-guess master

or even better,

	old=$(git rev-parse --verify HEAD)
	new=$(git rev-parse --verify refs/heads/master^0)
	git read-tree -m -u --exclude-standard $old $new
	git symbolic-ref -m "$me: switching branches" HEAD refs/heads/master

Requested-by: Dun Peal <dunpealer@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> That looks simpler than what I just did.  Can we have a few tests, too?

Sure.  The tests are not too heavy, though (e.g., no "ambiguous
ref" stuff).

>> -			    !check_filename(NULL, arg) &&
>> +			    (has_dash_dash || !check_filename(NULL, arg)) &&
>>  			    argc == 1) {

Should have been (argc == 2) in the has_dash_dash case.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9240faf..6c3de9f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -771,6 +771,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	 *   <ref> must be a valid tree, everything after the '--' must be
 	 *   a path.
 	 *
+	 *   Except: with no paths, if <something> does not resolve as
+	 *   an object, no -t nor -b was given, and there is a tracking
+	 *   branch whose name is <something> in one and only one remote,
+	 *   then this is a short-hand to fork local <something> from
+	 *   that remote-tracking branch.
+	 *
 	 * case 2: git checkout -- [<paths>]
 	 *
 	 *   everything after the '--' must be paths.
@@ -808,23 +814,24 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			arg = "@{-1}";
 
 		if (get_sha1_mb(arg, rev)) {
-			if (has_dash_dash)          /* case (1) */
-				die("invalid reference: %s", arg);
+			trace_printf("trace: guess = %d\n", dwim_new_local_branch);
 			if (!patch_mode &&
 			    dwim_new_local_branch &&
 			    opts.track == BRANCH_TRACK_UNSPECIFIED &&
 			    !opts.new_branch &&
-			    !check_filename(NULL, arg) &&
-			    argc == 1) {
+			    (has_dash_dash || !check_filename(NULL, arg)) &&
+			    (argc == has_dash_dash ? 2 : 1)) {
 				const char *remote = unique_tracking_name(arg);
 				if (!remote || get_sha1(remote, rev))
 					goto no_reference;
 				opts.new_branch = arg;
 				arg = remote;
 				/* DWIMmed to create local branch */
-			}
-			else
+			} else if (has_dash_dash) {	/* case (1) */
+				die("invalid reference: %s", arg);
+			} else {
 				goto no_reference;
+			}
 		}
 
 		/* we can't end up being in (2) anymore, eat the argument */
diff --git a/t/t2010-checkout-ambiguous.sh b/t/t2010-checkout-ambiguous.sh
index 7cc0a35..80ac7b5 100755
--- a/t/t2010-checkout-ambiguous.sh
+++ b/t/t2010-checkout-ambiguous.sh
@@ -4,10 +4,25 @@ test_description='checkout and pathspecs/refspecs ambiguities'
 
 . ./test-lib.sh
 
+test_branch_is () {
+	echo "refs/heads/$1" >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success 'setup' '
+	git init upstream &&
+	(
+		cd upstream &&
+		test_commit upstream-commit &&
+		git branch -m upstream-topic
+	) &&
+	git remote add upstream upstream &&
+	git fetch upstream &&
 	echo hello >world &&
 	echo hello >all &&
 	git add all world &&
+	test_tick &&
 	git commit -m initial &&
 	git branch world
 '
@@ -32,6 +47,74 @@ test_expect_success 'non ambiguous call' '
 	git checkout all
 '
 
+test_expect_success "autovivification (Dscho's DWIM)" '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	git checkout upstream-topic &&
+	git diff --exit-code upstream-topic upstream/upstream-topic &&
+	test_branch_is upstream-topic
+'
+
+test_expect_success 'autovivification with --' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	git checkout upstream-topic -- &&
+	git diff --exit-code upstream-topic upstream/upstream-topic &&
+	test_branch_is upstream-topic
+'
+
+test_expect_success 'no autovivification after --' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	test_must_fail git checkout -- upstream-topic &&
+	test_branch_is master
+'
+
+test_expect_success '--no-guess defeats autovivification' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	test_must_fail git checkout --no-guess upstream-topic &&
+	test_must_fail git checkout --no-guess upstream-topic -- &&
+	test_branch_is master
+'
+
+test_expect_success 'paths defeat autovivification' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	test_must_fail git checkout upstream-topic -- . &&
+	test_must_fail git checkout upstream-topic . &&
+	test_branch_is master
+'
+
+test_expect_success 'real branch defeats autovivification' '
+	git checkout master &&
+	git update-ref refs/heads/upstream-topic HEAD^0 &&
+	git checkout upstream-topic &&
+	test_must_fail git diff --exit-code upstream-topic upstream/upstream-topic &&
+	test_branch_is upstream-topic
+'
+
+test_expect_success '-t defeats autovivification' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	test_must_fail git checkout -t upstream-topic &&
+	test_branch_is master
+'
+
+test_expect_success 'checkout of remote-tracking branch detaches HEAD' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	git checkout upstream/upstream-topic &&
+	test_must_fail git symbolic-ref HEAD
+'
+
+test_expect_success 'checkout -t of remote-tracking branch does not detach HEAD' '
+	git checkout master &&
+	test_might_fail git branch -D upstream-topic &&
+	git checkout -t upstream/upstream-topic &&
+	test_branch_is upstream-topic
+'
+
 test_expect_success 'allow the most common case' '
 	git checkout world &&
 	test "refs/heads/world" = "$(git symbolic-ref HEAD)"

Re: [PATCH v2] checkout: apply Dscho's dwim even with "--" present

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> +			    (argc == has_dash_dash ? 2 : 1)) {

Hmph --- this should read

			    (argc == (has_dash_dash ? 2 : 1)))

for correctness and sanity.

Re: [PATCH v2] checkout: apply Dscho's dwim even with "--" present

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -808,23 +814,24 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  			arg = "@{-1}";
>  
>  		if (get_sha1_mb(arg, rev)) {
> +			trace_printf("trace: guess = %d\n", dwim_new_local_branch);

Gah!  This line does not belong, sorry (though presumably it wouldn't interfere
with testing).

Re: [PATCH v2] checkout: apply Dscho's dwim even with "--" present

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder wrote:
>
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -808,23 +814,24 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>  			arg = "@{-1}";
>>  
>>  		if (get_sha1_mb(arg, rev)) {
>> +			trace_printf("trace: guess = %d\n", dwim_new_local_branch);
>
> Gah!  This line does not belong, sorry (though presumably it wouldn't interfere
> with testing).

Heh, two Gah's in a row?  After all mine might have been simpler ;-)