[PATCH 0/2] progress: replace setitimer() with alarm()

[PATCH 0/2] progress: replace setitimer() with alarm()

[Carlo Marcelo Arenas Belón via GitGitGadget]

The first patch does the minimum changes required to swap the underlying
function, but introduce a race condition that is addressed in the second
patch.

A third patch that does further changes to the Windows compatibility layer
was punted.

Carlo Marcelo Arenas Belón (2): progress: replace setitimer() with alarm()
progress: add a shutting down state to the SIGALRM handler

Makefile | 12 ------------ compat/mingw-posix.h | 9 +-------- compat/mingw.c
| 46 ++++++++++++++++---------------------------- compat/posix.h | 17
---------------- configure.ac | 13 ------------- meson.build | 16
--------------- progress.c | 29 +++++++++++++++------------- 7 files
changed, 34 insertions(+), 108 deletions(-)

Carlo Marcelo Arenas Belón (2):
  progress: replace setitimer() with alarm()
  progress: add a shutting down state to the SIGALRM handler

 Makefile             | 12 ------------
 compat/mingw-posix.h |  9 +--------
 compat/mingw.c       | 46 ++++++++++++++++----------------------------
 compat/posix.h       | 17 ----------------
 configure.ac         | 13 -------------
 meson.build          | 16 ---------------
 progress.c           | 29 +++++++++++++++-------------
 7 files changed, 34 insertions(+), 108 deletions(-)


base-commit: 1fa68948c3d76328236cac73d2adf33c905bd8e3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1960%2Fcarenas%2Fnoitimer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1960/carenas/noitimer-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1960
-- 
gitgitgadget

[PATCH 1/2] progress: replace setitimer() with alarm()

[Carlo Marcelo Arenas Belón via GitGitGadget]

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>

setitimer() was marked obsolescent by IEEE Std 1003.1-2017 and is
no longer available in the latest edition.

While other alternatives of high fidelity are available, its use
in progress only requires whole second intervals, so it can be
cleanly replaced by a simpler alarm(); do that.

MinGW provided its own version of setitimer() so add an equivalent
implementation for alarm(), but do the minimum changes required in
the timer thread to adapt to the new interface.

Remove the compatibility layer for setitimer() and its related
struct that are no longer needed, and adjust the build system.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Makefile             | 12 ------------
 compat/mingw-posix.h |  9 +--------
 compat/mingw.c       | 46 ++++++++++++++++----------------------------
 compat/posix.h       | 17 ----------------
 configure.ac         | 13 -------------
 meson.build          | 16 ---------------
 progress.c           | 11 +++--------
 7 files changed, 21 insertions(+), 103 deletions(-)

diff --git a/Makefile b/Makefile
index e11340c1ae77..059382624c0e 100644
--- a/Makefile
+++ b/Makefile
@@ -142,11 +142,6 @@ include shared.mak
 # Define NO_PREAD if you have a problem with pread() system call (e.g.
 # cygwin1.dll before v1.5.22).
 #
-# Define NO_SETITIMER if you don't have setitimer()
-#
-# Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval
-# This also implies NO_SETITIMER
-#
 # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
 # generally faster on your platform than accessing the working directory.
 #
@@ -1888,13 +1883,6 @@ endif
 ifdef OBJECT_CREATION_USES_RENAMES
 	COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
 endif
