{"thread":{"id":"64019","subject":"[PATCH 0/2] progress: replace setitimer() with alarm()","startedAt":"2025-08-23T13:23:01Z","lastAt":"2025-08-25T22:52:21Z","messageCount":16,"participants":["Carlo Marcelo Arenas Belón via GitGitGadget","Johannes Sixt","Carlo Marcelo Arenas Belón","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"524775","messageId":"pull.1960.git.1755955377.gitgitgadget@gmail.com","threadId":"64019","inReplyTo":null,"subject":"[PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-23T13:22:55Z","receivedAt":"2025-08-23T13:23:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"The first patch does the minimum changes required to swap the underlying\nfunction, but introduce a race condition that is addressed in the second\npatch.\n\nA third patch that does further changes to the Windows compatibility layer\nwas punted.\n\nCarlo Marcelo Arenas Belón (2): progress: replace setitimer() with alarm()\nprogress: add a shutting down state to the SIGALRM handler\n\nMakefile | 12 ------------ compat/mingw-posix.h | 9 +-------- compat/mingw.c\n| 46 ++++++++++++++++---------------------------- compat/posix.h | 17\n---------------- configure.ac | 13 ------------- meson.build | 16\n--------------- progress.c | 29 +++++++++++++++------------- 7 files\nchanged, 34 insertions(+), 108 deletions(-)\n\nCarlo Marcelo Arenas Belón (2):\n  progress: replace setitimer() with alarm()\n  progress: add a shutting down state to the SIGALRM handler\n\n Makefile             | 12 ------------\n compat/mingw-posix.h |  9 +--------\n compat/mingw.c       | 46 ++++++++++++++++----------------------------\n compat/posix.h       | 17 ----------------\n configure.ac         | 13 -------------\n meson.build          | 16 ---------------\n progress.c           | 29 +++++++++++++++-------------\n 7 files changed, 34 insertions(+), 108 deletions(-)\n\n\nbase-commit: 1fa68948c3d76328236cac73d2adf33c905bd8e3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1960%2Fcarenas%2Fnoitimer-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1960/carenas/noitimer-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1960\n-- \ngitgitgadget\n"},{"id":"524776","messageId":"493f69caddacea3f31e458fc5c5c1e151d28b6f3.1755955378.git.gitgitgadget@gmail.com","threadId":"64019","inReplyTo":"pull.1960.git.1755955377.gitgitgadget@gmail.com","subject":"[PATCH 1/2] progress: replace setitimer() with alarm()","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-23T13:22:56Z","receivedAt":"2025-08-23T13:23:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>\n\nsetitimer() was marked obsolescent by IEEE Std 1003.1-2017 and is\nno longer available in the latest edition.\n\nWhile other alternatives of high fidelity are available, its use\nin progress only requires whole second intervals, so it can be\ncleanly replaced by a simpler alarm(); do that.\n\nMinGW provided its own version of setitimer() so add an equivalent\nimplementation for alarm(), but do the minimum changes required in\nthe timer thread to adapt to the new interface.\n\nRemove the compatibility layer for setitimer() and its related\nstruct that are no longer needed, and adjust the build system.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n Makefile             | 12 ------------\n compat/mingw-posix.h |  9 +--------\n compat/mingw.c       | 46 ++++++++++++++++----------------------------\n compat/posix.h       | 17 ----------------\n configure.ac         | 13 -------------\n meson.build          | 16 ---------------\n progress.c           | 11 +++--------\n 7 files changed, 21 insertions(+), 103 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex e11340c1ae77..059382624c0e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -142,11 +142,6 @@ include shared.mak\n # Define NO_PREAD if you have a problem with pread() system call (e.g.\n # cygwin1.dll before v1.5.22).\n #\n-# Define NO_SETITIMER if you don't have setitimer()\n-#\n-# Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval\n-# This also implies NO_SETITIMER\n-#\n # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n # generally faster on your platform than accessing the working directory.\n #\n@@ -1888,13 +1883,6 @@ endif\n ifdef OBJECT_CREATION_USES_RENAMES\n \tCOMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1\n endif\n-ifdef NO_STRUCT_ITIMERVAL\n-\tCOMPAT_CFLAGS += -DNO_STRUCT_ITIMERVAL\n-\tNO_SETITIMER = YesPlease\n-endif\n-ifdef NO_SETITIMER\n-\tCOMPAT_CFLAGS += -DNO_SETITIMER\n-endif\n ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\ndiff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\nindex 631a20868489..7626fbe90172 100644\n--- a/compat/mingw-posix.h\n+++ b/compat/mingw-posix.h\n@@ -98,11 +98,6 @@ struct sigaction {\n #define SA_RESTART 0\n #define SA_NOCLDSTOP 1\n \n-struct itimerval {\n-\tstruct timeval it_value, it_interval;\n-};\n-#define ITIMER_REAL 0\n-\n struct utsname {\n \tchar sysname[16];\n \tchar nodename[1];\n@@ -131,8 +126,6 @@ static inline int fchmod(int fildes UNUSED, mode_t mode UNUSED)\n static inline pid_t fork(void)\n { errno = ENOSYS; return -1; }\n #endif\n-static inline unsigned int alarm(unsigned int seconds UNUSED)\n-{ return 0; }\n static inline int fsync(int fd)\n { return _commit(fd); }\n static inline void sync(void)\n@@ -183,6 +176,7 @@ char *mingw_locate_in_PATH(const char *cmd);\n  * implementations of missing functions\n  */\n \n+unsigned alarm(unsigned seconds);\n int pipe(int filedes[2]);\n unsigned int sleep (unsigned int seconds);\n int mkstemp(char *template);\n@@ -193,7 +187,6 @@ struct tm *localtime_r(const time_t *timep, struct tm *result);\n #endif\n int getpagesize(void);\t/* defined in MinGW's libgcc.a */\n struct passwd *getpwuid(uid_t uid);\n-int setitimer(int type, struct itimerval *in, struct itimerval *out);\n int sigaction(int sig, struct sigaction *in, struct sigaction *out);\n int link(const char *oldpath, const char *newpath);\n int uname(struct utsname *buf);\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 8538e3d1729d..199f68ca6d9a 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2432,15 +2432,17 @@ static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;\n  * the thread to terminate by setting the timer_event to the signalled\n  * state.\n  * But ticktack() interrupts the wait state after the timer's interval\n- * length to call the signal handler.\n+ * length to call the signal handler if it still set.\n  */\n \n static unsigned __stdcall ticktack(void *dummy UNUSED)\n {\n+\tsig_handler_t fn = timer_fn;\n+\n \twhile (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {\n-\t\tmingw_raise(SIGALRM);\n-\t\tif (one_shot)\n+\t\tif (one_shot || timer_fn != fn)\n \t\t\tbreak;\n+\t\tmingw_raise(SIGALRM);\n \t}\n \treturn 0;\n }\n@@ -2478,37 +2480,23 @@ static void stop_timer_thread(void)\n \ttimer_thread = NULL;\n }\n \n-static inline int is_timeval_eq(const struct timeval *i1, const struct timeval *i2)\n+unsigned alarm(unsigned seconds)\n {\n-\treturn i1->tv_sec == i2->tv_sec && i1->tv_usec == i2->tv_usec;\n-}\n-\n-int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)\n-{\n-\tstatic const struct timeval zero;\n-\tstatic int atexit_done;\n-\n-\tif (out)\n-\t\treturn errno = EINVAL,\n-\t\t\terror(\"setitimer param 3 != NULL not implemented\");\n-\tif (!is_timeval_eq(&in->it_interval, &zero) &&\n-\t    !is_timeval_eq(&in->it_interval, &in->it_value))\n-\t\treturn errno = EINVAL,\n-\t\t\terror(\"setitimer: it_interval must be zero or eq it_value\");\n-\n-\tif (timer_thread)\n-\t\tstop_timer_thread();\n+\tstatic bool atexit_done;\n \n-\tif (is_timeval_eq(&in->it_value, &zero) &&\n-\t    is_timeval_eq(&in->it_interval, &zero))\n-\t\treturn 0;\n-\n-\ttimer_interval = in->it_value.tv_sec * 1000 + in->it_value.tv_usec / 1000;\n-\tone_shot = is_timeval_eq(&in->it_interval, &zero);\n \tif (!atexit_done) {\n \t\tatexit(stop_timer_thread);\n-\t\tatexit_done = 1;\n+\t\tatexit_done = true;\n \t}\n+\n+\ttimer_interval = seconds * 1000;\n+\tone_shot = !seconds;\n+\tif (timer_thread) {\n+\t\tif (!seconds)\n+\t\t\tstop_timer_thread();\n+\t\treturn 0;\n+\t}\n+\n \treturn start_timer_thread();\n }\n \ndiff --git a/compat/posix.h b/compat/posix.h\nindex 067a00f33b83..83af92d820f2 100644\n--- a/compat/posix.h\n+++ b/compat/posix.h\n@@ -198,23 +198,6 @@ static inline time_t git_time(time_t *tloc)\n }\n #define time git_time\n \n-#ifdef NO_STRUCT_ITIMERVAL\n-struct itimerval {\n-\tstruct timeval it_interval;\n-\tstruct timeval it_value;\n-};\n-#endif\n-\n-#ifdef NO_SETITIMER\n-static inline int git_setitimer(int which UNUSED,\n-\t\t\t\tconst struct itimerval *value UNUSED,\n-\t\t\t\tstruct itimerval *newvalue UNUSED) {\n-\treturn 0; /* pretend success */\n-}\n-#undef setitimer\n-#define setitimer(which,value,ovalue) git_setitimer(which,value,ovalue)\n-#endif\n-\n #ifndef NO_LIBGEN_H\n #include <libgen.h>\n #else\ndiff --git a/configure.ac b/configure.ac\nindex cfb50112bf81..6b04aa37a82c 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -872,13 +872,6 @@ case $ac_cv_type_socklen_t in\n esac\n GIT_CONF_SUBST([SOCKLEN_T])\n \n-#\n-# Define NO_STRUCT_ITIMERVAL if you don't have struct itimerval.\n-AC_CHECK_TYPES([struct itimerval],\n-[NO_STRUCT_ITIMERVAL=],\n-[NO_STRUCT_ITIMERVAL=UnfortunatelyYes],\n-[#include <sys/time.h>])\n-GIT_CONF_SUBST([NO_STRUCT_ITIMERVAL])\n #\n # Define USE_ST_TIMESPEC=YesPlease when stat.st_mtimespec.tv_nsec exists.\n # Define NO_NSEC=YesPlease when neither stat.st_mtim.tv_nsec nor\n@@ -1097,12 +1090,6 @@ GIT_CHECK_FUNC(sync_file_range,\n \t[HAVE_SYNC_FILE_RANGE=])\n GIT_CONF_SUBST([HAVE_SYNC_FILE_RANGE])\n \n-#\n-# Define NO_SETITIMER if you don't have setitimer.\n-GIT_CHECK_FUNC(setitimer,\n-[NO_SETITIMER=],\n-[NO_SETITIMER=YesPlease])\n-GIT_CONF_SUBST([NO_SETITIMER])\n #\n # Define NO_STRCASESTR if you don't have strcasestr.\n GIT_CHECK_FUNC(strcasestr,\ndiff --git a/meson.build b/meson.build\nindex 5dd299b4962d..f33f20312511 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1348,22 +1348,6 @@ else\n     error('Native regex support requested but not found')\n endif\n \n-# setitimer and friends are provided by compat/mingw.c.\n-if host_machine.system() != 'windows'\n-  if not compiler.compiles('''\n-    #include <sys/time.h>\n-    void func(void)\n-    {\n-      struct itimerval value;\n-    }\n-  ''', name: 'struct itimerval')\n-    libgit_c_args += '-DNO_STRUCT_ITIMERVAL'\n-    libgit_c_args += '-DNO_SETITIMER'\n-  elif not compiler.has_function('setitimer')\n-    libgit_c_args += '-DNO_SETITIMER'\n-  endif\n-endif\n-\n if compiler.has_member('struct stat', 'st_mtimespec.tv_nsec', prefix: '#include <sys/stat.h>')\n   libgit_c_args += '-DUSE_ST_TIMESPEC'\n elif not compiler.has_member('struct stat', 'st_mtim.tv_nsec', prefix: '#include <sys/stat.h>')\ndiff --git a/progress.c b/progress.c\nindex 8d5ae70f3a9e..71b305d1625d 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -67,12 +67,12 @@ void progress_test_force_update(void)\n static void progress_interval(int signum UNUSED)\n {\n \tprogress_update = 1;\n+\talarm(1);\n }\n \n static void set_progress_signal(void)\n {\n \tstruct sigaction sa;\n-\tstruct itimerval v;\n \n \tif (progress_testing)\n \t\treturn;\n@@ -85,20 +85,15 @@ static void set_progress_signal(void)\n \tsa.sa_flags = SA_RESTART;\n \tsigaction(SIGALRM, &sa, NULL);\n \n-\tv.it_interval.tv_sec = 1;\n-\tv.it_interval.tv_usec = 0;\n-\tv.it_value = v.it_interval;\n-\tsetitimer(ITIMER_REAL, &v, NULL);\n+\talarm(1);\n }\n \n static void clear_progress_signal(void)\n {\n-\tstruct itimerval v = {{0,},};\n-\n \tif (progress_testing)\n \t\treturn;\n \n-\tsetitimer(ITIMER_REAL, &v, NULL);\n+\talarm(0);\n \tsignal(SIGALRM, SIG_IGN);\n \tprogress_update = 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"524777","messageId":"0db98c3478e5e2f1aadcf6d773cf6519af482630.1755955378.git.gitgitgadget@gmail.com","threadId":"64019","inReplyTo":"pull.1960.git.1755955377.gitgitgadget@gmail.com","subject":"[PATCH 2/2] progress: add a shutting down state to the SIGALRM handler","fromName":"Carlo Marcelo Arenas Belón via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-23T13:22:57Z","receivedAt":"2025-08-23T13:23:02Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>\n\nIn a previous commit, sigitimer() was replaced by alarm(), but to\nkeep the timer active, an extra call to `alarm(1)` was added to\nthe signal handler, opening a potential race condition whem the\ntimer is being cleared.\n\nTo avoid that, add an extra state to set during shutdown and\nadjust the logic to flag the potential need to update progress\ninto the first bit instead.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n progress.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 71b305d1625d..49e58e094a3f 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -50,6 +50,11 @@ struct progress {\n \tint split;\n };\n \n+/*\n+ * 0: no progress to report\n+ * 1: potential update for progress to report\n+ * 2: no more progress to report\n+ */\n static volatile sig_atomic_t progress_update;\n \n /*\n@@ -66,8 +71,10 @@ void progress_test_force_update(void)\n \n static void progress_interval(int signum UNUSED)\n {\n-\tprogress_update = 1;\n-\talarm(1);\n+\tif (progress_update != 2) {\n+\t\talarm(1);\n+\t\tprogress_update = 1;\n+\t}\n }\n \n static void set_progress_signal(void)\n@@ -93,6 +100,7 @@ static void clear_progress_signal(void)\n \tif (progress_testing)\n \t\treturn;\n \n+\tprogress_update = 2;\n \talarm(0);\n \tsignal(SIGALRM, SIG_IGN);\n \tprogress_update = 0;\n@@ -111,14 +119,14 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tint show_update = 0;\n \tint last_count_len = counters_sb->len;\n \n-\tif (progress->delay && (!progress_update || --progress->delay))\n+\tif (progress->delay && (!(progress_update & 1) || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n-\t\tif (percent != progress->last_percent || progress_update) {\n+\t\tif (percent != progress->last_percent || (progress_update & 1)) {\n \t\t\tprogress->last_percent = percent;\n \n \t\t\tstrbuf_reset(counters_sb);\n@@ -128,7 +136,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\t    tp);\n \t\t\tshow_update = 1;\n \t\t}\n-\t} else if (progress_update) {\n+\t} else if (progress_update & 1) {\n \t\tstrbuf_reset(counters_sb);\n \t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n \t\tshow_update = 1;\n@@ -239,7 +247,7 @@ void display_throughput(struct progress *progress, uint64_t total)\n \ttp->idx = (tp->idx + 1) % TP_IDX_MAX;\n \n \tthroughput_string(&tp->display, total, rate);\n-\tif (progress->last_value != -1 && progress_update)\n+\tif (progress->last_value != -1 && (progress_update & 1))\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n \n-- \ngitgitgadget\n"},{"id":"524786","messageId":"86bf04c7-6315-46ef-8297-42efc3ed322d@kdbg.org","threadId":"64019","inReplyTo":"pull.1960.git.1755955377.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-08-23T16:24:57Z","receivedAt":"2025-08-23T16:25:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.08.25 um 15:22 schrieb Carlo Marcelo Arenas Belón via GitGitGadget:\n> The first patch does the minimum changes required to swap the underlying\n> function, but introduce a race condition that is addressed in the second\n> patch.\n> \n> A third patch that does further changes to the Windows compatibility layer\n> was punted.\n> \n> Carlo Marcelo Arenas Belón (2): progress: replace setitimer() with alarm()\n> progress: add a shutting down state to the SIGALRM handler\n> \nAfter having looked at the progress code for a bit, see this:\n\nWe use SIGALRM to raise a flag that tells the progress code to act in\nsome way. The progress code does not act asynchronously, but only when\nthere is an opportunity to look at the flag, i.e., it acts synchronously\nin response to a third party (SIGALRM) that told it that it's time to\nact.\n\nBut we can change the progress code to do the time keeping itself.\nInstead of looking whether a flag was raised, we can let it look at the\nwall clock and check whether an interval has elapsed.\n\nA prototype implementation looks like the patch below. It works quite\nwell for `git clone` on Linux. I have even lowered the interval to 1/8th\nof a second to get more frequent updates. After a change like this, we\ncan remove all the creepy SIGALRM/alarm/setitimer stuff.\n\nWhat do you think?\n\n progress.c               | 65 +++++++++++-----------------------------\n progress.h               |  2 +-\n t/helper/test-progress.c |  2 +-\n 3 files changed, 20 insertions(+), 49 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 8d5ae70f3a..eb1d2f14e0 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -38,6 +38,7 @@ struct throughput {\n struct progress {\n \tstruct repository *repo;\n \tconst char *title;\n+\tuint64_t last_update;\n \tuint64_t last_value;\n \tuint64_t total;\n \tunsigned last_percent;\n@@ -50,57 +51,26 @@ struct progress {\n \tint split;\n };\n \n-static volatile sig_atomic_t progress_update;\n-\n /*\n  * These are only intended for testing the progress output, i.e. exclusively\n  * for 'test-tool progress'.\n  */\n int progress_testing;\n uint64_t progress_test_ns = 0;\n-void progress_test_force_update(void)\n+void progress_test_force_update(struct progress *progress)\n {\n-\tprogress_update = 1;\n+\tprogress->last_update = 0;\n }\n \n \n-static void progress_interval(int signum UNUSED)\n+static bool check_update_delay_expired(struct progress *progress)\n {\n-\tprogress_update = 1;\n-}\n-\n-static void set_progress_signal(void)\n-{\n-\tstruct sigaction sa;\n-\tstruct itimerval v;\n-\n-\tif (progress_testing)\n-\t\treturn;\n-\n-\tprogress_update = 0;\n-\n-\tmemset(&sa, 0, sizeof(sa));\n-\tsa.sa_handler = progress_interval;\n-\tsigemptyset(&sa.sa_mask);\n-\tsa.sa_flags = SA_RESTART;\n-\tsigaction(SIGALRM, &sa, NULL);\n-\n-\tv.it_interval.tv_sec = 1;\n-\tv.it_interval.tv_usec = 0;\n-\tv.it_value = v.it_interval;\n-\tsetitimer(ITIMER_REAL, &v, NULL);\n-}\n-\n-static void clear_progress_signal(void)\n-{\n-\tstruct itimerval v = {{0,},};\n-\n-\tif (progress_testing)\n-\t\treturn;\n-\n-\tsetitimer(ITIMER_REAL, &v, NULL);\n-\tsignal(SIGALRM, SIG_IGN);\n-\tprogress_update = 0;\n+\tuint64_t now = getnanotime();\n+\t/* about every 1/8th of a second */\n+\tbool update = (now - progress->last_update) / (1024 * 1024 * 1024 / 8) > 0;\n+\tif (update)\n+\t\tprogress->last_update = now;\n+\treturn update;\n }\n \n static int is_foreground_fd(int fd)\n@@ -109,7 +79,8 @@ static int is_foreground_fd(int fd)\n \treturn tpgrp < 0 || tpgrp == getpgid(0);\n }\n \n-static void display(struct progress *progress, uint64_t n, const char *done)\n+static void display(struct progress *progress, uint64_t n, const char *done,\n+\t\t    bool progress_update)\n {\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n@@ -243,15 +214,16 @@ void display_throughput(struct progress *progress, uint64_t total)\n \ttp->last_misecs[tp->idx] = misecs;\n \ttp->idx = (tp->idx + 1) % TP_IDX_MAX;\n \n+\tbool progress_update = check_update_delay_expired(progress);\n \tthroughput_string(&tp->display, total, rate);\n \tif (progress->last_value != -1 && progress_update)\n-\t\tdisplay(progress, progress->last_value, NULL);\n+\t\tdisplay(progress, progress->last_value, NULL, progress_update);\n }\n \n void display_progress(struct progress *progress, uint64_t n)\n {\n \tif (progress)\n-\t\tdisplay(progress, n, NULL);\n+\t\tdisplay(progress, n, NULL, check_update_delay_expired(progress));\n }\n \n static struct progress *start_progress_delay(struct repository *r,\n@@ -262,6 +234,7 @@ static struct progress *start_progress_delay(struct repository *r,\n \tprogress->repo = r;\n \tprogress->title = title;\n \tprogress->total = total;\n+\tprogress->last_update = getnanotime();\n \tprogress->last_value = -1;\n \tprogress->last_percent = -1;\n \tprogress->delay = delay;\n@@ -271,7 +244,6 @@ static struct progress *start_progress_delay(struct repository *r,\n \tstrbuf_init(&progress->counters_sb, 0);\n \tprogress->title_len = utf8_strwidth(title);\n \tprogress->split = 0;\n-\tset_progress_signal();\n \ttrace2_region_enter(\"progress\", title, r);\n \treturn progress;\n }\n@@ -339,9 +311,9 @@ static void force_last_update(struct progress *progress, const char *msg)\n \t\trate = tp->curr_total / (misecs ? misecs : 1);\n \t\tthroughput_string(&tp->display, tp->curr_total, rate);\n \t}\n-\tprogress_update = 1;\n+\tprogress->last_update = 0;\n \tbuf = xstrfmt(\", %s.\\n\", msg);\n-\tdisplay(progress, progress->last_value, buf);\n+\tdisplay(progress, progress->last_value, buf, check_update_delay_expired(progress));\n \tfree(buf);\n }\n \n@@ -374,7 +346,6 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\tforce_last_update(progress, msg);\n \tlog_trace2(progress);\n \n-\tclear_progress_signal();\n \tstrbuf_release(&progress->counters_sb);\n \tif (progress->throughput)\n \t\tstrbuf_release(&progress->throughput->display);\ndiff --git a/progress.h b/progress.h\nindex ed068c7bab..cca479bd26 100644\n--- a/progress.h\n+++ b/progress.h\n@@ -9,7 +9,7 @@ struct repository;\n \n extern int progress_testing;\n extern uint64_t progress_test_ns;\n-void progress_test_force_update(void);\n+void progress_test_force_update(struct progress *progress);\n \n #endif\n \ndiff --git a/t/helper/test-progress.c b/t/helper/test-progress.c\nindex 1f75b7bd19..e2ca5fb726 100644\n--- a/t/helper/test-progress.c\n+++ b/t/helper/test-progress.c\n@@ -87,7 +87,7 @@ int cmd__progress(int argc, const char **argv)\n \t\t\tprogress_test_ns = test_ms * 1000 * 1000;\n \t\t\tdisplay_throughput(progress, byte_count);\n \t\t} else if (!strcmp(line.buf, \"update\")) {\n-\t\t\tprogress_test_force_update();\n+\t\t\tprogress_test_force_update(progress);\n \t\t} else if (!strcmp(line.buf, \"stop\")) {\n \t\t\tstop_progress(&progress);\n \t\t} else {\n-- \n2.51.0.205.g9a02ae2892\n\n"},{"id":"524792","messageId":"ubouc4oefkouvoikedo2lcui3wgjgjovbilxnf67g76gmrp75e@ujkfg5asyy76","threadId":"64019","inReplyTo":"86bf04c7-6315-46ef-8297-42efc3ed322d@kdbg.org","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-08-23T19:38:55Z","receivedAt":"2025-08-23T19:38:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Aug 23, 2025 at 06:24:57PM -0800, Johannes Sixt wrote:\n> Am 23.08.25 um 15:22 schrieb Carlo Marcelo Arenas Belón via GitGitGadget:\n> > The first patch does the minimum changes required to swap the underlying\n> > function, but introduce a race condition that is addressed in the second\n> > patch.\n> > \n> > A third patch that does further changes to the Windows compatibility layer\n> > was punted.\n> > \n> > Carlo Marcelo Arenas Belón (2): progress: replace setitimer() with alarm()\n> > progress: add a shutting down state to the SIGALRM handler\n> > \n> After having looked at the progress code for a bit, see this:\n> \n> We use SIGALRM to raise a flag that tells the progress code to act in\n> some way. The progress code does not act asynchronously, but only when\n> there is an opportunity to look at the flag, i.e., it acts synchronously\n> in response to a third party (SIGALRM) that told it that it's time to\n> act.\n> \n> But we can change the progress code to do the time keeping itself.\n> Instead of looking whether a flag was raised, we can let it look at the\n> wall clock and check whether an interval has elapsed.\n\nThis is an even better approach indeed, and will lead to an even nicer\ncleanup in the Windows emulation code.\n\n> A prototype implementation looks like the patch below. It works quite\n> well for `git clone` on Linux. I have even lowered the interval to 1/8th\n> of a second to get more frequent updates. After a change like this, we\n> can remove all the creepy SIGALRM/alarm/setitimer stuff.\n\nWould you mind cleaning it up and making it a patch I could rebase on?, or\nwould you rather finish it off, since you also know the Windows parts better?\n\nCarlo\n"},{"id":"524793","messageId":"ec2e704c-62f6-417e-9aad-94de91e354c3@kdbg.org","threadId":"64019","inReplyTo":"ubouc4oefkouvoikedo2lcui3wgjgjovbilxnf67g76gmrp75e@ujkfg5asyy76","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-08-23T19:55:02Z","receivedAt":"2025-08-23T19:55:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.08.25 um 21:38 schrieb Carlo Marcelo Arenas Belón:\n> Would you mind cleaning it up and making it a patch I could rebase on?, or\n> would you rather finish it off, since you also know the Windows parts better?\nYou can pull a much more polished version from\n\n   https://github.com/j6t/git.git progress-wall-clock\n\nI'll wait until CI shows all green before I submit the patches.\n\n-- Hannes\n\n"},{"id":"524798","messageId":"xmqq4itxvi3z.fsf@gitster.g","threadId":"64019","inReplyTo":"86bf04c7-6315-46ef-8297-42efc3ed322d@kdbg.org","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-23T21:33:04Z","receivedAt":"2025-08-23T21:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> We use SIGALRM to raise a flag that tells the progress code to act in\n> some way. The progress code does not act asynchronously, but only when\n> there is an opportunity to look at the flag, i.e., it acts synchronously\n> in response to a third party (SIGALRM) that told it that it's time to\n> act.\n>\n> But we can change the progress code to do the time keeping itself.\n> Instead of looking whether a flag was raised, we can let it look at the\n> wall clock and check whether an interval has elapsed.\n\nYes, the use of itimer to only change the flag without doing\nanything funky has been a very safe way to use signals, doing only\nabsolutely minimal thing in the signal handler.  Having to rearm the\nsignal in the signal handler in Carlo's patch made me feel dirtier.\n\nBut looking at the wallclock once every iteration of a busy loop?  \n\nOperating system folks may have worked hard to minimize the cost of\nsystem calls to gettimeofday() in order to help applications that do\nso, but I somehow feel even dirtier to hear proposal to do so to\nreplace a signal that we set and forget, to be reminded once every\nsecond.\n\nI dunno.\n"},{"id":"524799","messageId":"xmqqzfbpu2vd.fsf@gitster.g","threadId":"64019","inReplyTo":"xmqq4itxvi3z.fsf@gitster.g","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-23T21:47:34Z","receivedAt":"2025-08-23T21:47:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Operating system folks may have worked hard to minimize the cost of\n> system calls to gettimeofday() in order to help applications that do\n> so, but I somehow feel even dirtier to hear proposal to do so to\n> replace a signal that we set and forget, to be reminded once every\n> second.\n\nI actually think this is probably fine.  It is not like we are\nspinning only to wait.  Every iteration we are doing useful work,\nand modern gettimeofday() implementations would be fine with this\nuse case.\n"},{"id":"524800","messageId":"08f405a6-fd2e-40d7-850a-574356b4009e@kdbg.org","threadId":"64019","inReplyTo":"xmqq4itxvi3z.fsf@gitster.g","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-08-23T22:03:29Z","receivedAt":"2025-08-23T22:03:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.08.25 um 23:33 schrieb Junio C Hamano:\n> Yes, the use of itimer to only change the flag without doing\n> anything funky has been a very safe way to use signals, doing only\n> absolutely minimal thing in the signal handler.  Having to rearm the\n> signal in the signal handler in Carlo's patch made me feel dirtier.\n\nWhile this is clean on POSIX, it isn't the only platform where Git runs.\nOn Windows, this part of the progress indication is as dirty as it can\nget. Getting rid of it is a big bonus in my book.\n\n> But looking at the wallclock once every iteration of a busy loop?  \n> \n> Operating system folks may have worked hard to minimize the cost of\n> system calls to gettimeofday() in order to help applications that do\n> so, but I somehow feel even dirtier to hear proposal to do so to\n> replace a signal that we set and forget, to be reminded once every\n> second.\nI think that ship has sailed already. Look at display_throughput(). One\nof the first things it does is to look at the wallclock a.k.a.\ngetnanotime().\n\nThat said, I am not very happy about the new calls introduced in\ndisplay_progress(), either. I'll see whether I can produce some\nperformance measurements.\n\nI observe a behavior change with delayed progress indicators that I have\nto understand and fix it before I can submit the cleaned up patches.\n\n-- Hannes\n\n"},{"id":"524808","messageId":"2d56de10-f829-4bc8-9c76-76eab6b137ae@kdbg.org","threadId":"64019","inReplyTo":"08f405a6-fd2e-40d7-850a-574356b4009e@kdbg.org","subject":"[PATCH] progress: pay attention to (customized) delay time","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-08-24T15:31:18Z","receivedAt":"2025-08-24T15:31:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.08.25 um 00:03 schrieb Johannes Sixt:\n> That said, I am not very happy about the new calls introduced in\n> display_progress(), either. I'll see whether I can produce some\n> performance measurements.\n\nI haven't made up my mind, yet, whether I want to persue the direction\nany further. Since we do not have a low-latency implementation of\ngetnanotime() for all supported systems, calling the function tens of\nthousands of times per second could be too burdensome for some of them.\n\n> I observe a behavior change with delayed progress indicators that I have\n> to understand and fix it before I can submit the cleaned up patches.\n\nBut I found a bug in the delayed progress indicators: the initial delay\ntime is always just one second instead of the configured time (two\nseconds by default).\n\nBelow is a fix. This is a minimal patch. It could be extended to reduce\nthe number of local flag variables in display() and perhaps also reduce\nindentation levels with some early returns.\n\nIf Carlo wants to advance the original patches, this patch also provides\nan obvious point where the alarm clock can be re-armed outside the\nsignal handler. It would be where we set progress_update = 0.\n\n----- 8< -----\nSubject: [PATCH] progress: pay attention to (customized) delay time\n\nUsing one of the start_delayed_*() functions, clients of the progress\nAPI can request that a progress meter is only shown after some time.\nTo do that, the implementation intends to count down the number of\nseconds stored in struct progress by observing flag progress_update,\nwhich the timer interrupt handler sets when a second has elapsed. This\nworks during the first second of the delay. But the code forgets to\nreset the flag to zero, so that subsequent calls of display_progress()\nthink that another second has elapsed and decrease the count again\nuntil zero is reached. Due to the frequency of the calls, this happens\nwithout an observable delay in practice, so that the effective delay is\nalways just one second.\n\nThis bug has been with us since the inception of the feature. Despite\nhaving been touched on various occasions, such as 8aade107dd84\n(progress: simplify \"delayed\" progress API), 9c5951cacf5c (progress:\ndrop delay-threshold code), and 44a4693bfcec (progress: create\nGIT_PROGRESS_DELAY), the short delay went unnoticed.\n\nCopy the flag state into a local variable and reset the global flag\nright away so that we can detect the next clock tick correctly.\n\nSince we have not had any complaints that the delay of one second is\ntoo short nor that GIT_PROGRESS_DELAY is ignored, people seem to be\ncomfortable with the status quo. Therefore, set the default to 1 to\nkeep the current behavior.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Documentation/git.adoc |  2 +-\n progress.c             | 12 +++++++-----\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git.adoc b/Documentation/git.adoc\nindex 743b7b00e4..03e9e69d25 100644\n--- a/Documentation/git.adoc\n+++ b/Documentation/git.adoc\n@@ -684,7 +684,7 @@ other\n \n `GIT_PROGRESS_DELAY`::\n \tA number controlling how many seconds to delay before showing\n-\toptional progress indicators. Defaults to 2.\n+\toptional progress indicators. Defaults to 1.\n \n `GIT_EDITOR`::\n \tThis environment variable overrides `$EDITOR` and `$VISUAL`.\ndiff --git a/progress.c b/progress.c\nindex 8d5ae70f3a..39a1101420 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -114,16 +114,19 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n+\tsig_atomic_t update = progress_update;\n \tint last_count_len = counters_sb->len;\n \n-\tif (progress->delay && (!progress_update || --progress->delay))\n+\tprogress_update = 0;\n+\n+\tif (progress->delay && (!update || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n-\t\tif (percent != progress->last_percent || progress_update) {\n+\t\tif (percent != progress->last_percent || update) {\n \t\t\tprogress->last_percent = percent;\n \n \t\t\tstrbuf_reset(counters_sb);\n@@ -133,7 +136,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\t    tp);\n \t\t\tshow_update = 1;\n \t\t}\n-\t} else if (progress_update) {\n+\t} else if (update) {\n \t\tstrbuf_reset(counters_sb);\n \t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n \t\tshow_update = 1;\n@@ -166,7 +169,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n-\t\tprogress_update = 0;\n \t}\n }\n \n@@ -281,7 +283,7 @@ static int get_default_delay(void)\n \tstatic int delay_in_secs = -1;\n \n \tif (delay_in_secs < 0)\n-\t\tdelay_in_secs = git_env_ulong(\"GIT_PROGRESS_DELAY\", 2);\n+\t\tdelay_in_secs = git_env_ulong(\"GIT_PROGRESS_DELAY\", 1);\n \n \treturn delay_in_secs;\n }\n-- \n2.51.0.205.g9a02ae2892\n\n"},{"id":"524813","messageId":"xmqqsehgu2bh.fsf@gitster.g","threadId":"64019","inReplyTo":"08f405a6-fd2e-40d7-850a-574356b4009e@kdbg.org","subject":"Re: [PATCH 0/2] progress: replace setitimer() with alarm()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-24T16:11:46Z","receivedAt":"2025-08-24T16:11:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> Operating system folks may have worked hard to minimize the cost of\n>> system calls to gettimeofday() in order to help applications that do\n>> so, but I somehow feel even dirtier to hear proposal to do so to\n>> replace a signal that we set and forget, to be reminded once every\n>> second.\n>\n> I think that ship has sailed already. Look at display_throughput(). One\n> of the first things it does is to look at the wallclock a.k.a.\n> getnanotime().\n\nIt can be fixed if we wanted to, though, no?  Instead of doing all\nthe computation for the latest lap, and then decide not to show by\nlooking at the progress_update flag (set by the interrupt), we can\naccumulate the total in the progress->throughput struct until we see\nthe progress_update flag, at which time we can look at the wallclock\ntime, compute the time difference, perform clever division, etc.\n\n> That said, I am not very happy about the new calls introduced in\n> display_progress(), either. I'll see whether I can produce some\n> performance measurements.\n>\n> I observe a behavior change with delayed progress indicators that I have\n> to understand and fix it before I can submit the cleaned up patches.\n\nThanks.\n"},{"id":"524874","messageId":"xmqq349fs5ee.fsf@gitster.g","threadId":"64019","inReplyTo":"2d56de10-f829-4bc8-9c76-76eab6b137ae@kdbg.org","subject":"Re: [PATCH] progress: pay attention to (customized) delay time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T17:00:25Z","receivedAt":"2025-08-25T17:00:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Subject: [PATCH] progress: pay attention to (customized) delay time\n> ...\n> until zero is reached. Due to the frequency of the calls, this happens\n> without an observable delay in practice, so that the effective delay is\n> always just one second.\n> ...\n> Since we have not had any complaints that the delay of one second is\n> too short nor that GIT_PROGRESS_DELAY is ignored, people seem to be\n> comfortable with the status quo. Therefore, set the default to 1 to\n> keep the current behavior.\n\nOK.  This is documenting the established behaviour, which makes\nsense.\n\n>  \tstruct strbuf *counters_sb = &progress->counters_sb;\n>  \tint show_update = 0;\n> +\tsig_atomic_t update = progress_update;\n\nIt is somewhat misleading to use sig_atomic_t for \"update\", which is\nnever updated via the signal handler.  It confused me a bit during\nmy initial reading.  If it were\n\n\tint update = !!progress_update;\n\nit would have made it more obvious what is going on, at least to me.\nIn any case, I think it is an excellent idea to clear the global one\nfirst ...\n\n>  \tint last_count_len = counters_sb->len;\n>  \n> -\tif (progress->delay && (!progress_update || --progress->delay))\n> +\tprogress_update = 0;\n\n..., while remembering the fact that progress_update was originally\nset or unset, and consistently use the latter in the remainder of\nthe function, like ...\n\n> +\tif (progress->delay && (!update || --progress->delay))\n>  \t\treturn;\n\n... this one and everywhere below (omitted from quote).\n\nThanks.\n\n"},{"id":"524876","messageId":"jq5ul4zwdex6peuub3upwzxz3d5zcnuh7adseyg6wa6dpiu4ci@fuwe2t2vbguo","threadId":"64019","inReplyTo":"xmqq349fs5ee.fsf@gitster.g","subject":"Re: [PATCH] progress: pay attention to (customized) delay time","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-08-25T18:11:21Z","receivedAt":"2025-08-25T18:11:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 25, 2025 at 10:00:25AM -0800, Junio C Hamano wrote:\n> \n> >  \tstruct strbuf *counters_sb = &progress->counters_sb;\n> >  \tint show_update = 0;\n> > +\tsig_atomic_t update = progress_update;\n> \n> It is somewhat misleading to use sig_atomic_t for \"update\", which is\n> never updated via the signal handler.  It confused me a bit during\n> my initial reading.  If it were\n> \n> \tint update = !!progress_update;\n> \n> it would have made it more obvious what is going on, at least to me.\n\nIn that case, I would suggest doing instead:\n\n  bool update = !!progress_update;\n\nCarlo\n"},{"id":"524879","messageId":"xmqq8qj7qlqf.fsf@gitster.g","threadId":"64019","inReplyTo":"jq5ul4zwdex6peuub3upwzxz3d5zcnuh7adseyg6wa6dpiu4ci@fuwe2t2vbguo","subject":"Re: [PATCH] progress: pay attention to (customized) delay time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T18:50:32Z","receivedAt":"2025-08-25T18:50:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:\n\n> On Mon, Aug 25, 2025 at 10:00:25AM -0800, Junio C Hamano wrote:\n>> \n>> >  \tstruct strbuf *counters_sb = &progress->counters_sb;\n>> >  \tint show_update = 0;\n>> > +\tsig_atomic_t update = progress_update;\n>> \n>> It is somewhat misleading to use sig_atomic_t for \"update\", which is\n>> never updated via the signal handler.  It confused me a bit during\n>> my initial reading.  If it were\n>> \n>> \tint update = !!progress_update;\n>> \n>> it would have made it more obvious what is going on, at least to me.\n>\n> In that case, I would suggest doing instead:\n>\n>   bool update = !!progress_update;\n\nAny conventional type we would use for \"is it set or not?\" that is\nnot sig_atomic_t is good enough in this context.\n\nThe fact that we started adopting \"bool\" in new code is orthogonal\nand a bit off the point.\n"},{"id":"524887","messageId":"7b848623-ce64-4679-9b5e-9d91d947b269@kdbg.org","threadId":"64019","inReplyTo":"xmqq8qj7qlqf.fsf@gitster.g","subject":"[PATCH v2] progress: pay attention to (customized) delay time","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2025-08-25T19:16:12Z","receivedAt":"2025-08-25T19:16:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Using one of the start_delayed_*() functions, clients of the progress\nAPI can request that a progress meter is only shown after some time.\nTo do that, the implementation intends to count down the number of\nseconds stored in struct progress by observing flag progress_update,\nwhich the timer interrupt handler sets when a second has elapsed. This\nworks during the first second of the delay. But the code forgets to\nreset the flag to zero, so that subsequent calls of display_progress()\nthink that another second has elapsed and decrease the count again\nuntil zero is reached. Due to the frequency of the calls, this happens\nwithout an observable delay in practice, so that the effective delay is\nalways just one second.\n\nThis bug has been with us since the inception of the feature. Despite\nhaving been touched on various occasions, such as 8aade107dd84\n(progress: simplify \"delayed\" progress API), 9c5951cacf5c (progress:\ndrop delay-threshold code), and 44a4693bfcec (progress: create\nGIT_PROGRESS_DELAY), the short delay went unnoticed.\n\nCopy the flag state into a local variable and reset the global flag\nright away so that we can detect the next clock tick correctly.\n\nSince we have not had any complaints that the delay of one second is\ntoo short nor that GIT_PROGRESS_DELAY is ignored, people seem to be\ncomfortable with the status quo. Therefore, set the default to 1 to\nkeep the current behavior.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Compared to the first round, this replaces sig_atomic_t by int. I\n didn't use bool just in case the patch goes on top of a maintenance\n track that does not have the \"bool is allowed\" policy.\n\n Documentation/git.adoc |  2 +-\n progress.c             | 12 +++++++-----\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git.adoc b/Documentation/git.adoc\nindex 743b7b00e4..03e9e69d25 100644\n--- a/Documentation/git.adoc\n+++ b/Documentation/git.adoc\n@@ -684,7 +684,7 @@ other\n \n `GIT_PROGRESS_DELAY`::\n \tA number controlling how many seconds to delay before showing\n-\toptional progress indicators. Defaults to 2.\n+\toptional progress indicators. Defaults to 1.\n \n `GIT_EDITOR`::\n \tThis environment variable overrides `$EDITOR` and `$VISUAL`.\ndiff --git a/progress.c b/progress.c\nindex 8d5ae70f3a..8315bdc3d4 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -114,16 +114,19 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \tconst char *tp;\n \tstruct strbuf *counters_sb = &progress->counters_sb;\n \tint show_update = 0;\n+\tint update = !!progress_update;\n \tint last_count_len = counters_sb->len;\n \n-\tif (progress->delay && (!progress_update || --progress->delay))\n+\tprogress_update = 0;\n+\n+\tif (progress->delay && (!update || --progress->delay))\n \t\treturn;\n \n \tprogress->last_value = n;\n \ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n-\t\tif (percent != progress->last_percent || progress_update) {\n+\t\tif (percent != progress->last_percent || update) {\n \t\t\tprogress->last_percent = percent;\n \n \t\t\tstrbuf_reset(counters_sb);\n@@ -133,7 +136,7 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t\t    tp);\n \t\t\tshow_update = 1;\n \t\t}\n-\t} else if (progress_update) {\n+\t} else if (update) {\n \t\tstrbuf_reset(counters_sb);\n \t\tstrbuf_addf(counters_sb, \"%\"PRIuMAX\"%s\", (uintmax_t)n, tp);\n \t\tshow_update = 1;\n@@ -166,7 +169,6 @@ static void display(struct progress *progress, uint64_t n, const char *done)\n \t\t\t}\n \t\t\tfflush(stderr);\n \t\t}\n-\t\tprogress_update = 0;\n \t}\n }\n \n@@ -281,7 +283,7 @@ static int get_default_delay(void)\n \tstatic int delay_in_secs = -1;\n \n \tif (delay_in_secs < 0)\n-\t\tdelay_in_secs = git_env_ulong(\"GIT_PROGRESS_DELAY\", 2);\n+\t\tdelay_in_secs = git_env_ulong(\"GIT_PROGRESS_DELAY\", 1);\n \n \treturn delay_in_secs;\n }\n-- \n2.51.0.205.g9a02ae2892\n\n\n"},{"id":"524901","messageId":"xmqqh5xvnhek.fsf@gitster.g","threadId":"64019","inReplyTo":"7b848623-ce64-4679-9b5e-9d91d947b269@kdbg.org","subject":"Re: [PATCH v2] progress: pay attention to (customized) delay time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-25T22:52:19Z","receivedAt":"2025-08-25T22:52:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>  Compared to the first round, this replaces sig_atomic_t by int. I\n>  didn't use bool just in case the patch goes on top of a maintenance\n>  track that does not have the \"bool is allowed\" policy.\n\nThat's fair.  I happened to have chosen v2.51.0 as the base for v1\nduring today's integration, but as a fix-up topic, you are correct\nto point out that I should apply this on top of maint-2.50 or even\nolder.\n\nThanks, will requeue.\n"}]}