From: Patrick Steinhardt Date: Fri, 02 Oct 2026 10:56:45 GMT Subject: Re: [PATCH v2] refs: run copy and rename through transactions Message-ID: In-Reply-To: <20260923133651.74120-1-maciej.ciemborowicz@gmail.com> On Wed, Sep 23, 2026 at 03:36:51PM +0200, Maciej Ciemborowicz wrote: > Reference copy and rename operations bypass the transaction API. > Consequently, the reference-transaction hook sees only the source deletion > with the files backend and no useful update with the reftable backend. > > Represent both operations as reference transactions containing their > logical updates. A rename is a deletion of the old reference and creation > of the new reference in the same transaction. Attach operation-specific > state to the destination update instead of making copy or rename a property > of the entire transaction. Sorry, but what does this last sentence mean? What is the consequence of it? > Retain backend-specific reflog handling: the files backend stages its > existing rename procedure across prepare, finish and abort, while reftable > stages an addition while holding the stack lock. Suppress hooks for the > files backend's nested deletion transactions so that callers observe one > logical transaction. The fact that we retain the backend-specific logic is not really interesting by itself. The way more interesting question is _why_ we retain it. Or asked differently, why can't we make this whole mechanism completely agnostic of the backend and implement this via pure transactions? > Record and verify the source and destination values after taking backend > locks. This rejects concurrent changes instead of applying a rename or copy > that differs from the payload shown to the preparing hook. Preserve D/F > renames and restore overwritten references and reflogs when a prepared hook > rejects the operation. Is this new behaviour? Is this retaining old behaviour? I have no clue. > Add tests covering rename, copy, forced updates, both directions of D/F > conflicts, concurrent updates and prepared-hook rollback. This sentence doesn't really add much value to the message. How does all of this impact performance? > diff --git a/refs.c b/refs.c > index 92d5df5b7..f036ae4b9 100644 > --- a/refs.c > +++ b/refs.c > @@ -1027,6 +1029,15 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, > return 0; > } > > +int refs_delete_ref(struct ref_store *refs, const char *msg, > + const char *refname, > + const struct object_id *old_oid, > + unsigned int flags) > +{ > + return refs_delete_ref_with_transaction_flags(refs, msg, refname, > + old_oid, flags, 0); > +} > + > static void copy_reflog_msg(struct strbuf *sb, const char *msg) > { > char c; Refactorings like these could easily go into a separate commit to make this easier to review. > @@ -2710,7 +2747,8 @@ int ref_transaction_prepare(struct ref_transaction *transaction, > return REF_TRANSACTION_ERROR_GENERIC; > > /* Preparing checks before locking references */ > - ret = run_transaction_hook(transaction, "preparing"); > + ret = transaction->flags & REF_TRANSACTION_FLAG_SKIP_HOOK ? 0 : > + run_transaction_hook(transaction, "preparing"); > if (ret) { > ref_transaction_abort(transaction, err); > die(_(abort_by_ref_transaction_hook), "preparing"); Instead of teaching every site to conditionally call `run_transaction_hook()` only when the flag is not set, can't we adapt the function itself to skip? In any case, this is another change that could easily be split out into a separate commit. > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 71628550f..c28228116 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2962,6 +3059,14 @@ static int files_transaction_prepare(struct ref_store *ref_store, > struct ref_transaction *packed_transaction = NULL; > > assert(err); > + { > + struct ref_update *operation = > + ref_transaction_copy_or_rename_update(transaction); > + > + if (operation) > + return files_copy_or_rename_ref(ref_store, operation, > + transaction); > + } > > if (transaction->flags & REF_TRANSACTION_FLAG_INITIAL) > goto cleanup; I know this is a construct that AI loves, but that's not following our coding style. > @@ -3333,6 +3442,40 @@ static int files_transaction_finish(struct ref_store *ref_store, > > > assert(err); > + { > + struct ref_update *update = > + ref_transaction_copy_or_rename_update(transaction); > + > + if (update) { > + struct ref_copy_or_rename_update *operation = > + update->copy_or_rename; > + struct files_copy_or_rename_transaction_data *data = > + transaction->backend_data; > + int special_ret; > + > + special_ret = commit_ref_update(refs, data->lock, &data->orig_oid, > + operation->logmsg, 0, err); > + if (special_ret) { > + error("unable to write current sha1 into %s: %s", > + update->refname, err->buf); > + data->lock = NULL; > + files_transaction_abort(ref_store, transaction, err); > + return special_ret; > + } else if (data->destination_log_backed_up) { > + struct strbuf path = STRBUF_INIT; > + > + files_reflog_path(refs, &path, TMP_RENAMED_LOG_DESTINATION); > + if (unlink(path.buf) < 0 && errno != ENOENT) > + warning_errno("unable to remove '%s'", path.buf); > + strbuf_release(&path); > + } > + free(data->destination_target); > + free(data); > + transaction->backend_data = NULL; > + transaction->state = REF_TRANSACTION_CLOSED; > + return special_ret; > + } > + } > > if (transaction->flags & REF_TRANSACTION_FLAG_INITIAL) > return files_transaction_finish_initial(refs, transaction, err); Yeah... > @@ -3476,11 +3619,105 @@ static int files_transaction_finish(struct ref_store *ref_store, > > static int files_transaction_abort(struct ref_store *ref_store, > struct ref_transaction *transaction, > - struct strbuf *err UNUSED) > + struct strbuf *err) > { > struct files_ref_store *refs = > files_downcast(ref_store, 0, "ref_transaction_abort"); > > + { > + struct ref_update *update = > + ref_transaction_copy_or_rename_update(transaction); ... really? Sorry, but I'm going to stop reading here. This is not in a state that is reviewable and has way too much stuff that is obviously generated by an AI without much thought being put into it by the author. I don't want to invest my time into a topic where the author has obviously not spent their time thinking about it, either. Patrick