Re: [PATCH 2/4] remote: handle broken symrefs

From: Jeff King <peff@peff.net>
Date: Oct 19, 2017, 17:53 UTC
Message-ID: <20171019175315.aewiewjk3fvhifa4@sigill.intra.peff.net>
In-Reply-To: <20171019174730.urelfyc2kj37fokg@sigill.intra.peff.net>

On Thu, Oct 19, 2017 at 01:47:30PM -0400, Jeff King wrote:

Show 9 quoted lines

> This is hard to trigger in practice, since this function is
> used as a callback to for_each_ref(), which will skip broken
> refs in the first place (so it would have to be broken
> racily, or for us to see a transient filesystem error).
> 
> If we see such a racy broken outcome let's treat it as "not
> a symref". This is exactly the same thing that would happen
> in the non-racy case (our function would not be called at
> all, as for_each_ref would skip the broken symref).

The fact that we have to re-resolve the ref here to find the symref points to a short-coming in the for_each_ref() interface. It resolved the ref already to get us the oid, so it should (or at least could) know the symref details already. But it doesn't record them or make them available to callers.

Ditto for patch 3. It doesn't use for_each_ref(), but I suspect it could be recording the value of HEAD more carefully from the prior lookup, avoiding the re-resolution completely.

Refactoring for_each_ref() is probably a bit big for a #leftoverbits, but looking into the case in patch 3 might not be.

-Peff