Re: [PATCH v2] refs: run copy and rename through transactions
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 2, 2026, 10:56 UTC
- Message-ID
- <ar-N7SA63fN_xx9P@pks.im>
- In-Reply-To
- <20260923133651.74120-1-maciej.ciemborowicz@gmail.com>
On Wed, Sep 23, 2026 at 03:36:51PM +0200, Maciej Ciemborowicz wrote:
Show 9 quoted lines
> 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?
Show 5 quoted lines
> 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?
Show 5 quoted lines
> 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?
Show 20 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.
Show 19 quoted lines
> 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.
Show 41 quoted lines
> @@ -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...
Show 13 quoted lines
> @@ -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