{"thread":{"id":"35658","subject":"BUG: check-ref-format and rev-parse can not handle branches with an @ in their name combined with @{u}","startedAt":"2014-01-14T23:04:54Z","lastAt":"2014-01-15T21:03:35Z","messageCount":12,"participants":["Keith Derrick","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233112","messageId":"52D5C296.7050906@lge.com","threadId":"35658","inReplyTo":null,"subject":"BUG: check-ref-format and rev-parse can not handle branches with an @ in their name combined with @{u}","fromName":"Keith Derrick","fromEmail":"keith.derrick@lge.com","sentAt":"2014-01-14T23:04:54Z","receivedAt":"2014-01-14T23:04:54Z","isPatch":false,"sender":{"key":"keith.derrick@lge.com","avatar":null},"body":"I couldn't find a duplicate in the JIRA instance.\n\nAccording to the documentation of check-ref-format, a branch name such \nas @mybranch is valid. Yet 'git check-ref-format --branch @mybranch@{u}' \nclaims @mybranch is an invalid branch name. yet I am able to create the \nbranch (which would seem the obvious place to check for an invalid \nbranch name)\n\nPick any repository with an upstream remote (I cloned git itself from \ngithub). The following will reproduce the bug\n> $cd ~/play/gitsrc/\n> $\n> $git status\n> On branch master\n> Your branch is up-to-date with 'origin/master'.\n>\n> nothing to commit, working directory clean\n> $git checkout -t -b @mybranch origin/master\n> Branch @mybranch set up to track remote branch master from origin.\n> Switched to a new branch '@mybranch'\n> $git branch -av\n> * @mybranch             14598b9 Sync with 1.8.5.3\n>   master                14598b9 Sync with 1.8.5.3\n>   remotes/origin/HEAD   -> origin/master\n>   remotes/origin/maint  4224916 Git 1.8.5.3\n>   remotes/origin/master 14598b9 Sync with 1.8.5.3\n>   remotes/origin/next   b139ac2 Sync with master\n>   remotes/origin/pu     3d58c42 Merge branch 'ab/subtree-doc' into pu\n>   remotes/origin/todo   1066e10 What's cooking (2014/01 #02)\n> $git check-ref-format --branch @mybranch\n> @mybranch\n> $git check-ref-format --branch @mybranch@{u}\n> fatal: '@mybranch@{u}' is not a valid branch name\n> $git rev-parse --abbrev-ref @mybranch\n> @mybranch\n> $git rev-parse --abbrev-ref @mybranch@{u}\n> @mybranch@{u}\n> fatal: ambiguous argument '@mybranch@{u}': unknown revision or path \n> not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git <command> [<revision>...] -- [<file>...]'\n> $\n\nThe same problem appears if the single '@' is anywhere in the branch name.\n\nI *think* the problem lies in 'interpret_empty_at' in sha1_name.c with \nthese statements\n\n>     if (next != name + 1)\n>         return -1;\nWhich is trying to allow the sequence '@@' but I'm not certain enough of \nunintended consequences to suggest a patch.\n\nIn our real-world example, we wanted to use a naming convention where a \nbranch name beginning with @ was intended to be a long-lived branch (for \nexample, a support branch for an official release). Thus, our sequence \nabove actually begins with something like 'git checkout -t \norigin/@release1'.\n\nWe hit this problem some considerable time after initiating the naming \nconvention, so it's too late to turn back for us.\n\nAs a work around, if you have the branch checked out, then using HEAD \ninstead of the branch name works\n> $git checkout @mybranch\n> Switched to branch '@mybranch'\n> Your branch is up-to-date with 'origin/master'.\n> $git check-ref-format --branch HEAD@{u}\n> origin/master\n> $git rev-parse --abbrev-ref HEAD@{u}\n> origin/master\n> $\n\nEnvronment:\n\n> $git version\n> git version 1.8.5.2\n> $lsb_release -a\n> No LSB modules are available.\n> Distributor ID:    Ubuntu\n> Description:    Ubuntu 12.04.3 LTS\n> Release:    12.04\n> Codename:    precise\n\nKeith Derrick\n"},{"id":"233115","messageId":"xmqqvbxm2kmg.fsf@gitster.dls.corp.google.com","threadId":"35658","inReplyTo":"52D5C296.7050906@lge.com","subject":"Re: BUG: check-ref-format and rev-parse can not handle branches with an @ in their name combined with @{u}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-14T23:45:27Z","receivedAt":"2014-01-14T23:45:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Derrick <keith.derrick@lge.com> writes:\n\n> I couldn't find a duplicate in the JIRA instance.\n\nDon't worry, we do not use any JIRA instance ;-)\n\n> According to the documentation of check-ref-format, a branch name such \n> as @mybranch is valid.\n\nCorrect.\n\n> Yet 'git check-ref-format --branch @mybranch@{u}' \n> claims @mybranch is an invalid branch name.\n\nI do not think it claims any such thing.\n\n    $ git check-ref-format --branch @foo@{u}; echo $?\n    fatal: '@foo@{u}' is not a valid branch name\n    128\n    $ git check-ref-format --branch @foo; echo $?\n    @foo\n    0\n\nThe former asks \"Is the string '@foo@{u}' a suitable name to give a\nbranch?\" and the answer is no.  The latter asks the same question\nabout the string '@foo', and the answer is yes.\n\nSo I do not see any bug here.\n"},{"id":"233122","messageId":"20140115050003.GA27237@sigill.intra.peff.net","threadId":"35658","inReplyTo":"xmqqvbxm2kmg.fsf@gitster.dls.corp.google.com","subject":"Re: BUG: check-ref-format and rev-parse can not handle branches with an @ in their name combined with @{u}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T05:00:03Z","receivedAt":"2014-01-15T05:00:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 14, 2014 at 03:45:27PM -0800, Junio C Hamano wrote:\n\n> > Yet 'git check-ref-format --branch @mybranch@{u}' \n> > claims @mybranch is an invalid branch name.\n> \n> I do not think it claims any such thing.\n> \n>     $ git check-ref-format --branch @foo@{u}; echo $?\n>     fatal: '@foo@{u}' is not a valid branch name\n>     128\n>     $ git check-ref-format --branch @foo; echo $?\n>     @foo\n>     0\n> \n> The former asks \"Is the string '@foo@{u}' a suitable name to give a\n> branch?\" and the answer is no.  The latter asks the same question\n> about the string '@foo', and the answer is yes.\n\nIs that what \"--branch\" does? I have never used it, but the manpage\nseems to suggest it is about _parsing_ (which, IMHO, means it probably\nshould have been an option to rev-parse, but that is another issue\naltogether).\n\nSo a more interesting output than the above is:\n \n  $ git checkout -t -b @mybranch origin/master\n  Branch @mybranch set up to track remote branch master from origin.\n  Switched to a new branch '@mybranch'\n\n  $ git check-ref-format --branch @mybranch@{u}; echo $?\n  fatal: '@mybranch@{u}' is not a valid branch name\n  128\n\n  $ git check-ref-format --branch HEAD@{u}; echo $?\n  origin/master\n  0\n\nSo \"check-ref-format --branch\" does understand @{u} syntax. But it\ndoesn't like @mybranch@{u}. You can see the same problem with rev-parse:\n\n  $ git rev-parse --symbolic-full-name HEAD@{u}\n  refs/remotes/origin/master\n  $ git rev-parse --symbolic-full-name @mybranch@{u}\n  @mybranch@{u}\n  fatal: ambiguous argument '@mybranch@{u}': unknown revision or path\n  not in the working tree.\n\nSo I do think there is a bug. The interpret_branch_name parser somehow\ngets confused by the \"@\" in the name.\n\n-Peff\n"},{"id":"233126","messageId":"7v1u091ybx.fsf@alter.siamese.dyndns.org","threadId":"35658","inReplyTo":"20140115050003.GA27237@sigill.intra.peff.net","subject":"Re: BUG: check-ref-format and rev-parse can not handle branches with an @ in their name combined with @{u}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T07:46:58Z","receivedAt":"2014-01-15T07:46:58Z","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> Is that what \"--branch\" does? I have never used it, but the manpage\n> seems to suggest it is about _parsing_ (which, IMHO, means it probably\n> should have been an option to rev-parse, but that is another issue\n> altogether).\n\nAhh, of course you are right.  I never use it, and somehow thought\nit was just prepending refs/heads/ to its arguments, but it seems to\nwant to do a lot more than that.\n"},{"id":"233127","messageId":"20140115074758.GA8978@sigill.intra.peff.net","threadId":"35658","inReplyTo":"7v1u091ybx.fsf@alter.siamese.dyndns.org","subject":"Re: BUG: check-ref-format and rev-parse can not handle branches with an @ in their name combined with @{u}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T07:47:58Z","receivedAt":"2014-01-15T07:47:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 14, 2014 at 11:46:58PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Is that what \"--branch\" does? I have never used it, but the manpage\n> > seems to suggest it is about _parsing_ (which, IMHO, means it probably\n> > should have been an option to rev-parse, but that is another issue\n> > altogether).\n> \n> Ahh, of course you are right.  I never use it, and somehow thought\n> it was just prepending refs/heads/ to its arguments, but it seems to\n> want to do a lot more than that.\n\nI am just about done with a patch series to address this, and a few\nother related bugs I found. So don't look too hard; I should have\nsomething out in a few minutes.\n\n-Peff\n"},{"id":"233128","messageId":"20140115082528.GA18974@sigill.intra.peff.net","threadId":"35658","inReplyTo":"20140115050003.GA27237@sigill.intra.peff.net","subject":"[PATCH 0/5] interpret_branch_name bug potpourri","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T08:25:28Z","receivedAt":"2014-01-15T08:25:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2014 at 12:00:03AM -0500, Jeff King wrote:\n\n>   $ git rev-parse --symbolic-full-name HEAD@{u}\n>   refs/remotes/origin/master\n>   $ git rev-parse --symbolic-full-name @mybranch@{u}\n>   @mybranch@{u}\n>   fatal: ambiguous argument '@mybranch@{u}': unknown revision or path\n>   not in the working tree.\n> \n> So I do think there is a bug. The interpret_branch_name parser somehow\n> gets confused by the \"@\" in the name.\n\nThe \"somehow\" is because we only look for the first \"@\", and never\nconsider any possible marks after that. The series below fixes it, along\nwith two other bugs I found while looking at this code. Ugh. Remind me\nnever to look at our object name parser ever again.\n\nI feel pretty good that this is fixing real bugs and not regressing\nanything else. I would not be surprised if there are other weird things\nlurking, though. See the discussion in patch 4.\n\n  [1/5]: interpret_branch_name: factor out upstream handling\n  [2/5]: interpret_branch_name: rename \"cp\" variable to \"at\"\n  [3/5]: interpret_branch_name: always respect \"namelen\" parameter\n  [4/5]: interpret_branch_name: avoid @{upstream} past colon\n  [5/5]: interpret_branch_name: find all possible @-marks\n\n-Peff\n"},{"id":"233129","messageId":"20140115082632.GA19132@sigill.intra.peff.net","threadId":"35658","inReplyTo":"20140115082528.GA18974@sigill.intra.peff.net","subject":"[PATCH 1/5] interpret_branch_name: factor out upstream handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T08:26:33Z","receivedAt":"2014-01-15T08:26:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function checks a few different @{}-constructs. The\nearly part checks for and dispatches us to helpers for each\nconstruct, but the code for handling @{upstream} is inline.\n\nLet's factor this out into its own function. This makes\ninterpret_branch_name more readable, and will make it much\nsimpler to further refactor the function in future patches.\n\nWhile we're at it, let's also break apart the refactored\ncode into a few helper functions. These will be useful if we\neventually implement similar @{upstream}-like constructs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is identical to the cleanup I posted recently for the @{publish}\ntopic, though I did tweak the commit message a bit to make more sense in\nthe context of this series.\n\n sha1_name.c | 83 ++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 52 insertions(+), 31 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex a5578f7..5db742b 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1046,6 +1046,54 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu\n \treturn ret - used + len;\n }\n \n+static void set_shortened_ref(struct strbuf *buf, const char *ref)\n+{\n+\tchar *s = shorten_unambiguous_ref(ref, 0);\n+\tstrbuf_reset(buf);\n+\tstrbuf_addstr(buf, s);\n+\tfree(s);\n+}\n+\n+static const char *get_upstream_branch(const char *branch_buf, int len)\n+{\n+\tchar *branch = xstrndup(branch_buf, len);\n+\tstruct branch *upstream = branch_get(*branch ? branch : NULL);\n+\n+\t/*\n+\t * Upstream can be NULL only if branch refers to HEAD and HEAD\n+\t * points to something different than a branch.\n+\t */\n+\tif (!upstream)\n+\t\tdie(_(\"HEAD does not point to a branch\"));\n+\tif (!upstream->merge || !upstream->merge[0]->dst) {\n+\t\tif (!ref_exists(upstream->refname))\n+\t\t\tdie(_(\"No such branch: '%s'\"), branch);\n+\t\tif (!upstream->merge) {\n+\t\t\tdie(_(\"No upstream configured for branch '%s'\"),\n+\t\t\t\tupstream->name);\n+\t\t}\n+\t\tdie(\n+\t\t\t_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n+\t\t\tupstream->merge[0]->src);\n+\t}\n+\tfree(branch);\n+\n+\treturn upstream->merge[0]->dst;\n+}\n+\n+static int interpret_upstream_mark(const char *name, int namelen,\n+\t\t\t\t   int at, struct strbuf *buf)\n+{\n+\tint len;\n+\n+\tlen = upstream_mark(name + at, namelen - at);\n+\tif (!len)\n+\t\treturn -1;\n+\n+\tset_shortened_ref(buf, get_upstream_branch(name, at));\n+\treturn len + at;\n+}\n+\n /*\n  * This reads short-hand syntax that not only evaluates to a commit\n  * object name, but also can act as if the end user spelled the name\n@@ -1070,9 +1118,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu\n int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n {\n \tchar *cp;\n-\tstruct branch *upstream;\n \tint len = interpret_nth_prior_checkout(name, buf);\n-\tint tmp_len;\n \n \tif (!namelen)\n \t\tnamelen = strlen(name);\n@@ -1094,36 +1140,11 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \tif (len > 0)\n \t\treturn reinterpret(name, namelen, len, buf);\n \n-\ttmp_len = upstream_mark(cp, namelen - (cp - name));\n-\tif (!tmp_len)\n-\t\treturn -1;\n+\tlen = interpret_upstream_mark(name, namelen, cp - name, buf);\n+\tif (len > 0)\n+\t\treturn len;\n \n-\tlen = cp + tmp_len - name;\n-\tcp = xstrndup(name, cp - name);\n-\tupstream = branch_get(*cp ? cp : NULL);\n-\t/*\n-\t * Upstream can be NULL only if cp refers to HEAD and HEAD\n-\t * points to something different than a branch.\n-\t */\n-\tif (!upstream)\n-\t\tdie(_(\"HEAD does not point to a branch\"));\n-\tif (!upstream->merge || !upstream->merge[0]->dst) {\n-\t\tif (!ref_exists(upstream->refname))\n-\t\t\tdie(_(\"No such branch: '%s'\"), cp);\n-\t\tif (!upstream->merge) {\n-\t\t\tdie(_(\"No upstream configured for branch '%s'\"),\n-\t\t\t\tupstream->name);\n-\t\t}\n-\t\tdie(\n-\t\t\t_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n-\t\t\tupstream->merge[0]->src);\n-\t}\n-\tfree(cp);\n-\tcp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);\n-\tstrbuf_reset(buf);\n-\tstrbuf_addstr(buf, cp);\n-\tfree(cp);\n-\treturn len;\n+\treturn -1;\n }\n \n int strbuf_branchname(struct strbuf *sb, const char *name)\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233130","messageId":"20140115082732.GB19132@sigill.intra.peff.net","threadId":"35658","inReplyTo":"20140115082528.GA18974@sigill.intra.peff.net","subject":"[PATCH 2/5] interpret_branch_name: rename \"cp\" variable to \"at\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T08:27:32Z","receivedAt":"2014-01-15T08:27:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the original version of this function, \"cp\" acted as a\npointer to many different things. Since the refactoring in\nthe last patch, it only marks the at-sign in the string.\nLet's use a more descriptive variable name.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously can be squashed with the prior refactoring, but I think\nsplitting it makes the diffs easier to read.\n\n sha1_name.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 5db742b..958aa2e 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1117,7 +1117,7 @@ static int interpret_upstream_mark(const char *name, int namelen,\n  */\n int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n {\n-\tchar *cp;\n+\tchar *at;\n \tint len = interpret_nth_prior_checkout(name, buf);\n \n \tif (!namelen)\n@@ -1132,15 +1132,15 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\t\treturn reinterpret(name, namelen, len, buf);\n \t}\n \n-\tcp = strchr(name, '@');\n-\tif (!cp)\n+\tat = strchr(name, '@');\n+\tif (!at)\n \t\treturn -1;\n \n-\tlen = interpret_empty_at(name, namelen, cp - name, buf);\n+\tlen = interpret_empty_at(name, namelen, at - name, buf);\n \tif (len > 0)\n \t\treturn reinterpret(name, namelen, len, buf);\n \n-\tlen = interpret_upstream_mark(name, namelen, cp - name, buf);\n+\tlen = interpret_upstream_mark(name, namelen, at - name, buf);\n \tif (len > 0)\n \t\treturn len;\n \n-- \n1.8.5.2.500.g8060133\n"},{"id":"233131","messageId":"20140115083157.GC19132@sigill.intra.peff.net","threadId":"35658","inReplyTo":"20140115082528.GA18974@sigill.intra.peff.net","subject":"[PATCH 3/5] interpret_branch_name: always respect \"namelen\" parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T08:31:57Z","receivedAt":"2014-01-15T08:31:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"interpret_branch_name gets passed a \"name\" buffer to parse,\nalong with a \"namelen\" parameter representing its length. If\n\"namelen\" is zero, we fallback to the NUL-terminated\nstring-length of \"name\".\n\nHowever, it does not necessarily follow that if we have\ngotten a non-zero \"namelen\", it is the NUL-terminated\nstring-length of \"name\". E.g., when get_sha1() is parsing\n\"foo:bar\", we will be asked to operate only on the first\nthree characters.\n\nYet in interpret_branch_name and its helpers, we use string\nfunctions like strchr() to operate on \"name\", looking past\nthe length we were given.  This can result in us mis-parsing\nobject names.  We should instead be limiting our search to\n\"namelen\" bytes.\n\nThere are three distinct types of object names this patch\naddresses:\n\n  - The intrepret_empty_at helper uses strchr to find the\n    next @-expression after our potential empty-at.  In an\n    expression like \"@:foo@bar\", it erroneously thinks that\n    the second \"@\" is relevant, even if we were asked only\n    to look at the first character. This case is easy to\n    trigger (and we test it in this patch).\n\n  - When finding the initial @-mark for @{upstream}, we use\n    strchr.  This means we might treat \"foo:@{upstream}\" as\n    the upstream for \"foo:\", even though we were asked only\n    to look at \"foo\". We cannot test this one in practice,\n    because it is masked by another bug (which is fixed in\n    the next patch).\n\n  - The interpret_nth_prior_checkout helper did not receive\n    the name length at all. This turns out not to be a\n    problem in practice, though, because its parsing is so\n    limited: it always starts from the far-left of the\n    string, and will not tolerate a colon (which is\n    currently the only way to get a smaller-than-strlen\n    \"namelen\"). However, it's still worth fixing to make the\n    code more obviously correct, and to future-proof us\n    against callers with more exotic buffers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_name.c                | 17 ++++++++++-------\n t/t1508-at-combinations.sh | 15 ++++++++++++++-\n 2 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 958aa2e..6d5038d 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -430,7 +430,7 @@ static inline int upstream_mark(const char *string, int len)\n }\n \n static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags);\n-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);\n+static int interpret_nth_prior_checkout(const char *name, int namelen, struct strbuf *buf);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n {\n@@ -492,7 +492,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint detached;\n \n-\t\tif (interpret_nth_prior_checkout(str, &buf) > 0) {\n+\t\tif (interpret_nth_prior_checkout(str, len, &buf) > 0) {\n \t\t\tdetached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));\n \t\t\tstrbuf_release(&buf);\n \t\t\tif (detached)\n@@ -929,7 +929,8 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n  * Parse @{-N} syntax, return the number of characters parsed\n  * if successful; otherwise signal an error with negative value.\n  */\n-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n+static int interpret_nth_prior_checkout(const char *name, int namelen,\n+\t\t\t\t\tstruct strbuf *buf)\n {\n \tlong nth;\n \tint retval;\n@@ -937,9 +938,11 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n \tconst char *brace;\n \tchar *num_end;\n \n+\tif (namelen < 4)\n+\t\treturn -1;\n \tif (name[0] != '@' || name[1] != '{' || name[2] != '-')\n \t\treturn -1;\n-\tbrace = strchr(name, '}');\n+\tbrace = memchr(name, '}', namelen);\n \tif (!brace)\n \t\treturn -1;\n \tnth = strtol(name + 3, &num_end, 10);\n@@ -1012,7 +1015,7 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str\n \t\treturn -1;\n \n \t/* make sure it's a single @, or @@{.*}, not @foo */\n-\tnext = strchr(name + len + 1, '@');\n+\tnext = memchr(name + len + 1, '@', namelen - len - 1);\n \tif (next && next[1] != '{')\n \t\treturn -1;\n \tif (!next)\n@@ -1118,7 +1121,7 @@ static int interpret_upstream_mark(const char *name, int namelen,\n int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n {\n \tchar *at;\n-\tint len = interpret_nth_prior_checkout(name, buf);\n+\tint len = interpret_nth_prior_checkout(name, namelen, buf);\n \n \tif (!namelen)\n \t\tnamelen = strlen(name);\n@@ -1132,7 +1135,7 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\t\treturn reinterpret(name, namelen, len, buf);\n \t}\n \n-\tat = strchr(name, '@');\n+\tat = memchr(name, '@', namelen);\n \tif (!at)\n \t\treturn -1;\n \ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex ceb8449..078e119 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -9,8 +9,11 @@ check() {\n \t\tif test '$2' = 'commit'\n \t\tthen\n \t\t\tgit log -1 --format=%s '$1' >actual\n-\t\telse\n+\t\telif test '$2' = 'ref'\n+\t\tthen\n \t\t\tgit rev-parse --symbolic-full-name '$1' >actual\n+\t\telse\n+\t\t\tgit cat-file -p '$1' >actual\n \t\tfi &&\n \t\ttest_cmp expect actual\n \t\"\n@@ -82,4 +85,14 @@ check HEAD ref refs/heads/old-branch\n check \"HEAD@{1}\" commit new-two\n check \"@{1}\" commit old-one\n \n+test_expect_success 'create path with @' '\n+\techo content >normal &&\n+\techo content >fun@ny &&\n+\tgit add normal fun@ny &&\n+\tgit commit -m \"funny path\"\n+'\n+\n+check \"@:normal\" blob content\n+check \"@:fun@ny\" blob content\n+\n test_done\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233132","messageId":"20140115083723.GD19132@sigill.intra.peff.net","threadId":"35658","inReplyTo":"20140115082528.GA18974@sigill.intra.peff.net","subject":"[PATCH 4/5] interpret_branch_name: avoid @{upstream} past colon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T08:37:23Z","receivedAt":"2014-01-15T08:37:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"get_sha1() cannot currently parse a valid object name like\n\"HEAD:@{upstream}\" (assuming that such an oddly named file\nexists in the HEAD commit). It takes two passes to parse the\nstring:\n\n  1. It first considers the whole thing as a ref, which\n     results in looking for the upstream of \"HEAD:\".\n\n  2. It finds the colon, parses \"HEAD\" as a tree-ish, and then\n     finds the path \"@{upstream}\" in the tree.\n\nFor a path that looks like a normal reflog (e.g.,\n\"HEAD:@{yesterday}\"), the first pass is a no-op. We try to\ndwim_ref(\"HEAD:\"), that returns zero refs, and we proceed\nwith colon-parsing.\n\nFor \"HEAD:@{upstream}\", though, the first pass ends up in\ninterpret_upstream_mark, which tries to find the branch\n\"HEAD:\". When it sees that the branch does not exist, it\nactually dies rather than returning an error to the caller.\nAs a result, we never make it to the second pass.\n\nOne obvious way of fixing this would be to teach\ninterpret_upstream_mark to simply report \"no, this isn't an\nupstream\" in such a case. However, that would make the\nerror-reporting for legitimate upstream cases significantly\nworse. Something like \"bogus@{upstream}\" would simply report\n\"unknown revision: bogus@{upstream}\", while the current code\ndiagnoses a wide variety of possible misconfigurations (no\nsuch branch, branch exists but does not have upstream, etc).\n\nHowever, we can take advantage of the fact that a branch\nname cannot contain a colon. Therefore even if we find an\nupstream mark, any prefix with a colon must mean that\nthe upstream mark we found is actually a pathname, and\nshould be disregarded completely. This patch implements that\nlogic.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think this would actually be cleaner if get_sha1() simply did the\ncolon-parsing first, and omitted the first pass completely. Then\nsub-functions would not have to care about arbitrary junk that can come\nin paths; they would always be parsing just the revision-specifier.\n\nHowever, given the way this code has developed over the years and its\ngeneral fragility, I would be entirely unsurprised if there is some case\nthat relies on the current scheme. So I went with the simple fix here,\nwhich should be much less likely to have any fallout. And I could not\ncome up with an example that is actually broken under the current code\n(we just do some suboptimal things, like looking for \"foo:bar\" as a ref\nin the filesystem, even though it is syntactically bogus).\n\n sha1_name.c                   |  3 +++\n t/t1507-rev-parse-upstream.sh | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6d5038d..b253a88 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1093,6 +1093,9 @@ static int interpret_upstream_mark(const char *name, int namelen,\n \tif (!len)\n \t\treturn -1;\n \n+\tif (memchr(name, ':', at))\n+\t\treturn -1;\n+\n \tset_shortened_ref(buf, get_upstream_branch(name, at));\n \treturn len + at;\n }\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 2a19e79..cace1ca 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -210,4 +210,20 @@ test_expect_success 'log -g other@{u}@{now}' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '@{reflog}-parsing does not look beyond colon' '\n+\techo content >@{yesterday} &&\n+\tgit add @{yesterday} &&\n+\tgit commit -m \"funny reflog file\" &&\n+\tgit hash-object @{yesterday} >expect &&\n+\tgit rev-parse HEAD:@{yesterday} >actual\n+'\n+\n+test_expect_success '@{upstream}-parsing does not look beyond colon' '\n+\techo content >@{upstream} &&\n+\tgit add @{upstream} &&\n+\tgit commit -m \"funny upstream file\" &&\n+\tgit hash-object @{upstream} >expect &&\n+\tgit rev-parse HEAD:@{upstream} >actual\n+'\n+\n test_done\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233133","messageId":"20140115084045.GE19132@sigill.intra.peff.net","threadId":"35658","inReplyTo":"20140115082528.GA18974@sigill.intra.peff.net","subject":"[PATCH 5/5] interpret_branch_name: find all possible @-marks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-15T08:40:46Z","receivedAt":"2014-01-15T08:40:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse a string like \"foo@{upstream}\", we look for\nthe first \"@\"-sign, and check to see if it is an upstream\nmark. However, since branch names can contain an @, we may\nalso see \"@foo@{upstream}\". In this case, we check only the\nfirst @, and ignore the second. As a result, we do not find\nthe upstream.\n\nWe can solve this by iterating through all @-marks in the\nstring, and seeing if any is a legitimate upstream or\nempty-at mark.\n\nAnother strategy would be to parse from the right-hand side\nof the string. However, that does not work for the\n\"empty_at\" case, which allows \"@@{upstream}\". We need to\nfind the left-most one in this case (and we then recurse as\n\"HEAD@{upstream}\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnd this one actually fixes Keith's bug.\n\nThe diff is noisy due to indentation changes; try it with \"-b\" for\nincreased reading pleasure.\n\n sha1_name.c                   | 20 +++++++++++---------\n t/t1507-rev-parse-upstream.sh | 21 +++++++++++++++++++++\n 2 files changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex b253a88..6fca869 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1124,6 +1124,7 @@ static int interpret_upstream_mark(const char *name, int namelen,\n int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n {\n \tchar *at;\n+\tconst char *start;\n \tint len = interpret_nth_prior_checkout(name, namelen, buf);\n \n \tif (!namelen)\n@@ -1138,17 +1139,18 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\t\treturn reinterpret(name, namelen, len, buf);\n \t}\n \n-\tat = memchr(name, '@', namelen);\n-\tif (!at)\n-\t\treturn -1;\n+\tfor (start = name;\n+\t     (at = memchr(start, '@', namelen - (start - name)));\n+\t     start = at + 1) {\n \n-\tlen = interpret_empty_at(name, namelen, at - name, buf);\n-\tif (len > 0)\n-\t\treturn reinterpret(name, namelen, len, buf);\n+\t\tlen = interpret_empty_at(name, namelen, at - name, buf);\n+\t\tif (len > 0)\n+\t\t\treturn reinterpret(name, namelen, len, buf);\n \n-\tlen = interpret_upstream_mark(name, namelen, at - name, buf);\n-\tif (len > 0)\n-\t\treturn len;\n+\t\tlen = interpret_upstream_mark(name, namelen, at - name, buf);\n+\t\tif (len > 0)\n+\t\t\treturn len;\n+\t}\n \n \treturn -1;\n }\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex cace1ca..178694e 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -17,6 +17,9 @@ test_expect_success 'setup' '\n \t test_commit 4 &&\n \t git branch --track my-side origin/side &&\n \t git branch --track local-master master &&\n+\t git branch --track fun@ny origin/side &&\n+\t git branch --track @funny origin/side &&\n+\t git branch --track funny@ origin/side &&\n \t git remote add -t master master-only .. &&\n \t git fetch master-only &&\n \t git branch bad-upstream &&\n@@ -54,6 +57,24 @@ test_expect_success 'my-side@{upstream} resolves to correct full name' '\n \ttest refs/remotes/origin/side = \"$(full_name my-side@{u})\"\n '\n \n+test_expect_success 'upstream of branch with @ in middle' '\n+\tfull_name fun@ny@{u} >actual &&\n+\techo refs/remotes/origin/side >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'upstream of branch with @ at start' '\n+\tfull_name @funny@{u} >actual &&\n+\techo refs/remotes/origin/side >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'upstream of branch with @ at end' '\n+\tfull_name funny@@{u} >actual &&\n+\techo refs/remotes/origin/side >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '\n \ttest_must_fail full_name refs/heads/my-side@{upstream}\n '\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233169","messageId":"xmqqsisp0xg8.fsf@gitster.dls.corp.google.com","threadId":"35658","inReplyTo":"20140115082528.GA18974@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] interpret_branch_name bug potpourri","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-15T21:03:35Z","receivedAt":"2014-01-15T21:03:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jan 15, 2014 at 12:00:03AM -0500, Jeff King wrote:\n>\n>>   $ git rev-parse --symbolic-full-name HEAD@{u}\n>>   refs/remotes/origin/master\n>>   $ git rev-parse --symbolic-full-name @mybranch@{u}\n>>   @mybranch@{u}\n>>   fatal: ambiguous argument '@mybranch@{u}': unknown revision or path\n>>   not in the working tree.\n>> \n>> So I do think there is a bug. The interpret_branch_name parser somehow\n>> gets confused by the \"@\" in the name.\n>\n> The \"somehow\" is because we only look for the first \"@\", and never\n> consider any possible marks after that. The series below fixes it, along\n> with two other bugs I found while looking at this code. Ugh. Remind me\n> never to look at our object name parser ever again.\n>\n> I feel pretty good that this is fixing real bugs and not regressing\n> anything else. I would not be surprised if there are other weird things\n> lurking, though. See the discussion in patch 4.\n>\n>   [1/5]: interpret_branch_name: factor out upstream handling\n>   [2/5]: interpret_branch_name: rename \"cp\" variable to \"at\"\n>   [3/5]: interpret_branch_name: always respect \"namelen\" parameter\n>   [4/5]: interpret_branch_name: avoid @{upstream} past colon\n>   [5/5]: interpret_branch_name: find all possible @-marks\n>\n> -Peff\n\nAll the steps looked very sensible.  Thanks for a pleasant read.\n"}]}