{"thread":{"id":"27549","subject":"[BUG] \"git checkout -b\" erronously thinks a branch already exists","startedAt":"2011-06-05T11:05:13Z","lastAt":"2011-06-06T16:05:38Z","messageCount":5,"participants":["Stefano Lattarini","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"169314","messageId":"201106051305.13723.stefano.lattarini@gmail.com","threadId":"27549","inReplyTo":null,"subject":"[BUG] \"git checkout -b\" erronously thinks a branch already exists","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2011-06-05T11:05:13Z","receivedAt":"2011-06-05T11:05:13Z","isPatch":false,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"First of all, sorry if this is the wrong address for bug reporting,\nbut I couldn't find any `git-bug' mailing list, nor a bug tracker.\n\nApparently, \"git checkout -b NAME\" fails if NAME contains the output\nfrom \"git-describe\".  This happens for me both with git 1.7.2.3\ninstalled from debian packages, and with the latest developement\nversion of git.\n\nThe attached script demonstrate the failures; here is what it outputs\non my system:\n\n $ sh bug.sh \n + GIT=git\n + mkdir foo.dir\n + cd foo.dir\n + git init\n Initialized empty Git repository in /tmp/foo.dir/.git/\n + :\n + git add f1\n + git commit -m none\n [master (root-commit) 9b06795] none\n  0 files changed, 0 insertions(+), 0 deletions(-)\n  create mode 100644 f1\n + git tag -a -m 'first commit' v0\n + :\n + git add f2\n + git commit -m none\n [master ef5d975] none\n  0 files changed, 0 insertions(+), 0 deletions(-)\n  create mode 100644 f2\n ++ git describe\n + desc=v0-1-gef5d975\n + git checkout -b fix-v0-1-gef5d975\n fatal: git checkout: branch fix-v0-1-gef5d975 already exists\n\nIf you need more details, or if you can explain why this is an user error,\nplease keep me in CC:, as I'm not subscribed to this list.  In case it is\nindeed an user error, sorry for the noise.\n\nRegards,\n  Stefano\n"},{"id":"169332","messageId":"7v62ojbuyh.fsf@alter.siamese.dyndns.org","threadId":"27549","inReplyTo":"201106051305.13723.stefano.lattarini@gmail.com","subject":"Re* [BUG] \"git checkout -b\" erronously thinks a branch already exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-06T01:38:14Z","receivedAt":"2011-06-06T01:38:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Interesting.\n\nThis is because \"git checkout -b <name>\" tries to make sure that\n\"refs/heads/<name>\" does not exist, and the way it does it to pass it\nto get_sha1() and make sure it fails.\n\nIt almost always works, except when that string is passed from get_sha1()\nto get_describe_name(), which takes \"ANYTHING-g<hexstring>\" and as long as\nhexstring names an existing object, it says \"Yup, I found one.\"\n\nI think the right fix is to make sure that \"refs/heads/<name>\" does not\nexist by checking exactly that.\n\nPerhaps something like this.  I am not sure if we want to use the \"yield\nnon zero to signal not an error but an early return\" trick like I did in\nthis patch, though.\n\n---\n\n builtin/checkout.c |    2 +-\n refs.c             |   14 ++++++++++++++\n refs.h             |    1 +\n 3 files changed, 16 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 28cdc51..af1e7b5 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1071,7 +1071,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tif (strbuf_check_branch_ref(&buf, opts.new_branch))\n \t\t\tdie(_(\"git checkout: we do not like '%s' as a branch name.\"),\n \t\t\t    opts.new_branch);\n-\t\tif (!get_sha1(buf.buf, rev)) {\n+\t\tif (ref_exists(buf.buf)) {\n \t\t\topts.branch_exists = 1;\n \t\t\tif (!opts.new_branch_force)\n \t\t\t\tdie(_(\"git checkout: branch %s already exists\"),\ndiff --git a/refs.c b/refs.c\nindex e3c0511..138b1a0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1826,6 +1826,20 @@ int update_ref(const char *action, const char *refname,\n \treturn 0;\n }\n \n+static int compare_ref_name(const char *refname,\n+\t\t\t    const unsigned char *sha1, int flag,\n+\t\t\t    void *cb_data)\n+{\n+\tif (!strcmp(cb_data, refname))\n+\t\treturn -1; /* early return */\n+\treturn 0;\n+}\n+\n+int ref_exists(char *refname)\n+{\n+\treturn for_each_ref(compare_ref_name, refname) != 0;\n+}\n+\n struct ref *find_ref_by_name(const struct ref *list, const char *name)\n {\n \tfor ( ; list; list = list->next)\ndiff --git a/refs.h b/refs.h\nindex 5e7a9a5..5839487 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -35,6 +35,7 @@ extern int for_each_ref_in_submodule(const char *submodule, const char *prefix,\n extern int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n extern int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n extern int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);\n+extern int ref_exists(char *);\n \n static inline const char *has_glob_specials(const char *pattern)\n {\n"},{"id":"169333","messageId":"7v1uz7bkc9.fsf@alter.siamese.dyndns.org","threadId":"27549","inReplyTo":"7v62ojbuyh.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [BUG] \"git checkout -b\" erronously thinks a branch already exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-06T05:27:34Z","receivedAt":"2011-06-06T05:27:34Z","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> I think the right fix is to make sure that \"refs/heads/<name>\" does not\n> exist by checking exactly that.\n>\n> Perhaps something like this.  I am not sure if we want to use the \"yield\n> non zero to signal not an error but an early return\" trick like I did in\n> this patch, though.\n\nLet's do this instead. I don't know what I was thinking when I wrote that\ninefficient \"loop refs to see if there is that one\" patch.\n\n-- >8 --\nSubject: [PATCH] checkout -b <name>: correctly detect existing branch\n\nWhen create a new branch, we fed \"refs/heads/<proposed name>\" as a string\nto get_sha1() and expected it to fail when a branch already exists.\n\nThe right way to check if a ref exists is to check with resolve_ref().\n\nA naïve solution that might appear attractive but does not work is to\nforbid slashes in get_describe_name() but that will not work. A describe\nname is is in the form of \"ANYTHING-g<short sha1>\", and that ANYTHING part\ncomes from a original tag name used in the repository the user ran the\ndescribe command. A sick user could have a confusing hierarchical tag\nwhose name is \"refs/heads/foobar\" (stored as refs/tags/refs/heads/foobar\")\nto generate a describe name \"refs/heads/foobar-6-g02ac983\", and we should\nbe able to use that name to refer to the object whose name is 02ac983.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/checkout.c         |    2 +-\n refs.c                     |    6 ++++++\n refs.h                     |    1 +\n t/t2018-checkout-branch.sh |   11 +++++++++++\n 4 files changed, 19 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 4ad7427..88708d4 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -874,7 +874,7 @@ no_reference:\n \t\tif (strbuf_check_branch_ref(&buf, opts.new_branch))\n \t\t\tdie(\"git checkout: we do not like '%s' as a branch name.\",\n \t\t\t    opts.new_branch);\n-\t\tif (!get_sha1(buf.buf, rev)) {\n+\t\tif (ref_exists(buf.buf)) {\n \t\t\topts.branch_exists = 1;\n \t\t\tif (!opts.new_branch_force)\n \t\t\t\tdie(\"git checkout: branch %s already exists\",\ndiff --git a/refs.c b/refs.c\nindex 6f486ae..92cd0d1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1732,6 +1732,12 @@ int update_ref(const char *action, const char *refname,\n \treturn 0;\n }\n \n+int ref_exists(char *refname)\n+{\n+\tunsigned char sha1[20];\n+\treturn !!resolve_ref(refname, sha1, 1, NULL);\n+}\n+\n struct ref *find_ref_by_name(const struct ref *list, const char *name)\n {\n \tfor ( ; list; list = list->next)\ndiff --git a/refs.h b/refs.h\nindex 762ce50..070a7d9 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -46,6 +46,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn\n  */\n extern void add_extra_ref(const char *refname, const unsigned char *sha1, int flags);\n extern void clear_extra_refs(void);\n+extern int ref_exists(char *);\n \n extern int peel_ref(const char *, unsigned char *);\n \ndiff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh\nindex 1caffea..741d842 100755\n--- a/t/t2018-checkout-branch.sh\n+++ b/t/t2018-checkout-branch.sh\n@@ -163,4 +163,15 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes\n \ttest_must_fail test_dirty_mergeable\n '\n \n+test_expect_success 'checkout -b <describe>' '\n+\tgit tag -f -m \"First commit\" initial initial &&\n+\tgit checkout -f change1 &&\n+\tname=$(git describe) &&\n+\tgit checkout -b $name &&\n+\tgit diff --exit-code change1 &&\n+\techo \"refs/heads/$name\" >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.6.rc0.106.g68174\n"},{"id":"169346","messageId":"201106061026.38996.stefano.lattarini@gmail.com","threadId":"27549","inReplyTo":"7v1uz7bkc9.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [BUG] \"git checkout -b\" erronously thinks a branch already exists","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2011-06-06T08:26:38Z","receivedAt":"2011-06-06T08:26:38Z","isPatch":false,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"Hello Junio, and thanks for your quick answer.\n\nOn Monday 06 June 2011, Junio C wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > I think the right fix is to make sure that \"refs/heads/<name>\" does not\n> > exist by checking exactly that.\n> >\n> > Perhaps something like this.  I am not sure if we want to use the \"yield\n> > non zero to signal not an error but an early return\" trick like I did in\n> > this patch, though.\n> \n> Let's do this instead. I don't know what I was thinking when I wrote that\n> inefficient \"loop refs to see if there is that one\" patch.\n> \n> [CUT PATCH]\n>\nI can confirm this change fixes the problem for me.\n\nThanks,\n  Stefano\n"},{"id":"169372","messageId":"20110606160538.GA11485@sigill.intra.peff.net","threadId":"27549","inReplyTo":"7v1uz7bkc9.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [BUG] \"git checkout -b\" erronously thinks a branch already exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-06T16:05:38Z","receivedAt":"2011-06-06T16:05:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 05, 2011 at 10:27:34PM -0700, Junio C Hamano wrote:\n\n> Let's do this instead. I don't know what I was thinking when I wrote that\n> inefficient \"loop refs to see if there is that one\" patch.\n> \n> -- >8 --\n> Subject: [PATCH] checkout -b <name>: correctly detect existing branch\n\nYeah, I did a double-take seeing your other patch and wondering why you\nweren't just using resolve_ref.\n\nSo this patch looks good to me.\n\n> +int ref_exists(char *refname)\n> +{\n> +\tunsigned char sha1[20];\n> +\treturn !!resolve_ref(refname, sha1, 1, NULL);\n> +}\n\nI was tempted to suggest that ref_exists could be used in lots of other\nplaces to make the code slightly more readable. But in many cases, the\nvariable holding the dummy sha1 cannot go away (because it is used\nelsewhere in the function), which means manually figuring out whether or\nnot the sha1 was actually a dummy or not[1]. So it's probably not worth\nthe effort for such minor gain.\n\n-Peff\n\n[1] For example, the one in create_branch is OK to change, but the one\nin delete_branch is not.\n"}]}