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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2017, 01:49 UTC
Message-ID: <xmqqr2srxeu2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171121231739.GB21197@sigill>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> Here's a re-roll of patch 5 that behaves this way (the patch should be
> unsurprising, but I've updated the commit message to match).
>
> I did notice one other thing: the function looks up replace objects, so
> we have both the original and the replaced sha1 available. My earlier
> patch used the original sha1, but this one uses the replaced value.
> I'm not sure if that's sane or not. It lets the fast-path kick in if you
> point a replace ref at 0{40}. And it lets you point refs/replace/0{40}
> to something else. I doubt that's useful, but it could perhaps be for
> debugging, etc.
>
> In most cases, of course, it wouldn't matter (since pointing to or from
> the null sha1 is vaguely crazy in the first place).

I tend to agree that those who go crazy/fancy with replace mechanism can keep both halves when it breaks.

WRT existing codepaths that pass 0{40} and refuses to notice a potential repository corruption (from getting a NULL for a non null object name), I think we would need a sweep of the codebase and fix them in the longer term. As long as that will happen someday, either approach between "we know 'no loose object? let's redo the packs' is the part that matters performance-wise, so let's do a short-cut only for that" and "we know that callers that comes with 0{40} want to get NULL back" should be OK, I would think.