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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2017, 02:35 UTC
Message-ID: <xmqqpo89rac6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171122223613.GA1405@sigill>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> So there are four cases we care about for this call in fetch:
>
>   1. We fed a real sha1 and got a commit (or peeled to one).
>
>   2. We fed a real sha1 which resolved to a non-commit, and we got NULL.
>
>   3. We fed a real sha1 and the object was missing or corrupted, and we
>      got NULL.
>
>   4. We fed a null sha1 and got NULL.
>
> Right now we lump cases 2-4 together as "do not do a fast-forward
> check". That's fine for 2 and 4, but probably not for 3. We can easily
> catch case 4 ourselves (if we care to), but distinguishing case 3 from
> the others is hard. How should lookup_commit_reference_gently() signal
> it to us?

Not limiting us to the caller in the "fetch" codepath, I think the expectation by callers of lookup_commit_reference_gently() in the ideal world would be:

 - It has an object name, and wants to use it as point in the commit
   DAG to define the traversal over the DAG, if it refers to a
   commit known to us.

 - It does not know if these object names represent a tag object, a
   commit object, or some other object.  It does not know if the
   local repository actually has them (e.g. we received a "have"
   from the other side---missing is expected).

 - Hence, it would happily accept a NULL as "we do not have it" and
   "we do have it, but it is not a commit-ish".

And from that point of view, 2, 3a (missing), and 4 (0{40}) to yield NULL is perfectly fine. 3b (exists but broken) may be a noteworthy event, but for the purpose of the caller, it may want to proceed as if the object is missing from our end, so it might deserve warning() but not die(), at least as the default behaviour.