diff mbox series

[18/25] progress.c: emit progress on first signal, show "stalled"

Message ID patch-18.25-e21fc66623f-20210623T155626Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series progress.c: various fixes + SZEDER's RFC code | expand

Commit Message

Ævar Arnfjörð Bjarmason June 23, 2021, 5:48 p.m. UTC 
Ever since the progress.c code was added in 96a02f8f6d2 (common
progress display support, 2007-04-18) we have been driven purely by
calls to the display() function (via the public display_progress()),
or via stop_progress(). Even though we got a signal and invoked
progress_interval() that function would not actually emit progress
output for us.

Thus in cases like "git gc" we don't emit any "Enumerating Objects"
output until we get past the setup code, and start enumerating
objects, we'll now (at least on my laptop) show output earlier, and
emit a "stalled" message before we start the count.

But more generally, this is a first step towards never showing a
hanging progress bar from the user's perspective. If we're truly
taking a very long time with one item we can show some spinner that we
update every time we get a signal. We don't right now, and only
special-case the most common case of hanging before we get to the
first item.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 progress.c                  |  7 +++++
 t/t0500-progress-display.sh | 63 ++++++++++++++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 4 deletions(-)

Comments

SZEDER Gábor Sept. 16, 2021, 6:37 p.m. UTC | #1 
On Wed, Jun 23, 2021 at 07:48:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Ever since the progress.c code was added in 96a02f8f6d2 (common
> progress display support, 2007-04-18) we have been driven purely by
> calls to the display() function (via the public display_progress()),
> or via stop_progress(). Even though we got a signal and invoked
> progress_interval() that function would not actually emit progress
> output for us.
> 
> Thus in cases like "git gc" we don't emit any "Enumerating Objects"
> output until we get past the setup code, and start enumerating
> objects, we'll now (at least on my laptop) show output earlier, and
> emit a "stalled" message before we start the count.
> 
> But more generally, this is a first step towards never showing a
> hanging progress bar from the user's perspective. If we're truly
> taking a very long time with one item we can show some spinner that we
> update every time we get a signal. We don't right now, and only
> special-case the most common case of hanging before we get to the
> first item.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  progress.c                  |  7 +++++
>  t/t0500-progress-display.sh | 63 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/progress.c b/progress.c
> index 6c4038df791..35847d3a7f2 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -255,6 +255,13 @@ void display_progress(struct progress *progress, uint64_t n)
>  static void progress_interval(int signum)
>  {
>  	progress_update = 1;
> +
> +	if (global_progress->last_value != -1)
> +		return;
> +
> +	display(global_progress, 0, _(", stalled."), 0);

We have a few progress lines that are updated from multiple threads,
and to prevent concurrency issues those threads call display() while
holding a mutex.  This call without synchronization causes undefined
behavior.
diff mbox series

Patch

diff --git a/progress.c b/progress.c
index 6c4038df791..35847d3a7f2 100644
--- a/progress.c
+++ b/progress.c
@@ -255,6 +255,13 @@  void display_progress(struct progress *progress, uint64_t n)
 static void progress_interval(int signum)
 {
 	progress_update = 1;
+
+	if (global_progress->last_value != -1)
+		return;
+
+	display(global_progress, 0, _(", stalled."), 0);
+	progress_update = 1;
+	return;
 }
 
 void test_progress_force_update(void)
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 883e044fe64..bc458cfc28b 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -15,7 +15,8 @@  test_expect_success 'setup COLUMNS' '
 
 test_expect_success 'simple progress display' '
 	cat >expect <<-\EOF &&
-	Working hard: 1<CR>
+	Working hard: 0, stalled.<CR>
+	Working hard: 1          <CR>
 	Working hard: 2<CR>
 	Working hard: 5<CR>
 	Working hard: 5, done.
@@ -60,6 +61,57 @@  test_expect_success 'progress display with total' '
 	test_cmp expect out
 '
 
+test_expect_success 'stalled progress display' '
+	cat >expect <<-\EOF &&
+	Working hard:   0% (0/3), stalled.<CR>
+	Working hard:  33% (1/3)          <CR>
+	Working hard:  66% (2/3)<CR>
+	Working hard: 100% (3/3)<CR>
+	Working hard: 100% (3/3), done.
+	EOF
+
+	cat >in <<-\EOF &&
+	start 3
+	signal
+	signal
+	signal
+	progress 1
+	signal
+	update
+	signal
+	progress 2
+	update
+	progress 3
+	stop
+	EOF
+	STALLED=1 test-tool progress <in 2>stderr &&
+
+	show_cr <stderr >out &&
+	test_cmp expect out
+'
+
+test_expect_success 'progress display breaks long lines #0, stalled' '
+	sed -e "s/Z$//" >expect <<\EOF &&
+Working hard.......2.........3.........4.........5.........6.........7:
+    0% (0/100), stalled.<CR>
+    1% (1/100)          <CR>
+   50% (50/100)<CR>
+   50% (50/100), done.
+EOF
+
+	cat >in <<-\EOF &&
+	start 100 Working hard.......2.........3.........4.........5.........6.........7
+	signal
+	progress 1
+	progress 50
+	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>
@@ -183,7 +235,8 @@  test_expect_success 'progress shortens - crazy caller' '
 
 test_expect_success 'progress display with throughput' '
 	cat >expect <<-\EOF &&
-	Working hard: 10<CR>
+	Working hard: 0, stalled.<CR>
+	Working hard: 10         <CR>
 	Working hard: 20, 200.00 KiB | 100.00 KiB/s<CR>
 	Working hard: 30, 300.00 KiB | 100.00 KiB/s<CR>
 	Working hard: 40, 400.00 KiB | 100.00 KiB/s<CR>
@@ -241,7 +294,8 @@  test_expect_success 'progress display with throughput and total' '
 
 test_expect_success 'cover up after throughput shortens' '
 	cat >expect <<-\EOF &&
-	Working hard: 1<CR>
+	Working hard: 0, stalled.<CR>
+	Working hard: 1          <CR>
 	Working hard: 2, 800.00 KiB | 400.00 KiB/s<CR>
 	Working hard: 3, 1.17 MiB | 400.00 KiB/s  <CR>
 	Working hard: 4, 1.56 MiB | 400.00 KiB/s<CR>
@@ -272,7 +326,8 @@  test_expect_success 'cover up after throughput shortens' '
 
 test_expect_success 'cover up after throughput shortens a lot' '
 	cat >expect <<-\EOF &&
-	Working hard: 1<CR>
+	Working hard: 0, stalled.<CR>
+	Working hard: 1          <CR>
 	Working hard: 2, 1000.00 KiB | 1000.00 KiB/s<CR>
 	Working hard: 3, 3.00 MiB | 1.50 MiB/s      <CR>
 	Working hard: 3, 3.00 MiB | 1024.00 KiB/s, done.