Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`

Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`

[Ben Knoble]

> Le 27 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> The `gc_before_repack()` should only ever run once in git-gc(1), but we
> may end up calling it twice when the "--detach" flag is passed. The
> duplicated call is avoided though via a static flag in this function.
> 
> This pattern is somewhat unintuitive though. Refactor it to drop the
> static flag and instead guard the second call of `gc_before_repack()`
> via `opts.detach`.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/gc.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index e5d1114bd2d..174357b9c25 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -816,22 +816,14 @@ static int report_last_gc_error(void)
>  return ret;
> }
> 
> -static void gc_before_repack(struct maintenance_run_opts *opts,
> -                 struct gc_config *cfg)
> +static int gc_before_repack(struct maintenance_run_opts *opts,
> +                struct gc_config *cfg)
> {
> -    /*
> -     * We may be called twice, as both the pre- and
> -     * post-daemonized phases will call us, but running these
> -     * commands more than once is pointless and wasteful.
> -     */
> -    static int done = 0;
> -    if (done++)
> -        return;
> -
>  if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
> -        die(FAILED_RUN, "pack-refs");
> +        return error(FAILED_RUN, "pack-refs");
>  if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))
> -        die(FAILED_RUN, "reflog");
> +        return error(FAILED_RUN, "reflog");
> +    return 0;
> }
> 
> int cmd_gc(int argc,
> @@ -965,7 +957,8 @@ int cmd_gc(int argc,
>          goto out;
>      }
> 
> -        gc_before_repack(&opts, &cfg); /* dies on failure */
> +        if (gc_before_repack(&opts, &cfg) < 0)
> +            exit(127);

If I (a relative novice to this part of the code) am reading correctly, we trade an implicit die in a private helper for explicit exit in a « main » function, which I find much easier to reason about. Nice!

What I don’t see (being away from the rest of the source at the moment) is where 127 comes from. I don’t intend a crusade against magic numbers :) and I’ve certainly seen enough exit-codes of 127 to guess what this means, but reading only the patch the number does appear out of thin air.

>      delete_tempfile(&pidfile);
> 
>      /*
> @@ -995,7 +988,8 @@ int cmd_gc(int argc,
>      free(path);
>  }
> 
> -    gc_before_repack(&opts, &cfg);
> +    if (opts.detach <= 0)
> +        gc_before_repack(&opts, &cfg);
> 
>  if (!repository_format_precious_objects) {
>      struct child_process repack_cmd = CHILD_PROCESS_INIT;
> 
> --
> 2.49.0.1266.g31b7d2e469.dirty

Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt]

On Fri, May 30, 2025 at 08:56:36AM -0400, Ben Knoble wrote:
> > @@ -965,7 +957,8 @@ int cmd_gc(int argc,
> >          goto out;
> >      }
> > 
> > -        gc_before_repack(&opts, &cfg); /* dies on failure */
> > +        if (gc_before_repack(&opts, &cfg) < 0)
> > +            exit(127);
> 
> If I (a relative novice to this part of the code) am reading
> correctly, we trade an implicit die in a private helper for explicit
> exit in a « main » function, which I find much easier to reason about.
> Nice!
> 
> What I don’t see (being away from the rest of the source at the
> moment) is where 127 comes from. I don’t intend a crusade against
> magic numbers :) and I’ve certainly seen enough exit-codes of 127 to
> guess what this means, but reading only the patch the number does
> appear out of thin air.

The funny thing is that 127 isn't even correct -- it should be 128.
Maybe we can adapt `die_builtin()` so that it knows to not write
anything when the first argument is a NULL pointer?

Patrick

[PATCH 00/11] builtin/maintenance: fix ref lock races when detaching

[Patrick Steinhardt]

Hi,

this patch series fixes races around locking the "packed-refs" file when
auto-maintenance decides to repack it. This issue has been reported e.g.
via [1] and [2].

