Subject: Re: [PATCH v4 1/5] send-pack: fix inconsistent porcelain output

From: Junio C Hamano <[email-masked]>
Date: Fri, 17 Apr 2020 12:51:26 -0700

Jiang Xin <[email-masked]> writes:

> diff --git a/transport.c b/transport.c
> index 1fdc7dac1a..13d638d5fe 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -715,7 +715,15 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
>  
>  	close(data->fd[1]);
>  	close(data->fd[0]);
> -	ret |= finish_connect(data->conn);
> +	/*
> +	 * Atomic push may abort the connection early and close the pipe,
> +	 * which may cause an error for `finish_connect()`. Ignore this error
> +	 * for atomic git-push.
> +	 */
> +	if (ret || args.atomic)
> +		finish_connect(data->conn);
> +	else
> +		ret = finish_connect(data->conn);

Which means that the return value from this function under the
atomic mode is what we got from an earlier call to send_pack().

Which may have failed, in which case we do want to relay the error
to our caller, or may have succeeded, which is also fine to relay to
our caller.

> The following code at the end of function `send_pack()` indicates that
> `send_pack()` should not return an error if some references are rejected
> in porcelain mode.

Under porcelain mode, if we already saw an error before we get to
the "if porcelain, do not bother checking the remote status", we
do relay the error to our caller.  That happens in the part you
omitted from your quote:

>     int send_pack(...)
>         ... ...

	if (ret < 0)
		return ret;

>
>         if (args->porcelain)
>             return 0;
>
>         for (ref = remote_refs; ref; ref = ref->next) {
>             switch (ref->status) {
>             case REF_STATUS_NONE:
>             case REF_STATUS_UPTODATE:
>             case REF_STATUS_OK:
>                 break;
>             default:
>                 return -1;
>             }
>         }
>         return 0;
>     }

So it is not like we are disabling all errors under the porcelain
mode.

> diff --git a/send-pack.c b/send-pack.c
> index 0407841ae8..1835cd5582 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -489,7 +487,8 @@ int send_pack(struct send_pack_args *args,
>  			if (use_atomic) {
>  				strbuf_release(&req_buf);
>  				strbuf_release(&cap_buf);
> -				return atomic_push_failure(args, remote_refs, ref);
> +				atomic_push_failure(args, remote_refs, ref);
> +				return args->porcelain ? 0 : -1;
>  			}

And this is in line with the way how rejected pushes are handled, I guess.

Sounds a bit convoluted but correct ;-)