On 10/06/2017 04:42 PM, Jeff King wrote: > 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. Thanks for your detailed investigation and analysis and for the new tests. This makes sense to me at the level of fixing the bug. I do have one twinge of uneasiness at a deeper level, that I haven't had time to check... Does this patch make it easier to *set* HEAD to an unborn branch that d/f conflicts with an existing reference? If so, that might be a slightly worse UI for users. I'd rather learn about such a problem when setting HEAD (when I am thinking about the new branch name and am in the frame of mind to solve the problem) rather than later, when I try to commit to the new branch. Even if so, that wouldn't be a problem with this patch per se, but rather a possible accidental side-effect of fixing the bug. Michael > [...]
