bug? in checkout with ambiguous refnames

bug? in checkout with ambiguous refnames

[Uwe Kleine-König]

Hello,

everything is clean:

	ukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached

	ukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart
	warning: refname 'sgu/mxs-amba-uart' is ambiguous.
	Previous HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD
	Switched to branch 'sgu/mxs-amba-uart'

OK, it might be a bad idea to have this ambiguity, still ...

	ukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached --stat
	 arch/arm/mach-mxs/Kconfig                       |    2 ++
	 arch/arm/mach-mxs/clock-mx23.c                  |    2 +-
	 arch/arm/mach-mxs/clock-mx28.c                  |    2 +-
	 arch/arm/mach-mxs/devices-mx23.h                |    2 +-
	 arch/arm/mach-mxs/devices-mx28.h                |    2 +-
	 arch/arm/mach-mxs/devices.c                     |   17 ++---------------
	 arch/arm/mach-mxs/devices/Kconfig               |    1 -
	 arch/arm/mach-mxs/devices/amba-duart.c          |   15 +++++++--------
	 arch/arm/mach-mxs/include/mach/devices-common.h |    4 +---
	 9 files changed, 16 insertions(+), 31 deletions(-)
	ukl@octopus:~/gsrc/linux-2.6$ git diff refs/tags/sgu/mxs-amba-uart
	ukl@octopus:~/gsrc/linux-2.6$ git diff --cached refs/tags/sgu/mxs-amba-uart

So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
points to refs/heads/sgu/mxs-amba-uart

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: bug? in checkout with ambiguous refnames

[Junio C Hamano]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:

> So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
> points to refs/heads/sgu/mxs-amba-uart

I somehow thought that we had an explicit logic to favor an exact branch
name for "git checkout $branch" even when refs/something-other-than-head/$branch 
exists, while issuing the ambiguity warning.

Ahh, what this part of the code in builtin/checkout.c does is totally
wrong:

	/* we can't end up being in (2) anymore, eat the argument */
	argv++;
	argc--;

	new.name = arg;
	if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
		setup_branch_path(&new);

		if ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&
		    resolve_ref(new.path, rev, 1, NULL))
			;
		else
			new.path = NULL;
		parse_commit(new.commit);
		source_tree = new.commit->tree;
	} else
		source_tree = parse_tree_indirect(rev);

It uses lookup_commit_reference_gently() that follows the usual
tags-then-heads preference order, but then uses setup_branch_path() to
prefix the raw name with "refs/heads", which is totally backwards.
It should do something like:

 - use setup-branch-path to get refs/heads/$name

 - check-ref-format and resolve it; if these fail, then we are detaching
   head at rev;

 - otherwise, if the result of the resolution is not the same as rev, what
   we have in rev is incorrect (it was taken from the usual
   tags-then-heads rule but "checkout $name" must favor local branches).
   update the rev with the result of resolving refs/heads/$name

 - parse new.commit out of rev.  we are checking out the branch $name.

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Fri, Jan 07, 2011 at 11:46:50AM +0100, Uwe Kleine-König wrote:

> 	ukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached
> 
> 	ukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart
> 	warning: refname 'sgu/mxs-amba-uart' is ambiguous.
> 	Previous HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD
> 	Switched to branch 'sgu/mxs-amba-uart'
> 
> OK, it might be a bad idea to have this ambiguity, still ...
> [...]
> So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
> points to refs/heads/sgu/mxs-amba-uart

Yeah, we generally resolve ambiguities in favor of the tag (and that
warning comes from deep within get_sha1_basic). So the real bug here is
that it still said "Switched to branch", which is totally wrong.

That being said, it probably would make more sense for "git checkout" to
prefer branches to tags. That's probably going to take a lot more
surgery, and we're in -rc right now. So I think the best thing to do is
to fix the broken message and add some tests, and then if somebody wants
to revisit it with a larger patch, they can do so on top.

I'll work on the first part and post a patch in a few minutes.

-Peff

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Fri, Jan 07, 2011 at 02:49:09PM -0500, Jeff King wrote:

> That being said, it probably would make more sense for "git checkout" to
> prefer branches to tags. That's probably going to take a lot more
> surgery, and we're in -rc right now. So I think the best thing to do is
> to fix the broken message and add some tests, and then if somebody wants
> to revisit it with a larger patch, they can do so on top.
> 
> I'll work on the first part and post a patch in a few minutes.

Ah, never mind. After reading Junio's response, it looks like we already
try to do the right thing in checkout, but it's just broken. So forget
my two-step plan.

Here is the test script I worked out which shows the issue (and checks
that the right messages are shown to the user):

---
 t/t2019-checkout-amiguous-ref.sh |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100755 t/t2019-checkout-amiguous-ref.sh

