git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Taylor Blau" <me@ttaylorr.com>,
	"Charles Diza" <chdiza@gmail.com>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: [PATCH 1/2] Revert "progress: use term_clear_line()"
Date: Mon, 16 Sep 2019 22:54:11 +0200	[thread overview]
Message-ID: <20190916205412.8602-2-szeder.dev@gmail.com> (raw)
In-Reply-To: <20190916205412.8602-1-szeder.dev@gmail.com>

This reverts commit 5b12e3123b (progress: use term_clear_line(),
2019-06-24), because covering up the entire last line while refreshing
the progress line caused unexpected problems during 'git
clone/fetch/push':

  $ git clone ssh://localhost/home/szeder/src/tmp/linux.git/
  Cloning into 'linux'...
  remote:
  remote:
  remote:
  remote: Enumerating objects: 999295

The length of the progress bar line can shorten when it includes
throughput and the unit changes, or when its length exceeds the width
of the terminal and is broken into two lines.  In these cases the
previously displayed longer progress line should be covered up,
because otherwise the leftover characters from the previous progress
line make the output look weird [1].  term_clear_line() makes this
quite simple, as it covers up the entire last line either by using an
ANSI control sequence or by printing a terminal width worth of space
characters, depending on whether the terminal is smart or dumb.

Unfortunately, when accessing a remote repository via any non-local
protocol the remote 'git receive-pack/upload-pack' processes can't
possibly have any idea about the local terminal (smart of dumb? how
wide?) their progress will end up on.  Consequently, they assume the
worst, i.e. standard-width dumb terminal, and print 80 spaces to cover
up the previously displayed progress line.  The local 'git
clone/fetch/push' processes then display the remote's progress,
including these coverup spaces, with the 'remote: ' prefix, resulting
in a total line length of 88 characters.  If the local terminal is
narrower than that, then the coverup gets line-wrapped, and after that
the CR at the end doesn't return to the beginning of the progress
line, but to the first column of its last line, resulting in those
repeated 'remote: <many-spaces>' lines.

By reverting 5b12e3123b (progress: use term_clear_line(),
2019-06-24) we won't cover up the entire last line, but go back to
comparing the length of the current progress bar line with the
previous one, and cover up as many characters as needed.

[1] See commits 545dc345eb (progress: break too long progress bar
    lines, 2019-04-12) and 9f1fd84e15 (progress: clear previous
    progress update dynamically, 2019-04-12).

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 progress.c                 | 29 ++++++++++++++++++-----------
 t/t5541-http-push-smart.sh |  6 +++---
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/progress.c b/progress.c
index 277db8afa2..0eddf1804d 100644
--- a/progress.c
+++ b/progress.c
@@ -88,6 +88,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
 	int show_update = 0;
+	int last_count_len = counters_sb->len;
 
 	if (progress->delay && (!progress_update || --progress->delay))
 		return;
@@ -115,21 +116,27 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	if (show_update) {
 		if (is_foreground_fd(fileno(stderr)) || done) {
 			const char *eol = done ? done : "\r";
+			size_t clear_len = counters_sb->len < last_count_len ?
+					last_count_len - counters_sb->len + 1 :
+					0;
+			/* The "+ 2" accounts for the ": ". */
+			size_t progress_line_len = progress->title_len +
+						counters_sb->len + 2;
+			int cols = term_columns();
 
-			term_clear_line();
 			if (progress->split) {
-				fprintf(stderr, "  %s%s", counters_sb->buf,
-					eol);
-			} else if (!done &&
-				   /* The "+ 2" accounts for the ": ". */
-				   term_columns() < progress->title_len +
-						    counters_sb->len + 2) {
-				fprintf(stderr, "%s:\n  %s%s",
-					progress->title, counters_sb->buf, eol);
+				fprintf(stderr, "  %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",
+					progress->title, (int) clear_len, "",
+					counters_sb->buf, eol);
 				progress->split = 1;
 			} else {
-				fprintf(stderr, "%s: %s%s", progress->title,
-					counters_sb->buf, eol);
+				fprintf(stderr, "%s: %s%*s", progress->title,
+					counters_sb->buf, (int) clear_len, eol);
 			}
 			fflush(stderr);
 		}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index b86ddb60f2..92bac43257 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -262,7 +262,7 @@ test_expect_success TTY 'push shows progress when stderr is a tty' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	test_commit noisy &&
 	test_terminal git push >output 2>&1 &&
-	test_i18ngrep "Writing objects" output
+	test_i18ngrep "^Writing objects" output
 '
 
 test_expect_success TTY 'push --quiet silences status and progress' '
@@ -277,7 +277,7 @@ test_expect_success TTY 'push --no-progress silences progress but not status' '
 	test_commit no-progress &&
 	test_terminal git push --no-progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep ! "Writing objects" output
+	test_i18ngrep ! "^Writing objects" output
 '
 
 test_expect_success 'push --progress shows progress to non-tty' '
@@ -285,7 +285,7 @@ test_expect_success 'push --progress shows progress to non-tty' '
 	test_commit progress &&
 	git push --progress >output 2>&1 &&
 	test_i18ngrep "^To http" output &&
-	test_i18ngrep "Writing objects" output
+	test_i18ngrep "^Writing objects" output
 '
 
 test_expect_success 'http push gives sane defaults to reflog' '
-- 
2.23.0.331.g4e51dcdf11


  reply	other threads:[~2019-09-16 20:54 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-22 14:20 v2.22.1 and later regression wrt display of progress indicators Charles Diza
2019-08-22 16:07 ` SZEDER Gábor
2019-08-22 16:29   ` Jeff King
2019-08-22 16:40     ` SZEDER Gábor
2019-08-22 17:04       ` Jeff King
2019-08-22 17:19     ` Taylor Blau
2019-09-16 20:54     ` [PATCH 0/2] Revert "progress: use term_clear_line()" SZEDER Gábor
2019-09-16 20:54       ` SZEDER Gábor [this message]
2019-09-16 20:54       ` [PATCH 2/2] Test the progress display SZEDER Gábor
2019-10-02 15:47       ` [PATCH 0/2] Revert "progress: use term_clear_line()" Jeff King
2019-08-22 17:16   ` v2.22.1 and later regression wrt display of progress indicators Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190916205412.8602-2-szeder.dev@gmail.com \
    --to=szeder.dev@gmail.com \
    --cc=chdiza@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).