Re: [PATCH 3/5] receive-pack.c: use a single ref_transaction for atomic pushes

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

Stefan Beller <sbeller@google.com> writes:

> @@ -832,34 +834,56 @@ static const char *update(struct command *cmd, struct shallow_info *si)
>  				cmd->did_not_exist = 1;
>  			}
>  		}
> -		if (delete_ref(namespaced_name, old_sha1, 0)) {
> -			rp_error("failed to delete %s", name);
> -			return "failed to delete";
> +		if (!use_atomic_push) {
> +			if (delete_ref(namespaced_name, old_sha1, 0)) {
> +				rp_error("failed to delete %s", name);
> +				return "failed to delete";
> +			}
> +		} else {
> +			if (ref_transaction_delete(transaction,
> +						   namespaced_name,
> +						   old_sha1,
> +						   0, old_sha1 != NULL,
> +						   "push", &err)) {
> +				rp_error("%s", err.buf);
> +				strbuf_release(&err);
> +				return "failed to delete";
> +			}

Doesn't the asymmetry between the above (if transaction is there,
use it, otherwise call delete_ref() which conceptually has its sole
operation inside a single transaction by itself) and below (if
transaction is not there, create it and do its thing, and close the
transaction if we created it) bother you?

The above look much simpler, and if it does not switch on
use_atomic_push but on the presense of transaction, it would have
been even better, i.e.

	if (transaction
            ? ref_transaction_delete(transaction, ...)
            : delete_ref(...)) {
		error(...);
                return "failed to delete";
	}

I think it makes the code harder to read and maintain if you forced
a caller of the ref API that happen to touch only a single ref to
make three calls to

	ref_transaction_begin();
        ref_transaction_do_one_thing();
        ref_transaction_commit();

instead of making a single call to a simple wrapper

	ref_do_one_thing();

I think that I saw Michael make a similar observation in a near-by
thread.

Even if you insist using transactions explicitly in the user of the
ref API, I think a better code organization is possible in this
particular codepath.  Because execute_commands() has the loop over
all the proposed updates, why should the update() even need to know
how to open a new transaction and when?  In other words, can't the
code be more like this?

	static update(transaction, ...)
        {
		/* do my thing in the transaction given to me */
                compute what kind of update is needed;
                switch (kind) {
		case delete:
			ref_transaction_delete(transaction, ...);
			break;
		case update:
			ref_transaction_update(transaction, ...);
			break;
		...
		}
	}

	execute_commands(...)
        {
		if (atomic)
                	transaction = ref_transaction_begin(...);
        	for (cmd = commands; cmd; cmd = cmd->next) {
			if (!atomic)        	
                        	transaction = ref_transaction_begin(...);
			update(transaction, ...);
			if (!atomic)
                        	ref_transaction_commit(transaction);
		}
		if (atomic)
                       	ref_transaction_commit(transaction);
	}

That is, update() assumes it is always in _some_ transaction, and
execute_commands(), which is what drives multi-ref updates, knows
if it wants its repeated calls to update() to be in a single
transaction or separate transactions.