From: Jeff King <peff@peff.net>
To: Toon Claes <toon@iotcl.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 4/4] bundle-uri: enable git-remote-https progress
Date: Thu, 9 May 2024 12:46:46 -0400 [thread overview]
Message-ID: <20240509164646.GB1708095@coredump.intra.peff.net> (raw)
In-Reply-To: <20240508124453.600871-5-toon@iotcl.com>
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
next prev parent reply other threads:[~2024-05-09 16:46 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 [this message]
2025-02-14 11:26 ` Toon Claes
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=20240509164646.GB1708095@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=toon@iotcl.com \
/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).