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

Maciej Ciemborowicz [off-list ref] 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 [off-list ref]
Signed-off-by: Maciej Ciemborowicz <redacted>
---

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.

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

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