Re: [PATCH v2 7/8] refs: stop unsetting REF_HAVE_OLD for log-only updates

From: Jeff King <peff@peff.net>
Date: Jul 29, 2025, 07:14 UTC
Message-ID: <20250729071455.GB1426859@coredump.intra.peff.net>
In-Reply-To: <aIeMgE-11UnAJINI@pks.im>

On Mon, Jul 28, 2025 at 04:43:12PM +0200, Patrick Steinhardt wrote:

Show 21 quoted lines

> > > @@ -2508,8 +2507,9 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
> > >  						struct object_id *oid,
> > >  						struct strbuf *err)
> > >  {
> > > -	if (!(update->flags & REF_HAVE_OLD) ||
> > > -		   oideq(oid, &update->old_oid))
> > > +	if (update->flags & REF_LOG_ONLY ||
> > > +	    !(update->flags & REF_HAVE_OLD) ||
> > > +	    oideq(oid, &update->old_oid))
> > >  		return 0;
> > >  
> > >  	if (is_null_oid(&update->old_oid)) {
> > 
> > Which make sense to me. But the weird thing I noticed is that when we do
> > something similar for split_head_update(), we don't strip REF_HAVE_OLD!
> 
> And we shouldn't do that, as in the next commit we actually build on
> always having `REF_HAVE_OLD` set for reflog-only updates. So I'd argue
> that the problem is actually the other way round: when splitting off the
> HEAD update we must resolve the old object ID if `REF_HAVE_OLD` is not
> set.

Yeah, I agree that after your patches, split_head_update() should definitely not be clearing that flag. What I more meant was: this patch is introducing a behavior change for those split HEAD updates, which used to do the extra old-oid check but now won't (whereas for other symref log-only updates, you are preserving the behavior).

I _think_ that's a reasonable thing, but I wanted to make sure.

However...

> > (For those not familiar with that

...did you mean to write more? I know you've been running into weird email truncation issues lately.

-Peff