On Wed, Oct 10, 2018 at 11:56:45PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Oct 10 2018, SZEDER Gábor wrote:
> >> for (i = 0; i < oids->nr; i++) {
> >> + display_progress(progress, ++j);
>
> [...]
>
> > This display_progress() call, however, doesn't seem to be necessary.
> > First, it counts all commits for a second time, resulting in the ~2x
> > difference compared to the actual number of commits, and then causing
> > my confusion. Second, all what this loop is doing is setting a flag
> > in commits that were already looked up and parsed in the above loops.
> > IOW this loop is very fast, and the progress indicator jumps from
> > ~780k right to 1.5M, even on my tiny laptop, so it doesn't need a
> > progress indicator at all.
>
> You're right, I tried this patch on top:
[...]
> And on a large repo with around 3 million commits the 3rd progress bar
> doesn't kick in.
>
> But if I apply this on top:
>
[...]
>
> I.e. start the timer after 1/4 of a second instead of 1 second, I get
> that progress bar.
>
> So I'm inclined to keep it. It just needs to be 4x the size before it's
> noticeably hanging for 1 second.
Just to clarify: are you worried about a 1 second hang in an approx. 12
million commit repository? If so, then I'm unconvinced, that's not
even a blip on the radar, and the misleading numbers are far worse.
> That repo isn't all that big compared to what we've heard about out
> there, and inner loops like this have a tendency to accumulate some more
> code over time without a re-visit of why we weren't monitoring progress
> there.
>
> But maybe we can fix the message. We say "Annotating commits in commit
> grap", not "Counting" or whatever. I was trying to find something that
> didn't imply that we were doing this once. One can annotate a thing more
> than once, but maybe ther's a better way to explain this...
IMO just remove it.
