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

From: Justin Tobler <jltobler@gmail.com>
Date: Jul 23, 2025, 20:31 UTC
Message-ID: <j52ugdtik25i6dqgqafchvy5an3o7qbdfeavtqg6bcr2ouxvyv@qwtqzf7xk4hi>
In-Reply-To: <20250722-pks-reflog-append-v1-7-183e5949de16@pks.im>

On 25/07/22 01:20PM, Patrick Steinhardt wrote:

> The `REF_HAVE_OLD` flag indicates whether a given ref update has its old
> object ID set. If so, the value of that field is used to verify whteher

s/whteher/whether/

Show 22 quoted lines

> the current state of the reference matches this expected state. It is
> thus an important part of mitigating races with a concurrent process
> that updates the same set of references.
> 
> When writing reflogs though we explicitly unset that flag. This is a
> sensible thing to do: the old state of reflog entry updates may not
> necessarily match the current on-disk state of its accompanying ref, but
> it's only intended to signal what old object ID we want to write into
> the new reflog entry. For example when migrating refs we end up writing
> many reflog entries for a single reference, and most likely those reflog
> entries will have many different old object IDs.
> 
> But unsetting this flag also removes a useful signal, namely that the
> caller _did_ provide an old object ID for a given reflog entry. This
> signal is useful to determine whether we have to resolve the refname
> manually to figure out the current state, or whether we should just go
> with what the caller has provided.
> 
> This actually causes real issues when migrating reflogs, as we don't
> know to actually use the caller-provided old object ID when writing
> those entries. Instead, reflog entries simply end up with the all-zero
> object ID.

Ok, if I'm understanding this correctly, the `REF_HAVE_OLD` flag is also required to actually record a provided old OID in the reflog entry. If it is not set, a NUL OID is recorded instead.

> Stop unsetting the flag so that we can use it as this described signal,
> which we'll do in a subsequent commit. Skip checking the old object ID
> for log-only updates so that we don't expect it to match the current
> on-disk state.

Just to clarify, when migrating reflogs, are these operations always marked with `REF_LOG_ONLY`? The comment for that flag states:

  Used as a flag in ref_update::flags when we want to log a ref
  update but not actually perform it.  This is used when a symbolic ref
  update is split up.

I might be misunderstanding this though.

-Justin