diff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh
new file mode 100755
index 0000000..12edce8
--- /dev/null
+++ b/t/t2019-checkout-amiguous-ref.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='checkout handling of ambiguous (branch/tag) refs'
+. ./test-lib.sh
+
+test_expect_success 'setup ambiguous refs' '
+	test_commit branch file &&
+	git branch ambiguity &&
+	test_commit tag file &&
+	git tag ambiguity &&
+	test_commit other file
+'
+
+test_expect_success 'checkout ambiguous ref succeeds' '
+	git checkout ambiguity >stdout 2>stderr
+'
+
+test_expect_success 'checkout produces ambiguity warning' '
+	grep "warning.*ambiguous" stderr
+'
+
+test_expect_failure 'checkout chooses branch over tag' '
+	echo branch >expect &&
+	test_cmp expect file
+'
+
+test_expect_success 'checkout reports switch to detached HEAD' '
+	grep "Switched to branch" stderr &&
+	! grep "^HEAD is now at" stderr
+'
+
+test_done
-- 
1.7.4.rc1.23.g84303

Re: bug? in checkout with ambiguous refnames

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here is the test script I worked out which shows the issue (and checks
> that the right messages are shown to the user):

This is a band-aid that is not quite right (even though it passes the
tests).  Attempting to check out a branch "frotz" in a repository with a
tag "frotz" that point at a non-commit would still fail, which is not a
new problem.


 builtin/checkout.c               |    7 +++++++
 t/t2019-checkout-amiguous-ref.sh |   23 ++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index a54583b..f6fea2f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -840,6 +840,13 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 				;
 			else
 				new.path = NULL;
+			if (hashcmp(new.commit->object.sha1, rev))
+				/*
+				 * Yikes, arg is an ambiguous and higher
+				 * precedence SHA-1 expression than the
+				 * branch name
+				 */
+				new.commit = lookup_commit_reference_gently(rev, 1);
 			parse_commit(new.commit);
 			source_tree = new.commit->tree;
 		} else
diff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh
index 0981f11..fa1d4e6 100755
--- a/t/t2019-checkout-amiguous-ref.sh
+++ b/t/t2019-checkout-amiguous-ref.sh
@@ -6,8 +6,10 @@ test_description='checkout handling of ambiguous (branch/tag) refs'
 test_expect_success 'setup ambiguous refs' '
 	test_commit branch file &&
 	git branch ambiguity &&
+	git branch vagueness &&
 	test_commit tag file &&
 	git tag ambiguity &&
+	git tag vagueness HEAD:file &&
 	test_commit other file
 '
 
@@ -19,7 +21,7 @@ test_expect_success 'checkout produces ambiguity warning' '
 	grep "warning.*ambiguous" stderr
 '
 
-test_expect_failure 'checkout chooses branch over tag' '
+test_expect_success 'checkout chooses branch over tag' '
 	echo branch >expect &&
 	test_cmp expect file
 '
@@ -29,4 +31,23 @@ test_expect_success 'checkout reports switch to detached HEAD' '
 	! grep "^HEAD is now at" stderr
 '
 
+test_expect_failure 'checkout vague ref succeeds' '
+	git checkout vagueness >stdout 2>stderr &&
+	test_set_prereq VAGUENESS_SUCCESS
+'
+
+test_expect_success VAGUENESS_SUCCESS 'checkout produces ambiguity warning' '
+	grep "warning.*ambiguous" stderr
+'
+
+test_expect_success VAGUENESS_SUCCESS 'checkout chooses branch over tag' '
+	echo branch >expect &&
+	test_cmp expect file
+'
+
+test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to detached HEAD' '
+	grep "Switched to branch" stderr &&
+	! grep "^HEAD is now at" stderr
+'
+
 test_done

Re: bug? in checkout with ambiguous refnames

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> Here is the test script I worked out which shows the issue (and checks
>> that the right messages are shown to the user):
>
> This is a band-aid that is not quite right (even though it passes the
> tests).  Attempting to check out a branch "frotz" in a repository with a
> tag "frotz" that point at a non-commit would still fail, which is not a
> new problem.
>
>
>  builtin/checkout.c               |    7 +++++++
>  t/t2019-checkout-amiguous-ref.sh |   23 ++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletions(-)

... And this comes on top (should probably be squashed into one) to really
favor a branch over a tag.

 builtin/checkout.c               |   26 ++++++++++----------------
 t/t2019-checkout-amiguous-ref.sh |    2 +-
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f6fea2f..48e547b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -832,25 +832,19 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		argc--;
 
 		new.name = arg;
-		if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
-			setup_branch_path(&new);
+		setup_branch_path(&new);
 
-			if ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&
-			    resolve_ref(new.path, rev, 1, NULL))
-				;
-			else
-				new.path = NULL;
-			if (hashcmp(new.commit->object.sha1, rev))
-				/*
-				 * Yikes, arg is an ambiguous and higher
-				 * precedence SHA-1 expression than the
-				 * branch name
-				 */
-				new.commit = lookup_commit_reference_gently(rev, 1);
+		if ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||
+		    !resolve_ref(new.path, rev, 1, NULL))
+			new.path = NULL; /* not an existing branch */
+
+		if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {
+			/* not a commit */
+			source_tree = parse_tree_indirect(rev);
+		} else {
 			parse_commit(new.commit);
 			source_tree = new.commit->tree;
-		} else
-			source_tree = parse_tree_indirect(rev);
+		}
 
 		if (!source_tree)                   /* case (1): want a tree */
 			die("reference is not a tree: %s", arg);
diff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh
index fa1d4e6..e2b330b 100755
--- a/t/t2019-checkout-amiguous-ref.sh
+++ b/t/t2019-checkout-amiguous-ref.sh
@@ -31,7 +31,7 @@ test_expect_success 'checkout reports switch to detached HEAD' '
 	! grep "^HEAD is now at" stderr
 '
 
