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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> 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).

> @@ -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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux