refs_resolve_ref_unsafe: handle d/f conflicts for writes

If our call to refs_read_raw_ref() fails, we check errno to see if the ref is simply missing, or if we encountered a more serious error. If it's just missing, then in "write" mode (i.e., when RESOLVE_REFS_READING is not set), this is perfectly fine. However, checking for ENOENT isn't sufficient to catch all missing-ref cases. In the filesystem backend, we may also see EISDIR when we try to resolve "a" and "a/b" exists. Likewise, we may see ENOTDIR if we try to resolve "a/b" and "a" exists. In both of those cases, we know that our resolved ref doesn't exist, but we return an error (rather than reporting the refname and returning a null sha1). This has been broken for a long time, but nobody really noticed because the next step after resolving without the READING flag is usually to lock the ref and write it. But in both of those cases, the write will fail with the same errno due to the directory/file conflict. There are two cases where we can notice this, though: 1. If we try to write "a" and there's a leftover directory already at "a", even though there is no ref "a/b". The actual write is smart enough to move the empty "a" out of the way. This is reasonably rare, if only because the writing code has to do an independent resolution before trying its write (because the actual update_ref() code handles this case fine). The notes-merge code does this, and before the fix in the prior commit t3308 erroneously expected this case to fail. 2. When resolving symbolic refs, we typically do not use the READING flag because we want to resolve even symrefs that point to unborn refs. Even if those unborn refs could not actually be written because of d/f conflicts with existing refs. You can see this by asking "git symbolic-ref" to report the target of a symref pointing past a d/f conflict. We can fix the problem by recognizing the other "missing" errnos and treating them like ENOENT. This should be safe to do even for callers who are then going to actually write the ref, because the actual writing process will fail if the d/f conflict is a real one (and t1404 checks these cases). Arguably this should be the responsibility of the files-backend to normalize all "missing ref" errors into ENOENT (since something like EISDIR may not be meaningful at all to a database backend). However other callers of refs_read_raw_ref() may actually care about the distinction; putting this into resolve_ref() is the minimal fix for now. The new tests in t1401 use git-symbolic-ref, which is the most direct way to check the resolution by itself. Interestingly we actually had a test that setup this case already, but we only used it to verify that the funny state could be overwritten, not that it could be resolved. We also add a new test in t3200, as "branch -m" was the original motivation for looking into this. What happens is this: 0. HEAD is pointing to branch "a" 1. The user asks to rename "a" to "a/b". 2. We create "a/b" and delete "a". 3. We then try to update any worktree HEADs that point to the renamed ref (including the main repo HEAD). To do that, we have to resolve each HEAD. But now our HEAD is pointing at "a", and we get EISDIR due to the loose "a/b". As a result, we think there is no HEAD, and we do not update it. It now points to the bogus "a". Interestingly this case used to work, but only accidentally. Before 31824d180d (branch: fix branch renaming not updating HEADs correctly, 2017-08-24), we'd update any HEAD which we couldn't resolve. That was wrong, but it papered over the fact that we were incorrectly failing to resolve HEAD. So while the bug demonstrated by the git-symbolic-ref is quite old, the regression to "branch -m" is recent. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 6, 2017 at 10:42 UTC a1c1d8170dbc4a108dd2c05d2f93049d49e61328
3 files changed +49 -2
refs.c
+14 -1
@@ -1435,8 +1435,21 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
1435 if (refs_read_raw_ref(refs, refname,
1436 sha1, &sb_refname, &read_flags)) {
1437 *flags |= read_flags;
1438 - if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
1438 +
1439 + /* In reading mode, refs must eventually resolve */
1440 + if (resolve_flags & RESOLVE_REF_READING)
1441 + return NULL;
1442 +
1443 + /*
1444 + * Otherwise a missing ref is OK. But the files backend
1445 + * may show errors besides ENOENT if there are
1446 + * similarly-named refs.
1447 + */
1448 + if (errno != ENOENT &&
1449 + errno != EISDIR &&
1450 + errno != ENOTDIR)
1451 return NULL;
1452 +
1453 hashclr(sha1);
1454 if (*flags & REF_BAD_NAME)
1455 *flags |= REF_ISBROKEN;
t/t1401-symbolic-ref.sh
+25 -1
@@ -129,11 +129,35 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '
129 test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df
130 '
131
132 -test_expect_success 'symbolic-ref handles existing pointer to invalid name' '
132 +test_expect_success 'symbolic-ref can overwrite pointer to invalid name' '
133 + test_when_finished reset_to_sane &&
134 head=$(git rev-parse HEAD) &&
135 git symbolic-ref HEAD refs/heads/outer &&
136 + test_when_finished "git update-ref -d refs/heads/outer/inner" &&
137 git update-ref refs/heads/outer/inner $head &&
138 git symbolic-ref HEAD refs/heads/unrelated
139 '
140
141 +test_expect_success 'symbolic-ref can resolve d/f name (EISDIR)' '
142 + test_when_finished reset_to_sane &&
143 + head=$(git rev-parse HEAD) &&
144 + git symbolic-ref HEAD refs/heads/outer/inner &&
145 + test_when_finished "git update-ref -d refs/heads/outer" &&
146 + git update-ref refs/heads/outer $head &&
147 + echo refs/heads/outer/inner >expect &&
148 + git symbolic-ref HEAD >actual &&
149 + test_cmp expect actual
150 +'
151 +
152 +test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '
153 + test_when_finished reset_to_sane &&
154 + head=$(git rev-parse HEAD) &&
155 + git symbolic-ref HEAD refs/heads/outer &&
156 + test_when_finished "git update-ref -d refs/heads/outer/inner" &&
157 + git update-ref refs/heads/outer/inner $head &&
158 + echo refs/heads/outer >expect &&
159 + git symbolic-ref HEAD >actual &&
160 + test_cmp expect actual
161 +'
162 +
163 test_done
t/t3200-branch.sh
+10
@@ -117,6 +117,16 @@ test_expect_success 'git branch -m bbb should rename checked out branch' '
117 test_cmp expect actual
118 '
119
120 +test_expect_success 'renaming checked out branch works with d/f conflict' '
121 + test_when_finished "git branch -D foo/bar || git branch -D foo" &&
122 + test_when_finished git checkout master &&
123 + git checkout -b foo &&
124 + git branch -m foo/bar &&
125 + git symbolic-ref HEAD >actual &&
126 + echo refs/heads/foo/bar >expect &&
127 + test_cmp expect actual
128 +'
129 +
130 test_expect_success 'git branch -m o/o o should fail when o/p exists' '
131 git branch o/o &&
132 git branch o/p &&