From: Toon Claes <toon@iotcl.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 4/4] bundle-uri: enable git-remote-https progress
Date: Fri, 14 Feb 2025 12:26:03 +0100 [thread overview]
Message-ID: <87o6z43gz8.fsf@iotcl.com> (raw)
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:
> 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
next prev parent reply other threads:[~2025-02-14 11:26 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 12:44 [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs Toon Claes
2024-05-08 12:44 ` [PATCH 1/4] progress: add function to set total Toon Claes
2024-05-08 12:44 ` [PATCH 2/4] http: add the ability to log progress Toon Claes
2024-05-08 16:52 ` Eric Sunshine
2024-05-09 16:34 ` Jeff King
2024-05-09 16:51 ` Junio C Hamano
2024-05-09 17:04 ` Jeff King
2024-05-09 16:52 ` Jeff King
2024-05-08 12:44 ` [PATCH 3/4] remote-curl: optionally show progress for HTTP get Toon Claes
2024-05-08 22:29 ` Junio C Hamano
2024-05-08 12:44 ` [PATCH 4/4] bundle-uri: enable git-remote-https progress Toon Claes
2024-05-09 16:46 ` Jeff King
2025-02-14 11:26 ` Toon Claes [this message]
2025-02-21 7:36 ` Jeff King
2024-05-08 23:49 ` [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs Junio C Hamano
2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes
2025-02-19 14:30 ` [PATCH v2 1/7] progress: add function to set total Toon Claes
2025-02-21 7:43 ` Jeff King
2025-02-19 14:30 ` [PATCH v2 2/7] progress: allow pure-throughput progress meters Toon Claes
2025-02-19 14:30 ` [PATCH v2 3/7] http: turn off curl signals Toon Claes
2025-02-19 14:30 ` [PATCH v2 4/7] http: add the ability to log progress Toon Claes
2025-02-19 14:30 ` [PATCH v2 5/7] remote-curl: optionally show progress for HTTP get Toon Claes
2025-02-19 14:30 ` [PATCH v2 6/7] bundle-uri: enable git-remote-https progress Toon Claes
2025-02-19 14:30 ` [PATCH v2 7/7] http: silence stderr when progress is enabled Toon Claes
2025-02-21 7:48 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o6z43gz8.fsf@iotcl.com \
--to=toon@iotcl.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).