Re: [PATCH v3 2/8] refs: move duplicate refname update check to generic layer

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2025, 21:56 UTC
Message-ID: <xmqq5xknkup2.fsf@gitster.g>
In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-2-0c64e3052354@gmail.com>

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

Show 11 quoted lines

> Move the tracking of refnames in `affected_refnames` from individual
> backends into the generic layer in 'refs.c'. This centralizes the
> duplicate refname detection that was previously handled separately by
> each backend.
>
> Make some changes to accommodate this move:
>
>   - Add a `string_list` field `refnames` to `ref_transaction` to contain
>     all the references in a transaction. This field is updated whenever
>     a new update is added via `ref_transaction_add_update`, so manual
>     additions in reference backends are dropped.

The transaction object is the most logical place to keep track of what is involved in the transaction. Nice.

>   - Modify the backends to use this field internally as needed. The
>     backends need to check if an update for refname already exists when
>     splitting symrefs or adding an update for 'HEAD'.

The above reads to me as if you are saying that the files backend needs to notice that it is updating "HEAD", notice that it is a symbolic ref that points at "refs/heads/main", notice that "HEAD" and "refs/heads/main" are the two things involved in the transaction, and must check if an update is already queued.

But when an update changes a symbolic ref in the sense that the underlying ref gets updated through it, the need to update both the underlying ref and the symbolic ref is common across backends, isn't it? IOW, shouldn't "splitting symrefs" (which I take to mean "ah, we are updating HEAD so we need to update it and at the same time update the underlying refs/heads/main, two updates in total") be done also at the generic layer?

And if that happens at the generic layer, should .refname member even be visible to backends?

Show 6 quoted lines

>   - In the reftable backend, within `reftable_be_transaction_prepare()`,
>     move the `string_list_has_string()` check above
>     `ref_transaction_add_update()`. Since `ref_transaction_add_update()`
>     automatically adds the refname to `transaction->refnames`,
>     performing the check after will always return true, so we perform
>     the check before adding the update.

This change makes perfect tense. It is the most natural to check and modify at the transaction layer the .refnames member, as it belongs at the transaction layer after all.

> This helps reduce duplication of functionality between the backends and
> makes it easier to make changes in a more centralized manner.

Nice.