Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work

From: Elijah Newren <newren@gmail.com>
Date: Nov 13, 2017, 20:05 UTC
Message-ID: <CABPp-BH1Cpc9UfYpmBBAHWSqadg=QuD=28qx1oV29ZdvF4NbJw@mail.gmail.com>
In-Reply-To: <xmqqtvxy92ey.fsf@gitster.mtv.corp.google.com>

Thanks for the reviews and suggestions!

On Sun, Nov 12, 2017 at 9:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> Elijah Newren <newren@gmail.com> writes:
>
>> Subject: Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work
>
> Style: s/Fix/fix/;

I also messed this up in a lot of my patches in my other patch series. I've fixed them all up, but I'll wait to resubmit those other series until I get some other reviews.

> The middle part of the log message may waste more mental bandwidth
> of readers than it is worth.  It might have gave you satisfaction to
> be able to vent, but don't (the place to do so is after the three
> dash lines).

Cleaned it up, along with the other commit message you pointed out; I'll resubmit shortly.

> I am not sure if we want all codepaths to do 64-bit math for
> progress meter, but let's see what others would think.

If others don't want to do 64-bit math for the progress meter, what
would they like to see done instead?  I can see a few options:
  1) Have two separate progress codepaths, one for 32-bith math and
one for 64-bit math.
  2) Instead of counting pairs of source/dest files compared, just
count number of dest paths completed.  (Thus, we wouldn't need a value
big enough to hold rename_dst_nr * rename_src_nr, just big enough to
hold rename_dst_nr).
  3) just let the progress meter overflow and show nonsensical values
  4) don't show the progress meter if overflow would happen
  5) something else I'm not thinking of.

Show 5 quoted lines

>> -                             fprintf(stderr, "%s: %3u%% (%u/%u)%s%s",
>> +                             fprintf(stderr, "%s: %3u%% (%lu/%lu)%s%s",
>
> Are these (and there are probably other instances in this patch) %lu
> correct?

Oops, no. I think %llu is right, though looking around the code it appears folks use PRIuMAX and avoid %llu due to possible issues with old windows compilers. Not sure if that's still relevant, but I'll try to remain consistent with what I see elsewhere and include that fix in my re-roll.