* [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs
@ 2024-05-08 12:44 Toon Claes
2024-05-08 12:44 ` [PATCH 1/4] progress: add function to set total Toon Claes
` (5 more replies)
0 siblings, 6 replies; 25+ messages in thread
From: Toon Claes @ 2024-05-08 12:44 UTC (permalink / raw)
To: git; +Cc: Toon Claes
When a user clones a repository, they see what's happening in the
messages like "Enumerating objects" and "Receiving objects". But when
a user clones a repository that uses bundle URIs they see:
Cloning into 'repo.git'
And then they have to wait until all bundles are downloaded before they
see any other message. When the bundles are large, this can take a lot
of time and the user might consider the process hangs and they kill it.
This patch series introduces progress displayed to the user while
bundles are downloaded. The full output of a clone using bundle URIs
will look something like:
Cloning into 'repo.git'...
Downloading via HTTP: 21% (351812809/1620086598), 315.34 MiB | 49.84 MiB/s
Downloading via HTTP: 77% (1247493865/1620086598), 1.15 GiB | 34.31 MiB/s
Downloading via HTTP: 100% (1620086598/1620086598), 1.50 GiB | 37.97 MiB/s, done.
remote: Enumerating objects: 1322255, done.
remote: Counting objects: 100% (611708/611708), done.
remote: Total 1322255 (delta 611708), reused 611708 (delta 611708), pack-reused 710547
Receiving objects: 100% (1322255/1322255), 539.66 MiB | 31.57 MiB/s, done.
etc...
Toon Claes (4):
progress: add function to set total
http: add the ability to log progress
remote-curl: optionally show progress for HTTP get
bundle-uri: enable git-remote-https progress
bundle-uri.c | 4 +++-
http.c | 32 ++++++++++++++++++++++++++++++++
http.h | 5 +++++
progress.c | 6 ++++++
progress.h | 1 +
remote-curl.c | 8 +++++++-
t/helper/test-progress.c | 5 +++++
t/t0500-progress-display.sh | 24 ++++++++++++++++++++++++
t/t5557-http-get.sh | 15 +++++++++++++++
9 files changed, 98 insertions(+), 2 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/4] progress: add function to set total 2024-05-08 12:44 [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs Toon Claes @ 2024-05-08 12:44 ` Toon Claes 2024-05-08 12:44 ` [PATCH 2/4] http: add the ability to log progress Toon Claes ` (4 subsequent siblings) 5 siblings, 0 replies; 25+ messages in thread From: Toon Claes @ 2024-05-08 12:44 UTC (permalink / raw) To: git; +Cc: Toon Claes We're about to add the use of progress through curl. Although, curl doesn't know the total at the start of the download, but might receive this information in the Content-Length header when the download starts. To allow users set the total size after calling start_progress(), add a function progress_set_total(). Signed-off-by: Toon Claes <toon@iotcl.com> --- progress.c | 6 ++++++ progress.h | 1 + t/helper/test-progress.c | 5 +++++ t/t0500-progress-display.sh | 24 ++++++++++++++++++++++++ 4 files changed, 36 insertions(+) diff --git a/progress.c b/progress.c index c83cb60bf1..494b26eb20 100644 --- a/progress.c +++ b/progress.c @@ -271,6 +271,12 @@ static struct progress *start_progress_delay(const char *title, uint64_t total, return progress; } +void progress_set_total(struct progress *progress, uint64_t total) +{ + if (progress) + progress->total = total; +} + static int get_default_delay(void) { static int delay_in_secs = -1; diff --git a/progress.h b/progress.h index 3a945637c8..52f75ab1bf 100644 --- a/progress.h +++ b/progress.h @@ -14,6 +14,7 @@ void progress_test_force_update(void); void display_throughput(struct progress *progress, uint64_t total); void display_progress(struct progress *progress, uint64_t n); +void progress_set_total(struct progress *progress, uint64_t total); struct progress *start_progress(const char *title, uint64_t total); struct progress *start_sparse_progress(const char *title, uint64_t total); struct progress *start_delayed_progress(const char *title, uint64_t total); diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c index 66acb6a06c..622b1f738d 100644 --- a/t/helper/test-progress.c +++ b/t/helper/test-progress.c @@ -70,6 +70,11 @@ int cmd__progress(int argc, const char **argv) if (*end != '\0') die("invalid input: '%s'\n", line.buf); display_progress(progress, item_count); + } else if (skip_prefix(line.buf, "total ", (const char **) &end)) { + uint64_t total = strtoull(end, &end, 10); + if (*end != '\0') + die("invalid input: '%s'\n", line.buf); + progress_set_total(progress, total); } else if (skip_prefix(line.buf, "throughput ", (const char **) &end)) { uint64_t byte_count, test_ms; diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh index 1eb3a8306b..82a3b834a6 100755 --- a/t/t0500-progress-display.sh +++ b/t/t0500-progress-display.sh @@ -56,6 +56,30 @@ test_expect_success 'progress display with total' ' test_cmp expect out ' +test_expect_success 'progress display modify total' ' + cat >expect <<-\EOF && + Working hard: 1<CR> + Working hard: 66% (2/3)<CR> + Working hard: 100% (3/3)<CR> + Working hard: 100% (3/3), done. + EOF + + cat >in <<-\EOF && + start 0 + update + progress 1 + update + total 3 + progress 2 + progress 3 + stop + EOF + test-tool progress <in 2>stderr && + + show_cr <stderr >out && + test_cmp expect out +' + test_expect_success 'progress display breaks long lines #1' ' sed -e "s/Z$//" >expect <<\EOF && Working hard.......2.........3.........4.........5.........6: 0% (100/100000)<CR> -- 2.44.0.651.g21306a098c ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/4] http: add the ability to log progress 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 ` Toon Claes 2024-05-08 16:52 ` Eric Sunshine ` (2 more replies) 2024-05-08 12:44 ` [PATCH 3/4] remote-curl: optionally show progress for HTTP get Toon Claes ` (3 subsequent siblings) 5 siblings, 3 replies; 25+ messages in thread From: Toon Claes @ 2024-05-08 12:44 UTC (permalink / raw) To: git; +Cc: Toon Claes Add an option `progress` to `struct http_get_options` to allow the caller to enable download progress using the progress.c API. Signed-off-by: Toon Claes <toon@iotcl.com> --- http.c | 32 ++++++++++++++++++++++++++++++++ http.h | 5 +++++ 2 files changed, 37 insertions(+) diff --git a/http.c b/http.c index 3d80bd6116..15c5d53712 100644 --- a/http.c +++ b/http.c @@ -10,6 +10,7 @@ #include "credential.h" #include "version.h" #include "pkt-line.h" +#include "progress.h" #include "gettext.h" #include "trace.h" #include "transport.h" @@ -1457,6 +1458,9 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); /* * Default following to off unless "ALWAYS" is configured; this gives @@ -2017,6 +2021,21 @@ static void http_opt_request_remainder(CURL *curl, off_t pos) #define HTTP_REQUEST_STRBUF 0 #define HTTP_REQUEST_FILE 1 +static int http_progress_callback(void *clientp, curl_off_t dltotal, + curl_off_t dlnow, curl_off_t ultotal, + curl_off_t ulnow) +{ + struct progress *progress = clientp; + + if (progress) { + progress_set_total(progress, dltotal); + display_progress(progress, dlnow); + display_throughput(progress, dlnow); + } + + return 0; +} + static int http_request(const char *url, void *result, int target, const struct http_get_options *options) @@ -2025,6 +2044,7 @@ static int http_request(const char *url, struct slot_results results; struct curl_slist *headers = http_copy_default_headers(); struct strbuf buf = STRBUF_INIT; + struct progress *progress = NULL; const char *accept_language; int ret; @@ -2061,6 +2081,13 @@ static int http_request(const char *url, if (options && options->initial_request && http_follow_config == HTTP_FOLLOW_INITIAL) curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); + if (options && options->progress) { + progress = start_progress(_("Downloading via HTTP"), 0); + + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, &http_progress_callback); + } headers = curl_slist_append(headers, buf.buf); @@ -2079,6 +2106,11 @@ static int http_request(const char *url, ret = run_one_slot(slot, &results); + if (progress) { + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); + stop_progress(&progress); + } + if (options && options->content_type) { struct strbuf raw = STRBUF_INIT; curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, &raw); diff --git a/http.h b/http.h index 3af19a8bf5..37ecddec17 100644 --- a/http.h +++ b/http.h @@ -146,6 +146,11 @@ struct http_get_options { * request has completed. */ struct string_list *extra_headers; + + /* + * If not zero, display the progress. + */ + int progress; }; /* Return values for http_get_*() */ -- 2.44.0.651.g21306a098c ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] http: add the ability to log progress 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:52 ` Jeff King 2 siblings, 0 replies; 25+ messages in thread From: Eric Sunshine @ 2024-05-08 16:52 UTC (permalink / raw) To: Toon Claes; +Cc: git On Wed, May 8, 2024 at 8:45 AM Toon Claes <toon@iotcl.com> wrote: > Add an option `progress` to `struct http_get_options` to allow the > caller to enable download progress using the progress.c API. > > Signed-off-by: Toon Claes <toon@iotcl.com> > --- > diff --git a/http.c b/http.c > @@ -2061,6 +2081,13 @@ static int http_request(const char *url, > + if (options && options->progress) { > + progress = start_progress(_("Downloading via HTTP"), 0); > + > + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, &http_progress_callback); > + } > @@ -2079,6 +2106,11 @@ static int http_request(const char *url, > + if (progress) { > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); > + stop_progress(&progress); > + } The changes thus far in the series all seem very straightforward. Nicely done. Can you explain to this reviewer why you only reset CURLOPT_XFERINFODATA here, but not CURLOPT_NOPROGRESS and CURLOPT_XFERINFOFUNCTION? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] http: add the ability to log progress 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 16:52 ` Jeff King 2 siblings, 1 reply; 25+ messages in thread From: Jeff King @ 2024-05-09 16:34 UTC (permalink / raw) To: Toon Claes; +Cc: git On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote: > @@ -1457,6 +1458,9 @@ struct active_request_slot *get_active_slot(void) > curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); > curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); > curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); > + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); > + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); These last two CURLOPTs appeared in 7.32.0, but our INSTALL doc claims to support back to 7.21.3. Before that you're supposed to use PROGRESSFUNCTION instead, which has a slightly different signature. I think you could support both, though it would also be OK to just disable this extra progress for antique curl. It might also be reasonable to just bump to 7.32.0 as our minimum. The last bump was recent via c28ee09503 (INSTALL: bump libcurl version to 7.21.3, 2024-04-02), and the version picked there was arbitrary-ish (it was something we had happened to depend on accidentally). 7.32.0 is itself almost 11 years old now. -Peff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] http: add the ability to log progress 2024-05-09 16:34 ` Jeff King @ 2024-05-09 16:51 ` Junio C Hamano 2024-05-09 17:04 ` Jeff King 0 siblings, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2024-05-09 16:51 UTC (permalink / raw) To: Jeff King; +Cc: Toon Claes, git Jeff King <peff@peff.net> writes: > On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote: > >> @@ -1457,6 +1458,9 @@ struct active_request_slot *get_active_slot(void) >> curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); >> curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); >> curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); >> + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); >> + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); >> + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); > > These last two CURLOPTs appeared in 7.32.0, but our INSTALL doc claims > to support back to 7.21.3. Before that you're supposed to use > PROGRESSFUNCTION instead, which has a slightly different signature. I > think you could support both, though it would also be OK to just disable > this extra progress for antique curl. > > It might also be reasonable to just bump to 7.32.0 as our minimum. The > last bump was recent via c28ee09503 (INSTALL: bump libcurl version to > 7.21.3, 2024-04-02), and the version picked there was arbitrary-ish (it > was something we had happened to depend on accidentally). 7.32.0 is > itself almost 11 years old now. The last bump was 7.19.5 (May 2009, 14.9 years) to 7.21.3 (Dec 2010, 13.3 years). As 10 is a nice round number, we may even be able to pick randomly a slightly newer one, say, 7.35.0 (Mar 2014, 10.0 years). It is in a sense an inferiour way to pick the minimum dependency than the choice of 7.32.0, which is backed by "we use this and that, which appeared in that version", of course. But being able to update mechanically without thinking is tempting, and as long as the horizon is sufficiently long, such an approach would not have a huge downside. Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] http: add the ability to log progress 2024-05-09 16:51 ` Junio C Hamano @ 2024-05-09 17:04 ` Jeff King 0 siblings, 0 replies; 25+ messages in thread From: Jeff King @ 2024-05-09 17:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Toon Claes, git On Thu, May 09, 2024 at 09:51:51AM -0700, Junio C Hamano wrote: > > It might also be reasonable to just bump to 7.32.0 as our minimum. The > > last bump was recent via c28ee09503 (INSTALL: bump libcurl version to > > 7.21.3, 2024-04-02), and the version picked there was arbitrary-ish (it > > was something we had happened to depend on accidentally). 7.32.0 is > > itself almost 11 years old now. > > The last bump was 7.19.5 (May 2009, 14.9 years) to 7.21.3 (Dec 2010, > 13.3 years). As 10 is a nice round number, we may even be able to > pick randomly a slightly newer one, say, 7.35.0 (Mar 2014, 10.0 > years). > > It is in a sense an inferiour way to pick the minimum dependency > than the choice of 7.32.0, which is backed by "we use this and that, > which appeared in that version", of course. I am OK with just using round numbers, too. The biggest thing for a time-oriented scheme is really what made the cutoff for long-term distro releases. With a 10-year support term, we are often looking at 11 or so years to have made it into a release. OTOH, if somebody using a 10-year-old distro is forced to use a slightly older version of Git to match, IMHO that is not the end of the world. Bumping to 7.35.0 would also let us simplify some curl-compat code, which is always my real goal. ;) -Peff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] http: add the ability to log progress 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:52 ` Jeff King 2 siblings, 0 replies; 25+ messages in thread From: Jeff King @ 2024-05-09 16:52 UTC (permalink / raw) To: Toon Claes; +Cc: git On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote: > @@ -2017,6 +2021,21 @@ static void http_opt_request_remainder(CURL *curl, off_t pos) > #define HTTP_REQUEST_STRBUF 0 > #define HTTP_REQUEST_FILE 1 > > +static int http_progress_callback(void *clientp, curl_off_t dltotal, > + curl_off_t dlnow, curl_off_t ultotal, > + curl_off_t ulnow) > +{ > + struct progress *progress = clientp; > + > + if (progress) { > + progress_set_total(progress, dltotal); > + display_progress(progress, dlnow); > + display_throughput(progress, dlnow); > + } > + > + return 0; > +} A very long time ago I implemented something similar (for a proto-bundle-uri feature), and I found out the hard way that both curl and our progress code may rely on SIGALRM (curl may use it to put a timeout on blocking calls that don't support select, like DNS resolution). Making things even more confusing, it's platform dependent, since on modern platforms it spawns a separate resolving thread to avoid blocking the main one. So it seems to work OK for me on Linux without anything further, but we may want this to be extra careful: -- >8 -- Date: Thu, 10 Nov 2011 01:29:55 -0500 Subject: [PATCH] http: turn off curl signals Curl sets and clears the handler for SIGALRM, which makes it incompatible with git's progress code. However, we can ask curl not to do this. Signed-off-by: Jeff King <peff@peff.net> --- http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http.c b/http.c index 752c879c1f..e7986e1353 100644 --- a/http.c +++ b/http.c @@ -1230,6 +1230,8 @@ static CURL *get_curl_handle(void) set_curl_keepalive(result); + curl_easy_setopt(result, CURLOPT_NOSIGNAL, 1); + return result; } -- 8< -- In the other part of the thread I suggested the possibility of having remote-https shuttle machine-readable progress back to the caller. That would also solve this issue, because the actual display_progress() calls would happen in the parent. -Peff ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 3/4] remote-curl: optionally show progress for HTTP get 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 12:44 ` 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 ` (2 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Toon Claes @ 2024-05-08 12:44 UTC (permalink / raw) To: git; +Cc: Toon Claes git-remote-curl supports the `option progress` basically since it's inception. But this option had no effect for regular HTTP(S) downloads. Add progress indicator when downloading files through curl HTTP GET. Signed-off-by: Toon Claes <toon@iotcl.com> --- remote-curl.c | 8 +++++++- t/t5557-http-get.sh | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 0b6d7815fd..9fc7c3580c 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1293,6 +1293,7 @@ static void parse_get(const char *arg) { struct strbuf url = STRBUF_INIT; struct strbuf path = STRBUF_INIT; + struct http_get_options http_options = {0}; const char *space; space = strchr(arg, ' '); @@ -1303,7 +1304,12 @@ static void parse_get(const char *arg) strbuf_add(&url, arg, space - arg); strbuf_addstr(&path, space + 1); - if (http_get_file(url.buf, path.buf, NULL)) + http_options.initial_request = 1; + + if (options.progress) + http_options.progress = 1; + + if (http_get_file(url.buf, path.buf, &http_options)) die(_("failed to download file at URL '%s'"), url.buf); strbuf_release(&url); diff --git a/t/t5557-http-get.sh b/t/t5557-http-get.sh index 76a4bbd16a..92a138caaf 100755 --- a/t/t5557-http-get.sh +++ b/t/t5557-http-get.sh @@ -36,4 +36,19 @@ test_expect_success 'get by URL: 200' ' test_cmp "$HTTPD_DOCUMENT_ROOT_PATH/exists.txt" file2 ' +test_expect_success 'get by URL with progress' ' + echo hello >"$HTTPD_DOCUMENT_ROOT_PATH/hello.txt" && + + url="$HTTPD_URL/hello.txt" && + cat >input <<-EOF && + capabilities + option progress true + get $url file3 + + EOF + + git remote-http $url <input 2>err && + test_grep "^Downloading via HTTP: 100%" err +' + test_done -- 2.44.0.651.g21306a098c ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] remote-curl: optionally show progress for HTTP get 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 0 siblings, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2024-05-08 22:29 UTC (permalink / raw) To: Toon Claes; +Cc: git Toon Claes <toon@iotcl.com> writes: > git-remote-curl supports the `option progress` basically since it's "it's" -> "its". > inception. But this option had no effect for regular HTTP(S) downloads. > > Add progress indicator when downloading files through curl HTTP GET. > > Signed-off-by: Toon Claes <toon@iotcl.com> > --- > remote-curl.c | 8 +++++++- > t/t5557-http-get.sh | 15 +++++++++++++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/remote-curl.c b/remote-curl.c > index 0b6d7815fd..9fc7c3580c 100644 > --- a/remote-curl.c > +++ b/remote-curl.c > @@ -1293,6 +1293,7 @@ static void parse_get(const char *arg) > { > struct strbuf url = STRBUF_INIT; > struct strbuf path = STRBUF_INIT; > + struct http_get_options http_options = {0}; > const char *space; > > space = strchr(arg, ' '); > @@ -1303,7 +1304,12 @@ static void parse_get(const char *arg) > strbuf_add(&url, arg, space - arg); > strbuf_addstr(&path, space + 1); > > - if (http_get_file(url.buf, path.buf, NULL)) > + http_options.initial_request = 1; > + > + if (options.progress) > + http_options.progress = 1; > + > + if (http_get_file(url.buf, path.buf, &http_options)) > die(_("failed to download file at URL '%s'"), url.buf); > > strbuf_release(&url); > diff --git a/t/t5557-http-get.sh b/t/t5557-http-get.sh > index 76a4bbd16a..92a138caaf 100755 > --- a/t/t5557-http-get.sh > +++ b/t/t5557-http-get.sh > @@ -36,4 +36,19 @@ test_expect_success 'get by URL: 200' ' > test_cmp "$HTTPD_DOCUMENT_ROOT_PATH/exists.txt" file2 > ' > > +test_expect_success 'get by URL with progress' ' > + echo hello >"$HTTPD_DOCUMENT_ROOT_PATH/hello.txt" && > + > + url="$HTTPD_URL/hello.txt" && > + cat >input <<-EOF && > + capabilities > + option progress true > + get $url file3 > + > + EOF > + > + git remote-http $url <input 2>err && > + test_grep "^Downloading via HTTP: 100%" err > +' Are we sure that by the time we finish the transfer we know the total? Does this rely on the fact that somehow we are in control of the server and can force it to use content-length: (instead of chunked encoding without any explicit indication of the total length)? Also, I am curious if we know how often the progress indicator is updated by cURL library with the API used in [2/4] (and if we can control the frequency if we wanted to). Note that this is me just being curious, not a request to make it tweakable. The findings may help polish the [2/4], though. Thanks. > + > test_done ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/4] bundle-uri: enable git-remote-https progress 2024-05-08 12:44 [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs Toon Claes ` (2 preceding siblings ...) 2024-05-08 12:44 ` [PATCH 3/4] remote-curl: optionally show progress for HTTP get Toon Claes @ 2024-05-08 12:44 ` Toon Claes 2024-05-09 16:46 ` 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 5 siblings, 1 reply; 25+ messages in thread From: Toon Claes @ 2024-05-08 12:44 UTC (permalink / raw) To: git; +Cc: Toon Claes When using bundle URIs large files might get downloaded during clone. During that time, there's no feedback to the user what is happening. Enable HTTP download progress for bundle URIs to inform the user. Signed-off-by: Toon Claes <toon@iotcl.com> --- bundle-uri.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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; @@ -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); cleanup: -- 2.44.0.651.g21306a098c ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] bundle-uri: enable git-remote-https progress 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 0 siblings, 1 reply; 25+ messages in thread From: Jeff King @ 2024-05-09 16:46 UTC (permalink / raw) To: Toon Claes; +Cc: git 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] bundle-uri: enable git-remote-https progress 2024-05-09 16:46 ` Jeff King @ 2025-02-14 11:26 ` Toon Claes 2025-02-21 7:36 ` Jeff King 0 siblings, 1 reply; 25+ messages in thread From: Toon Claes @ 2025-02-14 11:26 UTC (permalink / raw) To: Jeff King; +Cc: git 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/4] bundle-uri: enable git-remote-https progress 2025-02-14 11:26 ` Toon Claes @ 2025-02-21 7:36 ` Jeff King 0 siblings, 0 replies; 25+ messages in thread From: Jeff King @ 2025-02-21 7:36 UTC (permalink / raw) To: Toon Claes; +Cc: git On Fri, Feb 14, 2025 at 12:26:03PM +0100, Toon Claes wrote: > 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. Yeah, agree that feels quite hacky (and you'd probably want to swallow /^error:/, /^warning:/, etc, too). > 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"? I was thinking of having some kind of "very quiet" mode where git-remote-http would not print any errors (except for progress). But I agree that doing it is non-trivial. Our die/warning/error functions are all pluggable, so remote-http could add its own implementations using set_die_routine(), etc. But that does feel pretty heavyweight, and you'd still have to pass through the "please suppress all your die calls" option into remote-http. Plus it wouldn't catch any spots in the code that happen to call fprintf(stderr), etc. > 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)? Exactly. > 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? No, I think you understand it perfectly. Your "log" example with arbitrary text seems like the simplest approach, and might be enough. Then we wouldn't have to define a schema for progress numbers (and we have at least two types of progress: counts and throughput, though I guess maybe this would always be throughput?). But I do wonder if we'd want the flexibility of the machine-readable numbers. In particular, would we ever fetch multiple bundle-uri files in parallel? If so, then we wouldn't want them stomping on each other's progress. You'd probably want the caller to present a unified view based on the progress reports from all of the child processes. -Peff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs 2024-05-08 12:44 [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs Toon Claes ` (3 preceding siblings ...) 2024-05-08 12:44 ` [PATCH 4/4] bundle-uri: enable git-remote-https progress Toon Claes @ 2024-05-08 23:49 ` Junio C Hamano 2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes 5 siblings, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2024-05-08 23:49 UTC (permalink / raw) To: Toon Claes; +Cc: git Toon Claes <toon@iotcl.com> writes: > Toon Claes (4): > progress: add function to set total > http: add the ability to log progress > remote-curl: optionally show progress for HTTP get > bundle-uri: enable git-remote-https progress > > bundle-uri.c | 4 +++- > http.c | 32 ++++++++++++++++++++++++++++++++ > http.h | 5 +++++ > progress.c | 6 ++++++ > progress.h | 1 + > remote-curl.c | 8 +++++++- > t/helper/test-progress.c | 5 +++++ > t/t0500-progress-display.sh | 24 ++++++++++++++++++++++++ > t/t5557-http-get.sh | 15 +++++++++++++++ > 9 files changed, 98 insertions(+), 2 deletions(-) This topic, when built on tip of recent tip of 'master' (d4cc1ec35f), or merged to 'seen', seems to break t5558. $ sh t5558-clone-bundle-uri.sh -i -v Initialized empty Git repository in /home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/.git/ expecting success of 5558.1 'fail to clone from non-existent file': test_when_finished rm -rf test && git clone --bundle-uri="$(pwd)/does-not-exist" . test 2>err && grep "failed to download bundle from URI" err warning: failed to download bundle from URI '/home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/does-not-exist' ok 1 - fail to clone from non-existent file expecting success of 5558.2 'fail to clone from non-bundle file': test_when_finished rm -rf test && echo bogus >bogus && git clone --bundle-uri="$(pwd)/bogus" . test 2>err && grep "is not a bundle" err warning: file at URI '/home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/bogus' is not a bundle or bundle list ok 2 - fail to clone from non-bundle file expecting success of 5558.3 'create bundle': git init clone-from && git -C clone-from checkout -b topic && test_commit -C clone-from A && test_commit -C clone-from B && git -C clone-from bundle create B.bundle topic Initialized empty Git repository in /home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/clone-from/.git/ Switched to a new branch 'topic' [topic (root-commit) 0ddfaf1] A Author: A U Thor <author@example.com> 1 file changed, 1 insertion(+) create mode 100644 A.t [topic d9df450] B Author: A U Thor <author@example.com> 1 file changed, 1 insertion(+) create mode 100644 B.t ok 3 - create bundle expecting success of 5558.4 'clone with path bundle': git clone --bundle-uri="clone-from/B.bundle" \ clone-from clone-path && git -C clone-path rev-parse refs/bundles/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual Cloning into 'clone-path'... done. ok 4 - clone with path bundle expecting success of 5558.5 'clone with path bundle and non-default hash': test_when_finished "rm -rf clone-path-non-default-hash" && GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="clone-from/B.bundle" \ clone-from clone-path-non-default-hash && git -C clone-path-non-default-hash rev-parse refs/bundles/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual Cloning into 'clone-path-non-default-hash'... done. ok 5 - clone with path bundle and non-default hash expecting success of 5558.6 'clone with file:// bundle': git clone --bundle-uri="file://$(pwd)/clone-from/B.bundle" \ clone-from clone-file && git -C clone-file rev-parse refs/bundles/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual Cloning into 'clone-file'... done. ok 6 - clone with file:// bundle expecting success of 5558.7 'construct incremental bundle list': ( cd clone-from && git checkout -b base && test_commit 1 && git checkout -b left && test_commit 2 && git checkout -b right base && test_commit 3 && git checkout -b merge left && git merge right -m "4" && git bundle create bundle-1.bundle base && git bundle create bundle-2.bundle base..left && git bundle create bundle-3.bundle base..right && git bundle create bundle-4.bundle merge --not left right ) Switched to a new branch 'base' [base 65d47d5] 1 Author: A U Thor <author@example.com> 1 file changed, 1 insertion(+) create mode 100644 1.t Switched to a new branch 'left' [left 918de9f] 2 Author: A U Thor <author@example.com> 1 file changed, 1 insertion(+) create mode 100644 2.t Switched to a new branch 'right' [right d1bc9f9] 3 Author: A U Thor <author@example.com> 1 file changed, 1 insertion(+) create mode 100644 3.t Switched to a new branch 'merge' Merge made by the 'ort' strategy. 3.t | 1 + 1 file changed, 1 insertion(+) create mode 100644 3.t ok 7 - construct incremental bundle list expecting success of 5558.8 'clone bundle list (file, no heuristic)': cat >bundle-list <<-EOF && [bundle] version = 1 mode = all [bundle "bundle-1"] uri = file://$(pwd)/clone-from/bundle-1.bundle [bundle "bundle-2"] uri = file://$(pwd)/clone-from/bundle-2.bundle [bundle "bundle-3"] uri = file://$(pwd)/clone-from/bundle-3.bundle [bundle "bundle-4"] uri = file://$(pwd)/clone-from/bundle-4.bundle EOF git clone --bundle-uri="file://$(pwd)/bundle-list" \ clone-from clone-list-file 2>err && ! grep "Repository lacks these prerequisite commits" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-list-file cat-file --batch-check <oids && git -C clone-list-file for-each-ref --format="%(refname)" >refs && grep "refs/bundles/" refs >actual && cat >expect <<-\EOF && refs/bundles/base refs/bundles/left refs/bundles/merge refs/bundles/right EOF test_cmp expect actual 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 475812ed564e3085586d0b0acc392cec0a90c18e commit 261 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 commit 165 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 ok 8 - clone bundle list (file, no heuristic) expecting success of 5558.9 'clone bundle list (file, all mode, some failures)': cat >bundle-list <<-EOF && [bundle] version = 1 mode = all # Does not exist. Should be skipped. [bundle "bundle-0"] uri = file://$(pwd)/clone-from/bundle-0.bundle [bundle "bundle-1"] uri = file://$(pwd)/clone-from/bundle-1.bundle [bundle "bundle-2"] uri = file://$(pwd)/clone-from/bundle-2.bundle # No bundle-3 means bundle-4 will not apply. [bundle "bundle-4"] uri = file://$(pwd)/clone-from/bundle-4.bundle # Does not exist. Should be skipped. [bundle "bundle-5"] uri = file://$(pwd)/clone-from/bundle-5.bundle EOF GIT_TRACE2_PERF=1 \ git clone --bundle-uri="file://$(pwd)/bundle-list" \ clone-from clone-all-some 2>err && ! grep "Repository lacks these prerequisite commits" err && ! grep "fatal" err && grep "warning: failed to download bundle from URI" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-all-some cat-file --batch-check <oids && git -C clone-all-some for-each-ref --format="%(refname)" >refs && grep "refs/bundles/" refs >actual && cat >expect <<-\EOF && refs/bundles/base refs/bundles/left EOF test_cmp expect actual warning: failed to download bundle from URI 'file:///home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/clone-from/bundle-5.bundle' warning: failed to download bundle from URI 'file:///home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/clone-from/bundle-0.bundle' 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 475812ed564e3085586d0b0acc392cec0a90c18e commit 261 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 commit 165 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 ok 9 - clone bundle list (file, all mode, some failures) expecting success of 5558.10 'clone bundle list (file, all mode, all failures)': cat >bundle-list <<-EOF && [bundle] version = 1 mode = all # Does not exist. Should be skipped. [bundle "bundle-0"] uri = file://$(pwd)/clone-from/bundle-0.bundle # Does not exist. Should be skipped. [bundle "bundle-5"] uri = file://$(pwd)/clone-from/bundle-5.bundle EOF git clone --bundle-uri="file://$(pwd)/bundle-list" \ clone-from clone-all-fail 2>err && ! grep "Repository lacks these prerequisite commits" err && ! grep "fatal" err && grep "warning: failed to download bundle from URI" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-all-fail cat-file --batch-check <oids && git -C clone-all-fail for-each-ref --format="%(refname)" >refs && ! grep "refs/bundles/" refs warning: failed to download bundle from URI 'file:///home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/clone-from/bundle-5.bundle' warning: failed to download bundle from URI 'file:///home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/clone-from/bundle-0.bundle' 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 475812ed564e3085586d0b0acc392cec0a90c18e commit 261 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 commit 165 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 ok 10 - clone bundle list (file, all mode, all failures) expecting success of 5558.11 'clone bundle list (file, any mode)': cat >bundle-list <<-EOF && [bundle] version = 1 mode = any # Does not exist. Should be skipped. [bundle "bundle-0"] uri = file://$(pwd)/clone-from/bundle-0.bundle [bundle "bundle-1"] uri = file://$(pwd)/clone-from/bundle-1.bundle # Does not exist. Should be skipped. [bundle "bundle-5"] uri = file://$(pwd)/clone-from/bundle-5.bundle EOF git clone --bundle-uri="file://$(pwd)/bundle-list" \ clone-from clone-any-file 2>err && ! grep "Repository lacks these prerequisite commits" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-any-file cat-file --batch-check <oids && git -C clone-any-file for-each-ref --format="%(refname)" >refs && grep "refs/bundles/" refs >actual && cat >expect <<-\EOF && refs/bundles/base EOF test_cmp expect actual 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 475812ed564e3085586d0b0acc392cec0a90c18e commit 261 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 commit 165 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 ok 11 - clone bundle list (file, any mode) expecting success of 5558.12 'clone bundle list (file, any mode, all failures)': cat >bundle-list <<-EOF && [bundle] version = 1 mode = any # Does not exist. Should be skipped. [bundle "bundle-0"] uri = $HTTPD_URL/bundle-0.bundle # Does not exist. Should be skipped. [bundle "bundle-5"] uri = $HTTPD_URL/bundle-5.bundle EOF git clone --bundle-uri="file://$(pwd)/bundle-list" \ clone-from clone-any-fail 2>err && ! grep "fatal" err && grep "warning: failed to download bundle from URI" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-any-fail cat-file --batch-check <oids && git -C clone-any-fail for-each-ref --format="%(refname)" >refs && ! grep "refs/bundles/" refs warning: failed to download bundle from URI '/bundle-5.bundle' warning: failed to download bundle from URI '/bundle-0.bundle' 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 475812ed564e3085586d0b0acc392cec0a90c18e commit 261 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 commit 165 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 ok 12 - clone bundle list (file, any mode, all failures) checking prerequisite: NOT_ROOT mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-NOT_ROOT" && ( cd "$TRASH_DIRECTORY/prereq-test-dir-NOT_ROOT" && uid=$(id -u) && test "$uid" != 0 ) prerequisite NOT_ROOT ok expecting success of 5558.13 'fail to fetch from non-existent HTTP URL': test_when_finished rm -rf test && git clone --bundle-uri="$HTTPD_URL/does-not-exist" . test 2>err && grep "failed to download bundle from URI" err warning: failed to download bundle from URI 'http://127.0.0.1:5558/does-not-exist' ok 13 - fail to fetch from non-existent HTTP URL expecting success of 5558.14 'fail to fetch from non-bundle HTTP URL': test_when_finished rm -rf test && echo bogus >"$HTTPD_DOCUMENT_ROOT_PATH/bogus" && git clone --bundle-uri="$HTTPD_URL/bogus" . test 2>err && grep "is not a bundle" err warning: file at URI 'http://127.0.0.1:5558/bogus' is not a bundle or bundle list ok 14 - fail to fetch from non-bundle HTTP URL expecting success of 5558.15 'clone HTTP bundle': cp clone-from/B.bundle "$HTTPD_DOCUMENT_ROOT_PATH/B.bundle" && git clone --no-local --mirror clone-from \ "$HTTPD_DOCUMENT_ROOT_PATH/fetch.git" && git clone --bundle-uri="$HTTPD_URL/B.bundle" \ "$HTTPD_URL/smart/fetch.git" clone-http && git -C clone-http rev-parse refs/bundles/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual && test_config -C clone-http log.excludedecoration refs/bundle/ Cloning into bare repository '/home/gitster/w/git.git/t/trash directory.t5558-clone-bundle-uri/httpd/www/fetch.git'... Cloning into 'clone-http'... Downloading via HTTP: 100% (520/520)^MDownloading via HTTP: 100% (520/520), 520 bytes | 520.00 KiB/s, done. ok 15 - clone HTTP bundle expecting success of 5558.16 'clone HTTP bundle with non-default hash': test_when_finished "rm -rf clone-http-non-default-hash" && GIT_DEFAULT_HASH=sha256 git clone --bundle-uri="$HTTPD_URL/B.bundle" \ "$HTTPD_URL/smart/fetch.git" clone-http-non-default-hash && git -C clone-http-non-default-hash rev-parse refs/bundles/topic >actual && git -C clone-from rev-parse topic >expect && test_cmp expect actual Cloning into 'clone-http-non-default-hash'... Downloading via HTTP: 100% (520/520)^MDownloading via HTTP: 100% (520/520), 520 bytes | 520.00 KiB/s, done. ok 16 - clone HTTP bundle with non-default hash expecting success of 5558.17 'clone bundle list (HTTP, no heuristic)': test_when_finished rm -f trace*.txt && cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" && cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF && [bundle] version = 1 mode = all [bundle "bundle-1"] uri = $HTTPD_URL/bundle-1.bundle [bundle "bundle-2"] uri = $HTTPD_URL/bundle-2.bundle [bundle "bundle-3"] uri = $HTTPD_URL/bundle-3.bundle [bundle "bundle-4"] uri = $HTTPD_URL/bundle-4.bundle EOF GIT_TRACE2_EVENT="$(pwd)/trace-clone.txt" \ git clone --bundle-uri="$HTTPD_URL/bundle-list" \ clone-from clone-list-http 2>err && ! grep "Repository lacks these prerequisite commits" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-list-http cat-file --batch-check <oids && cat >expect <<-EOF && $HTTPD_URL/bundle-1.bundle $HTTPD_URL/bundle-2.bundle $HTTPD_URL/bundle-3.bundle $HTTPD_URL/bundle-4.bundle $HTTPD_URL/bundle-list EOF # Sort the list, since the order is not well-defined # without a heuristic. test_remote_https_urls <trace-clone.txt | sort >actual && test_cmp expect actual 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 475812ed564e3085586d0b0acc392cec0a90c18e commit 261 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 65d47d507bde981075d2f2532bb104e4b58b9170 commit 213 918de9f3988f29866b94f657626edb7247c7ddbd commit 213 d1bc9f9c222df7b7de791db57cdf7e8ba0627c3e commit 213 0ddfaf193ff13d6ab39b7cbd9eed645e3ee2f050 commit 165 d9df4505cb3522088b9e29d6051ac16f1564154a commit 213 ok 17 - clone bundle list (HTTP, no heuristic) expecting success of 5558.18 'clone bundle list (HTTP, any mode)': cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" && cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF && [bundle] version = 1 mode = any # Does not exist. Should be skipped. [bundle "bundle-0"] uri = $HTTPD_URL/bundle-0.bundle [bundle "bundle-1"] uri = $HTTPD_URL/bundle-1.bundle # Does not exist. Should be skipped. [bundle "bundle-5"] uri = $HTTPD_URL/bundle-5.bundle EOF git clone --bundle-uri="$HTTPD_URL/bundle-list" \ clone-from clone-any-http 2>err && ! grep "fatal" err && grep "warning: failed to download bundle from URI" err && git -C clone-from for-each-ref --format="%(objectname)" >oids && git -C clone-any-http cat-file --batch-check <oids && git -C clone-list-file for-each-ref --format="%(refname)" >refs && grep "refs/bundles/" refs >actual && cat >expect <<-\EOF && refs/bundles/base refs/bundles/left refs/bundles/merge refs/bundles/right EOF test_cmp expect actual fatal: failed to download file at URL 'http://127.0.0.1:5558/bundle-5.bundle' fatal: failed to download file at URL 'http://127.0.0.1:5558/bundle-0.bundle' not ok 18 - clone bundle list (HTTP, any mode) # # cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" && # cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF && # [bundle] # version = 1 # mode = any # # # Does not exist. Should be skipped. # [bundle "bundle-0"] # uri = $HTTPD_URL/bundle-0.bundle # # [bundle "bundle-1"] # uri = $HTTPD_URL/bundle-1.bundle # # # Does not exist. Should be skipped. # [bundle "bundle-5"] # uri = $HTTPD_URL/bundle-5.bundle # EOF # # git clone --bundle-uri="$HTTPD_URL/bundle-list" \ # clone-from clone-any-http 2>err && # ! grep "fatal" err && # grep "warning: failed to download bundle from URI" err && # # git -C clone-from for-each-ref --format="%(objectname)" >oids && # git -C clone-any-http cat-file --batch-check <oids && # # git -C clone-list-file for-each-ref --format="%(refname)" >refs && # grep "refs/bundles/" refs >actual && # cat >expect <<-\EOF && # refs/bundles/base # refs/bundles/left # refs/bundles/merge # refs/bundles/right # EOF # test_cmp expect actual # 1..18 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 0/7] Show progress when downloading from bundle URIs 2024-05-08 12:44 [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs Toon Claes ` (4 preceding siblings ...) 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 ` Toon Claes 2025-02-19 14:30 ` [PATCH v2 1/7] progress: add function to set total Toon Claes ` (6 more replies) 5 siblings, 7 replies; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes, Jeff King When a user clones a repository, they see what's happening in the messages like "Enumerating objects" and "Receiving objects". But when a user clones a repository that uses bundle URIs they see: Cloning into 'repo.git' And then they have to wait until all bundles are downloaded before they see any other message. When the bundles are large, this can take a lot of time and the user might consider the process hangs and they kill it. This patch series introduces progress displayed to the user while bundles are downloaded. The full output of a clone using bundle URIs will look something like: Cloning into 'repo.git'... Downloading via HTTP: 21% (351812809/1620086598), 315.34 MiB | 49.84 MiB/s Downloading via HTTP: 77% (1247493865/1620086598), 1.15 GiB | 34.31 MiB/s Downloading via HTTP: 100% (1620086598/1620086598), 1.50 GiB | 37.97 MiB/s, done. remote: Enumerating objects: 1322255, done. remote: Counting objects: 100% (611708/611708), done. remote: Total 1322255 (delta 611708), reused 611708 (delta 611708), pack-reused 710547 Receiving objects: 100% (1322255/1322255), 539.66 MiB | 31.57 MiB/s, done. etc... In this version I'm adding two commits from Peff. And I'm adding one commit on top to fix the failure in t5558. I'd like to get some feedback on that commit in particular. --- Changes in v2: - Added commit "http: silence stderr when progress is enabled" - Added commit "progress: allow pure-throughput progress meters" from https://lore.kernel.org/git/20111110075300.GK27950@sigill.intra.peff.net/ - Added commit "http: turn off curl signals" from https://lore.kernel.org/git/20240509165212.GC1708095@coredump.intra.peff.net/ - Cleanup both CURLOPT_NOPROGRESS and CURLOPT_NOPROGRESS. - Remove unneeded fflush(). - Link to v1: https://lore.kernel.org/git/20240508124453.600871-1-toon@iotcl.com/ Signed-off-by: Toon Claes <toon@iotcl.com> --- Jeff King (2): progress: allow pure-throughput progress meters http: turn off curl signals Toon Claes (5): progress: add function to set total http: add the ability to log progress remote-curl: optionally show progress for HTTP get bundle-uri: enable git-remote-https progress http: silence stderr when progress is enabled bundle-uri.c | 2 +- http.c | 39 ++++++++++++++++++++++++++++++++ http.h | 5 +++++ progress.c | 40 ++++++++++++++++++++++++--------- progress.h | 2 ++ remote-curl.c | 8 ++++++- t/helper/test-progress.c | 5 +++++ t/t0500-progress-display.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++ t/t5557-http-get.sh | 15 +++++++++++++ 9 files changed, 159 insertions(+), 12 deletions(-) --- Range-diff versus v1: 1: 3eb1958b8b ! 1: 7acc83d92b progress: add function to set total @@ Commit message Signed-off-by: Toon Claes <toon@iotcl.com> ## progress.c ## -@@ progress.c: static struct progress *start_progress_delay(const char *title, uint64_t total, +@@ progress.c: static struct progress *start_progress_delay(struct repository *r, return progress; } @@ progress.h: void progress_test_force_update(void); void display_throughput(struct progress *progress, uint64_t total); void display_progress(struct progress *progress, uint64_t n); +void progress_set_total(struct progress *progress, uint64_t total); - struct progress *start_progress(const char *title, uint64_t total); - struct progress *start_sparse_progress(const char *title, uint64_t total); - struct progress *start_delayed_progress(const char *title, uint64_t total); + struct progress *start_progress(struct repository *r, + const char *title, uint64_t total); + struct progress *start_sparse_progress(struct repository *r, ## t/helper/test-progress.c ## @@ t/helper/test-progress.c: int cmd__progress(int argc, const char **argv) if (*end != '\0') - die("invalid input: '%s'\n", line.buf); + die("invalid input: '%s'", line.buf); display_progress(progress, item_count); + } else if (skip_prefix(line.buf, "total ", (const char **) &end)) { + uint64_t total = strtoull(end, &end, 10); -: ---------- > 2: 7a81cda492 progress: allow pure-throughput progress meters -: ---------- > 3: 86a9120539 http: turn off curl signals 2: b74ce999a1 ! 4: 3a73d68448 http: add the ability to log progress @@ http.c: static void http_opt_request_remainder(CURL *curl, off_t pos) #define HTTP_REQUEST_FILE 1 +static int http_progress_callback(void *clientp, curl_off_t dltotal, -+ curl_off_t dlnow, curl_off_t ultotal, -+ curl_off_t ulnow) ++ curl_off_t dlnow, curl_off_t ultotal UNUSED, ++ curl_off_t ulnow UNUSED) +{ + struct progress *progress = clientp; + @@ http.c: static int http_request(const char *url, http_follow_config == HTTP_FOLLOW_INITIAL) curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); + if (options && options->progress) { -+ progress = start_progress(_("Downloading via HTTP"), 0); ++ progress = start_progress(the_repository, _("Downloading via HTTP"), 0); + + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); @@ http.c: static int http_request(const char *url, ret = run_one_slot(slot, &results); + if (progress) { ++ curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); ++ curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); + stop_progress(&progress); + } + 3: 5b5e7e86c5 ! 5: 01b2a0042f remote-curl: optionally show progress for HTTP get @@ t/t5557-http-get.sh: test_expect_success 'get by URL: 200' ' + EOF + + git remote-http $url <input 2>err && -+ test_grep "^Downloading via HTTP: 100%" err ++ test_grep "^Downloading via HTTP: 100%" err +' + test_done 4: 98ef4dfade ! 6: 3df90aa17b bundle-uri: enable git-remote-https progress @@ bundle-uri.c: static int download_https_uri_to_file(const char *file, const char } + fprintf(child_in, "option progress true\n"); -+ fflush(child_in); -+ fprintf(child_in, "get %s %s\n\n", uri, file); cleanup: -: ---------- > 7: c30c3bf4fe http: silence stderr when progress is enabled --- base-commit: a554262210b4a2ee6fa2d594e1f09f5830888c56 change-id: 20250219-toon-bundleuri-progress-3d902efeba06 Thanks -- Toon ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 1/7] progress: add function to set total 2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes @ 2025-02-19 14:30 ` 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 ` (5 subsequent siblings) 6 siblings, 1 reply; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes We're about to add the use of progress through curl. Although, curl doesn't know the total at the start of the download, but might receive this information in the Content-Length header when the download starts. To allow users set the total size after calling start_progress(), add a function progress_set_total(). Signed-off-by: Toon Claes <toon@iotcl.com> --- progress.c | 6 ++++++ progress.h | 1 + t/helper/test-progress.c | 5 +++++ t/t0500-progress-display.sh | 24 ++++++++++++++++++++++++ 4 files changed, 36 insertions(+) diff --git a/progress.c b/progress.c index 8d5ae70f3a..e254877d2c 100644 --- a/progress.c +++ b/progress.c @@ -276,6 +276,12 @@ static struct progress *start_progress_delay(struct repository *r, return progress; } +void progress_set_total(struct progress *progress, uint64_t total) +{ + if (progress) + progress->total = total; +} + static int get_default_delay(void) { static int delay_in_secs = -1; diff --git a/progress.h b/progress.h index ed068c7bab..2e1bd738c2 100644 --- a/progress.h +++ b/progress.h @@ -15,6 +15,7 @@ void progress_test_force_update(void); void display_throughput(struct progress *progress, uint64_t total); void display_progress(struct progress *progress, uint64_t n); +void progress_set_total(struct progress *progress, uint64_t total); struct progress *start_progress(struct repository *r, const char *title, uint64_t total); struct progress *start_sparse_progress(struct repository *r, diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c index 1f75b7bd19..3a73d6fe0a 100644 --- a/t/helper/test-progress.c +++ b/t/helper/test-progress.c @@ -74,6 +74,11 @@ int cmd__progress(int argc, const char **argv) if (*end != '\0') die("invalid input: '%s'", line.buf); display_progress(progress, item_count); + } else if (skip_prefix(line.buf, "total ", (const char **) &end)) { + uint64_t total = strtoull(end, &end, 10); + if (*end != '\0') + die("invalid input: '%s'\n", line.buf); + progress_set_total(progress, total); } else if (skip_prefix(line.buf, "throughput ", (const char **) &end)) { uint64_t byte_count, test_ms; diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh index d1a498a216..b7ed1db3a0 100755 --- a/t/t0500-progress-display.sh +++ b/t/t0500-progress-display.sh @@ -55,6 +55,30 @@ test_expect_success 'progress display with total' ' test_cmp expect out ' +test_expect_success 'progress display modify total' ' + cat >expect <<-\EOF && + Working hard: 1<CR> + Working hard: 66% (2/3)<CR> + Working hard: 100% (3/3)<CR> + Working hard: 100% (3/3), done. + EOF + + cat >in <<-\EOF && + start 0 + update + progress 1 + update + total 3 + progress 2 + progress 3 + stop + EOF + test-tool progress <in 2>stderr && + + show_cr <stderr >out && + test_cmp expect out +' + test_expect_success 'progress display breaks long lines #1' ' sed -e "s/Z$//" >expect <<\EOF && Working hard.......2.........3.........4.........5.........6: 0% (100/100000)<CR> -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 1/7] progress: add function to set total 2025-02-19 14:30 ` [PATCH v2 1/7] progress: add function to set total Toon Claes @ 2025-02-21 7:43 ` Jeff King 0 siblings, 0 replies; 25+ messages in thread From: Jeff King @ 2025-02-21 7:43 UTC (permalink / raw) To: Toon Claes; +Cc: git On Wed, Feb 19, 2025 at 03:30:19PM +0100, Toon Claes wrote: > We're about to add the use of progress through curl. Although, curl > doesn't know the total at the start of the download, but might receive > this information in the Content-Length header when the download starts. > > To allow users set the total size after calling start_progress(), add a > function progress_set_total(). Makes sense. > +void progress_set_total(struct progress *progress, uint64_t total) > +{ > + if (progress) > + progress->total = total; > +} I wondered if we'd need to do any other computation here (that would have been done in start_progress() if we specified the total then). But it looks like we don't look at progress->total until we're ready to display something. Would we want to call display_progress() or similar here, to update the original view like: Working hard: 1<CR> to: Working hard: 33% (1/3)<CR> immediately, rather than waiting for more progress to be made? I guess it probably doesn't matter that much in practice as we'd remain stale for a brief period in most cases (particularly for transfers where we're updating based on bytes received, which is a pretty small unit of work). Plus I think it may be awkward, because we don't know whether to call display_progress() or display_throughput(). -Peff ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 2/7] progress: allow pure-throughput progress meters 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-19 14:30 ` Toon Claes 2025-02-19 14:30 ` [PATCH v2 3/7] http: turn off curl signals Toon Claes ` (4 subsequent siblings) 6 siblings, 0 replies; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes, Jeff King From: Jeff King <peff@peff.net> The progress code assumes we are counting something (usually objects), even if we are measuring throughput. This works for fetching packfiles, since they show us the object count alongside the throughput, like: Receiving objects: 2% (301/11968), 22.00 MiB | 10.97 MiB/s You can also tell the progress code you don't know how many items you have (by specifying a total of 0), and it looks like: Counting objects: 34957 However, if you're fetching a single large item, you want throughput but you might not have a meaningful count. You can say you are getting item 0 or 1 out of 1 total, but then the percent meter is misleading: Downloading: 0% (0/1), 22.00 MiB | 10.97 MiB/s or Downloading: 100% (0/1), 22.00 MiB | 10.97 MiB/s Neither of those is accurate. You are probably somewhere between zero and 100 percent through the operation, but you don't know how far. Telling it you don't know how many items is even uglier: Downloading: 1, 22.00 MiB | 10.97 MiB/s Instead, this patch will omit the count entirely if you are on the zero-th item of an unknown number of items. It looks like: Downloading: 22.00 MiB | 10.97 MiB/s Signed-off-by: Jeff King <peff@peff.net> --- progress.c | 17 +++++++++++------ t/t0500-progress-display.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/progress.c b/progress.c index e254877d2c..89abb231ae 100644 --- a/progress.c +++ b/progress.c @@ -135,7 +135,11 @@ static void display(struct progress *progress, uint64_t n, const char *done) } } else if (progress_update) { strbuf_reset(counters_sb); - strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp); + if (n > 0) + strbuf_addf(counters_sb, "%" PRIuMAX "%s", + (uintmax_t)n, tp); + else + strbuf_addstr(counters_sb, tp); show_update = 1; } @@ -170,11 +174,12 @@ static void display(struct progress *progress, uint64_t n, const char *done) } } -static void throughput_string(struct strbuf *buf, uint64_t total, - unsigned int rate) +static void throughput_string(struct progress *progress, struct strbuf *buf, + uint64_t total, unsigned int rate) { strbuf_reset(buf); - strbuf_addstr(buf, ", "); + if (progress->total || progress->last_value > 0) + strbuf_addstr(buf, ", "); strbuf_humanise_bytes(buf, total); strbuf_addstr(buf, " | "); strbuf_humanise_rate(buf, rate * 1024); @@ -243,7 +248,7 @@ void display_throughput(struct progress *progress, uint64_t total) tp->last_misecs[tp->idx] = misecs; tp->idx = (tp->idx + 1) % TP_IDX_MAX; - throughput_string(&tp->display, total, rate); + throughput_string(progress, &tp->display, total, rate); if (progress->last_value != -1 && progress_update) display(progress, progress->last_value, NULL); } @@ -343,7 +348,7 @@ static void force_last_update(struct progress *progress, const char *msg) unsigned int misecs, rate; misecs = ((now_ns - progress->start_ns) * 4398) >> 32; rate = tp->curr_total / (misecs ? misecs : 1); - throughput_string(&tp->display, tp->curr_total, rate); + throughput_string(progress, &tp->display, tp->curr_total, rate); } progress_update = 1; buf = xstrfmt(", %s.\n", msg); diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh index b7ed1db3a0..582b9fa899 100755 --- a/t/t0500-progress-display.sh +++ b/t/t0500-progress-display.sh @@ -263,6 +263,37 @@ test_expect_success 'progress display with throughput and total' ' test_cmp expect out ' +test_expect_success 'progress display throughput without total' ' + cat >expect <<-\EOF && + Working hard: <CR> + Working hard: 200.00 KiB | 100.00 KiB/s<CR> + Working hard: 300.00 KiB | 100.00 KiB/s<CR> + Working hard: 400.00 KiB | 100.00 KiB/s<CR> + Working hard: 400.00 KiB | 100.00 KiB/s, done. + EOF + + cat >in <<-\EOF && + start 0 + throughput 102400 1000 + update + progress 0 + throughput 204800 2000 + update + progress 0 + throughput 307200 3000 + update + progress 0 + throughput 409600 4000 + update + progress 0 + stop + EOF + test-tool progress <in 2>stderr && + + show_cr <stderr >out && + test_cmp expect out +' + test_expect_success 'cover up after throughput shortens' ' cat >expect <<-\EOF && Working hard: 1<CR> -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 3/7] http: turn off curl signals 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-19 14:30 ` [PATCH v2 2/7] progress: allow pure-throughput progress meters Toon Claes @ 2025-02-19 14:30 ` Toon Claes 2025-02-19 14:30 ` [PATCH v2 4/7] http: add the ability to log progress Toon Claes ` (3 subsequent siblings) 6 siblings, 0 replies; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes, Jeff King From: Jeff King <peff@peff.net> Curl sets and clears the handler for SIGALRM, which makes it incompatible with git's progress code. However, we can ask curl not to do this. Signed-off-by: Jeff King <peff@peff.net> --- http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http.c b/http.c index f4504133e8..38c7c0cd54 100644 --- a/http.c +++ b/http.c @@ -1245,6 +1245,8 @@ static CURL *get_curl_handle(void) set_curl_keepalive(result); + curl_easy_setopt(result, CURLOPT_NOSIGNAL, 1); + return result; } -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 4/7] http: add the ability to log progress 2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes ` (2 preceding siblings ...) 2025-02-19 14:30 ` [PATCH v2 3/7] http: turn off curl signals Toon Claes @ 2025-02-19 14:30 ` Toon Claes 2025-02-19 14:30 ` [PATCH v2 5/7] remote-curl: optionally show progress for HTTP get Toon Claes ` (2 subsequent siblings) 6 siblings, 0 replies; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes Add an option `progress` to `struct http_get_options` to allow the caller to enable download progress using the progress.c API. Signed-off-by: Toon Claes <toon@iotcl.com> --- http.c | 34 ++++++++++++++++++++++++++++++++++ http.h | 5 +++++ 2 files changed, 39 insertions(+) diff --git a/http.c b/http.c index 38c7c0cd54..5517863808 100644 --- a/http.c +++ b/http.c @@ -13,6 +13,7 @@ #include "credential.h" #include "version.h" #include "pkt-line.h" +#include "progress.h" #include "gettext.h" #include "trace.h" #include "transport.h" @@ -1504,6 +1505,9 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); /* * Default following to off unless "ALWAYS" is configured; this gives @@ -2068,6 +2072,21 @@ static void http_opt_request_remainder(CURL *curl, off_t pos) #define HTTP_REQUEST_STRBUF 0 #define HTTP_REQUEST_FILE 1 +static int http_progress_callback(void *clientp, curl_off_t dltotal, + curl_off_t dlnow, curl_off_t ultotal UNUSED, + curl_off_t ulnow UNUSED) +{ + struct progress *progress = clientp; + + if (progress) { + progress_set_total(progress, dltotal); + display_progress(progress, dlnow); + display_throughput(progress, dlnow); + } + + return 0; +} + static int http_request(const char *url, void *result, int target, const struct http_get_options *options) @@ -2076,6 +2095,7 @@ static int http_request(const char *url, struct slot_results results; struct curl_slist *headers = http_copy_default_headers(); struct strbuf buf = STRBUF_INIT; + struct progress *progress = NULL; const char *accept_language; int ret; @@ -2112,6 +2132,13 @@ static int http_request(const char *url, if (options && options->initial_request && http_follow_config == HTTP_FOLLOW_INITIAL) curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); + if (options && options->progress) { + progress = start_progress(the_repository, _("Downloading via HTTP"), 0); + + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, &http_progress_callback); + } headers = curl_slist_append(headers, buf.buf); @@ -2134,6 +2161,13 @@ static int http_request(const char *url, ret = run_one_slot(slot, &results); + if (progress) { + curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, NULL); + curl_easy_setopt(slot->curl, CURLOPT_XFERINFOFUNCTION, NULL); + stop_progress(&progress); + } + if (options && options->content_type) { struct strbuf raw = STRBUF_INIT; curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, &raw); diff --git a/http.h b/http.h index 36202139f4..09ebbdfefe 100644 --- a/http.h +++ b/http.h @@ -146,6 +146,11 @@ struct http_get_options { * request has completed. */ struct string_list *extra_headers; + + /* + * If not zero, display the progress. + */ + int progress; }; /* Return values for http_get_*() */ -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 5/7] remote-curl: optionally show progress for HTTP get 2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes ` (3 preceding siblings ...) 2025-02-19 14:30 ` [PATCH v2 4/7] http: add the ability to log progress Toon Claes @ 2025-02-19 14:30 ` 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 6 siblings, 0 replies; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes git-remote-curl supports the `option progress` basically since it's inception. But this option had no effect for regular HTTP(S) downloads. Add progress indicator when downloading files through curl HTTP GET. Signed-off-by: Toon Claes <toon@iotcl.com> --- remote-curl.c | 8 +++++++- t/t5557-http-get.sh | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 1273507a96..f710d6b3cb 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1317,6 +1317,7 @@ static void parse_get(const char *arg) { struct strbuf url = STRBUF_INIT; struct strbuf path = STRBUF_INIT; + struct http_get_options http_options = {0}; const char *space; space = strchr(arg, ' '); @@ -1327,7 +1328,12 @@ static void parse_get(const char *arg) strbuf_add(&url, arg, space - arg); strbuf_addstr(&path, space + 1); - if (http_get_file(url.buf, path.buf, NULL)) + http_options.initial_request = 1; + + if (options.progress) + http_options.progress = 1; + + if (http_get_file(url.buf, path.buf, &http_options)) die(_("failed to download file at URL '%s'"), url.buf); strbuf_release(&url); diff --git a/t/t5557-http-get.sh b/t/t5557-http-get.sh index 67fcc23f11..41f3d16ef9 100755 --- a/t/t5557-http-get.sh +++ b/t/t5557-http-get.sh @@ -35,4 +35,19 @@ test_expect_success 'get by URL: 200' ' test_cmp "$HTTPD_DOCUMENT_ROOT_PATH/exists.txt" file2 ' +test_expect_success 'get by URL with progress' ' + echo hello >"$HTTPD_DOCUMENT_ROOT_PATH/hello.txt" && + + url="$HTTPD_URL/hello.txt" && + cat >input <<-EOF && + capabilities + option progress true + get $url file3 + + EOF + + git remote-http $url <input 2>err && + test_grep "^Downloading via HTTP: 100%" err +' + test_done -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 6/7] bundle-uri: enable git-remote-https progress 2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes ` (4 preceding siblings ...) 2025-02-19 14:30 ` [PATCH v2 5/7] remote-curl: optionally show progress for HTTP get Toon Claes @ 2025-02-19 14:30 ` Toon Claes 2025-02-19 14:30 ` [PATCH v2 7/7] http: silence stderr when progress is enabled Toon Claes 6 siblings, 0 replies; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes When using bundle URIs large files might get downloaded during clone. During that time, there's no feedback to the user what is happening. Enable HTTP download progress for bundle URIs to inform the user. Signed-off-by: Toon Claes <toon@iotcl.com> --- bundle-uri.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle-uri.c b/bundle-uri.c index 744257c49c..0ef96cd00f 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -298,7 +298,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; @@ -333,6 +332,7 @@ static int download_https_uri_to_file(const char *file, const char *uri) goto cleanup; } + fprintf(child_in, "option progress true\n"); fprintf(child_in, "get %s %s\n\n", uri, file); cleanup: -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v2 7/7] http: silence stderr when progress is enabled 2025-02-19 14:30 ` [PATCH v2 0/7] Show " Toon Claes ` (5 preceding siblings ...) 2025-02-19 14:30 ` [PATCH v2 6/7] bundle-uri: enable git-remote-https progress Toon Claes @ 2025-02-19 14:30 ` Toon Claes 2025-02-21 7:48 ` Jeff King 6 siblings, 1 reply; 25+ messages in thread From: Toon Claes @ 2025-02-19 14:30 UTC (permalink / raw) To: git; +Cc: Toon Claes To download bundle URI bundles over HTTP(s), git-clone(1) spawns a git-remote-http(1) subprocess. Because clone can continue without bundles, all errors sent by the child process over stderr are suppressed. In previous commits, we've added progress output in the child process, and this happens over stderr, so we can no longer silence stderr. But in case a bundle could not be downloaded, the user sees the following messages: fatal: failed to download file at URL 'http://127.0.0.1:5558/bundle-5.bundle' warning: failed to download bundle from URI 'http://127.0.0.1:5558/bundle-5.bundle' Here the child git-remote-http(1) prints a "fatal" error and then the parent git-clone(1) prints a "warning". This is confusing to the user. Instead of suppressing stderr from the parent process, like we did before, modify stderr to write to /dev/null in the child process itself, while keep using the original stderr for progress logging only. Signed-off-by: Toon Claes <toon@iotcl.com> --- http.c | 5 ++++- progress.c | 17 +++++++++++++---- progress.h | 1 + 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/http.c b/http.c index 5517863808..5c0c6ef204 100644 --- a/http.c +++ b/http.c @@ -2133,7 +2133,10 @@ static int http_request(const char *url, http_follow_config == HTTP_FOLLOW_INITIAL) curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); if (options && options->progress) { - progress = start_progress(the_repository, _("Downloading via HTTP"), 0); + progress = start_progress(the_repository, + _("Downloading via HTTP"), 0); + progress_set_fd(progress, fileno(stderr)); + freopen("/dev/null", "w", stderr); curl_easy_setopt(slot->curl, CURLOPT_NOPROGRESS, 0L); curl_easy_setopt(slot->curl, CURLOPT_XFERINFODATA, progress); diff --git a/progress.c b/progress.c index 89abb231ae..1955262000 100644 --- a/progress.c +++ b/progress.c @@ -40,6 +40,7 @@ struct progress { const char *title; uint64_t last_value; uint64_t total; + int fd; unsigned last_percent; unsigned delay; unsigned sparse; @@ -144,7 +145,9 @@ static void display(struct progress *progress, uint64_t n, const char *done) } if (show_update) { - if (is_foreground_fd(fileno(stderr)) || done) { + int fd = progress->fd ? progress->fd : fileno(stderr); + + if (is_foreground_fd(fd) || done) { const char *eol = done ? done : "\r"; size_t clear_len = counters_sb->len < last_count_len ? last_count_len - counters_sb->len + 1 : @@ -155,17 +158,17 @@ static void display(struct progress *progress, uint64_t n, const char *done) int cols = term_columns(); if (progress->split) { - fprintf(stderr, " %s%*s", counters_sb->buf, + dprintf(fd, " %s%*s", counters_sb->buf, (int) clear_len, eol); } else if (!done && cols < progress_line_len) { clear_len = progress->title_len + 1 < cols ? cols - progress->title_len - 1 : 0; - fprintf(stderr, "%s:%*s\n %s%s", + dprintf(fd, "%s:%*s\n %s%s", progress->title, (int) clear_len, "", counters_sb->buf, eol); progress->split = 1; } else { - fprintf(stderr, "%s: %s%*s", progress->title, + dprintf(fd, "%s: %s%*s", progress->title, counters_sb->buf, (int) clear_len, eol); } fflush(stderr); @@ -287,6 +290,12 @@ void progress_set_total(struct progress *progress, uint64_t total) progress->total = total; } +void progress_set_fd(struct progress *progress, int fd) +{ + if (progress) + progress->fd = fd; +} + static int get_default_delay(void) { static int delay_in_secs = -1; diff --git a/progress.h b/progress.h index 2e1bd738c2..f12c82adc4 100644 --- a/progress.h +++ b/progress.h @@ -16,6 +16,7 @@ void progress_test_force_update(void); void display_throughput(struct progress *progress, uint64_t total); void display_progress(struct progress *progress, uint64_t n); void progress_set_total(struct progress *progress, uint64_t total); +void progress_set_fd(struct progress *progress, int fd); struct progress *start_progress(struct repository *r, const char *title, uint64_t total); struct progress *start_sparse_progress(struct repository *r, -- 2.48.1.658.g4767266eb4 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v2 7/7] http: silence stderr when progress is enabled 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 0 siblings, 0 replies; 25+ messages in thread From: Jeff King @ 2025-02-21 7:48 UTC (permalink / raw) To: Toon Claes; +Cc: git On Wed, Feb 19, 2025 at 03:30:25PM +0100, Toon Claes wrote: > diff --git a/http.c b/http.c > index 5517863808..5c0c6ef204 100644 > --- a/http.c > +++ b/http.c > @@ -2133,7 +2133,10 @@ static int http_request(const char *url, > http_follow_config == HTTP_FOLLOW_INITIAL) > curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); > if (options && options->progress) { > - progress = start_progress(the_repository, _("Downloading via HTTP"), 0); > + progress = start_progress(the_repository, > + _("Downloading via HTTP"), 0); > + progress_set_fd(progress, fileno(stderr)); > + freopen("/dev/null", "w", stderr); Hmm. I can't think of any reason this wouldn't work, and it certainly is less ugly than overriding die() and friends. It does still feel a bit hacky to me. For one thing, asking for progress does not necessarily mean you _also_ want to suppress errors. So it would have to be a separate option. But mostly the more I think about it, the more the "send progress data back to the caller over stdout" thing makes sense. I think we probably do eventually want to support parallel fetches, which would require coordination in the caller. -Peff ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2025-02-21 7:48 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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
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).