> Le 27 mai 2025 à 10:05, Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx>
> ---
> 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
