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

[Junio C Hamano <gitster@pobox.com>]

Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:

> 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.

>  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"?

>  {
> -	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.

> +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 ...

> @@ -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.