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

Commit Message

Toon Claes Feb. 19, 2025, 2:30 p.m. UTC

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 <[e-mail]>
---
 http.c     |  5 ++++-
 progress.c | 17 +++++++++++++----
 progress.h |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)

Comments

Jeff King Feb. 21, 2025, 7:48 a.m. UTC | #1

On Wed, Feb 19, 2025 at 03:30:25PM +0100, Toon Claes wrote:

> diff --git a/http.c b/http.c
> index 5517863808..5c0c6ef204 100644
> --- a/http.c
> +++ b/http.c
> @@ -2133,7 +2133,10 @@ static int http_request(const char *url,
>  	    http_follow_config == HTTP_FOLLOW_INITIAL)
>  		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
>  	if (options && options->progress) {
> -		progress = start_progress(the_repository, _("Downloading via HTTP"), 0);
> +		progress = start_progress(the_repository,
> +					  _("Downloading via HTTP"), 0);
> +		progress_set_fd(progress, fileno(stderr));
> +		freopen("/dev/null", "w", stderr);

Hmm. I can't think of any reason this wouldn't work, and it certainly is
less ugly than overriding die() and friends.

It does still feel a bit hacky to me. For one thing, asking for progress
does not necessarily mean you _also_ want to suppress errors. So it
would have to be a separate option.

But mostly the more I think about it, the more the "send progress data
back to the caller over stdout" thing makes sense. I think we probably
do eventually want to support parallel fetches, which would require
coordination in the caller.

-Peff

Patch

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,