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

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

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 21, 2026, 17:54 UTC
Message-ID
<xmqqjyoemqvu.fsf@gitster.g>
In-Reply-To
<20260920165037.88524-1-maciej.ciemborowicz@gmail.com>
Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:
Show 24 quoted lines
> Reference copy and rename operations currently 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. 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.
>
> 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.
>
> Add coverage for rename, copy, forced updates, both directions of D/F
> conflicts, concurrent updates and prepared-hook rollback.
>
> Helped-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
> ---

Drop unnecessary "currently" to the first sentence, and add "test" to the laste sentence somewhere, and this would be perfect.

Very pleasing to see an exceptionally well-written proposed commit log message by a new contributor.

Show 110 quoted lines
>  refs.c                           | 137 ++++++++++++---
>  refs.h                           |   3 +
>  refs/debug.c                     |  25 ---
>  refs/files-backend.c             | 276 +++++++++++++++++++++++++++----
>  refs/packed-backend.c            |   2 -
>  refs/refs-internal.h             |  38 +++--
>  refs/reftable-backend.c          | 194 ++++++++++++++++------
>  t/t1416-ref-transaction-hooks.sh | 142 ++++++++++++++++
>  8 files changed, 679 insertions(+), 138 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 92d5df5b7..22c000f7f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1004,15 +1004,17 @@ long get_files_ref_lock_timeout_ms(struct repository *repo)
>  	return timeout_ms;
>  }
>  
> -int refs_delete_ref(struct ref_store *refs, const char *msg,
> -		    const char *refname,
> -		    const struct object_id *old_oid,
> -		    unsigned int flags)
> +int refs_delete_ref_with_transaction_flags(struct ref_store *refs,
> +					   const char *msg,
> +					   const char *refname,
> +					   const struct object_id *old_oid,
> +					   unsigned int flags,
> +					   unsigned int transaction_flags)
>  {
>  	struct ref_transaction *transaction;
>  	struct strbuf err = STRBUF_INIT;
>  
> -	transaction = ref_store_transaction_begin(refs, 0, &err);
> +	transaction = ref_store_transaction_begin(refs, transaction_flags, &err);
>  	if (!transaction ||
>  	    ref_transaction_delete(transaction, refname, old_oid,
>  				   NULL, flags, msg, &err) ||
> @@ -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;
> @@ -1270,6 +1281,10 @@ void ref_transaction_free(struct ref_transaction *transaction)
>  
>  	string_list_clear(&transaction->refnames, 0);
>  	free(transaction->updates);
> +	free(transaction->old_refname);
> +	free(transaction->new_refname);
> +	free(transaction->logmsg);
> +	free(transaction->destination_target);
>  	free(transaction);
>  }
>  
> @@ -2710,7 +2725,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");
> @@ -2720,7 +2736,8 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
>  	if (ret)
>  		return ret;
>  
> -	ret = run_transaction_hook(transaction, "prepared");
> +	ret = transaction->flags & REF_TRANSACTION_FLAG_SKIP_HOOK ? 0 :
> +		run_transaction_hook(transaction, "prepared");
>  	if (ret) {
>  		ref_transaction_abort(transaction, err);
>  		die(_(abort_by_ref_transaction_hook), "prepared");
> @@ -2750,7 +2767,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,
>  		break;
>  	}
>  
> -	run_transaction_hook(transaction, "aborted");
> +	if (!(transaction->flags & REF_TRANSACTION_FLAG_SKIP_HOOK))
> +		run_transaction_hook(transaction, "aborted");
>  
>  	ref_transaction_free(transaction);
>  	return ret;
> @@ -2781,7 +2799,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  	}
>  
>  	ret = refs->be->transaction_finish(refs, transaction, err);
> -	if (!ret && !(transaction->flags & REF_TRANSACTION_FLAG_INITIAL))
> +	if (!ret && !(transaction->flags & (REF_TRANSACTION_FLAG_INITIAL |
> +					 REF_TRANSACTION_FLAG_SKIP_HOOK)))
>  		run_transaction_hook(transaction, "committed");
>  	return ret;
>  }
> @@ -3123,28 +3142,100 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,
>  	return ret;
>  }
>  
> -int refs_rename_ref(struct ref_store *refs, const char *oldref,
> -		    const char *newref, const char *logmsg)

It is annoying that we have to give random callers an unrestricted way to skip calling hooks. I suspect it may come from "this function should call hook when invoked as the top-level operation, but when it is used as a subroutine for a different top-level operation, we want to skip hooks" kind of reasoning, but is this something we can avoid by rearranging the call chain?

> +static int refs_copy_or_rename_ref(struct ref_store *refs, const char *oldref,
> +				   const char *newref, const char *logmsg,
> +				   int copy)

Will this function ever gain a third mode of operation other than copy or rename? If not, perhaps "bool copy"?

