git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Maciej CiemborowiczNext: Maciej Ciemborowicz
Message 9 of 35 in “[BUG] reference-transaction hook misses destination of git branch -m”
  1. Maciej CiemborowiczSep 19, 2026
  2. Karthik NayakSep 19, 2026
  3. refs: run copy and rename through transactionsMaciej Ciemborowicz, Sep 20, 2026
  4. Junio C HamanoSep 21, 2026
  5. Junio C HamanoSep 21, 2026
  6. Maciej CiemborowiczSep 22, 2026
  7. refs: run copy and rename through transactionsMaciej Ciemborowicz, Sep 23, 2026
  8. Maciej CiemborowiczSep 30, 2026
  9. Patrick SteinhardtOct 2, 2026
  10. Maciej CiemborowiczOct 2, 2026
  11. Patrick SteinhardtOct 5, 2026
  12. 0/4 refs: run copy and rename through transactionsMaciej Ciemborowicz, Oct 7, 2026
  13. 1/4 refs: distinguish internal transactions from logical updatesMaciej Ciemborowicz, Oct 7, 2026
  14. 2/4 refs: support replacing reflogs in a transactionMaciej Ciemborowicz, Oct 7, 2026
  15. 3/4 refs: run copy and rename through ordinary transactionsMaciej Ciemborowicz, Oct 7, 2026
  16. 4/4 refs: remove backend-specific copy and rename callbacksMaciej Ciemborowicz, Oct 7, 2026
  17. Junio C HamanoOct 7, 2026
  18. 0/4 refs: run copy and rename through transactionsMaciej Ciemborowicz, Oct 8, 2026
  19. 1/4 refs: distinguish internal transactions from logical updatesMaciej Ciemborowicz, Oct 8, 2026
  20. 2/4 refs: support replacing reflogs in a transactionMaciej Ciemborowicz, Oct 8, 2026
  21. 3/4 refs: run copy and rename through ordinary transactionsMaciej Ciemborowicz, Oct 8, 2026
  22. 4/4 refs: remove backend-specific copy and rename callbacksMaciej Ciemborowicz, Oct 8, 2026
  23. Patrick SteinhardtOct 8, 2026
  24. Maciej CiemborowiczOct 8, 2026
  25. Maciej CiemborowiczOct 8, 2026
  26. Junio C HamanoOct 8, 2026
  27. Maciej CiemborowiczOct 8, 2026
  28. Kristoffer HaugsbakkOct 8, 2026
  29. Maciej CiemborowiczOct 8, 2026
  30. Patrick SteinhardtOct 9, 2026
  31. brian m. carlsonOct 10, 2026
  32. Junio C HamanoOct 8, 2026
  33. Karthik NayakOct 9, 2026
  34. Maciej CiemborowiczOct 10, 2026
  35. Maciej CiemborowiczSep 23, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.