-test_expect_failure 'checkout vague ref succeeds' '
+test_expect_success 'checkout vague ref succeeds' '
 	git checkout vagueness >stdout 2>stderr &&
 	test_set_prereq VAGUENESS_SUCCESS
 '

Re: bug? in checkout with ambiguous refnames

[Martin von Zweigbergk]

On Fri, 7 Jan 2011, Jeff King wrote:

> On Fri, Jan 07, 2011 at 11:46:50AM +0100, Uwe Kleine-K?nig wrote:
> 
> > 	ukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached
> > 
> > 	ukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart
> > 	warning: refname 'sgu/mxs-amba-uart' is ambiguous.
> > 	Previous HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD
> > 	Switched to branch 'sgu/mxs-amba-uart'
> > 
> > OK, it might be a bad idea to have this ambiguity, still ...
> > [...]
> > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
> > points to refs/heads/sgu/mxs-amba-uart
> 
> Yeah, we generally resolve ambiguities in favor of the tag (and that
> warning comes from deep within get_sha1_basic). So the real bug here is
> that it still said "Switched to branch", which is totally wrong.
> 
> That being said, it probably would make more sense for "git checkout" to
> prefer branches to tags.

What was the rationale for generally favoring tags? Why does that
reasoning not apply to 'git checkout' too? At least without knowing
the answer to that question, I think I would prefer to have checkout
behave the same as the other commands do. Maybe a bit academic, but it
seems like it would be nice if e.g. 'git checkout $anything && git
show $anything' would show the HEAD commit and if 'git checkout
$anything && git diff HEAD $anything' was always empty.

Btw, what exactly does "generally" mean, i.e. which other commands
don't favor tags? I know rebase is one example of a command that does
not favor tags.

Slightly off topic, but why does 'git rev-parse --symbolic-full-name'
not output anything when the input is ambiguous? 'git rev-parse'
without any flags favors tags, so I would have expected to get
something like refs/tags/$name back.

The reason I'm asking is because I just happened to see in the rebase
code the other day that it will rebase a detached head if the <branch>
parameter is not "completely unqualified". For example 'git rebase
master heads/topic' or 'git rebase master refs/heads/topic' will not
update refs/heads/topic. I was trying to fix that by using 'git
rev-parse --symbolic-full-name' to parse <branch>. That seemed to work
fine until I saw this thread :-).

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:

> > Yeah, we generally resolve ambiguities in favor of the tag (and that
> > warning comes from deep within get_sha1_basic). So the real bug here is
> > that it still said "Switched to branch", which is totally wrong.
> > 
> > That being said, it probably would make more sense for "git checkout" to
> > prefer branches to tags.
> 
> What was the rationale for generally favoring tags?

I don't recall hearing any specific argument, but it has always been
that way from early on. I think it is from a vague sense of "tags are
more important than branch tips because they are about marking specific
points, not lines of development". But maybe other old-timers can say
more.

I don't necessarily buy that argument; my only reasoning is that we
should probably keep historic behavior.

> Why does that reasoning not apply to 'git checkout' too?

Because checkout has always been fundamentally about branches. It did
end up growing sane behavior for "git checkout tag" (i.e., a detached
HEAD), but branches are still the fundamental unit for most of its
arguments.

> Btw, what exactly does "generally" mean, i.e. which other commands
> don't favor tags? I know rebase is one example of a command that does
> not favor tags.

It means "we favor tags in resolve_ref, which is the underlying
machinery for most commands, so unless a command special-cases it, that
will be the behavior, and I am too lazy to exhaustively search for such
special cases".

> Slightly off topic, but why does 'git rev-parse --symbolic-full-name'
> not output anything when the input is ambiguous? 'git rev-parse'
> without any flags favors tags, so I would have expected to get
> something like refs/tags/$name back.

I dunno. I never tried it, but I would have expected to get the tag-name
back.

> The reason I'm asking is because I just happened to see in the rebase
> code the other day that it will rebase a detached head if the <branch>
> parameter is not "completely unqualified". For example 'git rebase
> master heads/topic' or 'git rebase master refs/heads/topic' will not
> update refs/heads/topic. I was trying to fix that by using 'git
> rev-parse --symbolic-full-name' to parse <branch>. That seemed to work
> fine until I saw this thread :-).

Heh. I think that would be an argument in favor of changing rev-parse's
behavior.

-Peff

Re: bug? in checkout with ambiguous refnames

[Martin von Zweigbergk]

On Sat, 8 Jan 2011, Jeff King wrote:

> On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:
> 
> > > Yeah, we generally resolve ambiguities in favor of the tag (and that
> > > warning comes from deep within get_sha1_basic). So the real bug here is
> > > that it still said "Switched to branch", which is totally wrong.
> > > 
> > > That being said, it probably would make more sense for "git checkout" to
> > > prefer branches to tags.
> > 
> > What was the rationale for generally favoring tags?
> 
> I don't recall hearing any specific argument, but it has always been
> that way from early on. I think it is from a vague sense of "tags are
> more important than branch tips because they are about marking specific
> points, not lines of development". But maybe other old-timers can say
> more.
> 
> I don't necessarily buy that argument; my only reasoning is that we
> should probably keep historic behavior.
> 
> > Why does that reasoning not apply to 'git checkout' too?
> 
> Because checkout has always been fundamentally about branches. It did
> end up growing sane behavior for "git checkout tag" (i.e., a detached
> HEAD), but branches are still the fundamental unit for most of its
> arguments.

