Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work
- Date: Mon, 13 Nov 2017 12:05:05 -0800
- From: Elijah Newren <newren@xxxxxxxxx>
- Subject: Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work
Thanks for the reviews and suggestions!
On Sun, Nov 12, 2017 at 9:24 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Elijah Newren <newren@xxxxxxxxx> 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
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.
>> - 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
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.