Re: [PATCH] refs: allow @{n} to work with n-sized reflog

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2021, 21:02 UTC
Message-ID: <xmqqft3drd82.fsf@gitster.c.googlers.com>
In-Reply-To: <X/V0DU+CD6mS36dK@generichostname>

Denton Liu <liu.denton@gmail.com> writes:

Show 14 quoted lines

>> > -	if (timestamp <= cb->at_time || cb->cnt == 0) {
>> > +	if (cb->cnt > 0)
>> > +		cb->cnt--;
>> > +	at_indexed_ent = cb->cnt == 0 && !is_null_oid(ooid);
>> 
>> The code treats two cases identically (i.e. the case where cb->cnt
>> was originally zero, and one).  Is that intended?
>
> It shouldn't be possible for cb->cnt == 0 on the first iteration
> because there's a special-case check at [0]. As a result, it can only be
> -1 or >= 1 on the first iteration.
>
> The -1 case happens when we're doing date-based lookup and that's what
> this if is intended to handle.

I knew about -1; it wasn't apparent that the caller won't call us with cnt==0. Perhaps it deserves a mention in an in-code comment.

> "at_indexed_ent" is meant to signal when we are indexing the reflog
> numerically (as opposed to by date), we have arrived at the correct
> entry. If you have a more fitting name, I'm open to suggestions.

When querying for <ref>@{24}, all the entries are indexed numerically (counted), not just the 24th one, and that contributed to my puzzlement.

I offhand do not think of a "name", but "at target", "found", "reached count", are phrases that come to my mind as starting points.

Thanks.