Re: Regression in 'git branch -m'?

From: Jeff King <peff@peff.net>
Date: Oct 6, 2017, 08:37 UTC
Message-ID: <20171006083719.jap56jucgmlsuvuo@sigill.intra.peff.net>
In-Reply-To: <20171006073913.yavdbdd3p3y5vjhd@sigill.intra.peff.net>

On Fri, Oct 06, 2017 at 03:39:13AM -0400, Jeff King wrote:

Show 15 quoted lines

> I got a chance to look at this again. I think the root of the problem is
> that resolve_ref() as it is implemented now is just totally unsuitable
> for asking the question "what does this symbolic link point to?".
> 
> Because you end up with either:
> 
>   1. If we pass RESOLVE_REF_READING, then we do not return the target
>      refname for orphaned commits (which is why 31824d180d dropped it).
> 
>   2. If not, then we do not return the target refname for commits with
>      names that are not available for writing. The d/f conflict here is
>      one example, but there may be others.
> 
> So I think we need to teach resolve_ref() a new mode that's like
> "reading", but just follows the symref chain.

This analysis is not _quite_ right. The "not available for writing" thing actually isn't intentionally enforced by the resolve_ref. It's just that it's not careful enough about checking errno. We see EISDIR instead of ENOENT when there's a d/f situation, but both have the same practical effect: that ref doesn't exist.

I.e., this lookup has _always_ been broken, even in the "reading" case. It's just that the fix from 31824d180d (correctly) made git-branch more careful about handling the cases where we couldn't resolve a HEAD.

So this patch fixes the problem:

diff --git a/refs.c b/refs.c
index df075fcd06..2ba74720c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 		if (refs_read_raw_ref(refs, refname,
 				      sha1, &sb_refname, &read_flags)) {
 			*flags |= read_flags;
-			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
+			if ((errno != ENOENT && errno != EISDIR) ||
+			    (resolve_flags & RESOLVE_REF_READING))
 				return NULL;
 			hashclr(sha1);
 			if (*flags & REF_BAD_NAME)

but seems to stimulate a test failure in t3308. I have a suspicion that
I've just uncovered another bug, but I'll dig in that. In the meantime I
wanted to post this update in case anybody else was looking into it.

-Peff