From 4e051d924a4b018a732cd5b9707b075fbfa7c553 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 19 Feb 2025 14:11:45 +0100 Subject: [PATCH 1/8] Show progress when downloading from bundle URIs 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 v3: - EDITME: describe what is new in this series revision. - EDITME: use bulletpoints and terse descriptions. - Link to v2: https://lore.kernel.org/r/20250219-toon-bundleuri-progress-v2-0-a84e7ffa921a@iotcl.com 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 --- b4-submit-tracking --- # This section is used internally by b4 prep for tracking purposes. { "series": { "revision": 3, "change-id": "20250219-toon-bundleuri-progress-3d902efeba06", "prefixes": [], "history": { "v1": [ "20240508124453.600871-1-toon@iotcl.com" ], "v2": [ "20250219-toon-bundleuri-progress-v2-0-a84e7ffa921a@iotcl.com" ] } } } -- GitLab From c5b4908f0d319096b21e3abb22fd3a8e4bda6b18 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 8 May 2024 13:39:22 +0200 Subject: [PATCH 2/8] progress: add function to set total 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 --- 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 8d5ae70f3a9..e254877d2c6 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 ed068c7bab8..2e1bd738c29 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 1f75b7bd199..3a73d6fe0ab 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 d1a498a216f..b7ed1db3a05 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 + Working hard: 66% (2/3) + Working hard: 100% (3/3) + 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 stderr && + + show_cr 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) -- GitLab From a98fa2f632b8b3efeac897d83f1ac83df9641a36 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 10 Nov 2011 02:53:00 -0500 Subject: [PATCH 3/8] progress: allow pure-throughput progress meters 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 --- 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 e254877d2c6..89abb231ae2 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 b7ed1db3a05..582b9fa899b 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: + Working hard: 200.00 KiB | 100.00 KiB/s + Working hard: 300.00 KiB | 100.00 KiB/s + Working hard: 400.00 KiB | 100.00 KiB/s + 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 stderr && + + show_cr out && + test_cmp expect out +' + test_expect_success 'cover up after throughput shortens' ' cat >expect <<-\EOF && Working hard: 1 -- GitLab From bac19d37e238e55c61d87e1d8fd767e7a2e5e1bd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 10 Nov 2011 01:29:55 -0500 Subject: [PATCH 4/8] 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 --- http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http.c b/http.c index f4504133e88..38c7c0cd54e 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; } -- GitLab From 4b4908ae1f35845f7aa67f69cc6ef36c6d681c50 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 3 May 2024 14:40:53 +0200 Subject: [PATCH 5/8] http: add the ability to log progress 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 --- http.c | 34 ++++++++++++++++++++++++++++++++++ http.h | 5 +++++ 2 files changed, 39 insertions(+) diff --git a/http.c b/http.c index 38c7c0cd54e..5517863808c 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 36202139f45..09ebbdfefe8 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_*() */ -- GitLab From 5c64183941387a2309de8ad4bdd060dae4d7cc38 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 3 May 2024 15:04:41 +0200 Subject: [PATCH 6/8] remote-curl: optionally show progress for HTTP get 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 --- 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 1273507a96c..f710d6b3cb8 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 67fcc23f110..41f3d16ef9f 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 err && + test_grep "^Downloading via HTTP: 100%" err +' + test_done -- GitLab From 4c717c6874f186381ca76005c466d91b6fc1f14b Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 3 May 2024 15:17:16 +0200 Subject: [PATCH 7/8] bundle-uri: enable git-remote-https progress 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 --- bundle-uri.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle-uri.c b/bundle-uri.c index 744257c49c1..0ef96cd00f3 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: -- GitLab From cef6a5f3f4495d258275b5ca88d7b4586f3357c0 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Wed, 19 Feb 2025 12:07:01 +0100 Subject: [PATCH 8/8] http: silence stderr when progress is enabled 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 --- 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 5517863808c..5c0c6ef2047 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 89abb231ae2..1955262000e 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 2e1bd738c29..f12c82adc42 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, -- GitLab