threads / patch / 61434

patch, 4 partsbundle-uri: show progress when downloading from bundle URIs

Subject: [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs

## tl;dr

25 messages between May 8, 2024 and Feb 21, 2025. Diffs are folded; open one to read it.

replies: 24people: 4as markdown or json

Toon Claes· May 8, 2024, 12:44 UTC · lore

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

Toon Claes· May 8, 2024, 12:44 UTC · re: Toon Claes · lore

[PATCH 1/4] 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 <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(+)
Show changes to 4 files +36 −0

progress.c, progress.h, t/helper/test-progress.c, t/t0500-progress-display.sh

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
Toon Claes· May 8, 2024, 12:44 UTC · re: Toon Claes · lore

[PATCH 2/4] 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 <toon@iotcl.com>
---
 http.c | 32 ++++++++++++++++++++++++++++++++
 http.h |  5 +++++
 2 files changed, 37 insertions(+)
Show changes to 2 files +37 −0

http.c, http.h

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
Eric Sunshine· May 8, 2024, 16:52 UTC · re: Toon Claes · lore

Re: [PATCH 2/4] http: add the ability to log progress

On Wed, May 8, 2024 at 8:45 AM Toon Claes <toon@iotcl.com> wrote:
Show 19 quoted lines
> 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?

Jeff King· May 9, 2024, 16:34 UTC · re: Toon Claes · lore

Re: [PATCH 2/4] http: add the ability to log progress

On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote:
Show 7 quoted lines
> @@ -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
Junio C Hamano· May 9, 2024, 16:51 UTC · re: Jeff King · lore

Re: [PATCH 2/4] http: add the ability to log progress

Jeff King <peff@peff.net> writes:
Show 21 quoted lines
> 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.
Jeff King· May 9, 2024, 17:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/4] http: add the ability to log progress

On Thu, May 09, 2024 at 09:51:51AM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> > 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
Jeff King· May 9, 2024, 16:52 UTC · re: Toon Claes · lore

Re: [PATCH 2/4] http: add the ability to log progress

On Wed, May 08, 2024 at 02:44:51PM +0200, Toon Claes wrote:
Show 18 quoted lines
> @@ -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(+)
Show changes to http.c +2 −2
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
Toon Claes· May 8, 2024, 12:44 UTC · re: Toon Claes · lore

[PATCH 3/4] 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 <toon@iotcl.com>
---
 remote-curl.c       |  8 +++++++-
 t/t5557-http-get.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
Show changes to 2 files +22 −1

remote-curl.c, t/t5557-http-get.sh

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
Junio C Hamano· May 8, 2024, 22:29 UTC · re: Toon Claes · lore

Re: [PATCH 3/4] remote-curl: optionally show progress for HTTP get

Toon Claes <toon@iotcl.com> writes:
> git-remote-curl supports the `option progress` basically since it's
"it's" -> "its".
Show 58 quoted lines
> 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
Toon Claes· May 8, 2024, 12:44 UTC · re: Toon Claes · lore

[PATCH 4/4] 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 <toon@iotcl.com>
---
 bundle-uri.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to bundle-uri.c +3 −1
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
Jeff King· May 9, 2024, 16:46 UTC · re: Toon Claes · lore

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

On Wed, May 08, 2024 at 02:44:53PM +0200, Toon Claes wrote:
Show 11 quoted lines
> 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).

Show 8 quoted lines
> @@ -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
Toon Claes· Feb 14, 2025, 11:26 UTC · re: Jeff King · lore

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

Hi Peff,

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

