From: Jeff King <peff@peff.net>
Date: Wed, 24 Jan 2018 20:27:54 GMT
Subject: Re: [PATCH 3/6] find_reference_location(): make function safe for empty snapshots
Message-ID: <20180124202754.GA7773@sigill.intra.peff.net>
In-Reply-To: <e9f9ed1944c297a68c2b76f5d4ddd73e279bd207.1516791909.git.mhagger@alum.mit.edu>

On Wed, Jan 24, 2018 at 12:14:13PM +0100, Michael Haggerty wrote:

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 08698de6ea..361affd7ad 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> [...]
> @@ -551,7 +553,7 @@ static const char *find_reference_location(struct snapshot *snapshot,
>  	 */
>  	const char *hi = snapshot->eof;
>  
> -	while (lo < hi) {
> +	while (lo != hi) {
>  		const char *mid, *rec;
>  		int cmp;

This tightens the binary search termination condition. If we ever did
see "hi > lo", we'd want to terminate the loop. Is that ever possible?

I think the answer is "no". Our "hi" here is an exclusive bound, so we
should never go past it via find_end_of_record() when assigning "lo".
And "hi" is always assigned from the start of the current record. That
can never cross "lo", because find_start_of_record() ensures it.

So I think it's fine, but I wanted to double check.

-Peff