Re: [PATCH v2 7/8] refs: allow multiple reflog entries for the same refname

[Patrick Steinhardt <ps@pks.im>]

On Fri, Dec 13, 2024 at 11:36:52AM +0100, Karthik Nayak wrote:
> The reference transaction only allows a single update for a given
> reference to avoid conflicts. This, however, isn't an issue for reflogs.
> There are no conflicts to be resolved in reflogs and when migrating
> reflogs between backends we'd have multiple reflog entries for the same
> refname.
> 
> So allow multiple reflog updates within a single transaction. Also the
> reflog creation logic isn't exposed to the end user. While this might
> change in the future, currently, this reduces the scope of issues to
> think about.
> 
> In the reftable backend, the writer sorts all updates based on the
> update_index before writing to the block. When there are multiple
> reflogs for a given refname, it is essential that the order of the
> reflogs is maintained. So add the `index` value to the `update_index`.
> The `index` field is only be set when multiple reflog entries for a

s/only be/only

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index b2e3ba877de9e59fea5a4d066eb13e60ef22a32b..bec5962debea7b62572d08f6fa8fd38ab4cd8af6 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1405,7 +1407,19 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
>  				}
>  
>  				fill_reftable_log_record(log, &c);
> -				log->update_index = ts;
> +
> +				/*
> +				 * Updates are sorted by the writer. So updates for the same
> +				 * refname need to contain different update indices.
> +				 */
> +				log->update_index = ts + u->index;

Okay. So instead of tracking things via a map, we now rely on the caller
to provide the update index. And if they don't provide one then we
cannot guarantee ordering.

I guess that's a good solution. After all, there will only be a very
limited amount of callers in the first place, so I think it's fine to
shift the responsibility onto them to maintain reflog ordering. They're
also the only ones who really know about the actual ordering.

> +				/*
> +				 * Note the max update_index so the limit can be set later on.
> +				 */
> +				if (log->update_index > max_update_index)
> +					max_update_index = log->update_index;

Makes sense, as well.

Patrick