Re: [PATCH 1/2] sha1_file: drop experimental GIT_USE_LOOKUP search

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2017, 18:12 UTC
Message-ID: <xmqqshh06264.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170809101432.6jquwmev423o56pb@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> This lets us remove sha1_entry_pos() entirely, as the .idx
> lookup code was the only caller.  Note that sha1-lookup.c
> still contains sha1_pos(), which differs from
> sha1_entry_pos() in two ways:
>
>   - it has a different interface; it uses a function pointer
>     to access sha1 entries rather than a size/offset pair
>     describing the table's memory layout
>
>   - it only scales the initial selection of "mi", rather
>     than each iteration of the search
>
> We can't get rid of this function, as it's called from
> several places. It may be that we could replace it with a
> simple binary search, but that's out of scope for this patch
> (and would need benchmarking).

Thanks for reducing the count of binary search functions by one.

I think the "just one round of newton-raphson" in sha1_pos() comes from [*1*]; I agree that it needs benchmarking before tweaking it.

We may want to tell libgit2 folks about this change, though [*2*]. I think they too are carrying dead code that is only used under CPP macro GIT_USE_LOOKUP, which they do not seem to define.

[Reference]

*1* https://public-inbox.org/git/7v38vso8kz.fsf@alter.siamese.dyndns.org/ *2* https://github.com/libgit2/libgit2/commit/dd453c4dbf9a1fa38530b1f51e079852736b8f66