Re: Bug in 2.48 with `git refs migrate`

[Junio C Hamano <gitster@pobox.com>]

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.