Re: [PATCHv2 4/6] receive-pack.c: use a single ref_transaction for atomic pushes

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

Stefan Beller <sbeller@google.com> writes:

>     	* update(...) assumes to be always in a transaction
>     	* Caring about when to begin/commit transactions is put
>     	  into execute_commands

I am obviously biased, but I find that the new code structure and
separation of responsibility between update() and execute()
functions a lot clearer than the previous one.

Thanks.

>  builtin/receive-pack.c | 64 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index e76e5d5..0803fd2 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -67,6 +67,8 @@ static const char *NONCE_SLOP = "SLOP";
>  static const char *nonce_status;
>  static long nonce_stamp_slop;
>  static unsigned long nonce_stamp_slop_limit;
> +struct strbuf err = STRBUF_INIT;
> +struct ref_transaction *transaction;
>  
>  static enum deny_action parse_deny_action(const char *var, const char *value)
>  {
> @@ -832,34 +834,32 @@ 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);
> +		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";
>  		}
>  		return NULL; /* good */
>  	}
>  	else {
> -		struct strbuf err = STRBUF_INIT;
> -		struct ref_transaction *transaction;
> -
>  		if (shallow_update && si->shallow_ref[cmd->index] &&
>  		    update_shallow_ref(cmd, si))
>  			return "shallow error";
>  
> -		transaction = ref_transaction_begin(&err);
> -		if (!transaction ||
> -		    ref_transaction_update(transaction, namespaced_name,
> -					   new_sha1, old_sha1, 0, 1, "push",
> -					   &err) ||
> -		    ref_transaction_commit(transaction, &err)) {
> -			ref_transaction_free(transaction);
> -
> +		if (ref_transaction_update(transaction,
> +					   namespaced_name,
> +					   new_sha1, old_sha1,
> +					   0, 1, "push",
> +					   &err)) {
>  			rp_error("%s", err.buf);
>  			strbuf_release(&err);
>  			return "failed to update ref";
>  		}
>  
> -		ref_transaction_free(transaction);
>  		strbuf_release(&err);
>  		return NULL; /* good */
>  	}
> @@ -1059,6 +1059,16 @@ static void execute_commands(struct command *commands,
>  		return;
>  	}
>  
> +	if (use_atomic) {
> +		transaction = ref_transaction_begin(&err);
> +		if (!transaction) {
> +			error("%s", err.buf);
> +			strbuf_release(&err);
> +			for (cmd = commands; cmd; cmd = cmd->next)
> +				cmd->error_string = "transaction error";
> +			return;
> +		}
> +	}
>  	data.cmds = commands;
>  	data.si = si;
>  	if (check_everything_connected(iterate_receive_command_list, 0, &data))
> @@ -1086,8 +1096,23 @@ static void execute_commands(struct command *commands,
>  
>  		if (cmd->skip_update)
>  			continue;
> -
> +		if (!use_atomic) {
> +			transaction = ref_transaction_begin(&err);
> +			if (!transaction) {
> +				rp_error("%s", err.buf);
> +				strbuf_release(&err);
> +				cmd->error_string = "failed to start transaction";
> +			}
> +		}
>  		cmd->error_string = update(cmd, si);
> +		if (!use_atomic)
> +			if (ref_transaction_commit(transaction, &err)) {
> +				ref_transaction_free(transaction);
> +				rp_error("%s", err.buf);
> +				strbuf_release(&err);
> +				cmd->error_string = "failed to update ref";
> +			}
> +
>  		if (shallow_update && !cmd->error_string &&
>  		    si->shallow_ref[cmd->index]) {
>  			error("BUG: connectivity check has not been run on ref %s",
> @@ -1096,6 +1121,14 @@ static void execute_commands(struct command *commands,
>  		}
>  	}
>  
> +	if (use_atomic) {
> +		if (ref_transaction_commit(transaction, &err)) {
> +			rp_error("%s", err.buf);
> +			for (cmd = commands; cmd; cmd = cmd->next)
> +				cmd->error_string = err.buf;
> +		}
> +		ref_transaction_free(transaction);
> +	}
>  	if (shallow_update && !checked_connectivity)
>  		error("BUG: run 'git fsck' for safety.\n"
>  		      "If there are errors, try to remove "
> @@ -1543,5 +1576,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  	sha1_array_clear(&shallow);
>  	sha1_array_clear(&ref);
>  	free((void *)push_cert_nonce);
> +	strbuf_release(&err);
>  	return 0;
>  }