The root cause is that git-gc(1) used to know to detach _after_ having
repacked references. As such, callers wouldn't continue with their thing
until we have already packed refs, and thus the race does not exist
there. git-maintenance(1) didn't have the same split though, so this
patch series retrofits that logic.

The series is structured as follows:

  - Patches 1 and 2 do some light refactorings.

  - Patches 3 to 5 refactor how we set up the list of tasks to not rely
    on globals anymore. Instead, we now have a single source of truth
    for which tasks exactly will be run.

  - The remaining patches introduce the split of before/after-detach
    tasks and wire them up for "pack-refs", "reflog-expire" and "gc"
    tasks.

Thanks!

Patrick

[1]: <CAJR-fbZ4X1+gN75m2dUvocR6NkowLOZ9F26cjBy8w1qd181OoQ@mail.gmail.com>
[2]: <CANi7bVAkNc+gY1NoXfJuDRjxjZLTgL8Lfn8_ZmWsvLAoiLPkNg@mail.gmail.com>

---
Patrick Steinhardt (11):
      builtin/gc: use designated field initializers for maintenance tasks
      builtin/gc: drop redundant local variable
      builtin/maintenance: centralize configuration of explicit tasks
      builtin/maintenance: mark "--task=" and "--schedule=" as incompatible
      builtin/maintenance: stop modifying global array of tasks
      builtin/maintenance: extract function to run tasks
      builtin/maintenance: fix typedef for function pointers
      builtin/maintenance: let tasks do maintenance before and after detach
      builtin/maintenance: fix locking race when packing refs and reflogs
      builtin/gc: avoid global state in `gc_before_repack()`
      builtin/maintenance: fix locking race when handling "gc" task

 builtin/gc.c           | 386 +++++++++++++++++++++++++++----------------------
 t/t7900-maintenance.sh |  19 ++-
 2 files changed, 229 insertions(+), 176 deletions(-)


---
base-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229
change-id: 20250527-b4-pks-maintenance-ref-lock-race-11ae5d68e06f

[PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`

[Patrick Steinhardt]

The `gc_before_repack()` should only ever run once in git-gc(1), but we
may end up calling it twice when the "--detach" flag is passed. The
duplicated call is avoided though via a static flag in this function.

This pattern is somewhat unintuitive though. Refactor it to drop the
static flag and instead guard the second call of `gc_before_repack()`
via `opts.detach`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e5d1114bd2d..174357b9c25 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -816,22 +816,14 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
-static void gc_before_repack(struct maintenance_run_opts *opts,
-			     struct gc_config *cfg)
+static int gc_before_repack(struct maintenance_run_opts *opts,
+			    struct gc_config *cfg)
 {
-	/*
-	 * We may be called twice, as both the pre- and
-	 * post-daemonized phases will call us, but running these
-	 * commands more than once is pointless and wasteful.
-	 */
-	static int done = 0;
-	if (done++)
-		return;
-
 	if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))
-		die(FAILED_RUN, "pack-refs");
+		return error(FAILED_RUN, "pack-refs");
 	if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))
-		die(FAILED_RUN, "reflog");
+		return error(FAILED_RUN, "reflog");
+	return 0;
 }
 
 int cmd_gc(int argc,
@@ -965,7 +957,8 @@ int cmd_gc(int argc,
 			goto out;
 		}
 
-		gc_before_repack(&opts, &cfg); /* dies on failure */
+		if (gc_before_repack(&opts, &cfg) < 0)
+			exit(127);
 		delete_tempfile(&pidfile);
 
 		/*
@@ -995,7 +988,8 @@ int cmd_gc(int argc,
 		free(path);
 	}
 
-	gc_before_repack(&opts, &cfg);
+	if (opts.detach <= 0)
+		gc_before_repack(&opts, &cfg);
 
 	if (!repository_format_precious_objects) {
 		struct child_process repack_cmd = CHILD_PROCESS_INIT;

-- 
2.49.0.1266.g31b7d2e469.dirty