{"thread":{"id":"46913","subject":"Regression in 'git branch -m'?","startedAt":"2017-10-05T17:32:45Z","lastAt":"2017-11-05T05:36:57Z","messageCount":14,"participants":["Andreas Krey","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"329775","messageId":"20171005172552.GA11497@inner.h.apk.li","threadId":"46913","inReplyTo":null,"subject":"Regression in 'git branch -m'?","fromName":"Andreas Krey","fromEmail":"a.krey@gmx.de","sentAt":"2017-10-05T17:25:52Z","receivedAt":"2017-10-05T17:32:45Z","isPatch":false,"sender":{"key":"a.krey@gmx.de","avatar":"https://avatars.githubusercontent.com/u/37810?v=4"},"body":"Hi everybody,\n\nI got something that looks like a regression somewhere since 2.11.\nThis script\n\n set -xe\n rm -rf repo\n git init repo\n cd repo\n git commit -m nix --allow-empty\n git branch -m master/master\n git rev-parse HEAD\n git branch\n git status\n\ncauses .git/HEAD to still contain 'ref: refs/heads/master' and to fail\nin the rev-parse step with\n\n + git rev-parse HEAD\n HEAD\n fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nThis is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still works.\n\nI'm going to do a bisect on this as battery permits.\n\n- Andreas\n\n-- \n\"Totally trivial. Famous last words.\"\nFrom: Linus Torvalds \nDate: Fri, 22 Jan 2010 07:29:21 -0800\n"},{"id":"329776","messageId":"20171005183303.f77dpkhs5ztxlmyv@sigill.intra.peff.net","threadId":"46913","inReplyTo":"20171005172552.GA11497@inner.h.apk.li","subject":"Re: Regression in 'git branch -m'?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-05T18:33:03Z","receivedAt":"2017-10-05T18:33:15Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2017 at 07:25:52PM +0200, Andreas Krey wrote:\n\n> I got something that looks like a regression somewhere since 2.11.\n> This script\n> \n> set -xe\n> rm -rf repo\n> git init repo\n> cd repo\n> git commit -m nix --allow-empty\n> git branch -m master/master\n> git rev-parse HEAD\n> git branch\n> git status\n> \n> causes .git/HEAD to still contain 'ref: refs/heads/master' and to fail\n> in the rev-parse step with\n> \n> + git rev-parse HEAD\n> HEAD\n> fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> \n> This is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still works.\n> \n> I'm going to do a bisect on this as battery permits.\n\nLooks like 31824d180d (branch: fix branch renaming not updating HEADs\ncorrectly, 2017-08-24). This is in v2.15.0-rc0, so we should figure it\nout before the upcoming release.\n\nI didn't dig very far, but it looks like the branch name is important\n\"foo\" doesn't trigger the problem but \"master/master\" does. \"master/foo\"\nalso does, but \"foo/master\" does not. So I suspect it's something about\nhow resolve_ref handles the failure when it would not be able to create\nthe ref because of the d/f conflict. So it's probably related to losing\nthe RESOLVE_REF_READING in the final hunk of that patch. That's just a\nguess for now, though.\n\n-Peff\n"},{"id":"329858","messageId":"20171006073913.yavdbdd3p3y5vjhd@sigill.intra.peff.net","threadId":"46913","inReplyTo":"20171005183303.f77dpkhs5ztxlmyv@sigill.intra.peff.net","subject":"Re: Regression in 'git branch -m'?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T07:39:14Z","receivedAt":"2017-10-06T07:39:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2017 at 02:33:03PM -0400, Jeff King wrote:\n\n> Looks like 31824d180d (branch: fix branch renaming not updating HEADs\n> correctly, 2017-08-24). This is in v2.15.0-rc0, so we should figure it\n> out before the upcoming release.\n> \n> I didn't dig very far, but it looks like the branch name is important\n> \"foo\" doesn't trigger the problem but \"master/master\" does. \"master/foo\"\n> also does, but \"foo/master\" does not. So I suspect it's something about\n> how resolve_ref handles the failure when it would not be able to create\n> the ref because of the d/f conflict. So it's probably related to losing\n> the RESOLVE_REF_READING in the final hunk of that patch. That's just a\n> guess for now, though.\n\nI got a chance to look at this again. I think the root of the problem is\nthat resolve_ref() as it is implemented now is just totally unsuitable\nfor asking the question \"what does this symbolic link point to?\".\n\nBecause you end up with either:\n\n 1. If we pass RESOLVE_REF_READING, then we do not return the target\n refname for orphaned commits (which is why 31824d180d dropped it).\n\n 2. If not, then we do not return the target refname for commits with\n names that are not available for writing. The d/f conflict here is\n one example, but there may be others.\n\nSo I think we need to teach resolve_ref() a new mode that's like\n\"reading\", but just follows the symref chain.\n\nIn the meantime, here's a test which shows off the regression.\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 3ac7ebf85f..503a88d029 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -117,6 +117,16 @@ test_expect_success 'git branch -m bbb should rename checked out branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'renaming checked out branch works with d/f conflict' '\n+\ttest_when_finished \"git branch -D foo/bar || git branch -D foo\" &&\n+\ttest_when_finished git checkout master &&\n+\tgit checkout -b foo &&\n+\tgit branch -m foo/bar &&\n+\tgit symbolic-ref HEAD >actual &&\n+\techo refs/heads/foo/bar >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git branch -m o/o o should fail when o/p exists' '\n \tgit branch o/o &&\n \tgit branch o/p &&\n\n-Peff\n"},{"id":"329862","messageId":"20171006083719.jap56jucgmlsuvuo@sigill.intra.peff.net","threadId":"46913","inReplyTo":"20171006073913.yavdbdd3p3y5vjhd@sigill.intra.peff.net","subject":"Re: Regression in 'git branch -m'?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T08:37:19Z","receivedAt":"2017-10-06T08:37:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 03:39:13AM -0400, Jeff King wrote:\n\n> I got a chance to look at this again. I think the root of the problem is\n> that resolve_ref() as it is implemented now is just totally unsuitable\n> for asking the question \"what does this symbolic link point to?\".\n> \n> Because you end up with either:\n> \n> 1. If we pass RESOLVE_REF_READING, then we do not return the target\n> refname for orphaned commits (which is why 31824d180d dropped it).\n> \n> 2. If not, then we do not return the target refname for commits with\n> names that are not available for writing. The d/f conflict here is\n> one example, but there may be others.\n> \n> So I think we need to teach resolve_ref() a new mode that's like\n> \"reading\", but just follows the symref chain.\n\nThis analysis is not _quite_ right. The \"not available for writing\"\nthing actually isn't intentionally enforced by the resolve_ref. It's\njust that it's not careful enough about checking errno. We see EISDIR\ninstead of ENOENT when there's a d/f situation, but both have the same\npractical effect: that ref doesn't exist.\n\nI.e., this lookup has _always_ been broken, even in the \"reading\" case.\nIt's just that the fix from 31824d180d (correctly) made git-branch more\ncareful about handling the cases where we couldn't resolve a HEAD.\n\nSo this patch fixes the problem:\n\ndiff --git a/refs.c b/refs.c\nindex df075fcd06..2ba74720c8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\tif (refs_read_raw_ref(refs, refname,\n \t\t\t\t sha1, &sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n-\t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n+\t\t\tif ((errno != ENOENT && errno != EISDIR) ||\n+\t\t\t (resolve_flags & RESOLVE_REF_READING))\n \t\t\t\treturn NULL;\n \t\t\thashclr(sha1);\n \t\t\tif (*flags & REF_BAD_NAME)\n\nbut seems to stimulate a test failure in t3308. I have a suspicion that\nI've just uncovered another bug, but I'll dig in that. In the meantime I\nwanted to post this update in case anybody else was looking into it.\n\n-Peff\n"},{"id":"329865","messageId":"xmqqzi94vcd7.fsf@gitster.mtv.corp.google.com","threadId":"46913","inReplyTo":"20171006083719.jap56jucgmlsuvuo@sigill.intra.peff.net","subject":"Re: Regression in 'git branch -m'?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T09:45:08Z","receivedAt":"2017-10-06T09:45:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So this patch fixes the problem:\n>\n> diff --git a/refs.c b/refs.c\n> index df075fcd06..2ba74720c8 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n> \t\tif (refs_read_raw_ref(refs, refname,\n> \t\t\t\t sha1, &sb_refname, &read_flags)) {\n> \t\t\t*flags |= read_flags;\n> -\t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n> +\t\t\tif ((errno != ENOENT && errno != EISDIR) ||\n> +\t\t\t (resolve_flags & RESOLVE_REF_READING))\n\nOoo, good find--is_missing_file_error() strikes back...\n\n> \t\t\t\treturn NULL;\n> \t\t\thashclr(sha1);\n> \t\t\tif (*flags & REF_BAD_NAME)\n>\n> but seems to stimulate a test failure in t3308. I have a suspicion that\n> I've just uncovered another bug, but I'll dig in that. In the meantime I\n> wanted to post this update in case anybody else was looking into it.\n>\n> -Peff\n"},{"id":"329866","messageId":"20171006100649.rrbsw5dqf5gionzb@sigill.intra.peff.net","threadId":"46913","inReplyTo":"xmqqzi94vcd7.fsf@gitster.mtv.corp.google.com","subject":"Re: Regression in 'git branch -m'?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T10:06:49Z","receivedAt":"2017-10-06T10:06:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 06:45:08PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So this patch fixes the problem:\n> >\n> > diff --git a/refs.c b/refs.c\n> > index df075fcd06..2ba74720c8 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n> > \t\tif (refs_read_raw_ref(refs, refname,\n> > \t\t\t\t sha1, &sb_refname, &read_flags)) {\n> > \t\t\t*flags |= read_flags;\n> > -\t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n> > +\t\t\tif ((errno != ENOENT && errno != EISDIR) ||\n> > +\t\t\t (resolve_flags & RESOLVE_REF_READING))\n> \n> Ooo, good find--is_missing_file_error() strikes back...\n\nAlmost. That uses ENOTDIR, so that looking for \"foo/bar\" handles the\ncase where \"foo\" is a regular file.\n\nBut this is the opposite: we ask about \"foo\", but \"foo/bar\" exists. The\nanswer isn't \"it's not there\" in the general case, but \"it's not the\nthing you were expecting\".\n\nBut in the case of refs, the filesystem is just a representation of the\nabstract namespace. In asking for \"refs/heads/foo\", if \"refs/heads/foo/bar\"\nexists, then answer is still \"no, it's not a ref\".\n\nSo EISDIR is needed for this case, though I suspect the opposite case\nwould need ENOTDIR. I actually wonder if the files-backend read_raw_ref\nought to be normalizing all of those to ENOENT.\n\n> > \t\t\t\treturn NULL;\n> > \t\t\thashclr(sha1);\n> > \t\t\tif (*flags & REF_BAD_NAME)\n> >\n> > but seems to stimulate a test failure in t3308. I have a suspicion that\n> > I've just uncovered another bug, but I'll dig in that. In the meantime I\n> > wanted to post this update in case anybody else was looking into it.\n\nThat failure indeed turned out to be a red herring. So I think I'm\ndefinitely onto the right track.\n\nI want to play with the ENOTDIR case, and then I'll write up the whole\nthing and send it in later today.\n\n-Peff\n"},{"id":"329893","messageId":"20171006143745.w6q2yfgy6nvd2m2a@sigill.intra.peff.net","threadId":"46913","inReplyTo":"20171005172552.GA11497@inner.h.apk.li","subject":"Re: Regression in 'git branch -m'?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T14:37:45Z","receivedAt":"2017-10-06T14:37:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2017 at 07:25:52PM +0200, Andreas Krey wrote:\n\n> I got something that looks like a regression somewhere since 2.11.\n> This script\n> \n> set -xe\n> rm -rf repo\n> git init repo\n> cd repo\n> git commit -m nix --allow-empty\n> git branch -m master/master\n> git rev-parse HEAD\n> git branch\n> git status\n> \n> causes .git/HEAD to still contain 'ref: refs/heads/master' and to fail\n> in the rev-parse step with\n> \n> + git rev-parse HEAD\n> HEAD\n> fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> \n> This is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still works.\n\nSo this turned out to be quite an interesting bug to explore. I think\nthe solution I ended up with in the second patch is the right thing. I'm\nadding Michael to the cc for wisdom on the ref code, though I think the\nbug I'm fixing actually goes back to the early days of Git.\n\nEarlier I blamed Duy's 31824d180d. And that is the start of the\nregression in v2.15, but only because it fixed another bug which was\npapering over the one I'm fixing here. :)\n\n [v1 1/2]: t3308: create a real ref directory/file conflict\n [v1 2/2]: refs_resolve_ref_unsafe: handle d/f conflicts for writes\n\n refs.c | 15 ++++++++++++++-\n t/t1401-symbolic-ref.sh | 26 +++++++++++++++++++++++++-\n t/t3200-branch.sh | 10 ++++++++++\n t/t3308-notes-merge.sh | 2 +-\n 4 files changed, 50 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"329894","messageId":"20171006143830.7sdfpv7jrsdjefxa@sigill.intra.peff.net","threadId":"46913","inReplyTo":"20171006143745.w6q2yfgy6nvd2m2a@sigill.intra.peff.net","subject":"[PATCH 1/2] t3308: create a real ref directory/file conflict","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T14:38:30Z","receivedAt":"2017-10-06T14:38:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A test in t3308 wants to make sure that we don't\naccidentally merge into \"refs/notes/dir\" when it exists as a\ndirectory, so it does:\n\n mkdir .git/refs/notes/dir\n git -c core.notesRef=refs/notes/dir merge ...\n\nand expects the second command to fail. But that\nunderstimates the refs code, which is smart enough to remove\nuseless directories in the refs hierarchy. The test\nsucceeded only because of a bug which prevented resolving\nrefs/notes/dir for writing, even though an actual ref update\nwould succeed.\n\nIn preparation for fixing that bug, let's switch to creating\na real ref in refs/notes/dir, which is a more realistic\nsituation.\n\nSigned-off-by: Jeff King \n---\n t/t3308-notes-merge.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh\nindex 19aed7ec95..ab946a5153 100755\n--- a/t/t3308-notes-merge.sh\n+++ b/t/t3308-notes-merge.sh\n@@ -79,7 +79,7 @@ test_expect_success 'fail to merge empty notes ref into empty notes ref (z => y)\n test_expect_success 'fail to merge into various non-notes refs' '\n \ttest_must_fail git -c \"core.notesRef=refs/notes\" notes merge x &&\n \ttest_must_fail git -c \"core.notesRef=refs/notes/\" notes merge x &&\n-\tmkdir -p .git/refs/notes/dir &&\n+\tgit update-ref refs/notes/dir/foo HEAD &&\n \ttest_must_fail git -c \"core.notesRef=refs/notes/dir\" notes merge x &&\n \ttest_must_fail git -c \"core.notesRef=refs/notes/dir/\" notes merge x &&\n \ttest_must_fail git -c \"core.notesRef=refs/heads/master\" notes merge x &&\n-- \n2.15.0.rc0.413.g9bb4ac64e2\n\n"},{"id":"329896","messageId":"20171006144217.y6oxux26hh2fb7og@sigill.intra.peff.net","threadId":"46913","inReplyTo":"20171006143745.w6q2yfgy6nvd2m2a@sigill.intra.peff.net","subject":"[PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T14:42:17Z","receivedAt":"2017-10-06T14:42:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If our call to refs_read_raw_ref() fails, we check errno to\nsee if the ref is simply missing, or if we encountered a\nmore serious error. If it's just missing, then in \"write\"\nmode (i.e., when RESOLVE_REFS_READING is not set), this is\nperfectly fine.\n\nHowever, checking for ENOENT isn't sufficient to catch all\nmissing-ref cases. In the filesystem backend, we may also\nsee EISDIR when we try to resolve \"a\" and \"a/b\" exists.\nLikewise, we may see ENOTDIR if we try to resolve \"a/b\" and\n\"a\" exists. In both of those cases, we know that our\nresolved ref doesn't exist, but we return an error (rather\nthan reporting the refname and returning a null sha1).\n\nThis has been broken for a long time, but nobody really\nnoticed because the next step after resolving without the\nREADING flag is usually to lock the ref and write it. But in\nboth of those cases, the write will fail with the same\nerrno due to the directory/file conflict.\n\nThere are two cases where we can notice this, though:\n\n 1. If we try to write \"a\" and there's a leftover directory\n already at \"a\", even though there is no ref \"a/b\". The\n actual write is smart enough to move the empty \"a\" out\n of the way.\n\n This is reasonably rare, if only because the writing\n code has to do an independent resolution before trying\n its write (because the actual update_ref() code handles\n this case fine). The notes-merge code does this, and\n before the fix in the prior commit t3308 erroneously\n expected this case to fail.\n\n 2. When resolving symbolic refs, we typically do not use\n the READING flag because we want to resolve even\n symrefs that point to unborn refs. Even if those unborn\n refs could not actually be written because of d/f\n conflicts with existing refs.\n\n You can see this by asking \"git symbolic-ref\" to report\n the target of a symref pointing past a d/f conflict.\n\nWe can fix the problem by recognizing the other \"missing\"\nerrnos and treating them like ENOENT. This should be safe to\ndo even for callers who are then going to actually write the\nref, because the actual writing process will fail if the d/f\nconflict is a real one (and t1404 checks these cases).\n\nArguably this should be the responsibility of the\nfiles-backend to normalize all \"missing ref\" errors into\nENOENT (since something like EISDIR may not be meaningful at\nall to a database backend). However other callers of\nrefs_read_raw_ref() may actually care about the distinction;\nputting this into resolve_ref() is the minimal fix for now.\n\nThe new tests in t1401 use git-symbolic-ref, which is the\nmost direct way to check the resolution by itself.\nInterestingly we actually had a test that setup this case\nalready, but we only used it to verify that the funny state\ncould be overwritten, not that it could be resolved.\n\nWe also add a new test in t3200, as \"branch -m\" was the\noriginal motivation for looking into this. What happens is\nthis:\n\n 0. HEAD is pointing to branch \"a\"\n\n 1. The user asks to rename \"a\" to \"a/b\".\n\n 2. We create \"a/b\" and delete \"a\".\n\n 3. We then try to update any worktree HEADs that point to\n the renamed ref (including the main repo HEAD). To do\n that, we have to resolve each HEAD. But now our HEAD is\n pointing at \"a\", and we get EISDIR due to the loose\n \"a/b\". As a result, we think there is no HEAD, and we\n do not update it. It now points to the bogus \"a\".\n\nInterestingly this case used to work, but only accidentally.\nBefore 31824d180d (branch: fix branch renaming not updating\nHEADs correctly, 2017-08-24), we'd update any HEAD which we\ncouldn't resolve. That was wrong, but it papered over the\nfact that we were incorrectly failing to resolve HEAD.\n\nSo while the bug demonstrated by the git-symbolic-ref is\nquite old, the regression to \"branch -m\" is recent.\n\nSigned-off-by: Jeff King \n---\n refs.c | 15 ++++++++++++++-\n t/t1401-symbolic-ref.sh | 26 +++++++++++++++++++++++++-\n t/t3200-branch.sh | 10 ++++++++++\n 3 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex df075fcd06..c590a992fb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1435,8 +1435,21 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\tif (refs_read_raw_ref(refs, refname,\n \t\t\t\t sha1, &sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n-\t\t\tif (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))\n+\n+\t\t\t/* In reading mode, refs must eventually resolve */\n+\t\t\tif (resolve_flags & RESOLVE_REF_READING)\n+\t\t\t\treturn NULL;\n+\n+\t\t\t/*\n+\t\t\t * Otherwise a missing ref is OK. But the files backend\n+\t\t\t * may show errors besides ENOENT if there are\n+\t\t\t * similarly-named refs.\n+\t\t\t */\n+\t\t\tif (errno != ENOENT &&\n+\t\t\t errno != EISDIR &&\n+\t\t\t errno != ENOTDIR)\n \t\t\t\treturn NULL;\n+\n \t\t\thashclr(sha1);\n \t\t\tif (*flags & REF_BAD_NAME)\n \t\t\t\t*flags |= REF_ISBROKEN;\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex eec3e90f9c..9e782a8122 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -129,11 +129,35 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '\n \ttest_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df\n '\n \n-test_expect_success 'symbolic-ref handles existing pointer to invalid name' '\n+test_expect_success 'symbolic-ref can overwrite pointer to invalid name' '\n+\ttest_when_finished reset_to_sane &&\n \thead=$(git rev-parse HEAD) &&\n \tgit symbolic-ref HEAD refs/heads/outer &&\n+\ttest_when_finished \"git update-ref -d refs/heads/outer/inner\" &&\n \tgit update-ref refs/heads/outer/inner $head &&\n \tgit symbolic-ref HEAD refs/heads/unrelated\n '\n \n+test_expect_success 'symbolic-ref can resolve d/f name (EISDIR)' '\n+\ttest_when_finished reset_to_sane &&\n+\thead=$(git rev-parse HEAD) &&\n+\tgit symbolic-ref HEAD refs/heads/outer/inner &&\n+\ttest_when_finished \"git update-ref -d refs/heads/outer\" &&\n+\tgit update-ref refs/heads/outer $head &&\n+\techo refs/heads/outer/inner >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '\n+\ttest_when_finished reset_to_sane &&\n+\thead=$(git rev-parse HEAD) &&\n+\tgit symbolic-ref HEAD refs/heads/outer &&\n+\ttest_when_finished \"git update-ref -d refs/heads/outer/inner\" &&\n+\tgit update-ref refs/heads/outer/inner $head &&\n+\techo refs/heads/outer >expect &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 3ac7ebf85f..503a88d029 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -117,6 +117,16 @@ test_expect_success 'git branch -m bbb should rename checked out branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'renaming checked out branch works with d/f conflict' '\n+\ttest_when_finished \"git branch -D foo/bar || git branch -D foo\" &&\n+\ttest_when_finished git checkout master &&\n+\tgit checkout -b foo &&\n+\tgit branch -m foo/bar &&\n+\tgit symbolic-ref HEAD >actual &&\n+\techo refs/heads/foo/bar >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git branch -m o/o o should fail when o/p exists' '\n \tgit branch o/o &&\n \tgit branch o/p &&\n-- \n2.15.0.rc0.413.g9bb4ac64e2\n"},{"id":"329903","messageId":"38c17fdc-7a3b-d166-1abe-afe64fc823c5@alum.mit.edu","threadId":"46913","inReplyTo":"20171006144217.y6oxux26hh2fb7og@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-06T17:09:10Z","receivedAt":"2017-10-06T17:09:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/06/2017 04:42 PM, Jeff King wrote:\n> If our call to refs_read_raw_ref() fails, we check errno to\n> see if the ref is simply missing, or if we encountered a\n> more serious error. If it's just missing, then in \"write\"\n> mode (i.e., when RESOLVE_REFS_READING is not set), this is\n> perfectly fine.\n> \n> However, checking for ENOENT isn't sufficient to catch all\n> missing-ref cases. In the filesystem backend, we may also\n> see EISDIR when we try to resolve \"a\" and \"a/b\" exists.\n> Likewise, we may see ENOTDIR if we try to resolve \"a/b\" and\n> \"a\" exists. In both of those cases, we know that our\n> resolved ref doesn't exist, but we return an error (rather\n> than reporting the refname and returning a null sha1).\n> \n> This has been broken for a long time, but nobody really\n> noticed because the next step after resolving without the\n> READING flag is usually to lock the ref and write it. But in\n> both of those cases, the write will fail with the same\n> errno due to the directory/file conflict.\n> \n> There are two cases where we can notice this, though:\n> \n> 1. If we try to write \"a\" and there's a leftover directory\n> already at \"a\", even though there is no ref \"a/b\". The\n> actual write is smart enough to move the empty \"a\" out\n> of the way.\n> \n> This is reasonably rare, if only because the writing\n> code has to do an independent resolution before trying\n> its write (because the actual update_ref() code handles\n> this case fine). The notes-merge code does this, and\n> before the fix in the prior commit t3308 erroneously\n> expected this case to fail.\n> \n> 2. When resolving symbolic refs, we typically do not use\n> the READING flag because we want to resolve even\n> symrefs that point to unborn refs. Even if those unborn\n> refs could not actually be written because of d/f\n> conflicts with existing refs.\n> \n> You can see this by asking \"git symbolic-ref\" to report\n> the target of a symref pointing past a d/f conflict.\n> \n> We can fix the problem by recognizing the other \"missing\"\n> errnos and treating them like ENOENT. This should be safe to\n> do even for callers who are then going to actually write the\n> ref, because the actual writing process will fail if the d/f\n> conflict is a real one (and t1404 checks these cases).\n> \n> Arguably this should be the responsibility of the\n> files-backend to normalize all \"missing ref\" errors into\n> ENOENT (since something like EISDIR may not be meaningful at\n> all to a database backend). However other callers of\n> refs_read_raw_ref() may actually care about the distinction;\n> putting this into resolve_ref() is the minimal fix for now.\n> \n> The new tests in t1401 use git-symbolic-ref, which is the\n> most direct way to check the resolution by itself.\n> Interestingly we actually had a test that setup this case\n> already, but we only used it to verify that the funny state\n> could be overwritten, not that it could be resolved.\n> \n> We also add a new test in t3200, as \"branch -m\" was the\n> original motivation for looking into this. What happens is\n> this:\n> \n> 0. HEAD is pointing to branch \"a\"\n> \n> 1. The user asks to rename \"a\" to \"a/b\".\n> \n> 2. We create \"a/b\" and delete \"a\".\n> \n> 3. We then try to update any worktree HEADs that point to\n> the renamed ref (including the main repo HEAD). To do\n> that, we have to resolve each HEAD. But now our HEAD is\n> pointing at \"a\", and we get EISDIR due to the loose\n> \"a/b\". As a result, we think there is no HEAD, and we\n> do not update it. It now points to the bogus \"a\".\n> \n> Interestingly this case used to work, but only accidentally.\n> Before 31824d180d (branch: fix branch renaming not updating\n> HEADs correctly, 2017-08-24), we'd update any HEAD which we\n> couldn't resolve. That was wrong, but it papered over the\n> fact that we were incorrectly failing to resolve HEAD.\n> \n> So while the bug demonstrated by the git-symbolic-ref is\n> quite old, the regression to \"branch -m\" is recent.\n\nThanks for your detailed investigation and analysis and for the new tests.\n\nThis makes sense to me at the level of fixing the bug.\n\nI do have one twinge of uneasiness at a deeper level, that I haven't had\ntime to check...\n\nDoes this patch make it easier to *set* HEAD to an unborn branch that\nd/f conflicts with an existing reference? If so, that might be a\nslightly worse UI for users. I'd rather learn about such a problem when\nsetting HEAD (when I am thinking about the new branch name and am in the\nframe of mind to solve the problem) rather than later, when I try to\ncommit to the new branch.\n\nEven if so, that wouldn't be a problem with this patch per se, but\nrather a possible accidental side-effect of fixing the bug.\n\nMichael\n\n> [...]\n"},{"id":"329904","messageId":"20171006171623.kjzeavnzopowvqzv@sigill.intra.peff.net","threadId":"46913","inReplyTo":"38c17fdc-7a3b-d166-1abe-afe64fc823c5@alum.mit.edu","subject":"Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T17:16:23Z","receivedAt":"2017-10-06T17:16:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:\n\n> I do have one twinge of uneasiness at a deeper level, that I haven't had\n> time to check...\n> \n> Does this patch make it easier to *set* HEAD to an unborn branch that\n> d/f conflicts with an existing reference? If so, that might be a\n> slightly worse UI for users. I'd rather learn about such a problem when\n> setting HEAD (when I am thinking about the new branch name and am in the\n> frame of mind to solve the problem) rather than later, when I try to\n> commit to the new branch.\n\nGood question. The answer is no, it's allowed both before and after my\npatch. At least via git-symbolic-ref.\n\nI agree it would be nice to know earlier for such a case. For\nsymbolic-ref, we probably should allow it, because it's plumbing that\nmay be used for tricky things. For things like \"checkout -b\", you'd\ngenerally get a timely warning as we try to create the ref.\n\nThe odd man out is \"checkout --orphan\", which leaves the branch unborn.\nIt might be nice if it did a manual check that the ref is available (and\nalso that it's syntactically acceptable, though I think we may do that\nalready).\n\nBut all of that is orthogonal to this fix, I think.\n\n-Peff\n"},{"id":"329956","messageId":"xmqq60brvj42.fsf@gitster.mtv.corp.google.com","threadId":"46913","inReplyTo":"20171006143745.w6q2yfgy6nvd2m2a@sigill.intra.peff.net","subject":"Re: Regression in 'git branch -m'?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-07T01:31:41Z","receivedAt":"2017-10-07T01:31:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Earlier I blamed Duy's 31824d180d. And that is the start of the\n> regression in v2.15, but only because it fixed another bug which was\n> papering over the one I'm fixing here. :)\n\nI haven't read Michael's reply, but 2/2 is very well explained and\nlooks correct.\n\nThanks for digging this through to the root. \n\n> [v1 1/2]: t3308: create a real ref directory/file conflict\n> [v1 2/2]: refs_resolve_ref_unsafe: handle d/f conflicts for writes\n>\n> refs.c | 15 ++++++++++++++-\n> t/t1401-symbolic-ref.sh | 26 +++++++++++++++++++++++++-\n> t/t3200-branch.sh | 10 ++++++++++\n> t/t3308-notes-merge.sh | 2 +-\n> 4 files changed, 50 insertions(+), 3 deletions(-)\n>\n> -Peff\n"},{"id":"329959","messageId":"cae7028d-b92e-a7ca-6d33-713665848da3@alum.mit.edu","threadId":"46913","inReplyTo":"20171006171623.kjzeavnzopowvqzv@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-07T04:36:09Z","receivedAt":"2017-10-07T04:36:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/06/2017 07:16 PM, Jeff King wrote:\n> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:\n> \n>> I do have one twinge of uneasiness at a deeper level, that I haven't had\n>> time to check...\n>>\n>> Does this patch make it easier to *set* HEAD to an unborn branch that\n>> d/f conflicts with an existing reference? If so, that might be a\n>> slightly worse UI for users. I'd rather learn about such a problem when\n>> setting HEAD (when I am thinking about the new branch name and am in the\n>> frame of mind to solve the problem) rather than later, when I try to\n>> commit to the new branch.\n> \n> Good question. The answer is no, it's allowed both before and after my\n> patch. At least via git-symbolic-ref.\n> \n> I agree it would be nice to know earlier for such a case. For\n> symbolic-ref, we probably should allow it, because it's plumbing that\n> may be used for tricky things. For things like \"checkout -b\", you'd\n> generally get a timely warning as we try to create the ref.\n> \n> The odd man out is \"checkout --orphan\", which leaves the branch unborn.\n> It might be nice if it did a manual check that the ref is available (and\n> also that it's syntactically acceptable, though I think we may do that\n> already).\n> \n> But all of that is orthogonal to this fix, I think.\n\nThanks for checking. Yes, I totally agree that this is orthogonal.\n\nMichael\n"},{"id":"331841","messageId":"267a64d4-2551-e5ae-c289-5d9d21d217fb@alum.mit.edu","threadId":"46913","inReplyTo":"cae7028d-b92e-a7ca-6d33-713665848da3@alum.mit.edu","subject":"Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-11-05T05:36:45Z","receivedAt":"2017-11-05T05:36:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/07/2017 06:36 AM, Michael Haggerty wrote:\n> On 10/06/2017 07:16 PM, Jeff King wrote:\n>> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:\n>>\n>>> I do have one twinge of uneasiness at a deeper level, that I haven't had\n>>> time to check...\n>>>\n>>> Does this patch make it easier to *set* HEAD to an unborn branch that\n>>> d/f conflicts with an existing reference? If so, that might be a\n>>> slightly worse UI for users. I'd rather learn about such a problem when\n>>> setting HEAD (when I am thinking about the new branch name and am in the\n>>> frame of mind to solve the problem) rather than later, when I try to\n>>> commit to the new branch.\n>>\n>> Good question. The answer is no, it's allowed both before and after my\n>> patch. At least via git-symbolic-ref.\n>>\n>> I agree it would be nice to know earlier for such a case. For\n>> symbolic-ref, we probably should allow it, because it's plumbing that\n>> may be used for tricky things. For things like \"checkout -b\", you'd\n>> generally get a timely warning as we try to create the ref.\n>>\n>> The odd man out is \"checkout --orphan\", which leaves the branch unborn.\n>> It might be nice if it did a manual check that the ref is available (and\n>> also that it's syntactically acceptable, though I think we may do that\n>> already).\n>>\n>> But all of that is orthogonal to this fix, I think.\n> \n> Thanks for checking. Yes, I totally agree that this is orthogonal.\n\nI also just checked but there don't seem to be any docstrings that need\nupdating.\n\nReviewed-by: Michael Haggerty \n\n(both patches in this series).\n\nMichael\n\n\n\n\n"}]}