I had a look at the history of the lines Junio mentioned in another
message on this thread (lookup_commit_reference_gently() and
setup_branch_path()). I don't understand the code, but just looking at
where the lines came from, they seem to have been there ever since
782c2d6 (Build in checkout, 2008-02-07). If that is correct (but
please check for yourselves), it seems like the broken checkout of
ambiguous references causes problems rarely enough that no one has
bothered to report it for almost two years. If it has been broken for
that long, it seems to me like we could resolve it whichever way makes
most sense, without much concern to how it used to work, no?

> > Btw, what exactly does "generally" mean, i.e. which other commands
> > don't favor tags? I know rebase is one example of a command that does
> > not favor tags.
> 
> It means "we favor tags in resolve_ref, which is the underlying
> machinery for most commands, so unless a command special-cases it, that
> will be the behavior, and I am too lazy to exhaustively search for such
> special cases".

Sensible definition :-). I was just hoping for an example where it
more obviously makes sense to favor branches. Maybe rebase is actually
one of the better examples.

> > The reason I'm asking is because I just happened to see in the rebase
> > code the other day that it will rebase a detached head if the <branch>
> > parameter is not "completely unqualified". For example 'git rebase
> > master heads/topic' or 'git rebase master refs/heads/topic' will not
> > update refs/heads/topic. I was trying to fix that by using 'git
> > rev-parse --symbolic-full-name' to parse <branch>. That seemed to work
> > fine until I saw this thread :-).
> 
> Heh. I think that would be an argument in favor of changing rev-parse's
> behavior.

I was hoping to do something like

