git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Denton Liu <liu.denton@gmail.com>
Date
Jan 6, 2021, 08:25 UTC
Message-ID
<X/V0DU+CD6mS36dK@generichostname>
In-Reply-To
<xmqqk0sqvcby.fsf@gitster.c.googlers.com>
Hi Junio,
On Tue, Jan 05, 2021 at 09:55:29PM -0800, Junio C Hamano wrote:
Show 29 quoted lines
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > This sequence works
> >
> > 	$ git checkout -b newbranch
> > 	$ git commit --allow-empty -m one
> > 	$ git show -s newbranch@{1}
> >
> > and shows the state that was immediately after the newbranch was
> > created.
> >
> > But then if you do
> >
> > 	$ git reflog expire --expire=now refs/heads/newbranch
> > 	$ git commit --allow=empty -m two
> > 	$ git show -s newbranch@{1}
> >
> > you'd be scolded with
> >
> > 	fatal: log for 'newbranch' only has 1 entries
> >
> > While it is true that it has only 1 entry, we have enough
> > information in that single entry that records the transition between
> > the state in which the tip of the branch was pointing at commit
> > 'one' to the new commit 'two' built on it, so we should be able to
> > answer "what object newbranch was pointing at?". But we refuse to
> > do so.
> 
> Yeah, I am often hit and irritated by this behaviour.
Yep, this was inspired by one of your emails ;)
Show 21 quoted lines
> > diff --git a/refs.c b/refs.c
> > index 13dc2c3291..c35c61a009 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -887,12 +887,16 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
> >  		const char *message, void *cb_data)
> >  {
> >  	struct read_ref_at_cb *cb = cb_data;
> > +	int at_indexed_ent;
> >  
> >  	cb->reccnt++;
> >  	cb->tz = tz;
> >  	cb->date = timestamp;
> >  
> > -	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.

In the case where it's >= 1, we will always enter the if and it will always pre-decrement. This essentially gets us the n-1 behaviour.

Show 6 quoted lines
> I thought the code was to special case only <ref>@{0}, but with this
> conditional decrement, cb->cnt==0 would not be usable by the rest
> of the code as the "we must read the new side instead" signal. Is
> that why null-ness of ooid is also tested here?  It is hard to tell
> the intention because "at_indexed_ent" does not quite tell me what
> the code wants to use the variable for.

The null-ness of the ooid is needed because on the last entry of the reflog, ooid will be null so we should skip that.

"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.

Show 59 quoted lines
> > +	if (timestamp <= cb->at_time || at_indexed_ent) {
> >  		if (cb->msg)
> >  			*cb->msg = xstrdup(message);
> >  		if (cb->cutoff_time)
> > @@ -905,28 +909,41 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
> >  		 * we have not yet updated cb->[n|o]oid so they still
> >  		 * hold the values for the previous record.
> >  		 */
> > -		if (!is_null_oid(&cb->ooid)) {
> > -			oidcpy(cb->oid, noid);
> > -			if (!oideq(&cb->ooid, noid))
> > -				warning(_("log for ref %s has gap after %s"),
> > +		if (!is_null_oid(&cb->ooid) && !oideq(&cb->ooid, noid))
> > +			warning(_("log for ref %s has gap after %s"),
> >  					cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
> > -		}
> > -		else if (cb->date == cb->at_time)
> > +		if (at_indexed_ent)
> > +			oidcpy(cb->oid, ooid);
> > +		else if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time)
> >  			oidcpy(cb->oid, noid);
> >  		else if (!oideq(noid, cb->oid))
> >  			warning(_("log for ref %s unexpectedly ended on %s"),
> >  				cb->refname, show_date(cb->date, cb->tz,
> >  						       DATE_MODE(RFC2822)));
> > -		oidcpy(&cb->ooid, ooid);
> > -		oidcpy(&cb->noid, noid);
> >  		cb->found_it = 1;
> > -		return 1;
> >  	}
> >  	oidcpy(&cb->ooid, ooid);
> >  	oidcpy(&cb->noid, noid);
> > -	if (cb->cnt > 0)
> > -		cb->cnt--;
> > -	return 0;
> > +	return cb->found_it;
> > +}
> > +
> > +static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid,
> > +				  const char *email, timestamp_t timestamp,
> > +				  int tz, const char *message, void *cb_data)
> > +{
> > +	struct read_ref_at_cb *cb = cb_data;
> > +
> > +	if (cb->msg)
> > +		*cb->msg = xstrdup(message);
> > +	if (cb->cutoff_time)
> > +		*cb->cutoff_time = timestamp;
> > +	if (cb->cutoff_tz)
> > +		*cb->cutoff_tz = tz;
> > +	if (cb->cutoff_cnt)
> > +		*cb->cutoff_cnt = cb->reccnt;
> > +	oidcpy(cb->oid, noid);
> > +	/* We just want the first entry */
> > +	return 1;
> >  }
> 
> The similarity of this to read_ref_at_ent_oldest is somehow
> striking.  Do we really need to invent a new callback?

Unfortunately, yes. The alternative is to add a flag into `struct read_ref_at_cb` and we could conditionally choose whether or not to copy noid or ooid but this seems like the lesser of two evils.

The duplicated part,
	if (cb->msg)
		*cb->msg = xstrdup(message);
	if (cb->cutoff_time)
		*cb->cutoff_time = timestamp;
	if (cb->cutoff_tz)
		*cb->cutoff_tz = tz;
	if (cb->cutoff_cnt)
		*cb->cutoff_cnt = cb->reccnt;

is actually repeated three times -- once in each of the callbacks. I considered extracting factoring it out into a function but I was on the fence because the function would still have some duplication since it'd still require cb, message, timestamp and tz to all be passed in.

Show 10 quoted lines
> >  static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,
> > @@ -967,6 +984,11 @@ int read_ref_at(struct ref_store *refs, const char *refname,
> >  	cb.cutoff_cnt = cutoff_cnt;
> >  	cb.oid = oid;
> >  
> > +	if (cb.cnt == 0) {
> > +		refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
> > +		return 0;
> > +	}
> > +
[0]
> >  	refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
> >  
> >  	if (!cb.reccnt) {

Thanks, Denton

Previous: Junio C HamanoNext: Junio C Hamano
Message 6 of 21 in “refs: allow @{n} to work with n-sized reflog”
  1. refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 2, 2021
  2. Martin ÅgrenJan 2, 2021
  3. Denton LiuJan 3, 2021
  4. SZEDER GáborJan 5, 2021
  5. Junio C HamanoJan 6, 2021
  6. Denton LiuJan 6, 2021
  7. Junio C HamanoJan 6, 2021
  8. 0/2 refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 6, 2021
  9. 1/2 refs: factor out set_read_ref_cutoffs()Denton Liu, Jan 6, 2021
  10. 2/2 refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 6, 2021
  11. SZEDER GáborJan 6, 2021
  12. 0/2 refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 7, 2021
  13. 1/2 refs: factor out set_read_ref_cutoffs()Denton Liu, Jan 7, 2021
  14. 2/2 refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 7, 2021
  15. Simon RuderichJan 10, 2021
  16. fixup! refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 12, 2021
  17. Denton LiuJan 12, 2021
  18. Junio C HamanoJan 12, 2021
  19. SZEDER GáborJan 10, 2021
  20. Junio C HamanoJan 10, 2021
  21. 3/2 fixup! refs: allow @{n} to work with n-sized reflogDenton Liu, Jan 7, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.