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

Am 15.06.21 um 18:46 schrieb Ævar Arnfjörð Bjarmason:
> We disagree, and I for one think I understand what you mean, perhaps you
> don't understand what I mean, but let's try to move on.

Perhaps.  You seem to think of progress as being represented by a real
number.  We show integers, though, and you want to round up.

The progress display's function is to inform the user that work is being
done and how much there is still to do.  It allows them to decide
whether to keep the program running.

A progress of "100%" being shown for an extended duration would lead me
to the conclusion that the program hangs and I'd cancel it.  Rounding
down (truncating) prevents that.

Showing an estimated time of completion as well would be even nicer,
but is only practical if the time taken for each work item is roughly
the same.

But let's move on indeed.  The part of your patch that moves the
display_progress() call to the top of the loop to avoid stalling is a
good idea and worth splitting out into its own patch (keeping the "i").

In general it seems that changes described with "Let's also ..." or
"While at it ..." almost always deserve their own patch.  I need to
follow that insight more myself..

> I think it would be better if you replied specifically to the comments I
> had later about throughput progress bars, i.e.:
>     How does the idea that we show "has been done" make sense when you
>     combine the progress.c API with the display_throughput(). I.e. output
>     like[...]

Junio already replied to that, but since you ask, here are my thoughts:

Progress and throughput are separate metrics.  Adding one doesn't change
the other.  The throughput value is not specific to the currently
processed item.

Say we download a number of files of different sizes and want to show
our progress.  Then from time to time we display the number of processed
files and how many bytes we got since the last update, divided by the
time passed since then.  The reported bytes could belong to multiple
files.  Or we could process lots of zero-sized files, which would keep
throughput low.

> Anyway, in this case I understood you to mean that you thought the
> off-by-one wasn't a big deal in practice most of the time, I don't think
> so either for e.g. counting objects in pack files.

Not exactly.  While I think a difference of one isn't a big deal most
of the time, also think there is a correct way, i.e. to show the number
of completed items.  You have found ways to use an off-by-one error, and
my point was that this usage is not reliable.  Perhaps that's a weak
and convoluted argument.

> I do think it's useful to be consistent though, and for e.g. cases of
> downloading 5 files it makes sense to show 1/5 if we are currently in
> the process of downloading files 1 out of 5, not 0/5 or whatever.

I agree that we should be consistent.  If we have downloaded 70% of the
first of five files then we have 0.7 files, which is not yet 1 file, so
we have to say 0/5.

But let's move on, for real.


      reply	other threads:[~2021-06-20 12:56 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
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 [this message]

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 \ \ \ \ \ \ \ \

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