-ifdef NO_STRUCT_ITIMERVAL
-	COMPAT_CFLAGS += -DNO_STRUCT_ITIMERVAL
-	NO_SETITIMER = YesPlease
-endif
-ifdef NO_SETITIMER
-	COMPAT_CFLAGS += -DNO_SETITIMER
-endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
index 631a20868489..7626fbe90172 100644
--- a/compat/mingw-posix.h
+++ b/compat/mingw-posix.h
@@ -98,11 +98,6 @@ struct sigaction {
 #define SA_RESTART 0
 #define SA_NOCLDSTOP 1
 
-struct itimerval {
-	struct timeval it_value, it_interval;
-};
-#define ITIMER_REAL 0
-
 struct utsname {
 	char sysname[16];
 	char nodename[1];
@@ -131,8 +126,6 @@ static inline int fchmod(int fildes UNUSED, mode_t mode UNUSED)
 static inline pid_t fork(void)
 { errno = ENOSYS; return -1; }
 #endif
-static inline unsigned int alarm(unsigned int seconds UNUSED)
-{ return 0; }
 static inline int fsync(int fd)
 { return _commit(fd); }
 static inline void sync(void)
@@ -183,6 +176,7 @@ char *mingw_locate_in_PATH(const char *cmd);
  * implementations of missing functions
  */
 
+unsigned alarm(unsigned seconds);
 int pipe(int filedes[2]);
 unsigned int sleep (unsigned int seconds);
 int mkstemp(char *template);
@@ -193,7 +187,6 @@ struct tm *localtime_r(const time_t *timep, struct tm *result);
 #endif
 int getpagesize(void);	/* defined in MinGW's libgcc.a */
 struct passwd *getpwuid(uid_t uid);
-int setitimer(int type, struct itimerval *in, struct itimerval *out);
 int sigaction(int sig, struct sigaction *in, struct sigaction *out);
 int link(const char *oldpath, const char *newpath);
 int uname(struct utsname *buf);
diff --git a/compat/mingw.c b/compat/mingw.c
index 8538e3d1729d..199f68ca6d9a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2432,15 +2432,17 @@ static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;
  * the thread to terminate by setting the timer_event to the signalled
  * state.
  * But ticktack() interrupts the wait state after the timer's interval
- * length to call the signal handler.
+ * length to call the signal handler if it still set.
  */
 
 static unsigned __stdcall ticktack(void *dummy UNUSED)
 {
+	sig_handler_t fn = timer_fn;
+
 	while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
-		mingw_raise(SIGALRM);
-		if (one_shot)
+		if (one_shot || timer_fn != fn)
 			break;
+		mingw_raise(SIGALRM);
 	}
 	return 0;
 }
@@ -2478,37 +2480,23 @@ static void stop_timer_thread(void)
 	timer_thread = NULL;
 }
 
-static inline int is_timeval_eq(const struct timeval *i1, const struct timeval *i2)
+unsigned alarm(unsigned seconds)
 {
-	return i1->tv_sec == i2->tv_sec && i1->tv_usec == i2->tv_usec;
-}
-
-int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)
-{
-	static const struct timeval zero;
-	static int atexit_done;
-
-	if (out)
-		return errno = EINVAL,
-			error("setitimer param 3 != NULL not implemented");
-	if (!is_timeval_eq(&in->it_interval, &zero) &&
-	    !is_timeval_eq(&in->it_interval, &in->it_value))
-		return errno = EINVAL,
-			error("setitimer: it_interval must be zero or eq it_value");
-
-	if (timer_thread)
-		stop_timer_thread();
+	static bool atexit_done;
 
-	if (is_timeval_eq(&in->it_value, &zero) &&
-	    is_timeval_eq(&in->it_interval, &zero))
-		return 0;
-
-	timer_interval = in->it_value.tv_sec * 1000 + in->it_value.tv_usec / 1000;
-	one_shot = is_timeval_eq(&in->it_interval, &zero);
 	if (!atexit_done) {
 		atexit(stop_timer_thread);
-		atexit_done = 1;
+		atexit_done = true;
 	}
+
+	timer_interval = seconds * 1000;
+	one_shot = !seconds;
+	if (timer_thread) {
+		if (!seconds)
+			stop_timer_thread();
+		return 0;
+	}
+
 	return start_timer_thread();
 }
 
diff --git a/compat/posix.h b/compat/posix.h
index 067a00f33b83..83af92d820f2 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -198,23 +198,6 @@ static inline time_t git_time(time_t *tloc)
 }
 #define time git_time
 
-#ifdef NO_STRUCT_ITIMERVAL
-struct itimerval {
-	struct timeval it_interval;
-	struct timeval it_value;
-};
-#endif
-
-#ifdef NO_SETITIMER
-static inline int git_setitimer(int which UNUSED,
-				const struct itimerval *value UNUSED,
-				struct itimerval *newvalue UNUSED) {
-	return 0; /* pretend success */
-}
-#undef setitimer
-#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)
-#endif
-
 #ifndef NO_LIBGEN_H
 #include <libgen.h>
 #else
diff --git a/configure.ac b/configure.ac
index cfb50112bf81..6b04aa37a82c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -872,13 +872,6 @@ case $ac_cv_type_socklen_t in
 esac
 GIT_CONF_SUBST([SOCKLEN_T])
 
