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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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?

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