git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "Derrick Stolee" <stolee@gmail.com>,
	git@vger.kernel.org, "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling
Date: Thu, 10 Jun 2021 17:14:50 +0200	[thread overview]
Message-ID: <f7a54b2d-42c6-1308-f1b0-2f67b2dcadc7@web.de> (raw)
In-Reply-To: <xmqq4ke6ffqv.fsf@gitster.g>

Am 10.06.21 um 07:30 schrieb Junio C Hamano:
> Æ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);
 	}

  reply	other threads:[~2021-06-10 15:15 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-07 14:43 [PATCH 0/2] trivial progress.c API usage fixes Ævar Arnfjörð Bjarmason
2021-06-07 14:43 ` [PATCH 1/2] read-cache.c: don't guard calls to progress.c API Ævar Arnfjörð Bjarmason
2021-06-07 15:28   ` Derrick Stolee
2021-06-07 15:52     ` Ævar Arnfjörð Bjarmason
2021-06-07 16:11       ` Derrick Stolee
2021-06-07 14:43 ` [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling Ævar Arnfjörð Bjarmason
2021-06-07 15:31   ` Derrick Stolee
2021-06-07 15:58     ` Ævar Arnfjörð Bjarmason
2021-06-07 19:20       ` René Scharfe
2021-06-07 19:49         ` Ævar Arnfjörð Bjarmason
2021-06-07 23:41           ` Junio C Hamano
2021-06-08 10:58             ` Ævar Arnfjörð Bjarmason
2021-06-08 16:14               ` René Scharfe
2021-06-08 22:12                 ` Ævar Arnfjörð Bjarmason
2021-06-10  5:30                   ` Junio C Hamano
2021-06-10 15:14                     ` René Scharfe [this message]
2021-06-10 15:14                   ` René Scharfe
2021-06-14 11:07                     ` Ævar Arnfjörð Bjarmason
2021-06-14 17:18                       ` René Scharfe
2021-06-14 19:08                         ` Ævar Arnfjörð Bjarmason
2021-06-15  2:32                           ` Junio C Hamano
2021-06-15 15:14                           ` René Scharfe
2021-06-15 16:46                             ` Ævar Arnfjörð Bjarmason
2021-06-20 12:53                               ` René Scharfe

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=f7a54b2d-42c6-1308-f1b0-2f67b2dcadc7@web.de \
    --to=l.s.r@web.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=stolee@gmail.com \
    /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).