-#
-# Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval.
-AC_CHECK_TYPES([struct itimerval],
-[NO_STRUCT_ITIMERVAL=],
-[NO_STRUCT_ITIMERVAL=UnfortunatelyYes],
-[#include <sys/time.h>])
-GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])
 #
 # Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exists.
 # Define NO_NSEC=YesPlease when neither stat.st_mtim.tv_nsec nor
@@ -1097,12 +1090,6 @@ GIT_CHECK_FUNC(sync_file_range,
 	[HAVE_SYNC_FILE_RANGE=])
 GIT_CONF_SUBST([HAVE_SYNC_FILE_RANGE])
 
-#
-# Define NO_SETITIMER if you don't have setitimer.
-GIT_CHECK_FUNC(setitimer,
-[NO_SETITIMER=],
-[NO_SETITIMER=YesPlease])
-GIT_CONF_SUBST([NO_SETITIMER])
 #
 # Define NO_STRCASESTR if you don't have strcasestr.
 GIT_CHECK_FUNC(strcasestr,
diff --git a/meson.build b/meson.build
index 5dd299b4962d..f33f20312511 100644
--- a/meson.build
+++ b/meson.build
@@ -1348,22 +1348,6 @@ else
     error('Native regex support requested but not found')
 endif
 
-# setitimer and friends are provided by compat/mingw.c.
-if host_machine.system() != 'windows'
-  if not compiler.compiles('''
-    #include <sys/time.h>
-    void func(void)
-    {
-      struct itimerval value;
-    }
-  ''', name: 'struct itimerval')
-    libgit_c_args += '-DNO_STRUCT_ITIMERVAL'
-    libgit_c_args += '-DNO_SETITIMER'
-  elif not compiler.has_function('setitimer')
-    libgit_c_args += '-DNO_SETITIMER'
-  endif
-endif
-
 if compiler.has_member('struct stat', 'st_mtimespec.tv_nsec', prefix: '#include <sys/stat.h>')
   libgit_c_args += '-DUSE_ST_TIMESPEC'
 elif not compiler.has_member('struct stat', 'st_mtim.tv_nsec', prefix: '#include <sys/stat.h>')
diff --git a/progress.c b/progress.c
index 8d5ae70f3a9e..71b305d1625d 100644
--- a/progress.c
+++ b/progress.c
@@ -67,12 +67,12 @@ void progress_test_force_update(void)
 static void progress_interval(int signum UNUSED)
 {
 	progress_update = 1;
+	alarm(1);
 }
 
 static void set_progress_signal(void)
 {
 	struct sigaction sa;
-	struct itimerval v;
 
 	if (progress_testing)
 		return;
@@ -85,20 +85,15 @@ static void set_progress_signal(void)
 	sa.sa_flags = SA_RESTART;
 	sigaction(SIGALRM, &sa, NULL);
 
-	v.it_interval.tv_sec = 1;
-	v.it_interval.tv_usec = 0;
-	v.it_value = v.it_interval;
-	setitimer(ITIMER_REAL, &v, NULL);
+	alarm(1);
 }
 
 static void clear_progress_signal(void)
 {
-	struct itimerval v = {{0,},};
-
 	if (progress_testing)
 		return;
 
-	setitimer(ITIMER_REAL, &v, NULL);
+	alarm(0);
 	signal(SIGALRM, SIG_IGN);
 	progress_update = 0;
 }
-- 
gitgitgadget

[PATCH 2/2] progress: add a shutting down state to the SIGALRM handler

[Carlo Marcelo Arenas Belón via GitGitGadget]

From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>

In a previous commit, sigitimer() was replaced by alarm(), but to
keep the timer active, an extra call to `alarm(1)` was added to
the signal handler, opening a potential race condition whem the
timer is being cleared.

To avoid that, add an extra state to set during shutdown and
adjust the logic to flag the potential need to update progress
into the first bit instead.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 progress.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/progress.c b/progress.c
index 71b305d1625d..49e58e094a3f 100644
--- a/progress.c
+++ b/progress.c
@@ -50,6 +50,11 @@ struct progress {
 	int split;
 };
 
+/*
+ * 0: no progress to report
+ * 1: potential update for progress to report
+ * 2: no more progress to report
+ */
 static volatile sig_atomic_t progress_update;
 
 /*
@@ -66,8 +71,10 @@ void progress_test_force_update(void)
 
 static void progress_interval(int signum UNUSED)
 {
-	progress_update = 1;
-	alarm(1);
+	if (progress_update != 2) {
+		alarm(1);
+		progress_update = 1;
+	}
 }
 
 static void set_progress_signal(void)
@@ -93,6 +100,7 @@ static void clear_progress_signal(void)
 	if (progress_testing)
 		return;
 
+	progress_update = 2;
 	alarm(0);
 	signal(SIGALRM, SIG_IGN);
 	progress_update = 0;
@@ -111,14 +119,14 @@ static void display(struct progress *progress, uint64_t n, const char *done)
 	int show_update = 0;
 	int last_count_len = counters_sb->len;
 
-	if (progress->delay && (!progress_update || --progress->delay))
+	if (progress->delay && (!(progress_update & 1) || --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 || (progress_update & 1)) {
 			progress->last_percent = percent;
 
 			strbuf_reset(counters_sb);
@@ -128,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 (progress_update & 1) {
 		strbuf_reset(counters_sb);
 		strbuf_addf(counters_sb, "%"PRIuMAX"%s", (uintmax_t)n, tp);
 		show_update = 1;
@@ -239,7 +247,7 @@ void display_throughput(struct progress *progress, uint64_t total)
 	tp->idx = (tp->idx + 1) % TP_IDX_MAX;
 
 	throughput_string(&tp->display, total, rate);
-	if (progress->last_value != -1 && progress_update)
+	if (progress->last_value != -1 && (progress_update & 1))
 		display(progress, progress->last_value, NULL);
 }
 
-- 
gitgitgadget

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Johannes Sixt]

Am 23.08.25 um 15:22 schrieb Carlo Marcelo Arenas Belón via GitGitGadget:
> The first patch does the minimum changes required to swap the underlying
> function, but introduce a race condition that is addressed in the second
> patch.
> 
> A third patch that does further changes to the Windows compatibility layer
> was punted.
> 
> Carlo Marcelo Arenas Belón (2): progress: replace setitimer() with alarm()
> progress: add a shutting down state to the SIGALRM handler
> 
After having looked at the progress code for a bit, see this:

We use SIGALRM to raise a flag that tells the progress code to act in
some way. The progress code does not act asynchronously, but only when
there is an opportunity to look at the flag, i.e., it acts synchronously
in response to a third party (SIGALRM) that told it that it's time to
act.

But we can change the progress code to do the time keeping itself.
Instead of looking whether a flag was raised, we can let it look at the
wall clock and check whether an interval has elapsed.

A prototype implementation looks like the patch below. It works quite
well for `git clone` on Linux. I have even lowered the interval to 1/8th
of a second to get more frequent updates. After a change like this, we
can remove all the creepy SIGALRM/alarm/setitimer stuff.

What do you think?

 progress.c               | 65 +++++++++++-----------------------------
 progress.h               |  2 +-
 t/helper/test-progress.c |  2 +-
 3 files changed, 20 insertions(+), 49 deletions(-)

diff --git a/progress.c b/progress.c
index 8d5ae70f3a..eb1d2f14e0 100644
--- a/progress.c
+++ b/progress.c
@@ -38,6 +38,7 @@ struct throughput {
 struct progress {
 	struct repository *repo;
 	const char *title;
+	uint64_t last_update;
 	uint64_t last_value;
 	uint64_t total;
 	unsigned last_percent;
@@ -50,57 +51,26 @@ struct progress {
 	int split;
 };
 
-static volatile sig_atomic_t progress_update;
-
 /*
  * These are only intended for testing the progress output, i.e. exclusively
  * for 'test-tool progress'.
  */
 int progress_testing;
 uint64_t progress_test_ns = 0;
-void progress_test_force_update(void)
+void progress_test_force_update(struct progress *progress)
 {
-	progress_update = 1;
+	progress->last_update = 0;
 }
 
 
-static void progress_interval(int signum UNUSED)
+static bool check_update_delay_expired(struct progress *progress)
 {
-	progress_update = 1;
-}
-
-static void set_progress_signal(void)
-{
-	struct sigaction sa;
-	struct itimerval v;
-
-	if (progress_testing)
-		return;
-
-	progress_update = 0;
-
-	memset(&sa, 0, sizeof(sa));
-	sa.sa_handler = progress_interval;
-	sigemptyset(&sa.sa_mask);
-	sa.sa_flags = SA_RESTART;
-	sigaction(SIGALRM, &sa, NULL);
-
-	v.it_interval.tv_sec = 1;
-	v.it_interval.tv_usec = 0;
-	v.it_value = v.it_interval;
-	setitimer(ITIMER_REAL, &v, NULL);
-}
-
-static void clear_progress_signal(void)
-{
-	struct itimerval v = {{0,},};
-
-	if (progress_testing)
-		return;
-
-	setitimer(ITIMER_REAL, &v, NULL);
-	signal(SIGALRM, SIG_IGN);
-	progress_update = 0;
+	uint64_t now = getnanotime();
+	/* about every 1/8th of a second */
+	bool update = (now - progress->last_update) / (1024 * 1024 * 1024 / 8) > 0;
+	if (update)
+		progress->last_update = now;
+	return update;
 }
 
 static int is_foreground_fd(int fd)
@@ -109,7 +79,8 @@ static int is_foreground_fd(int fd)
 	return tpgrp < 0 || tpgrp == getpgid(0);
 }
 