head_name=$(git rev-parse --symbolic-full-name -q --verify "$1")
case "$head_name" in
refs/heads/*) ;;
*) head_name="detached HEAD" ;;
esac

plus setting another variable or two in each case. So even if 'git
rev-parse --symbolic-full-name' would return refs/tags/$name, it
wouldn't actually help here, since rebase currently favors
branches. In order to keep that behavior, I will need to add an extra
check before the above code.

It still feels like rev-parse should return refs/tags/$name, though.

Re: bug? in checkout with ambiguous refnames

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:
>
>> > Yeah, we generally resolve ambiguities in favor of the tag (and that
>> > warning comes from deep within get_sha1_basic). So the real bug here is
>> > that it still said "Switched to branch", which is totally wrong.
>> > 
>> > That being said, it probably would make more sense for "git checkout" to
>> > prefer branches to tags.
>> 
>> What was the rationale for generally favoring tags?
>
> I don't recall hearing any specific argument, but it has always been
> that way from early on. I think it is from a vague sense of "tags are
> more important than branch tips because they are about marking specific
> points, not lines of development". But maybe other old-timers can say
> more.
>
> I don't necessarily buy that argument; my only reasoning is that we
> should probably keep historic behavior.

I don't think "tags are more important" has ever been a serious argument,
either.  We prefix refs/tags/ and refs/heads/ to see if what the user gave
us is a short hand, and we have to pick one to check first, and we
happened to have chosen to check tags/ before heads/.  Majority of people
have been trained by the ambiguity warning not to use the same name for
their tags and branches, and the rest have learned to live with this
convention.

Among those "rest who have learned to live with" minority are people who
use v1.0 branch to maintain v1.0 codebase after it is tagged, and they
would want to work on v1.0 branch (by checking out v1.0 branch) and
measure their progress by disambiguating between heads/v1.0 and tags/v1.0
when driving "git log" family.  There is no strong reason to forbid them
from doing this by requiring uniqueness if that is what they want,
although I personally would suggest them to use maint-1.0 branch that
forks from v1.0 tag.

Aside from your "'checkout branch' is about checking out a branch"
explanation, there are two reasons to favor branches over tags in
"checkout" command:

 (1) You cannot disambiguate "git checkout heads/master" when you have
     "master" tag, as this notation is used to tell the command "I want to
     detach HEAD at that commit"; and

 (2) The command already treats an unadorned branch name specially by not
     complaining ref/path ambiguity when you said "git checkout master"
     and you have a file called "master" in your working tree, so users
     already expect that an unadorned branch name is special to it.

Re: bug? in checkout with ambiguous refnames

[Martin von Zweigbergk]

On Sat, 8 Jan 2011, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:
> >
> >> > Yeah, we generally resolve ambiguities in favor of the tag (and that
> >> > warning comes from deep within get_sha1_basic). So the real bug here is
> >> > that it still said "Switched to branch", which is totally wrong.
> >> > 
> >> > That being said, it probably would make more sense for "git checkout" to
> >> > prefer branches to tags.
> >> 
> >> What was the rationale for generally favoring tags?
> >
> > I don't recall hearing any specific argument, but it has always been
> > that way from early on. I think it is from a vague sense of "tags are
> > more important than branch tips because they are about marking specific
> > points, not lines of development". But maybe other old-timers can say
> > more.
> >
> Aside from your "'checkout branch' is about checking out a branch"
> explanation, there are two reasons to favor branches over tags in
> "checkout" command:
> 
>  (1) You cannot disambiguate "git checkout heads/master" when you have
>      "master" tag, as this notation is used to tell the command "I want to
>      detach HEAD at that commit"; and

Interesting. I had no idea that 'git checkout heads/master' means to
detach the HEAD. Thanks.

By analogy, I guess that means that 'git rebase master heads/topic' is
supposed to rebase a detached HEAD, so I will stop trying to "fix"
that then :-)

>  (2) The command already treats an unadorned branch name specially by not
>      complaining ref/path ambiguity when you said "git checkout master"
>      and you have a file called "master" in your working tree, so users
>      already expect that an unadorned branch name is special to it.

If I understand correctly, that actually applies to tags as well.
Checking out a tag called e.g. Makefile doesn't give any warnings or
errors.


/Martin

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Fri, Jan 07, 2011 at 03:17:22PM -0800, Junio C Hamano wrote:

> ... And this comes on top (should probably be squashed into one) to really
> favor a branch over a tag.
> 
>  builtin/checkout.c               |   26 ++++++++++----------------
>  t/t2019-checkout-amiguous-ref.sh |    2 +-
>  2 files changed, 11 insertions(+), 17 deletions(-)

Yeah, that looks sane to me (assuming all three patches squashed
together). It took me a minute to figure out one subtlety, though:

> +		if ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||
> +		    !resolve_ref(new.path, rev, 1, NULL))
> +			new.path = NULL; /* not an existing branch */
> +
> +		if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {

We are relying on the fact that resolve_ref leaves "rev" alone in the
case that it does not find anything. Which is mostly true (the only
exception seems to be if you have a ref with non-hex garbage in it, in
which case you will get some bogus sha1 in the output). I dunno if it is
worth making it more explicit, like:

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f6f6172..afff56f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -678,7 +678,7 @@ static const char *unique_tracking_name(const char *name)
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
-	unsigned char rev[20];
+	unsigned char rev[20], branch_rev[20];
 	const char *arg;
 	struct branch_info new;
 	struct tree *source_tree = NULL;
@@ -834,8 +834,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		new.name = arg;
 		setup_branch_path(&new);
 
-		if ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||
-		    !resolve_ref(new.path, rev, 1, NULL))
+		if ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&
+		     resolve_ref(new.path, branch_rev, 1, NULL))
+			hashcpy(rev, branch_rev);
+		else
 			new.path = NULL; /* not an existing branch */
 
 		if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {

My version somehow looks uglier, but I just worry about resolve_ref
violating this undocumented subtlety sometime in the future.

Also, one other question while we are on the subject. I think we all
agree that "git checkout $foo" should prefer $foo as a branch. But what
about "git checkout -b $branch $start_point"? Should $start_point follow
the same "prefer branches" rule, or should it use the usual ref lookup
rules?

I was surprised to find that the current behavior is to die(), due to an
explicit case in branch.c:create_branch.

-Peff

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Fri, Jan 07, 2011 at 02:54:17PM -0500, Jeff King wrote:

> +test_expect_success 'checkout reports switch to detached HEAD' '
> +	grep "Switched to branch" stderr &&
> +	! grep "^HEAD is now at" stderr

Junio, one minor fixup here. The test is correct, but the description
should read "checkout reports switch to branch", not "...detached HEAD".

I had originally written the test the other way and forgot to update the
description when I tweaked it. The error is in my ambiguity test, but
got cut-and-pasted to your vagueness test, too.

-Peff

Re: bug? in checkout with ambiguous refnames

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 07, 2011 at 03:17:22PM -0800, Junio C Hamano wrote:
>
>> ... And this comes on top (should probably be squashed into one) to really
>> favor a branch over a tag.
>> 
>>  builtin/checkout.c               |   26 ++++++++++----------------
>>  t/t2019-checkout-amiguous-ref.sh |    2 +-
>>  2 files changed, 11 insertions(+), 17 deletions(-)
>
> Yeah, that looks sane to me (assuming all three patches squashed
> together). It took me a minute to figure out one subtlety, though:
>
>> +		if ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||
>> +		    !resolve_ref(new.path, rev, 1, NULL))
>> +			new.path = NULL; /* not an existing branch */
>> +
>> +		if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {
>
> We are relying on the fact that resolve_ref leaves "rev" alone in the
> case that it does not find anything. Which is mostly true (the only
> exception seems to be if you have a ref with non-hex garbage in it, in
> which case you will get some bogus sha1 in the output). I dunno if it is
> worth making it more explicit, like:

I've thought about it when I sent the patch.  I think this is safe as that
particular resolve is done on a full ref "refs/heads/$something" and upon
seeing the first 'r' get_sha1_hex() would give up without touching rev[],
but I agree it is too subtle.

> Also, one other question while we are on the subject. I think we all
> agree that "git checkout $foo" should prefer $foo as a branch. But what
> about "git checkout -b $branch $start_point"?

That has always been defined as a synonym for

	git branch $branch $start_point && git checkout $branch

so $start_point is just a random extended SHA-1 expression.

> I was surprised to find that the current behavior is to die(), due to an
> explicit case in branch.c:create_branch.

Good eyes.  At that point, "refname <start> is ambiguous."  warning has
already been issued, and there is no sane reason to die there.  I'd call
it a bug.

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Tue, Jan 11, 2011 at 09:02:18AM -0800, Junio C Hamano wrote:

> > Also, one other question while we are on the subject. I think we all
> > agree that "git checkout $foo" should prefer $foo as a branch. But what
> > about "git checkout -b $branch $start_point"?
> 
> That has always been defined as a synonym for
> 
> 	git branch $branch $start_point && git checkout $branch
> 
> so $start_point is just a random extended SHA-1 expression.

That's what I would have expected, but I wanted to write a test to make
sure it was the case.

But it's not. Even taking away the die, my second test here fails (built
on top of the three previous commits under discussion):

