Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog

From: Jeff King <peff@peff.net>
Date: Feb 27, 2024, 08:05 UTC
Message-ID: <20240227080501.GF3263678@coredump.intra.peff.net>
In-Reply-To: <xmqqmsrncf3r.fsf@gitster.g>

On Mon, Feb 26, 2024 at 09:25:28AM -0800, Junio C Hamano wrote:

Show 8 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >  	if (!cb.reccnt) {
> > +		if (cnt == 0) {
> 
> Style "if (!cnt)" ?  In this particular case I do not think it
> actually is an improvement, though, simply because zero is really
> special in this logic.

Yeah, I didn't really choose "== 0" as a conscious decision. But I do tend to write "!cnt" when available, so I think I sub-consciously chose this as a better description of the intent.

Show 22 quoted lines

> 
> > +			/*
> > +			 * The caller asked for ref@{0}, and we had no entries.
> > +			 * It's a bit subtle, but in practice all callers have
> > +			 * prepped the "oid" field with the current value of
> > +			 * the ref, which is the most reasonable fallback.
> > +			 *
> > +			 * We'll put dummy values into the out-parameters (so
> > +			 * they're not just uninitialized garbage), and the
> > +			 * caller can take our return value as a hint that
> > +			 * we did not find any such reflog.
> > +			 */
> > +			set_read_ref_cutoffs(&cb, 0, 0, "empty reflog");
> > +			return 1;
> > +		}
> 
> The dummy value I 100% agree with ;-).
> 
> You mentioned the convenience special case for time-based reflog
> query for a time older than (e.g. @{20.years.ago}) the reflog
> itself, and perhaps this one should be treated as its counterpart,
> that is only useful for count-based access.

I think the true counterpart to that would be extending what's added by patch 2 to get_oid_basic() to stop checking that we hit the count exactly.

Let me try to lay out my thinking. If you _do_ have a reflog and the request (whether time or count-based) goes too far back, read_ref_at() will give you the oldest entry and return "1". And then in get_oid_basic():

  - if it was a time based cutoff (like @{20.years.ago}), we will issue
    a warning ("log only goes back to 2024-01-01") but return the value
    anyway.

  - before this series, if it is a count based cutoff (like @{9999}), we
    fail and say "there are only have N entries".

  - after this series, we special case asking for @{9999} when there are
    9998 entries by returning the "old" oid but not issuing any warning
    (we do not need to, because we found the right answer for what it
    would have been had that old entry not been pruned).

  - what we _could_ do (but this series does not), and what would be the
    true counterpart to the @{20.years.ago} case, is to allow @{9999}
    for a reflog with only 20 entries, returning the old value from 20
    (or the new value if it was a creation!?) and issuing a warning
    saying "well, it only went back 20, but here you go".

I'm not so sure about that last one. It is pretty subjective, but somehow asking for timestamps feels more "fuzzy" to me, and Git returning a fuzzy answer is OK. Whereas asking for item 9999 in a list with 20 items and getting back an answer feels more absolutely wrong. I could be persuaded if there were a concrete use case, but I can't really think of one. It seems more likely to confuse and hinder a user than to help them.

-Peff