{"thread":{"id":"45233","subject":"[BUG] branch renamed to 'HEAD'","startedAt":"2017-02-27T04:53:18Z","lastAt":"2017-02-28T08:08:27Z","messageCount":14,"participants":["Luc Van Oostenryck","Karthik Nayak","Jeff King","Junio C Hamano","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"312702","messageId":"20170227045257.yazqlrqlnggosi5t@macbook.local","threadId":"45233","inReplyTo":null,"subject":"[BUG] branch renamed to 'HEAD'","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2017-02-27T04:52:58Z","receivedAt":"2017-02-27T04:53:18Z","isPatch":false,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"Hi,\n\nI just discover something which very much seems a bug to me\nwhile making an error in renaming a branch.\nThe scenario is the following:\n- I have a branch named 'orig'\n- I want to make some experimental changes on it:\n\t$ git checkout -b temp orig\n\t$ ... edit some files ...\n\t$ ... make some tests & commits ...\n- I'm happy with my changes, so I want to have my original\n branch to now points to the head of this temp branch\n but did it wrongly:\n\t$ git branch -m -f orig @\n- Now I discover that I don't have anymore a branch named 'orig'\n That's fine, I made an error.\n- I'm searching what had happened and discover the name my branch \n have been renamed to: 'HEAD'\n In others words I have now an entry .git/refs/heads/HEAD\n which points to where my original branch pointed.\n\nIn my opinion, it's a bug that '@' have been expanded/resolved\ninto a branch named 'HEAD'.\n\n\nLuc Van Oostenryck\n"},{"id":"312703","messageId":"CAOLa=ZSyQg9uoZWADOMYc90U-5AR9Lfii9mjLre0m0FQCSqfxg@mail.gmail.com","threadId":"45233","inReplyTo":"20170227045257.yazqlrqlnggosi5t@macbook.local","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2017-02-27T06:13:46Z","receivedAt":"2017-02-27T06:41:23Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello,\n\nThanks for reporting, but I don't think it is a bug.\n\nOn Mon, Feb 27, 2017 at 10:22 AM, Luc Van Oostenryck\n wrote:\n> Hi,\n>\n> I just discover something which very much seems a bug to me\n> while making an error in renaming a branch.\n> The scenario is the following:\n> - I have a branch named 'orig'\n> - I want to make some experimental changes on it:\n> $ git checkout -b temp orig\n> $ ... edit some files ...\n> $ ... make some tests & commits ...\n> - I'm happy with my changes, so I want to have my original\n> branch to now points to the head of this temp branch\n> but did it wrongly:\n> $ git branch -m -f orig @\n\nHere you are using the '-m' flag, which is to rename a branch. So what\nyou're essentially\ndoing is:\n $ git branch -m -f orig HEAD\nDo note that this won't reset 'orig' to point to 'HEAD', rather this\nrenames 'orig' to 'HEAD'.\n\nWhat you actually want to do (to reset 'orig' to 'HEAD') is:\n $ git branch -f orig @\nThis would make orig point to the current HEAD.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"312704","messageId":"20170227064724.fx246w4fn2morhb4@macbook.local","threadId":"45233","inReplyTo":"CAOLa=ZSyQg9uoZWADOMYc90U-5AR9Lfii9mjLre0m0FQCSqfxg@mail.gmail.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2017-02-27T06:47:25Z","receivedAt":"2017-02-27T07:49:57Z","isPatch":false,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"On Mon, Feb 27, 2017 at 11:43:46AM +0530, Karthik Nayak wrote:\n> Hello,\n> \n> Thanks for reporting, but I don't think it is a bug.\n> \n> On Mon, Feb 27, 2017 at 10:22 AM, Luc Van Oostenryck\n> wrote:\n> > Hi,\n> >\n> > I just discover something which very much seems a bug to me\n> > while making an error in renaming a branch.\n> > The scenario is the following:\n> > - I have a branch named 'orig'\n> > - I want to make some experimental changes on it:\n> > $ git checkout -b temp orig\n> > $ ... edit some files ...\n> > $ ... make some tests & commits ...\n> > - I'm happy with my changes, so I want to have my original\n> > branch to now points to the head of this temp branch\n> > but did it wrongly:\n> > $ git branch -m -f orig @\n> \n> Here you are using the '-m' flag, which is to rename a branch. So what\n> you're essentially\n> doing is:\n> $ git branch -m -f orig HEAD\n> Do note that this won't reset 'orig' to point to 'HEAD', rather this\n> renames 'orig' to 'HEAD'.\n> \n> What you actually want to do (to reset 'orig' to 'HEAD') is:\n> $ git branch -f orig @\n> This would make orig point to the current HEAD.\n\nSure. I said it the description that I made an error in the renaming.\n\nWhat I consider as a bug is that '@', which stands for refs/HEAD,\nhave been interpreted as a branch named 'HEAD' and thus created a\nreference refs/heads/HEAD.\n\nLuc\n"},{"id":"312705","messageId":"20170227080158.de2xarctzscfdsp2@sigill.intra.peff.net","threadId":"45233","inReplyTo":"20170227074915.xljfe5jox756rlyv@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T08:01:59Z","receivedAt":"2017-02-27T08:03:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 02:49:15AM -0500, Jeff King wrote:\n\n> > > $ git branch -m -f orig @\n> [...]\n> \n> Regardless of the original intent, I think it is wrong to convert \"@\" to\n> a branch named \"HEAD\". I think the bug is in strbuf_check_branch_ref(),\n> which blindly sticks \"refs/heads/\" in front of any value we get from\n> interpret_branch_name(), which clearly does not make sense for HEAD.\n\nI do think the bug is in strbuf_check_branch_ref(), but it's hard for it\nto do a better job. It needs to feed arbitrary expressions into\ninterpret_branch_name() to resolve things like \"@{upstream}\", \"@{-1}\",\n\"foo@{upstream}\", etc.\n\nThe problem is that it expects a branch name to come out of\ninterpret_branch_name(), which _mostly_ happens. The exception is HEAD,\nwhich is a \"special\" name. But the returned value doesn't indicate\nwhether it is special or not.\n\nMy first thought was that we might be handling \"@\" in the wrong place.\nBut it needs to happen here to make things like \"@@{upstream}\" work\n(which turns \"@\" into HEAD, and then finds its upstream).\n\nSo I think the options are:\n\n 1. Before calling interpret_branch_name(), strbuf_check_branch_ref()\n checks for \"@\". I don't like this because it's making assumptions\n about how the result will be parsed and interpreted.\n\n 2. interpret_branch_name() returns a flag that says \"this isn't\n _really_ a branch name\".\n\n 3. After interpret_branch_name() returns, check whether the result is\n \"HEAD\".\n\nDoing (2) is the \"right\" thing in the sense that the \"is it a branch\"\nlogic remains with the matching parsing code. But we have to surface\nthat value (maybe across recursion via reinterpret?). Since we're\nunlikely to ever grow a return value that matches this case except\n\"HEAD\", it might be simplest to just do (3).\n\n-Peff\n"},{"id":"312706","messageId":"20170227074915.xljfe5jox756rlyv@sigill.intra.peff.net","threadId":"45233","inReplyTo":"CAOLa=ZSyQg9uoZWADOMYc90U-5AR9Lfii9mjLre0m0FQCSqfxg@mail.gmail.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T07:49:16Z","receivedAt":"2017-02-27T08:16:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 11:43:46AM +0530, Karthik Nayak wrote:\n\n> On Mon, Feb 27, 2017 at 10:22 AM, Luc Van Oostenryck\n> wrote:\n> > Hi,\n> >\n> > I just discover something which very much seems a bug to me\n> > while making an error in renaming a branch.\n> > The scenario is the following:\n> > - I have a branch named 'orig'\n> > - I want to make some experimental changes on it:\n> > $ git checkout -b temp orig\n> > $ ... edit some files ...\n> > $ ... make some tests & commits ...\n> > - I'm happy with my changes, so I want to have my original\n> > branch to now points to the head of this temp branch\n> > but did it wrongly:\n> > $ git branch -m -f orig @\n> \n> Here you are using the '-m' flag, which is to rename a branch. So what\n> you're essentially\n> doing is:\n> $ git branch -m -f orig HEAD\n> Do note that this won't reset 'orig' to point to 'HEAD', rather this\n> renames 'orig' to 'HEAD'.\n> \n> What you actually want to do (to reset 'orig' to 'HEAD') is:\n> $ git branch -f orig @\n> This would make orig point to the current HEAD.\n\nRegardless of the original intent, I think it is wrong to convert \"@\" to\na branch named \"HEAD\". I think the bug is in strbuf_check_branch_ref(),\nwhich blindly sticks \"refs/heads/\" in front of any value we get from\ninterpret_branch_name(), which clearly does not make sense for HEAD.\n\n-Peff\n"},{"id":"312707","messageId":"20170227090233.uk7dfruggytgmuw2@sigill.intra.peff.net","threadId":"45233","inReplyTo":"20170227080158.de2xarctzscfdsp2@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T09:02:33Z","receivedAt":"2017-02-27T09:09:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 03:01:58AM -0500, Jeff King wrote:\n\n> I do think the bug is in strbuf_check_branch_ref(), but it's hard for it\n> to do a better job. It needs to feed arbitrary expressions into\n> interpret_branch_name() to resolve things like \"@{upstream}\", \"@{-1}\",\n> \"foo@{upstream}\", etc.\n> \n> The problem is that it expects a branch name to come out of\n> interpret_branch_name(), which _mostly_ happens. The exception is HEAD,\n> which is a \"special\" name. But the returned value doesn't indicate\n> whether it is special or not.\n> \n> My first thought was that we might be handling \"@\" in the wrong place.\n> But it needs to happen here to make things like \"@@{upstream}\" work\n> (which turns \"@\" into HEAD, and then finds its upstream).\n> \n> So I think the options are:\n> \n> 1. Before calling interpret_branch_name(), strbuf_check_branch_ref()\n> checks for \"@\". I don't like this because it's making assumptions\n> about how the result will be parsed and interpreted.\n> \n> 2. interpret_branch_name() returns a flag that says \"this isn't\n> _really_ a branch name\".\n> \n> 3. After interpret_branch_name() returns, check whether the result is\n> \"HEAD\".\n> \n> Doing (2) is the \"right\" thing in the sense that the \"is it a branch\"\n> logic remains with the matching parsing code. But we have to surface\n> that value (maybe across recursion via reinterpret?). Since we're\n> unlikely to ever grow a return value that matches this case except\n> \"HEAD\", it might be simplest to just do (3).\n\nUgh. Actually, there are a few complications I found:\n\n 1. Checking \"HEAD\" afterwards means you can't actually have a branch\n named \"HEAD\". Doing so is probably insane, but we probably really\n _do_ want to just disallow the @-conversion here.\n\n 2. This isn't limited to just HEAD and @-conversion. For instance:\n\n $ git clone /some/repo tmp\n $ cd tmp\n $ git branch -m master @{upstream}\n $ git for-each-ref --format='%(refname)'\n refs/heads/origin/master\n refs/remotes/origin/HEAD\n refs/remotes/origin/master\n\n Er, what? Now we have a branch called origin/master.\n\nSo I think it probably is fundamentally wrong to be calling\ninterpret_branch_name() here at all, if we're just going to tack\n\"refs/heads/\" in front of it. We don't know that we're getting out a\nreal branchname.\n\nBut we do still need to handle @{-1}. And I suppose it's even possible\nthat you could want to use foo@{upstream} as long as that upstream\npoints to a _local_ branch.\n\nSo perhaps the fundamental issue is that interpret_branch_name() does\nnot give us fully qualified refs in the return value. We don't have any\nclue if the return value is in \"refs/heads\" or \"refs/remotes\", or what.\nWe can fix that, but we're still stuck comparing the result to see if it\nstarts with \"refs/\" or is just \"HEAD\".\n\nWhich is wrong. If the user fed us a branch name of \"refs/remotes/foo\",\nthen the correct branch name is \"refs/heads/refs/remotes/foo\" (as stupid\nas that probably is). It's only the branch-reinterpretation that we need\nto be careful of. So we really do need to somehow have\ninterpret_branch_name() tell us whether or not it found an actual\nbranch. Actually, it passes through unknown names, so it can only tell\nus when it definitely found something _outside_ of refs/heads/.\n\nI guess something like the patch below works, but I wonder if there is a\nless-horrible way to accomplish the same thing.\n\ndiff --git a/cache.h b/cache.h\nindex 61fc86e6d..d52e24f4f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1319,7 +1319,8 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern char *oid_to_hex(const struct object_id *oid);\t/* same static buffer as sha1_to_hex */\n \n-extern int interpret_branch_name(const char *str, int len, struct strbuf *);\n+extern int interpret_branch_name(const char *str, int len, struct strbuf *,\n+\t\t\t\t int *not_in_refs_heads);\n extern int get_oid_mb(const char *str, struct object_id *oid);\n \n extern int validate_headref(const char *ref);\ndiff --git a/refs.c b/refs.c\nindex cd36b64ed..78b32dd22 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -404,7 +404,7 @@ int refname_match(const char *abbrev_name, const char *full_name)\n static char *substitute_branch_name(const char **string, int *len)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint ret = interpret_branch_name(*string, *len, &buf);\n+\tint ret = interpret_branch_name(*string, *len, &buf, NULL);\n \n \tif (ret == *len) {\n \t\tsize_t size;\ndiff --git a/revision.c b/revision.c\nindex b37dbec37..233764802 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,\n \t\trevs->no_walk = 0;\n \tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint len = interpret_branch_name(name, 0, &buf);\n+\t\tint len = interpret_branch_name(name, 0, &buf, NULL);\n \t\tint st;\n \n \t\tif (0 < len && name[len] && buf.len)\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 73a915ff1..2ef77afb0 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1155,7 +1155,8 @@ int get_oid_mb(const char *name, struct object_id *oid)\n }\n \n /* parse @something syntax, when 'something' is not {.*} */\n-static int interpret_empty_at(const char *name, int namelen, int len, struct strbuf *buf)\n+static int interpret_empty_at(const char *name, int namelen, int len,\n+\t\t\t struct strbuf *buf, int *not_in_refs_heads)\n {\n \tconst char *next;\n \n@@ -1173,10 +1174,13 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str\n \n \tstrbuf_reset(buf);\n \tstrbuf_add(buf, \"HEAD\", 4);\n+\t*not_in_refs_heads = 1;\n \treturn 1;\n }\n \n-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)\n+static int reinterpret(const char *name, int namelen, int len,\n+\t\t struct strbuf *buf, int *not_in_refs_heads)\n+\n {\n \t/* we have extra data, which might need further processing */\n \tstruct strbuf tmp = STRBUF_INIT;\n@@ -1184,7 +1188,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu\n \tint ret;\n \n \tstrbuf_add(buf, name + len, namelen - len);\n-\tret = interpret_branch_name(buf->buf, buf->len, &tmp);\n+\tret = interpret_branch_name(buf->buf, buf->len, &tmp, not_in_refs_heads);\n \t/* that data was not interpreted, remove our cruft */\n \tif (ret < 0) {\n \t\tstrbuf_setlen(buf, used);\n@@ -1209,7 +1213,8 @@ static int interpret_branch_mark(const char *name, int namelen,\n \t\t\t\t int at, struct strbuf *buf,\n \t\t\t\t int (*get_mark)(const char *, int),\n \t\t\t\t const char *(*get_data)(struct branch *,\n-\t\t\t\t\t\t\t struct strbuf *))\n+\t\t\t\t\t\t\t struct strbuf *),\n+\t\t\t\t int *not_in_refs_heads)\n {\n \tint len;\n \tstruct branch *branch;\n@@ -1234,6 +1239,9 @@ static int interpret_branch_mark(const char *name, int namelen,\n \tif (!value)\n \t\tdie(\"%s\", err.buf);\n \n+\tif (!starts_with(value, \"refs/heads/\"))\n+\t\t*not_in_refs_heads = 1;\n+\n \tset_shortened_ref(buf, value);\n \treturn len + at;\n }\n@@ -1259,12 +1267,17 @@ static int interpret_branch_mark(const char *name, int namelen,\n * If the input was ok but there are not N branch switches in the\n * reflog, it returns 0.\n */\n-int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n+int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,\n+\t\t\t int *not_in_refs_heads)\n {\n \tchar *at;\n \tconst char *start;\n+\tint dummy;\n \tint len = interpret_nth_prior_checkout(name, namelen, buf);\n \n+\tif (!not_in_refs_heads)\n+\t\tnot_in_refs_heads = &dummy;\n+\n \tif (!namelen)\n \t\tnamelen = strlen(name);\n \n@@ -1274,24 +1287,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \t\tif (len == namelen)\n \t\t\treturn len; /* consumed all */\n \t\telse\n-\t\t\treturn reinterpret(name, namelen, len, buf);\n+\t\t\treturn reinterpret(name, namelen, len, buf,\n+\t\t\t\t\t not_in_refs_heads);\n \t}\n \n \tfor (start = name;\n \t (at = memchr(start, '@', namelen - (start - name)));\n \t start = at + 1) {\n \n-\t\tlen = interpret_empty_at(name, namelen, at - name, buf);\n+\t\tlen = interpret_empty_at(name, namelen, at - name, buf,\n+\t\t\t\t\t not_in_refs_heads);\n \t\tif (len > 0)\n-\t\t\treturn reinterpret(name, namelen, len, buf);\n+\t\t\treturn reinterpret(name, namelen, len, buf,\n+\t\t\t\t\t not_in_refs_heads);\n \n \t\tlen = interpret_branch_mark(name, namelen, at - name, buf,\n-\t\t\t\t\t upstream_mark, branch_get_upstream);\n+\t\t\t\t\t upstream_mark, branch_get_upstream,\n+\t\t\t\t\t not_in_refs_heads);\n \t\tif (len > 0)\n \t\t\treturn len;\n \n \t\tlen = interpret_branch_mark(name, namelen, at - name, buf,\n-\t\t\t\t\t push_mark, branch_get_push);\n+\t\t\t\t\t push_mark, branch_get_push,\n+\t\t\t\t\t not_in_refs_heads);\n \t\tif (len > 0)\n \t\t\treturn len;\n \t}\n@@ -1299,10 +1317,11 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)\n \treturn -1;\n }\n \n-int strbuf_branchname(struct strbuf *sb, const char *name)\n+static int strbuf_branchname_1(struct strbuf *sb, const char *name,\n+\t\t\t int *not_in_refs_heads)\n {\n \tint len = strlen(name);\n-\tint used = interpret_branch_name(name, len, sb);\n+\tint used = interpret_branch_name(name, len, sb, not_in_refs_heads);\n \n \tif (used == len)\n \t\treturn 0;\n@@ -1312,9 +1331,17 @@ int strbuf_branchname(struct strbuf *sb, const char *name)\n \treturn len;\n }\n \n+int strbuf_branchname(struct strbuf *sb, const char *name)\n+{\n+\treturn strbuf_branchname_1(sb, name, NULL);\n+}\n+\n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n-\tstrbuf_branchname(sb, name);\n+\tint not_in_refs_heads = 0;\n+\tstrbuf_branchname_1(sb, name, ¬_in_refs_heads);\n+\tif (not_in_refs_heads)\n+\t\treturn -1;\n \tif (name[0] == '-')\n \t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n"},{"id":"312712","messageId":"20170227094738.mhv72nh4pp66vzny@macbook.local","threadId":"45233","inReplyTo":"20170227090233.uk7dfruggytgmuw2@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2017-02-27T09:47:39Z","receivedAt":"2017-02-27T09:58:15Z","isPatch":false,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"On Mon, Feb 27, 2017 at 04:02:33AM -0500, Jeff King wrote:\n> Ugh. Actually, there are a few complications I found:\n> \n> 1. Checking \"HEAD\" afterwards means you can't actually have a branch\n> named \"HEAD\". Doing so is probably insane, but we probably really\n> _do_ want to just disallow the @-conversion here.\n> \n> 2. This isn't limited to just HEAD and @-conversion. For instance:\n\nIt's a bit what I was afraid of.\n\nLuc\n"},{"id":"312792","messageId":"xmqq60jvnu9y.fsf@gitster.mtv.corp.google.com","threadId":"45233","inReplyTo":"20170227090233.uk7dfruggytgmuw2@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T22:28:09Z","receivedAt":"2017-02-27T22:31:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess something like the patch below works, but I wonder if there is a\n> less-horrible way to accomplish the same thing.\n\nI suspect that a less-horrible would be a lot more intrusive. It\nwould go like \"interpret-branch-name only gives local branch name,\nand when it does not show it, the callers that know they do not\nnecessarily need local branch name would call other at-mark things\".\nAs you pointed out with the @{upstream} that potentially point at a\nlocal branch, it will quickly get more involved, I would think, and\nI tend to think that this patch of yours is probably the least evil\none among possible solutions. \n\nPerhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping\npolarity, \"is_a_branch_name\"), the resulting code may not be too\nhard to read?\n\nThanks.\n"},{"id":"312805","messageId":"xmqqzih7kvbz.fsf@gitster.mtv.corp.google.com","threadId":"45233","inReplyTo":"CA+P7+xpVt6NtSajqMX8OQ_SKdC9tfHH40JgK=9DgBxo9nMaWLA@mail.gmail.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T00:33:36Z","receivedAt":"2017-02-28T00:34:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> What about changing interpret-branch-name gains a flag to return a\n> fully qualified ref rather than returning just the name? That seems\n> like it would be more reasonable behavior.\n\nThere are two kinds of callers to i-b-n. The ones that want a local\nbranch name because they are parsing special places on the command\nline that using a local branch name makes difference (as opposed to\nusing any extended SHA-1 expression), like \"git checkout master\"\n(which means different thing from \"git checkout master^0\"). And the\nones that can use any object name.\n\nIt depends on how your flag works, but if it means \"add refs/heads/\nwhen you got a local branch name\", then that would not work well for\nthe former callers, as end-user inputs @{-1} and refs/heads/master\nwould become indistinguishable. The former is expanded to 'master'\n(if you were on that branch) and ends up being refs/heads/master.\n\"git checkout refs/heads/master\" would be (unless you have a branch\nwith that name, i.e. refs/heads/refs/heads/master) a request to\ndetach HEAD at the commit, but the user wanted to be on the previous\nbranch. And the latter iclass of callers are probably already happy\nwith or without the flag, so they won't be helped, either.\n\nA flag to affect the behaviour (as opposed to &flag as a secondary\nreturn value, like Peff's patch does) can be made to work. Perhaps\na flag that says \"keep the input as is if the result is not a local\nbranch name\" would pass an input \"@\" intact and that may be\nsufficient to allow \"git branch -m @\" to rename the current branch\nto \"@\" (I do not think it is a sensible rename, though ;-). But\nprobably some callers need to keep the original input and compare\nwith the result to see if we expanded anything if we go that route.\nAt that point, I am not sure if there are much differences in the\nease of use between the two approaches.\n"},{"id":"312807","messageId":"20170228004220.66fq567am7chjxfe@sigill.intra.peff.net","threadId":"45233","inReplyTo":"xmqq60jvnu9y.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T00:42:21Z","receivedAt":"2017-02-28T00:42:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 02:28:09PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I guess something like the patch below works, but I wonder if there is a\n> > less-horrible way to accomplish the same thing.\n> \n> I suspect that a less-horrible would be a lot more intrusive. It\n> would go like \"interpret-branch-name only gives local branch name,\n> and when it does not show it, the callers that know they do not\n> necessarily need local branch name would call other at-mark things\".\n> As you pointed out with the @{upstream} that potentially point at a\n> local branch, it will quickly get more involved, I would think, and\n> I tend to think that this patch of yours is probably the least evil\n> one among possible solutions. \n> \n> Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping\n> polarity, \"is_a_branch_name\"), the resulting code may not be too\n> hard to read?\n\nI actually started with not_a_branch_name, but I wanted specifically to\ntalk about refs_heads, because we sometimes refer to remote-tracking\nbranches as \"branches\" (and the function is called interpret_branch_name(),\nafter all).\n\nI agree it would be easier to read if the logic were flipped, but I'm\nnot sure that would be correct. The function knows when it has done a\nreplacement that takes us outside of a normal branch name. But when it\nhasn't, it doesn't really know how the result should be interpreted.\n\nFor our purposes in this caller it is enough to say that \"foo\" should be\ntreated as \"refs/heads/foo\", but I don't think that is generally true of\ninterpret_branch_name(), which might be called as part of get_sha1().\n\nSo one alternative is to leave the logic the same way but try to give it\na better name. E.g., call it something like \"replaced_with_non_branch\"\nor something. But there, another negation snuck in. The correct\nnon-negated thing is really \"replaced_with_HEAD_or_refs_remotes\" but\nthat is rather awkward.\n\n-Peff\n"},{"id":"312808","messageId":"20170228004957.xphg6ludprkzo4yh@sigill.intra.peff.net","threadId":"45233","inReplyTo":"CA+P7+xpVt6NtSajqMX8OQ_SKdC9tfHH40JgK=9DgBxo9nMaWLA@mail.gmail.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T00:49:57Z","receivedAt":"2017-02-28T00:50:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 03:05:37PM -0800, Jacob Keller wrote:\n\n> > Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping\n> > polarity, \"is_a_branch_name\"), the resulting code may not be too\n> > hard to read?\n> \n> What about changing interpret-branch-name gains a flag to return a\n> fully qualified ref rather than returning just the name? That seems\n> like it would be more reasonable behavior.\n\nThat's not sufficient. If I feed \"refs/remotes/origin/master\" as a\nbranch name to git-branch, then as silly as that is, it is the branch\nwhose ref is \"refs/heads/refs/remotes/origin/master\".\n\nSince interpret_branch_name() is not fully qualifying everything, but\nrather just _sometimes_ replace @-marks with refnames, we cannot tell\nfrom just the string the difference between \"the user fed us\nrefs/remotes/foo\" and \"the @-mark expanded to a non-branch\nrefs/remotes/foo\". We need one extra bit of information to know whether\nan expansion occurred.\n\nYou could give a flag that says \"do not expand to anything outside of\nrefs/heads/\" that would suppress the @->HEAD mark, as well as\n@{upstream} when upstream is outside of refs/heads. That would certainly\nbe less nasty than the out-parameter, but I wasn't sure that the error\nhandling was what we wanted.\n\nIn strbuf_branchname(), we quietly turn that error into a \"0\", which\ncauses us to retain the original text. We then feed that into\ncheck_refname_format() in strbuf_check_branch_ref(). Which I think would\ncomplain, and you'd get \"not a valid refname: @{upstream}\". If we have\nthe out-bit, we can say \"I understand what @{upstream} means, but it\ndoes not expand to a local branch\". That's a more specific error, but\nmaybe it is not worth the hassle to produce it.\n\n-Peff\n"},{"id":"312810","messageId":"CA+P7+xpVt6NtSajqMX8OQ_SKdC9tfHH40JgK=9DgBxo9nMaWLA@mail.gmail.com","threadId":"45233","inReplyTo":"xmqq60jvnu9y.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-02-27T23:05:37Z","receivedAt":"2017-02-28T01:01:55Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 27, 2017 at 2:28 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I guess something like the patch below works, but I wonder if there is a\n>> less-horrible way to accomplish the same thing.\n>\n> I suspect that a less-horrible would be a lot more intrusive. It\n> would go like \"interpret-branch-name only gives local branch name,\n> and when it does not show it, the callers that know they do not\n> necessarily need local branch name would call other at-mark things\".\n> As you pointed out with the @{upstream} that potentially point at a\n> local branch, it will quickly get more involved, I would think, and\n> I tend to think that this patch of yours is probably the least evil\n> one among possible solutions.\n>\n> Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping\n> polarity, \"is_a_branch_name\"), the resulting code may not be too\n> hard to read?\n>\n> Thanks.\n\nWhat about changing interpret-branch-name gains a flag to return a\nfully qualified ref rather than returning just the name? That seems\nlike it would be more reasonable behavior.\n\nThanks,\nJake\n"},{"id":"312812","messageId":"20170228005302.k6fyfinaxyl3ti76@sigill.intra.peff.net","threadId":"45233","inReplyTo":"xmqqzih7kvbz.fsf@gitster.mtv.corp.google.com","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T00:53:02Z","receivedAt":"2017-02-28T01:20:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:\n\n> A flag to affect the behaviour (as opposed to &flag as a secondary\n> return value, like Peff's patch does) can be made to work. Perhaps\n> a flag that says \"keep the input as is if the result is not a local\n> branch name\" would pass an input \"@\" intact and that may be\n> sufficient to allow \"git branch -m @\" to rename the current branch\n> to \"@\" (I do not think it is a sensible rename, though ;-). But\n> probably some callers need to keep the original input and compare\n> with the result to see if we expanded anything if we go that route.\n> At that point, I am not sure if there are much differences in the\n> ease of use between the two approaches.\n\nI just went into more detail in my reply to Jacob, but I do think this\nis a workable approach (and fortunately we seem to have banned bare \"@\"\nas a name, along with anything containing \"@{}\", so I think we would end\nup rejecting these nonsense names).\n\nI'll see if I can work up a patch. We'll still need to pass the flag\naround through the various functions, but at least it will be a flag and\nnot a confusing negated out-parameter.\n\n-Peff\n"},{"id":"312823","messageId":"CA+P7+xp_w_BmkbDA03vRCg8JEmLkgd1_WbmtsBskpV0EarXejQ@mail.gmail.com","threadId":"45233","inReplyTo":"20170228005302.k6fyfinaxyl3ti76@sigill.intra.peff.net","subject":"Re: [BUG] branch renamed to 'HEAD'","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-02-28T07:58:40Z","receivedAt":"2017-02-28T08:08:27Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Feb 27, 2017 at 4:53 PM, Jeff King wrote:\n> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:\n>\n>> A flag to affect the behaviour (as opposed to &flag as a secondary\n>> return value, like Peff's patch does) can be made to work. Perhaps\n>> a flag that says \"keep the input as is if the result is not a local\n>> branch name\" would pass an input \"@\" intact and that may be\n>> sufficient to allow \"git branch -m @\" to rename the current branch\n>> to \"@\" (I do not think it is a sensible rename, though ;-). But\n>> probably some callers need to keep the original input and compare\n>> with the result to see if we expanded anything if we go that route.\n>> At that point, I am not sure if there are much differences in the\n>> ease of use between the two approaches.\n>\n> I just went into more detail in my reply to Jacob, but I do think this\n> is a workable approach (and fortunately we seem to have banned bare \"@\"\n> as a name, along with anything containing \"@{}\", so I think we would end\n> up rejecting these nonsense names).\n>\n> I'll see if I can work up a patch. We'll still need to pass the flag\n> around through the various functions, but at least it will be a flag and\n> not a confusing negated out-parameter.\n>\n> -Peff\n\nYes, this is pretty much what I had imagined. I look forward to seeing\nthe patch.\n\nThanks,\nJake\n"}]}