Re: [PATCH] remote-curl: Fix push status report when all branches fail

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 19, 2012, 22:57 UTC
Message-ID: <7vzkdjgv1i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1327011899-18883-1-git-send-email-spearce@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 8 quoted lines

> Always print a blank line after the send-pack process terminates,
> ensuring the helper status report (if it was output) will be
> correctly parsed by the calling transport-helper.c. This ensures
> the helper doesn't abort before the status report can be shown to
> the user.
>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---

Anybody wants to add a simple test for this failure mode?

Show 29 quoted lines

>  remote-curl.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 48c20b8..d6054e2 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -805,7 +805,7 @@ static int push(int nr_spec, char **specs)
>  static void parse_push(struct strbuf *buf)
>  {
>  	char **specs = NULL;
> -	int alloc_spec = 0, nr_spec = 0, i;
> +	int alloc_spec = 0, nr_spec = 0, i, ret;
>  
>  	do {
>  		if (!prefixcmp(buf->buf, "push ")) {
> @@ -822,12 +822,11 @@ static void parse_push(struct strbuf *buf)
>  			break;
>  	} while (1);
>  
> -	if (push(nr_spec, specs))
> +	ret = push(nr_spec, specs);
> +	xwrite(1, "\n", 1);
> +	if (ret)
>  		exit(128); /* error already reported */
>  
> -	printf("\n");
> -	fflush(stdout);
> -

This is not a fault of this patch, but could we fix this ugly mixture of xwrite() and printf() in the same program? I can see that the loop in the main() function carefully tries to call fflush(stdout) to make sure that nothing is pending after processing a single command so using xwrite() may not cause any harm here, but the thing is that you do not check the error return from this xwrite(), so use of it is not giving us any potential benefit of being able to detect I/O errors in a finer grained manner, i.e. it is no better than the printf("\n"); fflush(stdout); sequence it replaces.

Thanks.