Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 13, 2017, 05:24 UTC
- Message-ID
- <xmqqtvxy92ey.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20171110173956.25105-4-newren@gmail.com>
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/;
Show 9 quoted lines
> The possibility of setting merge.renameLimit beyond 2^16 raises the > possibility that the values passed to progress can exceed 2^32. For my > usecase of interest, I only needed to pass a value a little over 2^31. If > I were only interested in fixing my usecase, I could have simply changed > last_value from int to unsigned, and casted each of rename_dst_nr and > rename_src_nr (in merge-recursive.c) from int to unsigned just before > multiplying them. However, as long as we're making changes to allow > larger progress meters, we may as well make a little more room in general. > Use uint64_t, because it "ought to be enough for anybody". :-)
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).
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.
Show 11 quoted lines
> -static int display(struct progress *progress, unsigned n, const char *done)
> +static int display(struct progress *progress, uint64_t n, const char *done)
> {
> const char *eol, *tp;
>
> @@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned n, const char *done)
> if (percent != progress->last_percent || progress_update) {
> progress->last_percent = percent;
> if (is_foreground_fd(fileno(stderr)) || done) {
> - 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?