{"thread":{"id":"26226","subject":"bug? in checkout with ambiguous refnames","startedAt":"2011-01-07T10:46:50Z","lastAt":"2011-01-12T18:19:37Z","messageCount":23,"participants":["Uwe Kleine-König","Junio C Hamano","Jeff King","Martin von Zweigbergk"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159100","messageId":"20110107104650.GA5399@pengutronix.de","threadId":"26226","inReplyTo":null,"subject":"bug? in checkout with ambiguous refnames","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2011-01-07T10:46:50Z","receivedAt":"2011-01-07T10:46:50Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\neverything is clean:\n\n\tukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached\n\n\tukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart\n\twarning: refname 'sgu/mxs-amba-uart' is ambiguous.\n\tPrevious HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD\n\tSwitched to branch 'sgu/mxs-amba-uart'\n\nOK, it might be a bad idea to have this ambiguity, still ...\n\n\tukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached --stat\n\t arch/arm/mach-mxs/Kconfig                       |    2 ++\n\t arch/arm/mach-mxs/clock-mx23.c                  |    2 +-\n\t arch/arm/mach-mxs/clock-mx28.c                  |    2 +-\n\t arch/arm/mach-mxs/devices-mx23.h                |    2 +-\n\t arch/arm/mach-mxs/devices-mx28.h                |    2 +-\n\t arch/arm/mach-mxs/devices.c                     |   17 ++---------------\n\t arch/arm/mach-mxs/devices/Kconfig               |    1 -\n\t arch/arm/mach-mxs/devices/amba-duart.c          |   15 +++++++--------\n\t arch/arm/mach-mxs/include/mach/devices-common.h |    4 +---\n\t 9 files changed, 16 insertions(+), 31 deletions(-)\n\tukl@octopus:~/gsrc/linux-2.6$ git diff refs/tags/sgu/mxs-amba-uart\n\tukl@octopus:~/gsrc/linux-2.6$ git diff --cached refs/tags/sgu/mxs-amba-uart\n\nSo working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\npoints to refs/heads/sgu/mxs-amba-uart\n\nBest regards\nUwe\n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"},{"id":"159143","messageId":"7vzkrc5wil.fsf@alter.siamese.dyndns.org","threadId":"26226","inReplyTo":"20110107104650.GA5399@pengutronix.de","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-07T19:44:34Z","receivedAt":"2011-01-07T19:44:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:\n\n> So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\n> points to refs/heads/sgu/mxs-amba-uart\n\nI somehow thought that we had an explicit logic to favor an exact branch\nname for \"git checkout $branch\" even when refs/something-other-than-head/$branch \nexists, while issuing the ambiguity warning.\n\nAhh, what this part of the code in builtin/checkout.c does is totally\nwrong:\n\n\t/* we can't end up being in (2) anymore, eat the argument */\n\targv++;\n\targc--;\n\n\tnew.name = arg;\n\tif ((new.commit = lookup_commit_reference_gently(rev, 1))) {\n\t\tsetup_branch_path(&new);\n\n\t\tif ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&\n\t\t    resolve_ref(new.path, rev, 1, NULL))\n\t\t\t;\n\t\telse\n\t\t\tnew.path = NULL;\n\t\tparse_commit(new.commit);\n\t\tsource_tree = new.commit->tree;\n\t} else\n\t\tsource_tree = parse_tree_indirect(rev);\n\nIt uses lookup_commit_reference_gently() that follows the usual\ntags-then-heads preference order, but then uses setup_branch_path() to\nprefix the raw name with \"refs/heads\", which is totally backwards.\nIt should do something like:\n\n - use setup-branch-path to get refs/heads/$name\n\n - check-ref-format and resolve it; if these fail, then we are detaching\n   head at rev;\n\n - otherwise, if the result of the resolution is not the same as rev, what\n   we have in rev is incorrect (it was taken from the usual\n   tags-then-heads rule but \"checkout $name\" must favor local branches).\n   update the rev with the result of resolving refs/heads/$name\n\n - parse new.commit out of rev.  we are checking out the branch $name.\n"},{"id":"159144","messageId":"20110107194909.GB6175@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110107104650.GA5399@pengutronix.de","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-07T19:49:09Z","receivedAt":"2011-01-07T19:49:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 07, 2011 at 11:46:50AM +0100, Uwe Kleine-König wrote:\n\n> \tukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached\n> \n> \tukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart\n> \twarning: refname 'sgu/mxs-amba-uart' is ambiguous.\n> \tPrevious HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD\n> \tSwitched to branch 'sgu/mxs-amba-uart'\n> \n> OK, it might be a bad idea to have this ambiguity, still ...\n> [...]\n> So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\n> points to refs/heads/sgu/mxs-amba-uart\n\nYeah, we generally resolve ambiguities in favor of the tag (and that\nwarning comes from deep within get_sha1_basic). So the real bug here is\nthat it still said \"Switched to branch\", which is totally wrong.\n\nThat being said, it probably would make more sense for \"git checkout\" to\nprefer branches to tags. That's probably going to take a lot more\nsurgery, and we're in -rc right now. So I think the best thing to do is\nto fix the broken message and add some tests, and then if somebody wants\nto revisit it with a larger patch, they can do so on top.\n\nI'll work on the first part and post a patch in a few minutes.\n\n-Peff\n"},{"id":"159147","messageId":"20110107195417.GC6175@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110107194909.GB6175@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-07T19:54:17Z","receivedAt":"2011-01-07T19:54:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 07, 2011 at 02:49:09PM -0500, Jeff King wrote:\n\n> That being said, it probably would make more sense for \"git checkout\" to\n> prefer branches to tags. That's probably going to take a lot more\n> surgery, and we're in -rc right now. So I think the best thing to do is\n> to fix the broken message and add some tests, and then if somebody wants\n> to revisit it with a larger patch, they can do so on top.\n> \n> I'll work on the first part and post a patch in a few minutes.\n\nAh, never mind. After reading Junio's response, it looks like we already\ntry to do the right thing in checkout, but it's just broken. So forget\nmy two-step plan.\n\nHere is the test script I worked out which shows the issue (and checks\nthat the right messages are shown to the user):\n\n---\n t/t2019-checkout-amiguous-ref.sh |   32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n create mode 100755 t/t2019-checkout-amiguous-ref.sh\n\ndiff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh\nnew file mode 100755\nindex 0000000..12edce8\n--- /dev/null\n+++ b/t/t2019-checkout-amiguous-ref.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='checkout handling of ambiguous (branch/tag) refs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ambiguous refs' '\n+\ttest_commit branch file &&\n+\tgit branch ambiguity &&\n+\ttest_commit tag file &&\n+\tgit tag ambiguity &&\n+\ttest_commit other file\n+'\n+\n+test_expect_success 'checkout ambiguous ref succeeds' '\n+\tgit checkout ambiguity >stdout 2>stderr\n+'\n+\n+test_expect_success 'checkout produces ambiguity warning' '\n+\tgrep \"warning.*ambiguous\" stderr\n+'\n+\n+test_expect_failure 'checkout chooses branch over tag' '\n+\techo branch >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'checkout reports switch to detached HEAD' '\n+\tgrep \"Switched to branch\" stderr &&\n+\t! grep \"^HEAD is now at\" stderr\n+'\n+\n+test_done\n-- \n1.7.4.rc1.23.g84303\n"},{"id":"159173","messageId":"7vsjx449bv.fsf@alter.siamese.dyndns.org","threadId":"26226","inReplyTo":"20110107195417.GC6175@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-07T22:50:44Z","receivedAt":"2011-01-07T22:50:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here is the test script I worked out which shows the issue (and checks\n> that the right messages are shown to the user):\n\nThis is a band-aid that is not quite right (even though it passes the\ntests).  Attempting to check out a branch \"frotz\" in a repository with a\ntag \"frotz\" that point at a non-commit would still fail, which is not a\nnew problem.\n\n\n builtin/checkout.c               |    7 +++++++\n t/t2019-checkout-amiguous-ref.sh |   23 ++++++++++++++++++++++-\n 2 files changed, 29 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a54583b..f6fea2f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -840,6 +840,13 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\t\t\t;\n \t\t\telse\n \t\t\t\tnew.path = NULL;\n+\t\t\tif (hashcmp(new.commit->object.sha1, rev))\n+\t\t\t\t/*\n+\t\t\t\t * Yikes, arg is an ambiguous and higher\n+\t\t\t\t * precedence SHA-1 expression than the\n+\t\t\t\t * branch name\n+\t\t\t\t */\n+\t\t\t\tnew.commit = lookup_commit_reference_gently(rev, 1);\n \t\t\tparse_commit(new.commit);\n \t\t\tsource_tree = new.commit->tree;\n \t\t} else\ndiff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh\nindex 0981f11..fa1d4e6 100755\n--- a/t/t2019-checkout-amiguous-ref.sh\n+++ b/t/t2019-checkout-amiguous-ref.sh\n@@ -6,8 +6,10 @@ test_description='checkout handling of ambiguous (branch/tag) refs'\n test_expect_success 'setup ambiguous refs' '\n \ttest_commit branch file &&\n \tgit branch ambiguity &&\n+\tgit branch vagueness &&\n \ttest_commit tag file &&\n \tgit tag ambiguity &&\n+\tgit tag vagueness HEAD:file &&\n \ttest_commit other file\n '\n \n@@ -19,7 +21,7 @@ test_expect_success 'checkout produces ambiguity warning' '\n \tgrep \"warning.*ambiguous\" stderr\n '\n \n-test_expect_failure 'checkout chooses branch over tag' '\n+test_expect_success 'checkout chooses branch over tag' '\n \techo branch >expect &&\n \ttest_cmp expect file\n '\n@@ -29,4 +31,23 @@ test_expect_success 'checkout reports switch to detached HEAD' '\n \t! grep \"^HEAD is now at\" stderr\n '\n \n+test_expect_failure 'checkout vague ref succeeds' '\n+\tgit checkout vagueness >stdout 2>stderr &&\n+\ttest_set_prereq VAGUENESS_SUCCESS\n+'\n+\n+test_expect_success VAGUENESS_SUCCESS 'checkout produces ambiguity warning' '\n+\tgrep \"warning.*ambiguous\" stderr\n+'\n+\n+test_expect_success VAGUENESS_SUCCESS 'checkout chooses branch over tag' '\n+\techo branch >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to detached HEAD' '\n+\tgrep \"Switched to branch\" stderr &&\n+\t! grep \"^HEAD is now at\" stderr\n+'\n+\n test_done\n"},{"id":"159179","messageId":"7vipy0483h.fsf@alter.siamese.dyndns.org","threadId":"26226","inReplyTo":"7vsjx449bv.fsf@alter.siamese.dyndns.org","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-07T23:17:22Z","receivedAt":"2011-01-07T23:17:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> Here is the test script I worked out which shows the issue (and checks\n>> that the right messages are shown to the user):\n>\n> This is a band-aid that is not quite right (even though it passes the\n> tests).  Attempting to check out a branch \"frotz\" in a repository with a\n> tag \"frotz\" that point at a non-commit would still fail, which is not a\n> new problem.\n>\n>\n>  builtin/checkout.c               |    7 +++++++\n>  t/t2019-checkout-amiguous-ref.sh |   23 ++++++++++++++++++++++-\n>  2 files changed, 29 insertions(+), 1 deletions(-)\n\n... And this comes on top (should probably be squashed into one) to really\nfavor a branch over a tag.\n\n builtin/checkout.c               |   26 ++++++++++----------------\n t/t2019-checkout-amiguous-ref.sh |    2 +-\n 2 files changed, 11 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f6fea2f..48e547b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -832,25 +832,19 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\targc--;\n \n \t\tnew.name = arg;\n-\t\tif ((new.commit = lookup_commit_reference_gently(rev, 1))) {\n-\t\t\tsetup_branch_path(&new);\n+\t\tsetup_branch_path(&new);\n \n-\t\t\tif ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&\n-\t\t\t    resolve_ref(new.path, rev, 1, NULL))\n-\t\t\t\t;\n-\t\t\telse\n-\t\t\t\tnew.path = NULL;\n-\t\t\tif (hashcmp(new.commit->object.sha1, rev))\n-\t\t\t\t/*\n-\t\t\t\t * Yikes, arg is an ambiguous and higher\n-\t\t\t\t * precedence SHA-1 expression than the\n-\t\t\t\t * branch name\n-\t\t\t\t */\n-\t\t\t\tnew.commit = lookup_commit_reference_gently(rev, 1);\n+\t\tif ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||\n+\t\t    !resolve_ref(new.path, rev, 1, NULL))\n+\t\t\tnew.path = NULL; /* not an existing branch */\n+\n+\t\tif (!(new.commit = lookup_commit_reference_gently(rev, 1))) {\n+\t\t\t/* not a commit */\n+\t\t\tsource_tree = parse_tree_indirect(rev);\n+\t\t} else {\n \t\t\tparse_commit(new.commit);\n \t\t\tsource_tree = new.commit->tree;\n-\t\t} else\n-\t\t\tsource_tree = parse_tree_indirect(rev);\n+\t\t}\n \n \t\tif (!source_tree)                   /* case (1): want a tree */\n \t\t\tdie(\"reference is not a tree: %s\", arg);\ndiff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh\nindex fa1d4e6..e2b330b 100755\n--- a/t/t2019-checkout-amiguous-ref.sh\n+++ b/t/t2019-checkout-amiguous-ref.sh\n@@ -31,7 +31,7 @@ test_expect_success 'checkout reports switch to detached HEAD' '\n \t! grep \"^HEAD is now at\" stderr\n '\n \n-test_expect_failure 'checkout vague ref succeeds' '\n+test_expect_success 'checkout vague ref succeeds' '\n \tgit checkout vagueness >stdout 2>stderr &&\n \ttest_set_prereq VAGUENESS_SUCCESS\n '\n"},{"id":"159234","messageId":"alpine.DEB.1.10.1101081449220.12031@debian","threadId":"26226","inReplyTo":"20110107194909.GB6175@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-08T20:40:33Z","receivedAt":"2011-01-08T20:40:33Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Fri, 7 Jan 2011, Jeff King wrote:\n\n> On Fri, Jan 07, 2011 at 11:46:50AM +0100, Uwe Kleine-K?nig wrote:\n> \n> > \tukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached\n> > \n> > \tukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart\n> > \twarning: refname 'sgu/mxs-amba-uart' is ambiguous.\n> > \tPrevious HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD\n> > \tSwitched to branch 'sgu/mxs-amba-uart'\n> > \n> > OK, it might be a bad idea to have this ambiguity, still ...\n> > [...]\n> > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\n> > points to refs/heads/sgu/mxs-amba-uart\n> \n> Yeah, we generally resolve ambiguities in favor of the tag (and that\n> warning comes from deep within get_sha1_basic). So the real bug here is\n> that it still said \"Switched to branch\", which is totally wrong.\n> \n> That being said, it probably would make more sense for \"git checkout\" to\n> prefer branches to tags.\n\nWhat was the rationale for generally favoring tags? Why does that\nreasoning not apply to 'git checkout' too? At least without knowing\nthe answer to that question, I think I would prefer to have checkout\nbehave the same as the other commands do. Maybe a bit academic, but it\nseems like it would be nice if e.g. 'git checkout $anything && git\nshow $anything' would show the HEAD commit and if 'git checkout\n$anything && git diff HEAD $anything' was always empty.\n\nBtw, what exactly does \"generally\" mean, i.e. which other commands\ndon't favor tags? I know rebase is one example of a command that does\nnot favor tags.\n\nSlightly off topic, but why does 'git rev-parse --symbolic-full-name'\nnot output anything when the input is ambiguous? 'git rev-parse'\nwithout any flags favors tags, so I would have expected to get\nsomething like refs/tags/$name back.\n\nThe reason I'm asking is because I just happened to see in the rebase\ncode the other day that it will rebase a detached head if the <branch>\nparameter is not \"completely unqualified\". For example 'git rebase\nmaster heads/topic' or 'git rebase master refs/heads/topic' will not\nupdate refs/heads/topic. I was trying to fix that by using 'git\nrev-parse --symbolic-full-name' to parse <branch>. That seemed to work\nfine until I saw this thread :-).\n"},{"id":"159235","messageId":"20110108214011.GA4753@sigill.intra.peff.net","threadId":"26226","inReplyTo":"alpine.DEB.1.10.1101081449220.12031@debian","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-08T21:40:11Z","receivedAt":"2011-01-08T21:40:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:\n\n> > Yeah, we generally resolve ambiguities in favor of the tag (and that\n> > warning comes from deep within get_sha1_basic). So the real bug here is\n> > that it still said \"Switched to branch\", which is totally wrong.\n> > \n> > That being said, it probably would make more sense for \"git checkout\" to\n> > prefer branches to tags.\n> \n> What was the rationale for generally favoring tags?\n\nI don't recall hearing any specific argument, but it has always been\nthat way from early on. I think it is from a vague sense of \"tags are\nmore important than branch tips because they are about marking specific\npoints, not lines of development\". But maybe other old-timers can say\nmore.\n\nI don't necessarily buy that argument; my only reasoning is that we\nshould probably keep historic behavior.\n\n> Why does that reasoning not apply to 'git checkout' too?\n\nBecause checkout has always been fundamentally about branches. It did\nend up growing sane behavior for \"git checkout tag\" (i.e., a detached\nHEAD), but branches are still the fundamental unit for most of its\narguments.\n\n> Btw, what exactly does \"generally\" mean, i.e. which other commands\n> don't favor tags? I know rebase is one example of a command that does\n> not favor tags.\n\nIt means \"we favor tags in resolve_ref, which is the underlying\nmachinery for most commands, so unless a command special-cases it, that\nwill be the behavior, and I am too lazy to exhaustively search for such\nspecial cases\".\n\n> Slightly off topic, but why does 'git rev-parse --symbolic-full-name'\n> not output anything when the input is ambiguous? 'git rev-parse'\n> without any flags favors tags, so I would have expected to get\n> something like refs/tags/$name back.\n\nI dunno. I never tried it, but I would have expected to get the tag-name\nback.\n\n> The reason I'm asking is because I just happened to see in the rebase\n> code the other day that it will rebase a detached head if the <branch>\n> parameter is not \"completely unqualified\". For example 'git rebase\n> master heads/topic' or 'git rebase master refs/heads/topic' will not\n> update refs/heads/topic. I was trying to fix that by using 'git\n> rev-parse --symbolic-full-name' to parse <branch>. That seemed to work\n> fine until I saw this thread :-).\n\nHeh. I think that would be an argument in favor of changing rev-parse's\nbehavior.\n\n-Peff\n"},{"id":"159236","messageId":"alpine.DEB.1.10.1101081902150.12031@debian","threadId":"26226","inReplyTo":"20110108214011.GA4753@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-09T02:43:57Z","receivedAt":"2011-01-09T02:43:57Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Sat, 8 Jan 2011, Jeff King wrote:\n\n> On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:\n> \n> > > Yeah, we generally resolve ambiguities in favor of the tag (and that\n> > > warning comes from deep within get_sha1_basic). So the real bug here is\n> > > that it still said \"Switched to branch\", which is totally wrong.\n> > > \n> > > That being said, it probably would make more sense for \"git checkout\" to\n> > > prefer branches to tags.\n> > \n> > What was the rationale for generally favoring tags?\n> \n> I don't recall hearing any specific argument, but it has always been\n> that way from early on. I think it is from a vague sense of \"tags are\n> more important than branch tips because they are about marking specific\n> points, not lines of development\". But maybe other old-timers can say\n> more.\n> \n> I don't necessarily buy that argument; my only reasoning is that we\n> should probably keep historic behavior.\n> \n> > Why does that reasoning not apply to 'git checkout' too?\n> \n> Because checkout has always been fundamentally about branches. It did\n> end up growing sane behavior for \"git checkout tag\" (i.e., a detached\n> HEAD), but branches are still the fundamental unit for most of its\n> arguments.\n\nI had a look at the history of the lines Junio mentioned in another\nmessage on this thread (lookup_commit_reference_gently() and\nsetup_branch_path()). I don't understand the code, but just looking at\nwhere the lines came from, they seem to have been there ever since\n782c2d6 (Build in checkout, 2008-02-07). If that is correct (but\nplease check for yourselves), it seems like the broken checkout of\nambiguous references causes problems rarely enough that no one has\nbothered to report it for almost two years. If it has been broken for\nthat long, it seems to me like we could resolve it whichever way makes\nmost sense, without much concern to how it used to work, no?\n\n> > Btw, what exactly does \"generally\" mean, i.e. which other commands\n> > don't favor tags? I know rebase is one example of a command that does\n> > not favor tags.\n> \n> It means \"we favor tags in resolve_ref, which is the underlying\n> machinery for most commands, so unless a command special-cases it, that\n> will be the behavior, and I am too lazy to exhaustively search for such\n> special cases\".\n\nSensible definition :-). I was just hoping for an example where it\nmore obviously makes sense to favor branches. Maybe rebase is actually\none of the better examples.\n\n> > The reason I'm asking is because I just happened to see in the rebase\n> > code the other day that it will rebase a detached head if the <branch>\n> > parameter is not \"completely unqualified\". For example 'git rebase\n> > master heads/topic' or 'git rebase master refs/heads/topic' will not\n> > update refs/heads/topic. I was trying to fix that by using 'git\n> > rev-parse --symbolic-full-name' to parse <branch>. That seemed to work\n> > fine until I saw this thread :-).\n> \n> Heh. I think that would be an argument in favor of changing rev-parse's\n> behavior.\n\nI was hoping to do something like\n\nhead_name=$(git rev-parse --symbolic-full-name -q --verify \"$1\")\ncase \"$head_name\" in\nrefs/heads/*) ;;\n*) head_name=\"detached HEAD\" ;;\nesac\n\nplus setting another variable or two in each case. So even if 'git\nrev-parse --symbolic-full-name' would return refs/tags/$name, it\nwouldn't actually help here, since rebase currently favors\nbranches. In order to keep that behavior, I will need to add an extra\ncheck before the above code.\n\nIt still feels like rev-parse should return refs/tags/$name, though.\n"},{"id":"159239","messageId":"7v1v4m8reh.fsf@alter.siamese.dyndns.org","threadId":"26226","inReplyTo":"20110108214011.GA4753@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-09T07:31:18Z","receivedAt":"2011-01-09T07:31:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:\n>\n>> > Yeah, we generally resolve ambiguities in favor of the tag (and that\n>> > warning comes from deep within get_sha1_basic). So the real bug here is\n>> > that it still said \"Switched to branch\", which is totally wrong.\n>> > \n>> > That being said, it probably would make more sense for \"git checkout\" to\n>> > prefer branches to tags.\n>> \n>> What was the rationale for generally favoring tags?\n>\n> I don't recall hearing any specific argument, but it has always been\n> that way from early on. I think it is from a vague sense of \"tags are\n> more important than branch tips because they are about marking specific\n> points, not lines of development\". But maybe other old-timers can say\n> more.\n>\n> I don't necessarily buy that argument; my only reasoning is that we\n> should probably keep historic behavior.\n\nI don't think \"tags are more important\" has ever been a serious argument,\neither.  We prefix refs/tags/ and refs/heads/ to see if what the user gave\nus is a short hand, and we have to pick one to check first, and we\nhappened to have chosen to check tags/ before heads/.  Majority of people\nhave been trained by the ambiguity warning not to use the same name for\ntheir tags and branches, and the rest have learned to live with this\nconvention.\n\nAmong those \"rest who have learned to live with\" minority are people who\nuse v1.0 branch to maintain v1.0 codebase after it is tagged, and they\nwould want to work on v1.0 branch (by checking out v1.0 branch) and\nmeasure their progress by disambiguating between heads/v1.0 and tags/v1.0\nwhen driving \"git log\" family.  There is no strong reason to forbid them\nfrom doing this by requiring uniqueness if that is what they want,\nalthough I personally would suggest them to use maint-1.0 branch that\nforks from v1.0 tag.\n\nAside from your \"'checkout branch' is about checking out a branch\"\nexplanation, there are two reasons to favor branches over tags in\n\"checkout\" command:\n\n (1) You cannot disambiguate \"git checkout heads/master\" when you have\n     \"master\" tag, as this notation is used to tell the command \"I want to\n     detach HEAD at that commit\"; and\n\n (2) The command already treats an unadorned branch name specially by not\n     complaining ref/path ambiguity when you said \"git checkout master\"\n     and you have a file called \"master\" in your working tree, so users\n     already expect that an unadorned branch name is special to it.\n"},{"id":"159244","messageId":"alpine.DEB.1.10.1101090847120.12031@debian","threadId":"26226","inReplyTo":"7v1v4m8reh.fsf@alter.siamese.dyndns.org","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-09T16:18:09Z","receivedAt":"2011-01-09T16:18:09Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Sat, 8 Jan 2011, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Sat, Jan 08, 2011 at 03:40:33PM -0500, Martin von Zweigbergk wrote:\n> >\n> >> > Yeah, we generally resolve ambiguities in favor of the tag (and that\n> >> > warning comes from deep within get_sha1_basic). So the real bug here is\n> >> > that it still said \"Switched to branch\", which is totally wrong.\n> >> > \n> >> > That being said, it probably would make more sense for \"git checkout\" to\n> >> > prefer branches to tags.\n> >> \n> >> What was the rationale for generally favoring tags?\n> >\n> > I don't recall hearing any specific argument, but it has always been\n> > that way from early on. I think it is from a vague sense of \"tags are\n> > more important than branch tips because they are about marking specific\n> > points, not lines of development\". But maybe other old-timers can say\n> > more.\n> >\n> Aside from your \"'checkout branch' is about checking out a branch\"\n> explanation, there are two reasons to favor branches over tags in\n> \"checkout\" command:\n> \n>  (1) You cannot disambiguate \"git checkout heads/master\" when you have\n>      \"master\" tag, as this notation is used to tell the command \"I want to\n>      detach HEAD at that commit\"; and\n\nInteresting. I had no idea that 'git checkout heads/master' means to\ndetach the HEAD. Thanks.\n\nBy analogy, I guess that means that 'git rebase master heads/topic' is\nsupposed to rebase a detached HEAD, so I will stop trying to \"fix\"\nthat then :-)\n\n>  (2) The command already treats an unadorned branch name specially by not\n>      complaining ref/path ambiguity when you said \"git checkout master\"\n>      and you have a file called \"master\" in your working tree, so users\n>      already expect that an unadorned branch name is special to it.\n\nIf I understand correctly, that actually applies to tags as well.\nChecking out a tag called e.g. Makefile doesn't give any warnings or\nerrors.\n\n\n/Martin\n"},{"id":"159304","messageId":"20110111065207.GF10094@sigill.intra.peff.net","threadId":"26226","inReplyTo":"7vipy0483h.fsf@alter.siamese.dyndns.org","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-11T06:52:07Z","receivedAt":"2011-01-11T06:52:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 07, 2011 at 03:17:22PM -0800, Junio C Hamano wrote:\n\n> ... And this comes on top (should probably be squashed into one) to really\n> favor a branch over a tag.\n> \n>  builtin/checkout.c               |   26 ++++++++++----------------\n>  t/t2019-checkout-amiguous-ref.sh |    2 +-\n>  2 files changed, 11 insertions(+), 17 deletions(-)\n\nYeah, that looks sane to me (assuming all three patches squashed\ntogether). It took me a minute to figure out one subtlety, though:\n\n> +\t\tif ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||\n> +\t\t    !resolve_ref(new.path, rev, 1, NULL))\n> +\t\t\tnew.path = NULL; /* not an existing branch */\n> +\n> +\t\tif (!(new.commit = lookup_commit_reference_gently(rev, 1))) {\n\nWe are relying on the fact that resolve_ref leaves \"rev\" alone in the\ncase that it does not find anything. Which is mostly true (the only\nexception seems to be if you have a ref with non-hex garbage in it, in\nwhich case you will get some bogus sha1 in the output). I dunno if it is\nworth making it more explicit, like:\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex f6f6172..afff56f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -678,7 +678,7 @@ static const char *unique_tracking_name(const char *name)\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n-\tunsigned char rev[20];\n+\tunsigned char rev[20], branch_rev[20];\n \tconst char *arg;\n \tstruct branch_info new;\n \tstruct tree *source_tree = NULL;\n@@ -834,8 +834,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tnew.name = arg;\n \t\tsetup_branch_path(&new);\n \n-\t\tif ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||\n-\t\t    !resolve_ref(new.path, rev, 1, NULL))\n+\t\tif ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&\n+\t\t     resolve_ref(new.path, branch_rev, 1, NULL))\n+\t\t\thashcpy(rev, branch_rev);\n+\t\telse\n \t\t\tnew.path = NULL; /* not an existing branch */\n \n \t\tif (!(new.commit = lookup_commit_reference_gently(rev, 1))) {\n\nMy version somehow looks uglier, but I just worry about resolve_ref\nviolating this undocumented subtlety sometime in the future.\n\nAlso, one other question while we are on the subject. I think we all\nagree that \"git checkout $foo\" should prefer $foo as a branch. But what\nabout \"git checkout -b $branch $start_point\"? Should $start_point follow\nthe same \"prefer branches\" rule, or should it use the usual ref lookup\nrules?\n\nI was surprised to find that the current behavior is to die(), due to an\nexplicit case in branch.c:create_branch.\n\n-Peff\n"},{"id":"159306","messageId":"20110111065509.GG10094@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110107195417.GC6175@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-11T06:55:09Z","receivedAt":"2011-01-11T06:55:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 07, 2011 at 02:54:17PM -0500, Jeff King wrote:\n\n> +test_expect_success 'checkout reports switch to detached HEAD' '\n> +\tgrep \"Switched to branch\" stderr &&\n> +\t! grep \"^HEAD is now at\" stderr\n\nJunio, one minor fixup here. The test is correct, but the description\nshould read \"checkout reports switch to branch\", not \"...detached HEAD\".\n\nI had originally written the test the other way and forgot to update the\ndescription when I tweaked it. The error is in my ambiguity test, but\ngot cut-and-pasted to your vagueness test, too.\n\n-Peff\n"},{"id":"159347","messageId":"7vvd1v4bmt.fsf@alter.siamese.dyndns.org","threadId":"26226","inReplyTo":"20110111065207.GF10094@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-11T17:02:18Z","receivedAt":"2011-01-11T17:02:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jan 07, 2011 at 03:17:22PM -0800, Junio C Hamano wrote:\n>\n>> ... And this comes on top (should probably be squashed into one) to really\n>> favor a branch over a tag.\n>> \n>>  builtin/checkout.c               |   26 ++++++++++----------------\n>>  t/t2019-checkout-amiguous-ref.sh |    2 +-\n>>  2 files changed, 11 insertions(+), 17 deletions(-)\n>\n> Yeah, that looks sane to me (assuming all three patches squashed\n> together). It took me a minute to figure out one subtlety, though:\n>\n>> +\t\tif ((check_ref_format(new.path) != CHECK_REF_FORMAT_OK) ||\n>> +\t\t    !resolve_ref(new.path, rev, 1, NULL))\n>> +\t\t\tnew.path = NULL; /* not an existing branch */\n>> +\n>> +\t\tif (!(new.commit = lookup_commit_reference_gently(rev, 1))) {\n>\n> We are relying on the fact that resolve_ref leaves \"rev\" alone in the\n> case that it does not find anything. Which is mostly true (the only\n> exception seems to be if you have a ref with non-hex garbage in it, in\n> which case you will get some bogus sha1 in the output). I dunno if it is\n> worth making it more explicit, like:\n\nI've thought about it when I sent the patch.  I think this is safe as that\nparticular resolve is done on a full ref \"refs/heads/$something\" and upon\nseeing the first 'r' get_sha1_hex() would give up without touching rev[],\nbut I agree it is too subtle.\n\n> Also, one other question while we are on the subject. I think we all\n> agree that \"git checkout $foo\" should prefer $foo as a branch. But what\n> about \"git checkout -b $branch $start_point\"?\n\nThat has always been defined as a synonym for\n\n\tgit branch $branch $start_point && git checkout $branch\n\nso $start_point is just a random extended SHA-1 expression.\n\n> I was surprised to find that the current behavior is to die(), due to an\n> explicit case in branch.c:create_branch.\n\nGood eyes.  At that point, \"refname <start> is ambiguous.\"  warning has\nalready been issued, and there is no sane reason to die there.  I'd call\nit a bug.\n"},{"id":"159355","messageId":"20110111180208.GC1833@sigill.intra.peff.net","threadId":"26226","inReplyTo":"7vvd1v4bmt.fsf@alter.siamese.dyndns.org","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-11T18:02:08Z","receivedAt":"2011-01-11T18:02:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 11, 2011 at 09:02:18AM -0800, Junio C Hamano wrote:\n\n> > Also, one other question while we are on the subject. I think we all\n> > agree that \"git checkout $foo\" should prefer $foo as a branch. But what\n> > about \"git checkout -b $branch $start_point\"?\n> \n> That has always been defined as a synonym for\n> \n> \tgit branch $branch $start_point && git checkout $branch\n> \n> so $start_point is just a random extended SHA-1 expression.\n\nThat's what I would have expected, but I wanted to write a test to make\nsure it was the case.\n\nBut it's not. Even taking away the die, my second test here fails (built\non top of the three previous commits under discussion):\n\ndiff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh\nindex e2b330b..7a6b30b 100755\n--- a/t/t2019-checkout-amiguous-ref.sh\n+++ b/t/t2019-checkout-amiguous-ref.sh\n@@ -50,4 +50,13 @@ test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to detached HEAD'\n \t! grep \"^HEAD is now at\" stderr\n '\n \n+test_expect_success 'new branch from ambiguous start_point works' '\n+\tgit checkout -b newbranch ambiguity\n+'\n+\n+test_expect_success 'checkout chooses tag over branch for start_point' '\n+\techo tag >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_done\n\nFor bonus fun, doing this:\n\n  git branch newbranch ambiguity\n  git checkout newbranch\n\n_does_ prefer the branch. So it is checkout feeding create_branch() the\nmodified sha1. I'll see if I can dig further.\n\n-Peff\n"},{"id":"159368","messageId":"20110111192020.GA15608@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110111065509.GG10094@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-11T19:20:20Z","receivedAt":"2011-01-11T19:20:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 11, 2011 at 01:55:09AM -0500, Jeff King wrote:\n\n> On Fri, Jan 07, 2011 at 02:54:17PM -0500, Jeff King wrote:\n> \n> > +test_expect_success 'checkout reports switch to detached HEAD' '\n> > +\tgrep \"Switched to branch\" stderr &&\n> > +\t! grep \"^HEAD is now at\" stderr\n> \n> Junio, one minor fixup here. The test is correct, but the description\n> should read \"checkout reports switch to branch\", not \"...detached HEAD\".\n\nHmm. One other thing to note on these ambiguity tests. Apparently we did\nalready have a test for this in t7201 (which should perhaps be renamed\ninto the t20* area with the other checkout tests), but it was passing.\n\nThe problem is that the current behavior is way more broken than just\n\"accidentally choose tag over branch\". It actually writes the branch ref\ninto HEAD, but checks out the tree from the tag! The test in 7201 only\nchecks the former, but our new test checked the latter.\n\nProbably we should be checking both, just to be sure (and yes, with your\npatch it does the right thing):\n\ndiff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh\nindex e2b330b..606081b 100755\n--- a/t/t2019-checkout-amiguous-ref.sh\n+++ b/t/t2019-checkout-amiguous-ref.sh\n@@ -22,6 +22,9 @@ test_expect_success 'checkout produces ambiguity warning' '\n '\n \n test_expect_success 'checkout chooses branch over tag' '\n+\techo refs/heads/ambiguity >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual &&\n \techo branch >expect &&\n \ttest_cmp expect file\n '\n@@ -41,6 +44,9 @@ test_expect_success VAGUENESS_SUCCESS 'checkout produces ambiguity warning' '\n '\n \n test_expect_success VAGUENESS_SUCCESS 'checkout chooses branch over tag' '\n+\techo refs/heads/vagueness >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual &&\n \techo branch >expect &&\n \ttest_cmp expect file\n '\n"},{"id":"159377","messageId":"20110111200037.GA24777@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110111192020.GA15608@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-11T20:00:38Z","receivedAt":"2011-01-11T20:00:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 11, 2011 at 02:20:20PM -0500, Jeff King wrote:\n\n> Hmm. One other thing to note on these ambiguity tests.\n\nI was starting to get confused with all the back-and-forth, so here is a\nsquashed patch that contains everything discussed so far: my original\ntests, your squashed patches, my test fixups, and the less-subtle\nresolve_ref thing.\n\nI didn't bisect the breakage, so I'm not sure how far back it goes. But\nthis is potentially maint-worthy.\n\nThis doesn't include any tests or fixes for \"git checkout -b newbranch\nambiguous\". I'll work on that separately as a patch on top.\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nSubject: [PATCH] checkout: fix bug with ambiguous refs\n\nThe usual dwim_ref lookup prefers tags to branches. Because\ncheckout primarily works on branches, though, we switch that\nbehavior to prefer branches.\n\nHowever, there was a bug in the implementation in which we\nused lookup_commit_reference (which used the regular lookup\nrules) to get the actual commit to checkout. Checking out an\nambiguous ref therefore ended up putting us in an extremely\nbroken state in which we wrote the branch ref into HEAD, but\nactually checked out the tree for the tag.\n\nThis patch fixes the bug by always attempting to pull the\ncommit to be checked out from the branch-ified version of\nthe name we were given.\n\nPatch by Junio, tests and commit message from Jeff King.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/checkout.c               |   23 ++++++++------\n t/t2019-checkout-amiguous-ref.sh |   59 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 72 insertions(+), 10 deletions(-)\n create mode 100755 t/t2019-checkout-amiguous-ref.sh\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 757f9a0..d3cfaf0 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -678,7 +678,7 @@ static const char *unique_tracking_name(const char *name)\n int cmd_checkout(int argc, const char **argv, const char *prefix)\n {\n \tstruct checkout_opts opts;\n-\tunsigned char rev[20];\n+\tunsigned char rev[20], branch_rev[20];\n \tconst char *arg;\n \tstruct branch_info new;\n \tstruct tree *source_tree = NULL;\n@@ -832,18 +832,21 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\targc--;\n \n \t\tnew.name = arg;\n-\t\tif ((new.commit = lookup_commit_reference_gently(rev, 1))) {\n-\t\t\tsetup_branch_path(&new);\n+\t\tsetup_branch_path(&new);\n \n-\t\t\tif ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&\n-\t\t\t    resolve_ref(new.path, rev, 1, NULL))\n-\t\t\t\t;\n-\t\t\telse\n-\t\t\t\tnew.path = NULL;\n+\t\tif (check_ref_format(new.path) == CHECK_REF_FORMAT_OK &&\n+\t\t    resolve_ref(new.path, branch_rev, 1, NULL))\n+\t\t\thashcpy(rev, branch_rev);\n+\t\telse\n+\t\t\tnew.path = NULL; /* not an existing branch */\n+\n+\t\tif (!(new.commit = lookup_commit_reference_gently(rev, 1))) {\n+\t\t\t/* not a commit */\n+\t\t\tsource_tree = parse_tree_indirect(rev);\n+\t\t} else {\n \t\t\tparse_commit(new.commit);\n \t\t\tsource_tree = new.commit->tree;\n-\t\t} else\n-\t\t\tsource_tree = parse_tree_indirect(rev);\n+\t\t}\n \n \t\tif (!source_tree)                   /* case (1): want a tree */\n \t\t\tdie(\"reference is not a tree: %s\", arg);\ndiff --git a/t/t2019-checkout-amiguous-ref.sh b/t/t2019-checkout-amiguous-ref.sh\nnew file mode 100755\nindex 0000000..943541d\n--- /dev/null\n+++ b/t/t2019-checkout-amiguous-ref.sh\n@@ -0,0 +1,59 @@\n+#!/bin/sh\n+\n+test_description='checkout handling of ambiguous (branch/tag) refs'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ambiguous refs' '\n+\ttest_commit branch file &&\n+\tgit branch ambiguity &&\n+\tgit branch vagueness &&\n+\ttest_commit tag file &&\n+\tgit tag ambiguity &&\n+\tgit tag vagueness HEAD:file &&\n+\ttest_commit other file\n+'\n+\n+test_expect_success 'checkout ambiguous ref succeeds' '\n+\tgit checkout ambiguity >stdout 2>stderr\n+'\n+\n+test_expect_success 'checkout produces ambiguity warning' '\n+\tgrep \"warning.*ambiguous\" stderr\n+'\n+\n+test_expect_success 'checkout chooses branch over tag' '\n+\techo refs/heads/ambiguity >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\techo branch >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'checkout reports switch to branch' '\n+\tgrep \"Switched to branch\" stderr &&\n+\t! grep \"^HEAD is now at\" stderr\n+'\n+\n+test_expect_success 'checkout vague ref succeeds' '\n+\tgit checkout vagueness >stdout 2>stderr &&\n+\ttest_set_prereq VAGUENESS_SUCCESS\n+'\n+\n+test_expect_success VAGUENESS_SUCCESS 'checkout produces ambiguity warning' '\n+\tgrep \"warning.*ambiguous\" stderr\n+'\n+\n+test_expect_success VAGUENESS_SUCCESS 'checkout chooses branch over tag' '\n+\techo refs/heads/vagueness >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\techo branch >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success VAGUENESS_SUCCESS 'checkout reports switch to branch' '\n+\tgrep \"Switched to branch\" stderr &&\n+\t! grep \"^HEAD is now at\" stderr\n+'\n+\n+test_done\n-- \n1.7.3.5.4.g0dc52\n"},{"id":"159391","messageId":"20110112012515.GA30856@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110111180208.GC1833@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-12T01:25:15Z","receivedAt":"2011-01-12T01:25:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 11, 2011 at 01:02:08PM -0500, Jeff King wrote:\n\n> On Tue, Jan 11, 2011 at 09:02:18AM -0800, Junio C Hamano wrote:\n> \n> > > Also, one other question while we are on the subject. I think we all\n> > > agree that \"git checkout $foo\" should prefer $foo as a branch. But what\n> > > about \"git checkout -b $branch $start_point\"?\n> > \n> > That has always been defined as a synonym for\n> > \n> > \tgit branch $branch $start_point && git checkout $branch\n> > \n> > so $start_point is just a random extended SHA-1 expression.\n> \n> That's what I would have expected, but I wanted to write a test to make\n> sure it was the case.\n\nLooking into this more, I'm not sure what the right behavior is. The\noffending lines are in branch.c:create_branch:\n\n        switch (dwim_ref(start_name, strlen(start_name), sha1, &real_ref)) {\n        case 0:\n                /* Not branching from any existing branch */\n                if (explicit_tracking)\n                        die(\"Cannot setup tracking information; starting point is not a branch.\");\n                break;\n        case 1:\n                /* Unique completion -- good, only if it is a real ref */\n                if (explicit_tracking && !strcmp(real_ref, \"HEAD\"))\n                        die(\"Cannot setup tracking information; starting point is not a branch.\");\n                break;\n        default:\n                die(\"Ambiguous object name: '%s'.\", start_name);\n                break;\n        }\n\nThe die(\"Ambiguous...\") blames all the way back to 0746d19 (git-branch,\ngit-checkout: autosetup for remote branch tracking, 2007-03-08) and\nseems to just be a regression there that we didn't catch.\n\nObviously we can loosen the die() there and just handle the ambiguous\ncase like the unique case. But I'm not sure how tracking should interact\nwith the branch/tag thing.\n\nThis code seems to be trying to only track branches, but it fails at\nthat. It actually will track _any_ ref. So I can happily do:\n\n  git branch --track foo v1.5\n\nand start tracking refs/tags/v1.5. Is that a bug or a feature?\n\nAnd then that makes me wonder. What should:\n\n  git branch --track foo ambiguity\n\ndo? Should it choose the branch, because that is more useful for\ntracking? Or choose the tag? And if branch, then what should:\n\n  git branch foo ambiguity\n\ndo? It won't track a tag unless --track is given explicitly. Should it\nprefer a branch with implicit tracking, or an untracked tag?\n\nI'm not sure. And to be honest I don't really care, because I think\npeople with ambiguous refs are little bit crazy anyway (after all, in\nthe current code it simply calls die()). But I think there is some\nargument to be made that due to tracking, start_point is not _just_\na regular ref. We do care about its branchiness.\n\n-Peff\n"},{"id":"159398","messageId":"7vd3o232xk.fsf@alter.siamese.dyndns.org","threadId":"26226","inReplyTo":"20110112012515.GA30856@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-12T09:07:51Z","receivedAt":"2011-01-12T09:07:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm not sure. And to be honest I don't really care, because I think\n> people with ambiguous refs are little bit crazy anyway (after all, in\n> the current code it simply calls die()). But I think there is some\n> argument to be made that due to tracking, start_point is not _just_\n> a regular ref. We do care about its branchiness.\n\nI do not really care either myself, and if \n\n    git branch --track foo heads/ambiguity\n    git branch --track foo tags/ambiguity\n\nallows the user to differentiate between the branch and the tag, it would\nbe more than sufficient.\n"},{"id":"159399","messageId":"20110112091130.GC24920@pengutronix.de","threadId":"26226","inReplyTo":"20110107194909.GB6175@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2011-01-12T09:11:30Z","receivedAt":"2011-01-12T09:11:30Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hi Peff,\n\nOn Fri, Jan 07, 2011 at 02:49:09PM -0500, Jeff King wrote:\n> On Fri, Jan 07, 2011 at 11:46:50AM +0100, Uwe Kleine-König wrote:\n> \n> > \tukl@octopus:~/gsrc/linux-2.6$ git diff; git diff --cached\n> > \n> > \tukl@octopus:~/gsrc/linux-2.6$ git checkout sgu/mxs-amba-uart\n> > \twarning: refname 'sgu/mxs-amba-uart' is ambiguous.\n> > \tPrevious HEAD position was c13fb17... Merge commit '65e29a85a419f9a161ab0f09f9d69924e36d940e' into HEAD\n> > \tSwitched to branch 'sgu/mxs-amba-uart'\n> > \n> > OK, it might be a bad idea to have this ambiguity, still ...\n> > [...]\n> > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\n> > points to refs/heads/sgu/mxs-amba-uart\n> \n> Yeah, we generally resolve ambiguities in favor of the tag (and that\n> warning comes from deep within get_sha1_basic). So the real bug here is\n> that it still said \"Switched to branch\", which is totally wrong.\nI wonder how I can resolve the ambiguity when calling checkout.  (Well\napart from changing either branch name or tag name)\n\ngit checkout refs/heads/sgu/mxs-amba-uart results in a detached HEAD.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"},{"id":"159407","messageId":"20110112172751.GA31747@sigill.intra.peff.net","threadId":"26226","inReplyTo":"7vd3o232xk.fsf@alter.siamese.dyndns.org","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-12T17:27:51Z","receivedAt":"2011-01-12T17:27:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 12, 2011 at 01:07:51AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I'm not sure. And to be honest I don't really care, because I think\n> > people with ambiguous refs are little bit crazy anyway (after all, in\n> > the current code it simply calls die()). But I think there is some\n> > argument to be made that due to tracking, start_point is not _just_\n> > a regular ref. We do care about its branchiness.\n> \n> I do not really care either myself, and if \n> \n>     git branch --track foo heads/ambiguity\n>     git branch --track foo tags/ambiguity\n> \n> allows the user to differentiate between the branch and the tag, it would\n> be more than sufficient.\n\nIt does already. So I am inclined to leave it alone, then. I doubt\nanyone actually cares, and if they do, they will get an error message,\nafter which they can manually disambiguate themselves.\n\nSo let's leave it at the mega-patch I posted earlier.\n\n-Peff\n"},{"id":"159408","messageId":"20110112174600.GB31747@sigill.intra.peff.net","threadId":"26226","inReplyTo":"20110112091130.GC24920@pengutronix.de","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-01-12T17:46:00Z","receivedAt":"2011-01-12T17:46:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 12, 2011 at 10:11:30AM +0100, Uwe Kleine-König wrote:\n\n> > > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\n> > > points to refs/heads/sgu/mxs-amba-uart\n> > \n> > Yeah, we generally resolve ambiguities in favor of the tag (and that\n> > warning comes from deep within get_sha1_basic). So the real bug here is\n> > that it still said \"Switched to branch\", which is totally wrong.\n> I wonder how I can resolve the ambiguity when calling checkout.  (Well\n> apart from changing either branch name or tag name)\n> \n> git checkout refs/heads/sgu/mxs-amba-uart results in a detached HEAD.\n\nYou can't disambiguate to the branch without going to a detached HEAD in\nthe current code; it's just broken[1].\n\nWith the patch here:\n\n  http://article.gmane.org/gmane.comp.version-control.git/164986\n\nit will disambiguate to the branch by default, and if you want the tag,\nyou can do:\n\n  git checkout tags/sgu/mxs-amba-uart\n\n-Peff\n\n[1]: You can't do it with checkout, that is. You can still hack around\n     it with:\n\n        branch=refs/heads/sgu/mxs-amba-uart\n        git read-tree -m -u $branch &&\n        git symbolic-ref HEAD $branch\n\n     which is a simplified version of what checkout is doing.\n"},{"id":"159410","messageId":"20110112181937.GK24920@pengutronix.de","threadId":"26226","inReplyTo":"20110112174600.GB31747@sigill.intra.peff.net","subject":"Re: bug? in checkout with ambiguous refnames","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2011-01-12T18:19:37Z","receivedAt":"2011-01-12T18:19:37Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hi,\n\nOn Wed, Jan 12, 2011 at 12:46:00PM -0500, Jeff King wrote:\n> On Wed, Jan 12, 2011 at 10:11:30AM +0100, Uwe Kleine-König wrote:\n> > > > So working copy and cache are at refs/tags/sgu/mxs-amba-uart, HEAD\n> > > > points to refs/heads/sgu/mxs-amba-uart\n> > > \n> > > Yeah, we generally resolve ambiguities in favor of the tag (and that\n> > > warning comes from deep within get_sha1_basic). So the real bug here is\n> > > that it still said \"Switched to branch\", which is totally wrong.\n> > I wonder how I can resolve the ambiguity when calling checkout.  (Well\n> > apart from changing either branch name or tag name)\n> > \n> > git checkout refs/heads/sgu/mxs-amba-uart results in a detached HEAD.\n> \n> You can't disambiguate to the branch without going to a detached HEAD in\n> the current code; it's just broken[1].\n> \n> With the patch here:\n> \n>   http://article.gmane.org/gmane.comp.version-control.git/164986\n> \n> it will disambiguate to the branch by default, and if you want the tag,\n> you can do:\n> \n>   git checkout tags/sgu/mxs-amba-uart\nthat's fine.\n\n> [1]: You can't do it with checkout, that is. You can still hack around\n>      it with:\n> \n>         branch=refs/heads/sgu/mxs-amba-uart\n>         git read-tree -m -u $branch &&\n>         git symbolic-ref HEAD $branch\nI did\n\n\tgit checkout refs/heads/sgu/mxs-amba-uart\n\tgit symbolic-ref HEAD refs/heads/sgu/mxs-amba-uart\n\n:-)\n\nUwe\n> \n>      which is a simplified version of what checkout is doing.\n> \n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"}]}