On Thu, Jul 24, 2025 at 03:21:30AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