-static void display(struct progress *progress, uint64_t n, const char *done)
+static void display(struct progress *progress, uint64_t n, const char *done,
+		    bool progress_update)
 {
 	const char *tp;
 	struct strbuf *counters_sb = &progress->counters_sb;
@@ -243,15 +214,16 @@ void display_throughput(struct progress *progress, uint64_t total)
 	tp->last_misecs[tp->idx] = misecs;
 	tp->idx = (tp->idx + 1) % TP_IDX_MAX;
 
+	bool progress_update = check_update_delay_expired(progress);
 	throughput_string(&tp->display, total, rate);
 	if (progress->last_value != -1 && progress_update)
-		display(progress, progress->last_value, NULL);
+		display(progress, progress->last_value, NULL, progress_update);
 }
 
 void display_progress(struct progress *progress, uint64_t n)
 {
 	if (progress)
-		display(progress, n, NULL);
+		display(progress, n, NULL, check_update_delay_expired(progress));
 }
 
 static struct progress *start_progress_delay(struct repository *r,
@@ -262,6 +234,7 @@ static struct progress *start_progress_delay(struct repository *r,
 	progress->repo = r;
 	progress->title = title;
 	progress->total = total;
+	progress->last_update = getnanotime();
 	progress->last_value = -1;
 	progress->last_percent = -1;
 	progress->delay = delay;
@@ -271,7 +244,6 @@ static struct progress *start_progress_delay(struct repository *r,
 	strbuf_init(&progress->counters_sb, 0);
 	progress->title_len = utf8_strwidth(title);
 	progress->split = 0;
-	set_progress_signal();
 	trace2_region_enter("progress", title, r);
 	return progress;
 }
@@ -339,9 +311,9 @@ static void force_last_update(struct progress *progress, const char *msg)
 		rate = tp->curr_total / (misecs ? misecs : 1);
 		throughput_string(&tp->display, tp->curr_total, rate);
 	}
-	progress_update = 1;
+	progress->last_update = 0;
 	buf = xstrfmt(", %s.\n", msg);
-	display(progress, progress->last_value, buf);
+	display(progress, progress->last_value, buf, check_update_delay_expired(progress));
 	free(buf);
 }
 