Show 85 quoted lines
>  {
> -	char *msg;
> -	int retval;
> +	struct ref_transaction *transaction = NULL;
> +	struct object_id old_oid, new_oid;
> +	struct strbuf new_target = STRBUF_INIT;
> +	struct strbuf err = STRBUF_INIT;
> +	char *msg = normalize_reflog_message(logmsg);
> +	int old_flags, new_flags = 0, new_exists = 0, ret = 1;
>  
> -	msg = normalize_reflog_message(logmsg);
> -	retval = refs->be->rename_ref(refs, oldref, newref, msg);
> +	if (!strcmp(oldref, newref)) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	if (!refs_resolve_ref_unsafe(refs, oldref,
> +				     RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> +				     &old_oid, &old_flags)) {
> +		error("refname %s not found", oldref);
> +		goto out;
> +	}
> +	if (old_flags & REF_ISSYMREF) {
> +		error("refname %s is a symbolic ref, %s it is not supported",
> +		      oldref, copy ? "copying" : "renaming");
> +		goto out;
> +	}
> +
> +	transaction = ref_store_transaction_begin(refs, 0, &err);
> +	if (!transaction)
> +		goto error;
> +	transaction->type = copy ? REF_TRANSACTION_TYPE_COPY :
> +		REF_TRANSACTION_TYPE_RENAME;
> +	transaction->old_refname = xstrdup(oldref);
> +	transaction->new_refname = xstrdup(newref);
> +	transaction->logmsg = xstrdup(msg);
> +	oidcpy(&transaction->source_oid, &old_oid);
> +
> +	if (!copy && ref_transaction_delete(transaction, oldref, &old_oid, NULL,
> +					    REF_NO_DEREF, msg, &err))
> +		goto error;
> +
> +	if (refs_resolve_ref_unsafe(refs, newref,
> +				    RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> +				    &new_oid, &new_flags)) {
> +		new_exists = 1;
> +		if ((new_flags & REF_ISSYMREF) &&
> +		    refs_read_symbolic_ref(refs, newref, &new_target) < 0) {
> +			strbuf_addf(&err, "unable to read symbolic ref %s", newref);
> +			goto error;
> +		}
> +	} else {
> +		oidclr(&new_oid, refs->repo->hash_algo);
> +	}
> +	transaction->destination_exists = new_exists;
> +	if (new_flags & REF_ISSYMREF)
> +		transaction->destination_target = xstrdup(new_target.buf);
> +	else if (transaction->destination_exists)
> +		oidcpy(&transaction->destination_oid, &new_oid);
> +
> +	if (ref_transaction_update(transaction, newref, &old_oid,
> +				   (new_flags & REF_ISSYMREF) ? NULL : &new_oid,
> +				   NULL,
> +				   (new_flags & REF_ISSYMREF) ? new_target.buf : NULL,
> +				   REF_NO_DEREF | REF_SKIP_CREATE_REFLOG,
> +				   NULL, &err))
> +		goto error;
> +
> +	if (ref_transaction_commit(transaction, &err))
> +		goto error;
> +
> +	ret = 0;
> +	goto out;
> +
> +error:
> +	error("%s", err.buf);
> +out:
> +	ref_transaction_free(transaction);
> +	strbuf_release(&new_target);
> +	strbuf_release(&err);
>  	free(msg);
> -	return retval;
> +	return ret;
>  }

That's quite a lot of new code. I see ref_transaction_delete(), ref_transaction_update() and others are already reused from existing code paths, which is good.

Show 9 quoted lines
> +struct files_copy_or_rename_transaction_data {
> +	struct ref_lock *lock;
> +	struct object_id orig_oid;
> +	struct object_id destination_oid;
> +	char *destination_target;
> +	int logmoved;
> +	int destination_exists;
> +	int destination_log_backed_up;
> +};

Good to have a type that can be used to hold pieces of information specific to the operation. Can't we do without rename/copy specific addition to the generic ref_transaction struct by following the same principle?

The comment above the members does make it understandable, but ...
Show 20 quoted lines
> @@ -240,6 +253,21 @@ struct ref_transaction {
>  	void *backend_data;
>  	unsigned int flags;
>  	uint64_t max_index;
> +
> +	/*
> +	 * Rename and copy operations need backend-specific reflog handling.
> +	 * Their logical updates still live in `updates`, so hooks see the
> +	 * operation like any other reference transaction. The fields below
> +	 * retain the state that backends verify after taking their locks.
> +	 */
> +	enum ref_transaction_type type;
> +	char *old_refname;
> +	char *new_refname;
> +	char *logmsg;
> +	struct object_id source_oid;
> +	struct object_id destination_oid;
> +	char *destination_target;
> +	unsigned int destination_exists:1;
>  };

... is it the best we can do to contaminate a rather generic data structure for such a details relevant only to one specific operation?

Thanks.
Previous: Maciej CiemborowiczNext: Junio C Hamano
Message 4 of 28 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. Junio C HamanoOct 8, 2026
  28. 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.