Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling

From: René Scharfe <l.s.r@web.de>
Date: Jun 10, 2021, 15:14 UTC
Message-ID: <f7a54b2d-42c6-1308-f1b0-2f67b2dcadc7@web.de>
In-Reply-To: <xmqq4ke6ffqv.fsf@gitster.g>

Am 10.06.21 um 07:30 schrieb Junio C Hamano:

Show 12 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I was too quick with that "But yes, I agree with the issue in theory".
>>
>> Having thought about it some more I think you're wrong, it doesn't make
>> sense to use the API in the way you're suggesting.
>
> Sorry, I've read the message I am responding to a few times, but I
> couldn't tell what you are arguing against in the suggestion given
> earlier by René, which offered two possibile ways to consistently
> call display() with the number of things that we have DONE
> processing (not the number of things that we are about to touch) [*1*].

Same here.

Perhaps a demo helps. The patch at the bottom adds an echo command to the test helper. This way we can intersperse the progress lines with indications when items are processed.

So here's the pattern for calling display_progress at the top of the loop and again with the number of items after the loop:

  $ (
    for i in 0 1 2
    do
      echo progress $i
      echo update
      echo echo WORK
    done
    echo progress 3
  ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'

  test:   0% (0/3)
  WORK
  test:  33% (1/3)
  WORK
  test:  66% (2/3)
  WORK
  test: 100% (3/3)
  test: 100% (3/3), done.

The progress lines reflect the number of finished items at all times.

Here's the pattern for display_progress at the bottom of the loop:

  $ (
    for i in 0 1 2
    do
      echo echo WORK
      echo progress $(( $i + 1 ))
      echo update
    done
  ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'

  WORK
  test:  33% (1/3)
  WORK
  test:  66% (2/3)
  WORK
  test: 100% (3/3)
  test: 100% (3/3), done.

Same here, the progress line shows the correct number of finished items.

Here's the pattern suggested in your patch:

  $ (
    for i in 0 1 2
    do
      echo progress $(( $i + 1 ))
      echo update
      echo echo WORK
    done
  ) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'

  test:  33% (1/3)
  WORK
  test:  66% (2/3)
  WORK
  test: 100% (3/3)
  WORK
  test: 100% (3/3), done.

It reports one item too many in the intermediate progress lines and is correct only at the very end.

René

diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 5d05cbe789..b6589f3878 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -65,6 +65,8 @@ int cmd__progress(int argc, const char **argv)
 			display_throughput(progress, byte_count);
 		} else if (!strcmp(line.buf, "update"))
 			progress_test_force_update();
+		else if (skip_prefix(line.buf, "echo ", (const char **) &end))
+			fprintf(stderr, "%s\n", end);
 		else
 			die("invalid input: '%s'\n", line.buf);
 	}