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

> 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