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

From: Toon Claes <toon@iotcl.com>
Date: Feb 14, 2025, 11:26 UTC
Message-ID: <87o6z43gz8.fsf@iotcl.com>
In-Reply-To: <20240509164646.GB1708095@coredump.intra.peff.net>

Hi Peff,

I am attempting to revive this patch series, and ran into your valuable feedback below:

Jeff King <peff@peff.net> writes:

Show 40 quoted lines

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

I've been playing around with things and I haven't found a good way forward with this. We could have the parent process ingest stderr of git-remote-https and swallow messages that match `/^fatal:/`, but that feels like a hack and not foolproof.

I was thinking if we could override `die()` in the child process to have it not print anything, but because git-remote-http basically can call die() basically from anywhere in the codebase, I don't think we can ensure the silenced die() function is called.

Or what do you mean by "squelch non-progress errors"?

And yes, sending progress logging over a separate fd seems like the ideal approach, but I haven't tried it yet. I'm afraid it's not worth attempting so.

So I think that leaves us with your suggestion to "ferry machine-readable output back to the parent". If I understand correctly you mean the child process will not write progress logging to stderr but to stdout (with some kind of command prefix the parent process knows what to do with this)?

I imagine communication between parent and child will then look something like this:

-> capabilities <- stateless-connect <- fetch <- get <- option <- push <- check-connectivity <- object-format -> option progress true <- ok -> get http://example.com git.bundle <- progress 123 345 40 <- progress 234 345 50 <- progress 345 345 40

~fin~

But then we need to decide on the format the child sends back to the parent. In the above example it's something like `progress <size> <total> <throughput>`. An alternative proposal could be:

<- log Downloading via HTTP: <- log Downloading via HTTP: 200.00 KiB | 100.00 KiB/s <- log Downloading via HTTP: 300.00 KiB | 100.00 KiB/s <- log Downloading via HTTP: 400.00 KiB | 100.00 KiB/s <- log Downloading via HTTP: 400.00 KiB | 100.00 KiB/s, done.

So the child sends the progress text with a `log` prefix, which the parent simply has to send that logging to where it wants it to go.

Or am I completely misunderstanding your proposal? Do you maybe happen to have any examples of a similar solution?

-- Toon