diff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh
index e2b330b..7a6b30b 100755
--- a/t/t2019-checkout-amiguous-ref.sh
+++ b/t/t2019-checkout-amiguous-ref.sh
@@ -50,4 +50,13 @@ test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to detached HEAD'
 	! grep "^HEAD is now at" stderr
 '
 
+test_expect_success 'new branch from ambiguous start_point works' '
+	git checkout -b newbranch ambiguity
+'
+
+test_expect_success 'checkout chooses tag over branch for start_point' '
+	echo tag >expect &&
+	test_cmp expect file
+'
+
 test_done

For bonus fun, doing this:

  git branch newbranch ambiguity
  git checkout newbranch

_does_ prefer the branch. So it is checkout feeding create_branch() the
modified sha1. I'll see if I can dig further.

-Peff

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Tue, Jan 11, 2011 at 01:55:09AM -0500, Jeff King wrote:

> On Fri, Jan 07, 2011 at 02:54:17PM -0500, Jeff King wrote:
> 
> > +test_expect_success 'checkout reports switch to detached HEAD' '
> > +	grep "Switched to branch" stderr &&
> > +	! grep "^HEAD is now at" stderr
> 
> Junio, one minor fixup here. The test is correct, but the description
> should read "checkout reports switch to branch", not "...detached HEAD".

Hmm. One other thing to note on these ambiguity tests. Apparently we did
already have a test for this in t7201 (which should perhaps be renamed
into the t20* area with the other checkout tests), but it was passing.

The problem is that the current behavior is way more broken than just
"accidentally choose tag over branch". It actually writes the branch ref
into HEAD, but checks out the tree from the tag! The test in 7201 only
checks the former, but our new test checked the latter.

Probably we should be checking both, just to be sure (and yes, with your
patch it does the right thing):

diff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh
index e2b330b..606081b 100755
--- a/t/t2019-checkout-amiguous-ref.sh
+++ b/t/t2019-checkout-amiguous-ref.sh
@@ -22,6 +22,9 @@ test_expect_success 'checkout produces ambiguity warning' '
 '
 
 test_expect_success 'checkout chooses branch over tag' '
+	echo refs/heads/ambiguity >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
 	echo branch >expect &&
 	test_cmp expect file
 '
@@ -41,6 +44,9 @@ test_expect_success VAGUENESS_SUCCESS 'checkout produces ambiguity warning' '
 '
 
 test_expect_success VAGUENESS_SUCCESS 'checkout chooses branch over tag' '
+	echo refs/heads/vagueness >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
 	echo branch >expect &&
 	test_cmp expect file
 '

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Tue, Jan 11, 2011 at 02:20:20PM -0500, Jeff King wrote:

> Hmm. One other thing to note on these ambiguity tests.

I was starting to get confused with all the back-and-forth, so here is a
squashed patch that contains everything discussed so far: my original
tests, your squashed patches, my test fixups, and the less-subtle
resolve_ref thing.

I didn't bisect the breakage, so I'm not sure how far back it goes. But
this is potentially maint-worthy.

This doesn't include any tests or fixes for "git checkout -b newbranch
ambiguous". I'll work on that separately as a patch on top.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] checkout: fix bug with ambiguous refs

The usual dwim_ref lookup prefers tags to branches. Because
checkout primarily works on branches, though, we switch that
behavior to prefer branches.

However, there was a bug in the implementation in which we
used lookup_commit_reference (which used the regular lookup
rules) to get the actual commit to checkout. Checking out an
ambiguous ref therefore ended up putting us in an extremely
broken state in which we wrote the branch ref into HEAD, but
actually checked out the tree for the tag.

This patch fixes the bug by always attempting to pull the
commit to be checked out from the branch-ified version of
the name we were given.

Patch by Junio, tests and commit message from Jeff King.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout.c               |   23 ++++++++------
 t/t2019-checkout-amiguous-ref.sh |   59 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 10 deletions(-)
 create mode 100755 t/t2019-checkout-amiguous-ref.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 757f9a0..d3cfaf0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -678,7 +678,7 @@ static const char *unique_tracking_name(const char *name)
 int cmd_checkout(int argc, const char **argv, const char *prefix)
 {
 	struct checkout_opts opts;
-	unsigned char rev[20];
+	unsigned char rev[20], branch_rev[20];
 	const char *arg;
 	struct branch_info new;
 	struct tree *source_tree = NULL;
@@ -832,18 +832,21 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		argc--;
 
 		new.name = arg;
-		if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
-			setup_branch_path(&new);
+		setup_branch_path(&new);
 
-			if ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&
-			    resolve_ref(new.path, rev, 1, NULL))
-				;
-			else
-				new.path = NULL;
+		if (check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&
+		    resolve_ref(new.path, branch_rev, 1, NULL))
+			hashcpy(rev, branch_rev);
+		else
+			new.path = NULL; /* not an existing branch */
+
+		if (!(new.commit = lookup_commit_reference_gently(rev, 1))) {
+			/* not a commit */
+			source_tree = parse_tree_indirect(rev);
+		} else {
 			parse_commit(new.commit);
 			source_tree = new.commit->tree;
-		} else
-			source_tree = parse_tree_indirect(rev);
+		}
 
 		if (!source_tree)                   /* case (1): want a tree */
 			die("reference is not a tree: %s", arg);
diff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh
new file mode 100755
index 0000000..943541d
--- /dev/null
+++ b/t/t2019-checkout-amiguous-ref.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+
+test_description='checkout handling of ambiguous (branch/tag) refs'
+. ./test-lib.sh
+
+test_expect_success 'setup ambiguous refs' '
+	test_commit branch file &&
+	git branch ambiguity &&
+	git branch vagueness &&
+	test_commit tag file &&
+	git tag ambiguity &&
+	git tag vagueness HEAD:file &&
+	test_commit other file
+'
+
+test_expect_success 'checkout ambiguous ref succeeds' '
+	git checkout ambiguity >stdout 2>stderr
+'
+
+test_expect_success 'checkout produces ambiguity warning' '
+	grep "warning.*ambiguous" stderr
+'
+
+test_expect_success 'checkout chooses branch over tag' '
+	echo refs/heads/ambiguity >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
+	echo branch >expect &&
+	test_cmp expect file
+'
+
+test_expect_success 'checkout reports switch to branch' '
+	grep "Switched to branch" stderr &&
+	! grep "^HEAD is now at" stderr
+'
+
+test_expect_success 'checkout vague ref succeeds' '
+	git checkout vagueness >stdout 2>stderr &&
+	test_set_prereq VAGUENESS_SUCCESS
+'
+
+test_expect_success VAGUENESS_SUCCESS 'checkout produces ambiguity warning' '
+	grep "warning.*ambiguous" stderr
+'
+
+test_expect_success VAGUENESS_SUCCESS 'checkout chooses branch over tag' '
+	echo refs/heads/vagueness >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual &&
+	echo branch >expect &&
+	test_cmp expect file
+'
+
+test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to branch' '
+	grep "Switched to branch" stderr &&
+	! grep "^HEAD is now at" stderr
+'
+
+test_done
-- 
1.7.3.5.4.g0dc52

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Tue, Jan 11, 2011 at 01:02:08PM -0500, Jeff King wrote:

> On Tue, Jan 11, 2011 at 09:02:18AM -0800, Junio C Hamano wrote:
> 
> > > Also, one other question while we are on the subject. I think we all
> > > agree that "git checkout $foo" should prefer $foo as a branch. But what
> > > about "git checkout -b $branch $start_point"?
> > 
> > That has always been defined as a synonym for
> > 
> > 	git branch $branch $start_point && git checkout $branch
> > 
> > so $start_point is just a random extended SHA-1 expression.
> 
> That's what I would have expected, but I wanted to write a test to make
> sure it was the case.

Looking into this more, I'm not sure what the right behavior is. The
offending lines are in branch.c:create_branch:

        switch (dwim_ref(start_name, strlen(start_name), sha1, &real_ref)) {
        case 0:
                /* Not branching from any existing branch */
                if (explicit_tracking)
                        die("Cannot setup tracking information; starting point is not a branch.");
                break;
        case 1:
                /* Unique completion -- good, only if it is a real ref */
                if (explicit_tracking && !strcmp(real_ref, "HEAD"))
                        die("Cannot setup tracking information; starting point is not a branch.");
                break;
        default:
                die("Ambiguous object name: '%s'.", start_name);
                break;
        }

The die("Ambiguous...") blames all the way back to 0746d19 (git-branch,
git-checkout: autosetup for remote branch tracking, 2007-03-08) and
seems to just be a regression there that we didn't catch.

Obviously we can loosen the die() there and just handle the ambiguous
case like the unique case. But I'm not sure how tracking should interact
with the branch/tag thing.

This code seems to be trying to only track branches, but it fails at
that. It actually will track _any_ ref. So I can happily do:

  git branch --track foo v1.5

and start tracking refs/tags/v1.5. Is that a bug or a feature?

And then that makes me wonder. What should:

  git branch --track foo ambiguity

do? Should it choose the branch, because that is more useful for
tracking? Or choose the tag? And if branch, then what should:

  git branch foo ambiguity

do? It won't track a tag unless --track is given explicitly. Should it
prefer a branch with implicit tracking, or an untracked tag?

I'm not sure. And to be honest I don't really care, because I think
people with ambiguous refs are little bit crazy anyway (after all, in
the current code it simply calls die()). But I think there is some
argument to be made that due to tracking, start_point is not _just_
a regular ref. We do care about its branchiness.

