mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: "Ævar Arnfjörð Bjarmason" <>
Cc: "René Scharfe" <>,
	"Derrick Stolee" <>,, "Nguyễn Thái Ngọc Duy" <>
Subject: Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling
Date: Thu, 10 Jun 2021 14:30:48 +0900	[thread overview]
Message-ID: <xmqq4ke6ffqv.fsf@gitster.g> (raw)

Ævar Arnfjörð Bjarmason <> 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*].

> Why? Because:
>     /* 1. Setup progress */
>     large_number = 3;
>     progress = start_progress(title, total);
>     /* 2. Before we "actually" do anything */
>     for (i = 0; i < 3; i++) {
>         /* 3. Now doing O(large_number) work */
>         display_progress(progress, i + 1);
>     }
>     /* 4. Done */
>     stop_progress(&progress);
> As you'll see if you insert a sleep or whatever at "2" we'll signal and
> display the progress bar even if we haven't called display_progress()
> yet.

That is, the signal start_progress_delay() kicking in?  But
progress_test_force_update() is a curiosity that appears only in
test-tool and in real life programs, you'd have to call display
yourself, so a long delay at "3" will appear as a long silence,
I would think.

In any case, if we somehow managed to cause display_progress() to
happen somewhere near "2", we haven't finished even a single item
yet at that point, so "we are giving progress bar, and we have
finished 0%" that is an output from such a call would make sense.
As we finish one item, we'd increment it to 1 (that happens after we
spent time at "3" during the first iteration, and display_progress()
is called with "i+1").

> Thus calling display_progress with n=0 makes "we are doing the first
> item" indistinguishable from "we haven't gotten to the first item
> yet".

I am guessing that you are talking about "what value should we call
display() if '2' takes a long time and we want to give progress
early even before we enter the loop?"

I do not view such a call as "we haven't gotten to" or "we are
doing"; an extra call we would make with display(n=0) around "2" is
"how much have we finished?  we have completed none".

> When you're in the loop the first item should be n=1.

Doesn't that depend on where in the loop you do the work?

If you spend cycles and finish processing the first item _before_
calling display_progress(), you are reporting how many you have
finished, so at the end of the first iteration of the loop, you'd
call with n=1.

If on the other hand you somehow report at the beginning (perhaps
because otherwise you'd have tough time structuring the code when
the loop body has a continue etc.) before finishing the processing
for the first item, you'd call with n=0 (and make sure before
calling stop_progress(), you'd call another display() after exiting
the loop).

And I think that is consistent with what Réne suggested earlier, so
I am not sure where your "I am right you are wrong" is coming from.
To me, you two seem to be agreeing.


*1* <>

> Showing only the completed items makes sense.  That the next one is
> being processed is self-understood.  Once all of them are done, 100% is
> shown and the progress line is finished.
> So I think this pattern works:
> 	for (i = 0; i < nr; i++) {
> 		display_progress(p, i);
> 		/* work work work */
> 	}
> 	display_progress(p, nr);
> Alternatively, if the work part doesn't contain continue statements:
> 	for (i = 0; i < nr; i++) {
> 		/* work work work */
> 		display_progress(p, i + 1);
> 	}

  reply	other threads:[~2021-06-10  5:30 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 [this message]
2021-06-10 15:14                     ` René Scharfe
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:

  List information:

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

  git send-email \
    --in-reply-to=xmqq4ke6ffqv.fsf@gitster.g \ \ \ \ \ \ \

* 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

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).