@@ -374,7 +346,6 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 		force_last_update(progress, msg);
 	log_trace2(progress);
 
-	clear_progress_signal();
 	strbuf_release(&progress->counters_sb);
 	if (progress->throughput)
 		strbuf_release(&progress->throughput->display);
diff --git a/progress.h b/progress.h
index ed068c7bab..cca479bd26 100644
--- a/progress.h
+++ b/progress.h
@@ -9,7 +9,7 @@ struct repository;
 
 extern int progress_testing;
 extern uint64_t progress_test_ns;
-void progress_test_force_update(void);
+void progress_test_force_update(struct progress *progress);
 
 #endif
 
diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 1f75b7bd19..e2ca5fb726 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -87,7 +87,7 @@ int cmd__progress(int argc, const char **argv)
 			progress_test_ns = test_ms * 1000 * 1000;
 			display_throughput(progress, byte_count);
 		} else if (!strcmp(line.buf, "update")) {
-			progress_test_force_update();
+			progress_test_force_update(progress);
 		} else if (!strcmp(line.buf, "stop")) {
 			stop_progress(&progress);
 		} else {
-- 
2.51.0.205.g9a02ae2892

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Carlo Marcelo Arenas Belón]

On Sat, Aug 23, 2025 at 06:24:57PM -0800, Johannes Sixt wrote:
> Am 23.08.25 um 15:22 schrieb Carlo Marcelo Arenas Belón via GitGitGadget:
> > The first patch does the minimum changes required to swap the underlying
> > function, but introduce a race condition that is addressed in the second
> > patch.
> > 
> > A third patch that does further changes to the Windows compatibility layer
> > was punted.
> > 
> > Carlo Marcelo Arenas Belón (2): progress: replace setitimer() with alarm()
> > progress: add a shutting down state to the SIGALRM handler
> > 
> After having looked at the progress code for a bit, see this:
> 
> We use SIGALRM to raise a flag that tells the progress code to act in
> some way. The progress code does not act asynchronously, but only when
> there is an opportunity to look at the flag, i.e., it acts synchronously
> in response to a third party (SIGALRM) that told it that it's time to
> act.
> 
> But we can change the progress code to do the time keeping itself.
> Instead of looking whether a flag was raised, we can let it look at the
> wall clock and check whether an interval has elapsed.

