[v3,5/5] progress: use term_clear_line()
diff mbox series

Message ID 20190624181318.17388-6-szeder.dev@gmail.com
State New
Headers show
Series
  • rebase/progress: add and use term_clear_line()
Related show

Commit Message

SZEDER Gábor June 24, 2019, 6:13 p.m. UTC
To make sure that the previously displayed progress line is completely
covered up when the new line is shorter, commit 545dc345eb (progress:
break too long progress bar lines, 2019-04-12) added a bunch of
calculations to figure out how many characters it needs to overwrite
with spaces.

Use the just introduced term_clear_line() helper function to, well,
clear the last line, making all these calculations unnecessary, and
thus simplifying the code considerably.

Three tests in 't5541-http-push-smart.sh' 'grep' for specific text
shown in the progress lines at the beginning of the line, but now
those lines begin either with the ANSI escape sequence or with the
terminal width worth of space characters clearing the line.  Relax the
'grep' patterns to match anywhere on the line.  Note that only two of
these three tests fail without relaxing their 'grep' pattern, but the
third looks for the absence of the pattern, so it still succeeds, but
without the adjustment would potentially hide future regressions.

Note also that with this change we no longer need the length of the
previously displayed progress line, so the strbuf added to 'struct
progress' in d53ba841d4 (progress: assemble percentage and counters in
a strbuf before printing, 2019-04-05) is not strictly necessary
anymore.  We still keep it, though, as it avoids allocating and
releasing a strbuf each time the progress is updated.

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

Comments

Johannes Schindelin June 25, 2019, 9:10 a.m. UTC | #1
Hi Gábor,

On Mon, 24 Jun 2019, SZEDER Gábor wrote:

> To make sure that the previously displayed progress line is completely
> covered up when the new line is shorter, commit 545dc345eb (progress:
> break too long progress bar lines, 2019-04-12) added a bunch of
> calculations to figure out how many characters it needs to overwrite
> with spaces.
>
> Use the just introduced term_clear_line() helper function to, well,
> clear the last line, making all these calculations unnecessary, and
> thus simplifying the code considerably.
>
> Three tests in 't5541-http-push-smart.sh' 'grep' for specific text
> shown in the progress lines at the beginning of the line, but now
> those lines begin either with the ANSI escape sequence or with the
> terminal width worth of space characters clearing the line.  Relax the
> 'grep' patterns to match anywhere on the line.  Note that only two of
> these three tests fail without relaxing their 'grep' pattern, but the
> third looks for the absence of the pattern, so it still succeeds, but
> without the adjustment would potentially hide future regressions.
>
> Note also that with this change we no longer need the length of the
> previously displayed progress line, so the strbuf added to 'struct
> progress' in d53ba841d4 (progress: assemble percentage and counters in
> a strbuf before printing, 2019-04-05) is not strictly necessary
> anymore.  We still keep it, though, as it avoids allocating and
> releasing a strbuf each time the progress is updated.

Very nice change, indeed, I totally love how much simpler the post-image
looks. Well done, for the entire patch series, and thank you so much!
Dscho

>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  progress.c                 | 28 +++++++++++-----------------
>  t/t5541-http-push-smart.sh |  6 +++---
>  2 files changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index a2e8cf64a8..095dcd0ddf 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -88,7 +88,6 @@ 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;
> @@ -116,26 +115,21 @@ 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;
> -			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,
> -					(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);
> +				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);
>  				progress->split = 1;
>  			} else {
> -				fprintf(stderr, "%s: %s%*s", progress->title,
> -					counters_sb->buf, (int) clear_len, eol);
> +				fprintf(stderr, "%s: %s%s", progress->title,
> +					counters_sb->buf, eol);
>  			}
>  			fflush(stderr);
>  		}
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index 8ef8763e06..2e4802e206 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -213,7 +213,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' '
> @@ -228,7 +228,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' '
> @@ -236,7 +236,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.22.0.589.g5bd7971b91
>
>
Junio C Hamano June 25, 2019, 7:44 p.m. UTC | #2
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> To make sure that the previously displayed progress line is completely
>> covered up when the new line is shorter, commit 545dc345eb (progress:
>> break too long progress bar lines, 2019-04-12) added a bunch of
>> calculations to figure out how many characters it needs to overwrite
>> with spaces.
>> ...
> Very nice change, indeed, I totally love how much simpler the post-image
> looks. Well done, for the entire patch series, and thank you so much!
> Dscho

Thanks, both.  Let's merge down sg/rebase-progress soonish.

Patch
diff mbox series

diff --git a/progress.c b/progress.c
index a2e8cf64a8..095dcd0ddf 100644
--- a/progress.c
+++ b/progress.c
@@ -88,7 +88,6 @@  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;
@@ -116,26 +115,21 @@  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;
-			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,
-					(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);
+				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);
 				progress->split = 1;
 			} else {
-				fprintf(stderr, "%s: %s%*s", progress->title,
-					counters_sb->buf, (int) clear_len, eol);
+				fprintf(stderr, "%s: %s%s", progress->title,
+					counters_sb->buf, eol);
 			}
 			fflush(stderr);
 		}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ef8763e06..2e4802e206 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -213,7 +213,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' '
@@ -228,7 +228,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' '
@@ -236,7 +236,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' '