Re: Bug in 2.48 with `git refs migrate`

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2025, 17:07 UTC
Message-ID: <xmqqa5bsypht.fsf@gitster.g>
In-Reply-To: <CAOLa=ZTL9n_DPhNr49XAd6bT838kc09oVx_AH7Pb4o8VK_xQ9w@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 5 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 13 quoted lines

> 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.