Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> 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? Yup that is correct, in the files backend, we do this via the 'split_symref_update()' function and in the reftable backend it is directly handled in the 'reftable_be_transaction_prepare()' function. I don't have a reason for why I didn't undertake that too in this series. Mostly I think I didn't observe it. But it something that can/should be done in the future. > > And if that happens at the generic layer, should .refname member > even be visible to backends? > It shouldn't be necessary anymore with that change. I think this is good step in that direction. >> - 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.
Attachment:
signature.asc
Description: PGP signature