-Peff

Re: bug? in checkout with ambiguous refnames

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure. And to be honest I don't really care, because I think
> people with ambiguous refs are little bit crazy anyway (after all, in
> the current code it simply calls die()). But I think there is some
> argument to be made that due to tracking, start_point is not _just_
> a regular ref. We do care about its branchiness.

I do not really care either myself, and if 

    git branch --track foo heads/ambiguity
    git branch --track foo tags/ambiguity

allows the user to differentiate between the branch and the tag, it would
be more than sufficient.

Re: bug? in checkout with ambiguous refnames

[Uwe Kleine-König]

Hi Peff,

On Fri, Jan 07, 2011 at 02:49:09PM -0500, Jeff King wrote:
> On Fri, Jan 07, 2011 at 11:46:50AM +0100, Uwe Kleine-König wrote:
> 
> > 	ukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached
> > 
> > 	ukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart
> > 	warning: refname 'sgu/mxs-amba-uart' is ambiguous.
> > 	Previous HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD
> > 	Switched to branch 'sgu/mxs-amba-uart'
> > 
> > OK, it might be a bad idea to have this ambiguity, still ...
> > [...]
> > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
> > points to refs/heads/sgu/mxs-amba-uart
> 
> Yeah, we generally resolve ambiguities in favor of the tag (and that
> warning comes from deep within get_sha1_basic). So the real bug here is
> that it still said "Switched to branch", which is totally wrong.
I wonder how I can resolve the ambiguity when calling checkout.  (Well
apart from changing either branch name or tag name)

git checkout refs/heads/sgu/mxs-amba-uart results in a detached HEAD.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Wed, Jan 12, 2011 at 01:07:51AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm not sure. And to be honest I don't really care, because I think
> > people with ambiguous refs are little bit crazy anyway (after all, in
> > the current code it simply calls die()). But I think there is some
> > argument to be made that due to tracking, start_point is not _just_
> > a regular ref. We do care about its branchiness.
> 
> I do not really care either myself, and if 
> 
>     git branch --track foo heads/ambiguity
>     git branch --track foo tags/ambiguity
> 
> allows the user to differentiate between the branch and the tag, it would
> be more than sufficient.

It does already. So I am inclined to leave it alone, then. I doubt
anyone actually cares, and if they do, they will get an error message,
after which they can manually disambiguate themselves.

So let's leave it at the mega-patch I posted earlier.

-Peff

Re: bug? in checkout with ambiguous refnames

[Jeff King]

On Wed, Jan 12, 2011 at 10:11:30AM +0100, Uwe Kleine-König wrote:

> > > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
> > > points to refs/heads/sgu/mxs-amba-uart
> > 
> > Yeah, we generally resolve ambiguities in favor of the tag (and that
> > warning comes from deep within get_sha1_basic). So the real bug here is
> > that it still said "Switched to branch", which is totally wrong.
> I wonder how I can resolve the ambiguity when calling checkout.  (Well
> apart from changing either branch name or tag name)
> 
> git checkout refs/heads/sgu/mxs-amba-uart results in a detached HEAD.

You can't disambiguate to the branch without going to a detached HEAD in
the current code; it's just broken[1].

With the patch here:

  http://article.gmane.org/gmane.comp.version-control.git/164986

it will disambiguate to the branch by default, and if you want the tag,
you can do:

  git checkout tags/sgu/mxs-amba-uart

-Peff

[1]: You can't do it with checkout, that is. You can still hack around
     it with:

        branch=refs/heads/sgu/mxs-amba-uart
        git read-tree -m -u $branch &&
        git symbolic-ref HEAD $branch

     which is a simplified version of what checkout is doing.

Re: bug? in checkout with ambiguous refnames

[Uwe Kleine-König]

Hi,

On Wed, Jan 12, 2011 at 12:46:00PM -0500, Jeff King wrote:
> On Wed, Jan 12, 2011 at 10:11:30AM +0100, Uwe Kleine-König wrote:
> > > > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD
> > > > points to refs/heads/sgu/mxs-amba-uart
> > > 
> > > Yeah, we generally resolve ambiguities in favor of the tag (and that
> > > warning comes from deep within get_sha1_basic). So the real bug here is
> > > that it still said "Switched to branch", which is totally wrong.
> > I wonder how I can resolve the ambiguity when calling checkout.  (Well
> > apart from changing either branch name or tag name)
> > 
> > git checkout refs/heads/sgu/mxs-amba-uart results in a detached HEAD.
> 
> You can't disambiguate to the branch without going to a detached HEAD in
> the current code; it's just broken[1].
> 
> With the patch here:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/164986
> 
> it will disambiguate to the branch by default, and if you want the tag,
> you can do:
> 
>   git checkout tags/sgu/mxs-amba-uart
that's fine.

> [1]: You can't do it with checkout, that is. You can still hack around
>      it with:
> 
>         branch=refs/heads/sgu/mxs-amba-uart
>         git read-tree -m -u $branch &&
>         git symbolic-ref HEAD $branch
I did

	git checkout refs/heads/sgu/mxs-amba-uart
	git symbolic-ref HEAD refs/heads/sgu/mxs-amba-uart

:-)

Uwe
> 
>      which is a simplified version of what checkout is doing.
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |