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