Am 24.08.25 um 00:03 schrieb Johannes Sixt:
> That said, I am not very happy about the new calls introduced in
> display_progress(), either. I'll see whether I can produce some
> performance measurements.
I haven't made up my mind, yet, whether I want to persue the direction
any further. Since we do not have a low-latency implementation of
getnanotime() for all supported systems, calling the function tens of
thousands of times per second could be too burdensome for some of them.
> I observe a behavior change with delayed progress indicators that I have
> to understand and fix it before I can submit the cleaned up patches.
But I found a bug in the delayed progress indicators: the initial delay
time is always just one second instead of the configured time (two
seconds by default).
Below is a fix. This is a minimal patch. It could be extended to reduce
the number of local flag variables in display() and perhaps also reduce
indentation levels with some early returns.
If Carlo wants to advance the original patches, this patch also provides
an obvious point where the alarm clock can be re-armed outside the
signal handler. It would be where we set progress_update = 0.
----- 8< -----
Subject: [PATCH] progress: pay attention to (customized) delay time
Using one of the start_delayed_*() functions, clients of the progress
API can request that a progress meter is only shown after some time.
To do that, the implementation intends to count down the number of
seconds stored in struct progress by observing flag progress_update,
which the timer interrupt handler sets when a second has elapsed. This
works during the first second of the delay. But the code forgets to
reset the flag to zero, so that subsequent calls of display_progress()
think that another second has elapsed and decrease the count again
until zero is reached. Due to the frequency of the calls, this happens
without an observable delay in practice, so that the effective delay is
always just one second.
This bug has been with us since the inception of the feature. Despite
having been touched on various occasions, such as 8aade107dd84
(progress: simplify "delayed" progress API), 9c5951cacf5c (progress:
drop delay-threshold code), and 44a4693bfcec (progress: create
GIT_PROGRESS_DELAY), the short delay went unnoticed.
Copy the flag state into a local variable and reset the global flag
right away so that we can detect the next clock tick correctly.
Since we have not had any complaints that the delay of one second is
too short nor that GIT_PROGRESS_DELAY is ignored, people seem to be
comfortable with the status quo. Therefore, set the default to 1 to
keep the current behavior.
Signed-off-by: Johannes Sixt <j6t@xxxxxxxx>
---
Documentation/git.adoc | 2 +-
progress.c | 12 +++++++-----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/Documentation/git.adoc b/Documentation/git.adoc
index 743b7b00e4..03e9e69d25 100644
--- a/Documentation/git.adoc
+++ b/Documentation/git.adoc
@@ -684,7 +684,7 @@ other
`GIT_PROGRESS_DELAY`::
A number controlling how many seconds to delay before showing
- optional progress indicators. Defaults to 2.
+ optional progress indicators. Defaults to 1.
`GIT_EDITOR`::
This environment variable overrides `$EDITOR` and `$VISUAL`.
diff --git a/progress.c b/progress.c
index 8d5ae70f3a..39a1101420 100644
--- a/progress.c
+++ b/progress.c
@@ -114,16 +114,19 @@ static void display(struct progress *progress, uint64_t n, const char *done)
const char *tp;
struct strbuf *counters_sb = &progress->counters_sb;
int show_update = 0;
+ sig_atomic_t update = progress_update;
int last_count_len = counters_sb->len;
- if (progress->delay && (!progress_update || --progress->delay))
+ progress_update = 0;
+
+ if (progress->delay && (!update || --progress->delay))
return;
progress->last_value = n;
tp = (progress->throughput) ? progress->throughput->display.buf : "";
if (progress->total) {
unsigned percent = n * 100 / progress->total;
- if (percent != progress->last_percent || progress_update) {
+ if (percent != progress->last_percent || update) {
progress->last_percent = percent;
strbuf_reset(counters_sb);
@@ -133,7 +136,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)
tp);
show_update = 1;
}
- } else if (progress_update) {
+ } else if (update) {
strbuf_reset(counters_sb);
strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
show_update = 1;
@@ -166,7 +169,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)
}
fflush(stderr);
}
- progress_update = 0;
}
}
@@ -281,7 +283,7 @@ static int get_default_delay(void)
static int delay_in_secs = -1;
if (delay_in_secs < 0)
- delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 2);
+ delay_in_secs = git_env_ulong("GIT_PROGRESS_DELAY", 1);
return delay_in_secs;
}
--
2.51.0.205.g9a02ae2892
