Re: [PATCH v2] refs: run copy and rename through transactions

[Patrick Steinhardt <ps@pks.im>]

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