This is an even better approach indeed, and will lead to an even nicer
cleanup in the Windows emulation code.

> A prototype implementation looks like the patch below. It works quite
> well for `git clone` on Linux. I have even lowered the interval to 1/8th
> of a second to get more frequent updates. After a change like this, we
> can remove all the creepy SIGALRM/alarm/setitimer stuff.

Would you mind cleaning it up and making it a patch I could rebase on?, or
would you rather finish it off, since you also know the Windows parts better?

Carlo

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Johannes Sixt]

Am 23.08.25 um 21:38 schrieb Carlo Marcelo Arenas Belón:
> Would you mind cleaning it up and making it a patch I could rebase on?, or
> would you rather finish it off, since you also know the Windows parts better?
You can pull a much more polished version from

   https://github.com/j6t/git.git progress-wall-clock

I'll wait until CI shows all green before I submit the patches.

-- Hannes

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> We use SIGALRM to raise a flag that tells the progress code to act in
> some way. The progress code does not act asynchronously, but only when
> there is an opportunity to look at the flag, i.e., it acts synchronously
> in response to a third party (SIGALRM) that told it that it's time to
> act.
>
> But we can change the progress code to do the time keeping itself.
> Instead of looking whether a flag was raised, we can let it look at the
> wall clock and check whether an interval has elapsed.

Yes, the use of itimer to only change the flag without doing
anything funky has been a very safe way to use signals, doing only
absolutely minimal thing in the signal handler.  Having to rearm the
signal in the signal handler in Carlo's patch made me feel dirtier.

But looking at the wallclock once every iteration of a busy loop?  

Operating system folks may have worked hard to minimize the cost of
system calls to gettimeofday() in order to help applications that do
so, but I somehow feel even dirtier to hear proposal to do so to
replace a signal that we set and forget, to be reminded once every
second.

I dunno.

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Operating system folks may have worked hard to minimize the cost of
> system calls to gettimeofday() in order to help applications that do
> so, but I somehow feel even dirtier to hear proposal to do so to
> replace a signal that we set and forget, to be reminded once every
> second.

I actually think this is probably fine.  It is not like we are
spinning only to wait.  Every iteration we are doing useful work,
and modern gettimeofday() implementations would be fine with this
use case.

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Johannes Sixt]

Am 23.08.25 um 23:33 schrieb Junio C Hamano:
> Yes, the use of itimer to only change the flag without doing
> anything funky has been a very safe way to use signals, doing only
> absolutely minimal thing in the signal handler.  Having to rearm the
> signal in the signal handler in Carlo's patch made me feel dirtier.

While this is clean on POSIX, it isn't the only platform where Git runs.
On Windows, this part of the progress indication is as dirty as it can
get. Getting rid of it is a big bonus in my book.

> But looking at the wallclock once every iteration of a busy loop?  
> 
> Operating system folks may have worked hard to minimize the cost of
> system calls to gettimeofday() in order to help applications that do
> so, but I somehow feel even dirtier to hear proposal to do so to
> replace a signal that we set and forget, to be reminded once every
> second.
I think that ship has sailed already. Look at display_throughput(). One
of the first things it does is to look at the wallclock a.k.a.
getnanotime().

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 observe a behavior change with delayed progress indicators that I have
to understand and fix it before I can submit the cleaned up patches.

-- Hannes

[PATCH] progress: pay attention to (customized) delay time

[Johannes Sixt]

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@kdbg.org>
---
 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

