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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 15, 2014, 21:37 UTC
Message-ID: <xmqqiohcd272.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1418673368-20785-4-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 22 quoted lines

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