Re: [PATCHv3 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:

> @@ -1086,8 +1100,25 @@ 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";
> +				return;
> +			}
> +		}
>  		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";
> +				return;
> +			}

Hmm, should the code even attempt to commit if update() returned a
non NULL, signaling a failure?

Or would we want to do this instead?

	if (cmd->error_string)
        	goto transaction_abort;
	else if (!use_atomic) {
		if (ref_transaction_commit(...)) {
			...
                        cmd->error_string = "...";
                        return;
        	}
	}

and then ...

>  		if (shallow_update && !cmd->error_string &&
>  		    si->shallow_ref[cmd->index]) {
>  			error("BUG: connectivity check has not been run on ref %s",
> @@ -1096,10 +1127,32 @@ static void execute_commands(struct command *commands,
>  		}
>  	}
>  
> +	if (use_atomic) {
> +		/*
> +		 * update(...) may abort early (i.e. because the hook refused to
> +		 * update that ref) which then doesn't even record a transaction
> +		 * regarding that ref. Make sure all commands are without error
> +		 * and then commit atomically.
> +		 */
> +		for (cmd = commands; cmd; cmd = cmd->next)
> +			if (cmd->error_string)
> +				break;
> +		if (cmd) {
> +			for (cmd = commands; cmd; cmd = cmd->next)
> +				if (!cmd->error_string)
> +					cmd->error_string = "atomic push failure";
> +		} else if (ref_transaction_commit(transaction, &err)) {
> +			rp_error("%s", err.buf);
> +			for (cmd = commands; cmd; cmd = cmd->next)
> +				cmd->error_string = err.buf;
> +		}

... have the label to jump to here:

	transaction_abort:

> +		ref_transaction_free(transaction);

I was confused by the fact that you did not have any call to
transaction-abort, until I realized that there is no such API
function and ref_transaction_free() serves that "don't commit,
roll it back" purpose.

> +	}
>  	if (shallow_update && !checked_connectivity)
>  		error("BUG: run 'git fsck' for safety.\n"
>  		      "If there are errors, try to remove "
>  		      "the reported refs above");
> +	strbuf_release(&err);
>  }
>  
>  static struct command **queue_command(struct command **tail,