Re: [PATCH 4/4] bundle-uri: enable git-remote-https progress

From: Jeff King <peff@peff.net>
Date: May 9, 2024, 16:46 UTC
Message-ID: <20240509164646.GB1708095@coredump.intra.peff.net>
In-Reply-To: <20240508124453.600871-5-toon@iotcl.com>

On Wed, May 08, 2024 at 02:44:53PM +0200, Toon Claes wrote:

Show 11 quoted lines

> diff --git a/bundle-uri.c b/bundle-uri.c
> index ca32050a78..462f00f668 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -293,7 +293,6 @@ static int download_https_uri_to_file(const char *file, const char *uri)
>  	int found_get = 0;
>  
>  	strvec_pushl(&cp.args, "git-remote-https", uri, NULL);
> -	cp.err = -1;
>  	cp.in = -1;
>  	cp.out = -1;

This is the cause of at least one test failure in t5558, I think. We spawn a remote-https to try to download the bundle, but it may not be present, and we continue without it. In that case, the child remote-https says something like:

  fatal: failed to download file at URL 'http://127.0.0.1:5558/bundle-5.bundle'

and then the caller says:

  warning: failed to download bundle from URI 'http://127.0.0.1:5558/bundle-5.bundle'

Prior to this patch, the "fatal" part coming from the child process was suppressed (and the test checks that this is so, which is why it fails, even though the clone itself works fine).

Obviously you need to enable stderr to see the progress, so I'm not sure how to resolve it. In an ideal world, you'd ask for the two over separate descriptors, but I think run_command() only supports 0/1/2 stdio due to portability limitations for Windows.

One option is that remote-https could ferry machine-readable output back to the parent over stdout, which could then format it for the user.

Another is that we could somehow ask remote-https to squelch non-progress errors, though that feels a bit weird (and awkward to implement, since the message comes from a die() call).

Show 8 quoted lines

> @@ -328,6 +327,9 @@ static int download_https_uri_to_file(const char *file, const char *uri)
>  		goto cleanup;
>  	}
>  
> +	fprintf(child_in, "option progress true\n");
> +	fflush(child_in);
> +
>  	fprintf(child_in, "get %s %s\n\n", uri, file);

I was curious why you'd flush here. It makes sense if you are going to then read back the response from stdout, but we never do (it should be a single "ok" line). I think it's OK to ignore it, given that we'd just continue without progress in that case.

But I did wonder why this wasn't messing up reading the response from the "get" command just below. The answer is that we don't read that response either! We just assume that the process will exit successfully or not based on the result. We are relying on the pipe buffer to avoid deadlock, but I think that's OK since the output is always going to be small.

-Peff