Re: [PATCH 5/5] sha1_file: don't re-scan pack directory for null sha1

From: Jeff King <peff@peff.net>
Date: Nov 21, 2017, 22:57 UTC
Message-ID: <20171121225744.GA21197@sigill>
In-Reply-To: <xmqqd14cjr13.fsf@gitster.mtv.corp.google.com>

On Tue, Nov 21, 2017 at 11:37:28AM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > In an ideal world, we'd simply fix all of the callers to
> > notice the null sha1 and avoid passing it to us. But a
> > simple experiment to catch this with a BUG() shows that
> > there are a large number of code paths.
> 
> Well, we can view this (or the alternative you sent later that does
> the same a bit earlier in the function) as "fixing the caller" but
> has already refactord the common logic to a helper function that all
> of these callers call into ;-).

Yes, I'm definitely tempted by that view. :)

What worries me, though, is that callers who lazily propagate the null sha1 to the lookup functions cannot reasonably tell the difference between "this object was corrupt or missing" and "we passed the null sha1, and a missing object is expected".

For example, look at how fetch.c:update_local_ref() looks up objects. It feeds the old and new sha1 to lookup_commit_reference_gently(), and if either is NULL, it skips the fast-forward check. That makes sense if we expect the null sha1 to get translated into a NULL commit. But it also triggers for a broken ref, and we'd overwrite it (when the right thing is probably refusing to update).

Here's a runnable example:

-- >8 -- git init parent git -C parent commit --allow-empty -m base

git clone parent child git -C parent commit --allow-empty -m more

cd child rm -f .git/objects/??/* git fetch -- 8< --

That final fetch spews a bunch of errors about broken refs, and then overwrites the value of origin/master, even though it's broken (in this case it actually is a fast-forward, but the child repo doesn't even know that).

I'm not sure what the right behavior is, but I'm pretty sure that's not it. Probably one of:

  - skip updating the ref when we see the breakage

  - ditto, but terminate the whole operation, since we might be deleting
    other refs and in a broken repo we're probably best to make as few
    changes as possible

  - behave as if it was a non-ff, which would allow "--force" to
    overwrite the broken ref. Maybe convenient for fixing things, but
    possibly surprising (and it's not that hard to just delete the
    broken refs manually before proceeding).

-Peff