Re: [PATCH] remote-curl: pass on atomic capability to remote side

From: Jeff King <peff@peff.net>
Date: Oct 15, 2019, 16:40 UTC
Message-ID: <20191015164028.GA4710@sigill.intra.peff.net>
In-Reply-To: <20191015010759.2259-1-sandals@crustytoothpaste.net>

On Tue, Oct 15, 2019 at 01:07:59AM +0000, brian m. carlson wrote:

Show 6 quoted lines

> Fix this by passing the option from the transport code through to remote
> helpers, and from the HTTP remote helper down to send-pack.  With this
> change, we can detect if the server side rejects the push and report
> back appropriately.  Note the difference in the messages: the remote
> side reports "atomic transaction failed", while our own checking rejects
> pushes with the message "atomic push failed".

Good find. The patch looks good to me, except for one minor style nit in the documentation (see below).

> Document the atomic option in the remote helper documentation, so other
> implementers can implement it if they like.

I wondered what would happen for existing helpers that do not implement the option, but the behavior here:

> +	if (flags & TRANSPORT_PUSH_ATOMIC)
> +		if (set_helper_option(transport, TRANS_OPT_ATOMIC, "true") != 0)
> +			die(_("helper %s does not support --atomic"), name);
> +

looks like the right thing.

> As I mentioned in the commit message, to my knowledge, this
> functionality has been broken since the atomic capability was introduced
> circa 2.4.0.

Yeah, I tried this with v2.4.0 and it had the same problem (plus it's very clear from your patch that it's not a regression, but just that nobody bothered to implement it in the first place).

Show 13 quoted lines

> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
> index a5c3c04371..670d72c174 100644
> --- a/Documentation/gitremote-helpers.txt
> +++ b/Documentation/gitremote-helpers.txt
> @@ -509,6 +509,11 @@ set by Git if the remote helper has the 'option' capability.
>  	Indicate that only the objects wanted need to be fetched, not
>  	their dependents.
>  
> +'option atomic' {'true'|'false'}::
> +  When pushing, request the remote server to update refs in a single atomic
> +  transaction.  If successful, all refs will be updated, or none will.  If the
> +  remote side does not support this capability, the push will fail.
> +

This is implemented with a single space, but the rest of the option bodies are indented with a tab. Asciidoc seems to format it identically either way, though.

-Peff