Re: [PATCH 0/2] progress: replace setitimer() with alarm()

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Operating system folks may have worked hard to minimize the cost of
>> system calls to gettimeofday() in order to help applications that do
>> so, but I somehow feel even dirtier to hear proposal to do so to
>> replace a signal that we set and forget, to be reminded once every
>> second.
>
> I think that ship has sailed already. Look at display_throughput(). One
> of the first things it does is to look at the wallclock a.k.a.
> getnanotime().

It can be fixed if we wanted to, though, no?  Instead of doing all
the computation for the latest lap, and then decide not to show by
looking at the progress_update flag (set by the interrupt), we can
accumulate the total in the progress->throughput struct until we see
the progress_update flag, at which time we can look at the wallclock
time, compute the time difference, perform clever division, etc.

> 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 observe a behavior change with delayed progress indicators that I have
> to understand and fix it before I can submit the cleaned up patches.

Thanks.

Re: [PATCH] progress: pay attention to (customized) delay time

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Subject: [PATCH] progress: pay attention to (customized) delay time
> ...
> 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.
> ...
> 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.

OK.  This is documenting the established behaviour, which makes
sense.

>  	struct strbuf *counters_sb = &progress->counters_sb;
>  	int show_update = 0;
> +	sig_atomic_t update = progress_update;

It is somewhat misleading to use sig_atomic_t for "update", which is
never updated via the signal handler.  It confused me a bit during
my initial reading.  If it were

	int update = !!progress_update;

it would have made it more obvious what is going on, at least to me.
In any case, I think it is an excellent idea to clear the global one
first ...

>  	int last_count_len = counters_sb->len;
>  
> -	if (progress->delay && (!progress_update || --progress->delay))
> +	progress_update = 0;

..., while remembering the fact that progress_update was originally
set or unset, and consistently use the latter in the remainder of
the function, like ...

> +	if (progress->delay && (!update || --progress->delay))
>  		return;

... this one and everywhere below (omitted from quote).

Thanks.

Re: [PATCH] progress: pay attention to (customized) delay time

[Carlo Marcelo Arenas Belón]

On Mon, Aug 25, 2025 at 10:00:25AM -0800, Junio C Hamano wrote:
> 
> >  	struct strbuf *counters_sb = &progress->counters_sb;
> >  	int show_update = 0;
> > +	sig_atomic_t update = progress_update;
> 
> It is somewhat misleading to use sig_atomic_t for "update", which is
> never updated via the signal handler.  It confused me a bit during
> my initial reading.  If it were
> 
> 	int update = !!progress_update;
> 
> it would have made it more obvious what is going on, at least to me.

In that case, I would suggest doing instead:

  bool update = !!progress_update;

Carlo

Re: [PATCH] progress: pay attention to (customized) delay time

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> On Mon, Aug 25, 2025 at 10:00:25AM -0800, Junio C Hamano wrote:
>> 
>> >  	struct strbuf *counters_sb = &progress->counters_sb;
>> >  	int show_update = 0;
>> > +	sig_atomic_t update = progress_update;
>> 
>> It is somewhat misleading to use sig_atomic_t for "update", which is
>> never updated via the signal handler.  It confused me a bit during
>> my initial reading.  If it were
>> 
>> 	int update = !!progress_update;
>> 
>> it would have made it more obvious what is going on, at least to me.
>
> In that case, I would suggest doing instead:
>
>   bool update = !!progress_update;

Any conventional type we would use for "is it set or not?" that is
not sig_atomic_t is good enough in this context.

The fact that we started adopting "bool" in new code is orthogonal
and a bit off the point.

[PATCH v2] progress: pay attention to (customized) delay time

[Johannes Sixt]

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@kdbg.org>
---
 Compared to the first round, this replaces sig_atomic_t by int. I
 didn't use bool just in case the patch goes on top of a maintenance
 track that does not have the "bool is allowed" policy.

 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..8315bdc3d4 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;
+	int 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

Re: [PATCH v2] progress: pay attention to (customized) delay time

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>  Compared to the first round, this replaces sig_atomic_t by int. I
>  didn't use bool just in case the patch goes on top of a maintenance
>  track that does not have the "bool is allowed" policy.

That's fair.  I happened to have chosen v2.51.0 as the base for v1
during today's integration, but as a fix-up topic, you are correct
to point out that I should apply this on top of maint-2.50 or even
older.

Thanks, will requeue.