Re: Bug in 2.48 with `git refs migrate`

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Subject: [PATCH] reftable: write correct max_update_index to header
>>
>> In 297c09eabb (refs: allow multiple reflog entries for the same refname,
>> 2024-12-16), the reftable backend learned to handle multiple reflog
>> entries within the same transaction. This was done modifying the
>
> "done" -> "done by", I think.
>

>> To fix the issue, the appropriate `max_update_index` limit must be set
>> even before the first block is written. Add a `max_index` field to the
>> transaction which holds the `max_index` within all its updates, then
>> propagate this value to the reftable backend, wherein this is used to
>> the set the `max_update_index` correctly.
>
>
>> diff --git a/refs.c b/refs.c
>> index 0f41b2fd4a..f7b6f0f897 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1345,6 +1345,13 @@ int ref_transaction_update_reflog(struct
>
> Not the focus of this topic/fix, but I notice that the only caller
> of this ref_transaction_update_reflog() function is a static
> function migrate_one_reflog_entry() in the same file.  Do we expect
> that we would add more callers?  Otherwise we should make it a file
> scope static and remove it from <refs.h> file.
>

>> ref_transaction *transaction,
>>  	update->flags &= ~REF_HAVE_OLD;
>>  	update->index = index;
>>
>> +	/*
>> +	 * Reference backends may need to know the max index to optimize
>> +	 * their writes. So we store the max_index on the transaction level.
>> +	 */
>> +	if (index > transaction->max_index)
>> +		transaction->max_index = index;
>> +
>>  	return 0;
>>  }
>
> So from the problem description, whenever we consume an index by
> assigning it to an update that belongs to a transaction, we must
> make sure that transaction's max_index covers that value of the
> index?  I was wondering if we should have a less error prone way to
> do that by having a helper function that takes ref_update and
> ref_transaction objects to do the above, but this is exclusively
> used by reflog migration code path and nowhere else, so it may
> probably be fine as-is.  I dunno.