Jeff King <peff@peff.net> writes:
Show 40 quoted lines
> On Wed, May 08, 2024 at 02:44:53PM +0200, Toon Claes wrote:
>
>> diff --git a/bundle-uri.c b/bundle-uri.c
>> index ca32050a78..462f00f668 100644
>> --- a/bundle-uri.c
>> +++ b/bundle-uri.c
>> @@ -293,7 +293,6 @@ static int download_https_uri_to_file(const char *file, const char *uri)
>>  	int found_get = 0;
>>  
>>  	strvec_pushl(&cp.args, "git-remote-https", uri, NULL);
>> -	cp.err = -1;
>>  	cp.in = -1;
>>  	cp.out = -1;
>
> This is the cause of at least one test failure in t5558, I think. We
> spawn a remote-https to try to download the bundle, but it may not be
> present, and we continue without it. In that case, the child
> remote-https says something like:
>
>   fatal: failed to download file at URL 'http://127.0.0.1:5558/bundle-5.bundle'
>
> and then the caller says:
>
>   warning: failed to download bundle from URI 'http://127.0.0.1:5558/bundle-5.bundle'
>
> Prior to this patch, the "fatal" part coming from the child process was
> suppressed (and the test checks that this is so, which is why it fails,
> even though the clone itself works fine).
>
> Obviously you need to enable stderr to see the progress, so I'm not sure
> how to resolve it. In an ideal world, you'd ask for the two over
> separate descriptors, but I think run_command() only supports 0/1/2
> stdio due to portability limitations for Windows.
>
> One option is that remote-https could ferry machine-readable output back
> to the parent over stdout, which could then format it for the user.
>
> Another is that we could somehow ask remote-https to squelch
> non-progress errors, though that feels a bit weird (and awkward to
> implement, since the message comes from a die() call).

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

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

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

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

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

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

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

~fin~

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

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

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

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

-- Toon

Jeff King· Feb 21, 2025, 07:36 UTC · re: Toon Claes · lore

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

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

Show 6 quoted lines
> 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.

Show 9 quoted lines
> 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.
Show 35 quoted lines
> 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
Junio C Hamano· May 8, 2024, 23:49 UTC · re: Toon Claes · lore

Re: [PATCH 0/4] bundle-uri: show progress when downloading from bundle URIs

Toon Claes <toon@iotcl.com> writes:
Show 16 quoted lines
> 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

Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 0/7] 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 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

Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 1/7] 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 <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(+)
Show changes to 4 files +36 −0

progress.c, progress.h, t/helper/test-progress.c, t/t0500-progress-display.sh

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
Jeff King· Feb 21, 2025, 07:43 UTC · re: Toon Claes · lore

Re: [PATCH v2 1/7] progress: add function to set total

On Wed, Feb 19, 2025 at 03:30:19PM +0100, Toon Claes wrote:
Show 6 quoted lines
> 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.
Show 5 quoted lines
> +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
Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 2/7] progress: allow pure-throughput progress meters

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(-)
Show changes to 2 files +42 −6

progress.c, t/t0500-progress-display.sh

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
Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 3/7] http: turn off curl signals

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(+)
Show changes to http.c +2 −0
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
Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 4/7] 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 <toon@iotcl.com>
---
 http.c | 34 ++++++++++++++++++++++++++++++++++
 http.h |  5 +++++
 2 files changed, 39 insertions(+)
Show changes to 2 files +39 −0

http.c, http.h

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
Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 5/7] 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 <toon@iotcl.com>
---
 remote-curl.c       |  8 +++++++-
 t/t5557-http-get.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
Show changes to 2 files +22 −1

remote-curl.c, t/t5557-http-get.sh

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
Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 6/7] 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 <toon@iotcl.com>
---
 bundle-uri.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to bundle-uri.c +1 −1
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
Toon Claes· Feb 19, 2025, 14:30 UTC · re: Toon Claes · lore

[PATCH v2 7/7] 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 <toon@iotcl.com>
---
 http.c     |  5 ++++-
 progress.c | 17 +++++++++++++----
 progress.h |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)
Show changes to 3 files +18 −5

http.c, progress.c, progress.h

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
Jeff King· Feb 21, 2025, 07:48 UTC · re: Toon Claes · lore

Re: [PATCH v2 7/7] http: silence stderr when progress is enabled

On Wed, Feb 19, 2025 at 03:30:25PM +0100, Toon Claes wrote:
Show 13 quoted lines
> 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

← back to recent threads