{"thread":{"id":"45235","subject":"[PATCH] t6300: avoid creating refs/heads/HEAD","startedAt":"2017-02-27T09:30:30Z","lastAt":"2017-02-27T23:33:34Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312709","messageId":"20170227092931.7iquwaxomeuuusi2@sigill.intra.peff.net","threadId":"45235","inReplyTo":null,"subject":"[PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T09:29:31Z","receivedAt":"2017-02-27T09:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In one test, we use \"git checkout --orphan HEAD\" to create\nan unborn branch. Confusingly, the resulting branch is named\n\"refs/heads/HEAD\". The original probably meant something\nlike:\n\n  git checkout --orphan orphaned-branch HEAD\n\nLet's just use \"orphaned-branch\" here to make this less\nconfusing. Putting HEAD in the second argument is already\nimplied.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis comes originally from Junio's 84679d470. I cannot see how naming\nthe new branch HEAD would make any difference to the test, but perhaps I\nam missing something.\n\nI noticed this while digging on a nearby issue around \"git branch -m @\".\nThis does happen to be the only test that checks that we can make a\nbranch called refs/heads/HEAD, and I found it because it triggers if you\ntry to disallow \"git branch -m HEAD\". :)\n\nIf we care about that, though, I think we should make an explicit test\nfor \"git branch HEAD\". But I'm not sure we _do_ care about that. Making\na branch called HEAD is moderately insane, and I don't think it would be\nunreasonable for us to outlaw it at some point.\n\n t/t6300-for-each-ref.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex aea1dfc71..a468041c5 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -558,7 +558,7 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '\n \ttest_when_finished \"git checkout master\" &&\n \tgit for-each-ref --format=\"%(HEAD) %(refname:short)\" refs/heads/ >actual &&\n \tsed -e \"s/^\\* /  /\" actual >expect &&\n-\tgit checkout --orphan HEAD &&\n+\tgit checkout --orphan orphaned-branch &&\n \tgit for-each-ref --format=\"%(HEAD) %(refname:short)\" refs/heads/ >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.12.0.624.gbb1b07a2c\n"},{"id":"312761","messageId":"xmqqzih7whrw.fsf@gitster.mtv.corp.google.com","threadId":"45235","inReplyTo":"20170227092931.7iquwaxomeuuusi2@sigill.intra.peff.net","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T19:33:23Z","receivedAt":"2017-02-27T19:33: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> This comes originally from Junio's 84679d470. I cannot see how naming\n> the new branch HEAD would make any difference to the test, but perhaps I\n> am missing something.\n\nNah, I think it was just a random string that came to mind and the\ntopic being \"ah we blindly dereference something when showing %(HEAD)\"\nit was plausible I thought of \"H E A D\" as that random string before\nI used my usual other random strings like frotz ;-)\n\n> I noticed this while digging on a nearby issue around \"git branch -m @\".\n> This does happen to be the only test that checks that we can make a\n> branch called refs/heads/HEAD, and I found it because it triggers if you\n> try to disallow \"git branch -m HEAD\". :)\n\nAbout that \"nearby\" one, does it even make sense to do the interpret\nthing on the <new> name?  I can understand \"please rename the branch\nI was previously on to this new name\" wanting to say @{-1} when the\nuser does not recall the exact spelling of a long name, but I do not\nquite see how \"to this new name\" part benefits by the \"interpret\nbranch name\" magic in the first place.\n\n> If we care about that, though, I think we should make an explicit test\n> for \"git branch HEAD\". But I'm not sure we _do_ care about that. Making\n> a branch called HEAD is moderately insane, and I don't think it would be\n> unreasonable for us to outlaw it at some point.\n\nYeah, at that point we would have \"test_must_fail git branch HEAD\".\n\n>  t/t6300-for-each-ref.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index aea1dfc71..a468041c5 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -558,7 +558,7 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '\n>  \ttest_when_finished \"git checkout master\" &&\n>  \tgit for-each-ref --format=\"%(HEAD) %(refname:short)\" refs/heads/ >actual &&\n>  \tsed -e \"s/^\\* /  /\" actual >expect &&\n> -\tgit checkout --orphan HEAD &&\n> +\tgit checkout --orphan orphaned-branch &&\n>  \tgit for-each-ref --format=\"%(HEAD) %(refname:short)\" refs/heads/ >actual &&\n>  \ttest_cmp expect actual\n>  '\n"},{"id":"312771","messageId":"20170227205151.rjhod347ddhmdmxp@sigill.intra.peff.net","threadId":"45235","inReplyTo":"xmqqzih7whrw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T20:51:51Z","receivedAt":"2017-02-27T20:58:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 11:33:23AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This comes originally from Junio's 84679d470. I cannot see how naming\n> > the new branch HEAD would make any difference to the test, but perhaps I\n> > am missing something.\n> \n> Nah, I think it was just a random string that came to mind and the\n> topic being \"ah we blindly dereference something when showing %(HEAD)\"\n> it was plausible I thought of \"H E A D\" as that random string before\n> I used my usual other random strings like frotz ;-)\n\nOK, thanks for confirming.\n\n> > I noticed this while digging on a nearby issue around \"git branch -m @\".\n> > This does happen to be the only test that checks that we can make a\n> > branch called refs/heads/HEAD, and I found it because it triggers if you\n> > try to disallow \"git branch -m HEAD\". :)\n> \n> About that \"nearby\" one, does it even make sense to do the interpret\n> thing on the <new> name?  I can understand \"please rename the branch\n> I was previously on to this new name\" wanting to say @{-1} when the\n> user does not recall the exact spelling of a long name, but I do not\n> quite see how \"to this new name\" part benefits by the \"interpret\n> branch name\" magic in the first place.\n\nYeah, it's arguable whether the \"new\" side of a rename should do any\ninterpretation at all. At the same time, the bug is in the underlying\nfunction that assumes you can slap \"refs/heads/\" in front of the results\nof interpret_branch_name(). And that function gets used in a lot of\nplaces, including the \"old\" side of a rename. So:\n\n  git branch @{-1} foo\n\nshould clearly work. Doing:\n\n  git branch @{upstream} foo\n\nis more debatable. It _does_ work, but only if your upstream is actually\na local branch (otherwise it tries to rename refs/heads/origin/master or\nsome such nonsense. It happens to fail most of the time because you\nprobably don't have such a branch, but it's still wrong to even look at\nthat).\n\nI suspect there are a lot of other places that are less clear cut. E.g.,\nI think just:\n\n  git branch foo bar\n\nwill put \"foo\" through the same interpretation. So you could do:\n\n  git branch -f @{-1} bar\n\nIs that insane? Maybe. But it does work now.\n\n-Peff\n"},{"id":"312775","messageId":"xmqqshmzuyam.fsf@gitster.mtv.corp.google.com","threadId":"45235","inReplyTo":"20170227205151.rjhod347ddhmdmxp@sigill.intra.peff.net","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T21:19:29Z","receivedAt":"2017-02-27T21:21:38Z","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> I suspect there are a lot of other places that are less clear cut. E.g.,\n> I think just:\n>\n>   git branch foo bar\n>\n> will put \"foo\" through the same interpretation. So you could do:\n>\n>   git branch -f @{-1} bar\n>\n> Is that insane? Maybe. But it does work now.\n\nNo, it _is_ very sensible, so is \"git checkout -B @{-1} <someplace>\"\n\nPerhaps interpret-branch-name that does not error out when given \"@\"\nis what is broken?  I suspect that calling interpret_empty_at() from\nthat function is fundamentally flawed.  The \"@\" end user types never\nmeans refs/heads/HEAD, and HEAD@{either reflog or -1} would not mean\nanything that should be taken as a branch_name, either.  \n\nSo perhaps what interpret_empty_at() does is necessary for the \"four\ncapital letters is too many to type, so just type one key while\nholding a shift\", but it should be called from somewhere else, and\nnot from interpret_branch_name()?\n\n\n\n\n\n"},{"id":"312780","messageId":"20170227214147.5ezxskhihi3cc77m@sigill.intra.peff.net","threadId":"45235","inReplyTo":"xmqqshmzuyam.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T21:41:47Z","receivedAt":"2017-02-27T21:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 01:19:29PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I suspect there are a lot of other places that are less clear cut. E.g.,\n> > I think just:\n> >\n> >   git branch foo bar\n> >\n> > will put \"foo\" through the same interpretation. So you could do:\n> >\n> >   git branch -f @{-1} bar\n> >\n> > Is that insane? Maybe. But it does work now.\n> \n> No, it _is_ very sensible, so is \"git checkout -B @{-1} <someplace>\"\n> \n> Perhaps interpret-branch-name that does not error out when given \"@\"\n> is what is broken?  I suspect that calling interpret_empty_at() from\n> that function is fundamentally flawed.  The \"@\" end user types never\n> means refs/heads/HEAD, and HEAD@{either reflog or -1} would not mean\n> anything that should be taken as a branch_name, either.\n> \n> So perhaps what interpret_empty_at() does is necessary for the \"four\n> capital letters is too many to type, so just type one key while\n> holding a shift\", but it should be called from somewhere else, and\n> not from interpret_branch_name()?\n\nI think _most_ of interpret_branch_name() is in the same boat. The\n\"@{upstream}\" mark is not likely to give you a branch in refs/heads\neither.\n\nSo in practice, I think strbuf_check_branch_ref() could probably get by\nwith just calling interpret_nth_prior_checkout(). Or if you prefer, to\nrip everything out of interpret_branch_name() except that. :) But that\nother stuff has to go somewhere, and there are some challenges with the\nrecursion from reinterpret().\n\nThe \"other\" stuff could sometimes be useful, I guess. It's not _always_\nwrong to do:\n\n  git branch -f @{upstream} foo\n\nIt depends on what your @{upstream} resolves to. Switching to just using\ninterpret_nth_prior_checkout() would break the case when it resolves to\na local branch. I'm not sure if we're OK with that or not. If we want to\nkeep all the existing cases working, I think we need something like the\n\"not_in_refs_heads\" patch I posted elsewhere.\n\n-Peff\n"},{"id":"312783","messageId":"xmqqefyjnwat.fsf@gitster.mtv.corp.google.com","threadId":"45235","inReplyTo":"xmqqshmzuyam.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T21:44:26Z","receivedAt":"2017-02-27T21:53:40Z","isPatch":true,"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 suspect that calling interpret_empty_at() from\n> that function is fundamentally flawed.  The \"@\" end user types never\n> means refs/heads/HEAD, and HEAD@{either reflog or -1} would not mean\n> anything that should be taken as a branch_name, either.  \n\nThe latter should read \"HEAD@{either reflog or -1 or 'upstream'}\"\n\nOr do we make HEAD@{upstream} to mean \"deref HEAD to learn the\ncurrent branch name and then take its upstream\"?  If so @@{upstream}\nmight logically make sense, but I do not see why @{upstream} without\nHEAD or @ is not sufficient to begin with, so...\n\n"},{"id":"312786","messageId":"xmqqa897nupx.fsf@gitster.mtv.corp.google.com","threadId":"45235","inReplyTo":"20170227214147.5ezxskhihi3cc77m@sigill.intra.peff.net","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T22:18:34Z","receivedAt":"2017-02-27T22:20:22Z","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> The \"other\" stuff could sometimes be useful, I guess. It's not _always_\n> wrong to do:\n>\n>   git branch -f @{upstream} foo\n>\n> It depends on what your @{upstream} resolves to. Switching to just using\n> interpret_nth_prior_checkout() would break the case when it resolves to\n> a local branch. I'm not sure if we're OK with that or not. If we want to\n> keep all the existing cases working, I think we need something like the\n> \"not_in_refs_heads\" patch I posted elsewhere.\n\nI haven't seen that patch, but yes, telling the caller if the\nreturned value is meant to be used inside refs/heads/ is the right\napproach and makes it possible for \"@{upstream}\" (and just \"@\") to\nbe handled sensibly in \"git branch -m @{that at-mark thing}\".\n\n"},{"id":"312802","messageId":"20170227214640.h2qi5znay76szm35@sigill.intra.peff.net","threadId":"45235","inReplyTo":"xmqqefyjnwat.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6300: avoid creating refs/heads/HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-27T21:46:40Z","receivedAt":"2017-02-27T23:33:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 01:44:26PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > ...  I suspect that calling interpret_empty_at() from\n> > that function is fundamentally flawed.  The \"@\" end user types never\n> > means refs/heads/HEAD, and HEAD@{either reflog or -1} would not mean\n> > anything that should be taken as a branch_name, either.  \n> \n> The latter should read \"HEAD@{either reflog or -1 or 'upstream'}\"\n> \n> Or do we make HEAD@{upstream} to mean \"deref HEAD to learn the\n> current branch name and then take its upstream\"?  If so @@{upstream}\n> might logically make sense, but I do not see why @{upstream} without\n> HEAD or @ is not sufficient to begin with, so...\n\nYes, HEAD@{upstream} and @@{upstream} are both resolved to the actual\nbranch name. I also was puzzled whether there was any real use over just\n@{upstream}. But it does work, and if you had a script which looked for,\nsay, $branch@{upstream}, you'd probably want branch=HEAD to keep\nworking.\n\nThe \"branch=@\" case I am less sympathetic to, as it was mainly supposed\nto be a command-line convenience. But it _does_ work now.\n\n-Peff\n"}]}