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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 24, 2025 at 03:21:30AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/refs.c b/refs.c
> > index 64544300dc3..c78d5be6e20 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -3310,7 +3305,7 @@ int repo_migrate_ref_storage_format(struct repository *repo,
> >
> >  int ref_update_expects_existing_old_ref(struct ref_update *update)
> 
> Nit: Wonder if we should update the comment for the function to reflect
> how this works with reflog only entries.

Yeah, let's.

> >  {
> > -	return (update->flags & REF_HAVE_OLD) &&
> > +	return (update->flags & (REF_HAVE_OLD | REF_LOG_ONLY)) == REF_HAVE_OLD &&
> >  		(!is_null_oid(&update->old_oid) || update->old_target);
> >  }
> >
> 
> Okay this is check now says, if this is a reflog only entry, we don't
> expect the reference to exist.
> 
> Nit: I wonder if can make this a bit more readable, perhaps:
> 
>   int ref_update_expects_existing_old_ref(struct ref_update *update)
>   {
>       /* reflog only entries may not match on-disk status of a reference */
>       if (update->flags & REF_LOG_ONLY)
>           return 0;
> 
>       return (update->flags & REF_HAVE_OLD &&
>           (!is_null_oid(&update->old_oid) || update->old_target);
>   }
> 
> I'm okay with the current version too.

I think yours reads more straight-forward though.

Patrick