{"thread":{"id":"63549","subject":"Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`","startedAt":"2025-05-30T13:36:54Z","lastAt":"2025-05-30T14:05:32Z","messageCount":2,"participants":["Ben Knoble","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"519237","messageId":"6EE3E4F8-16DB-45AB-AD67-FADE12FBFBBD@gmail.com","threadId":"63549","inReplyTo":null,"subject":"Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-05-30T12:56:36Z","receivedAt":"2025-05-30T13:36:54Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 27 mai 2025 à 10:05, Patrick Steinhardt <ps@pks.im> a écrit :\n> \n> The `gc_before_repack()` should only ever run once in git-gc(1), but we\n> may end up calling it twice when the \"--detach\" flag is passed. The\n> duplicated call is avoided though via a static flag in this function.\n> \n> This pattern is somewhat unintuitive though. Refactor it to drop the\n> static flag and instead guard the second call of `gc_before_repack()`\n> via `opts.detach`.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> builtin/gc.c | 24 +++++++++---------------\n> 1 file changed, 9 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index e5d1114bd2d..174357b9c25 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -816,22 +816,14 @@ static int report_last_gc_error(void)\n>  return ret;\n> }\n> \n> -static void gc_before_repack(struct maintenance_run_opts *opts,\n> -                 struct gc_config *cfg)\n> +static int gc_before_repack(struct maintenance_run_opts *opts,\n> +                struct gc_config *cfg)\n> {\n> -    /*\n> -     * We may be called twice, as both the pre- and\n> -     * post-daemonized phases will call us, but running these\n> -     * commands more than once is pointless and wasteful.\n> -     */\n> -    static int done = 0;\n> -    if (done++)\n> -        return;\n> -\n>  if (cfg->pack_refs && maintenance_task_pack_refs(opts, cfg))\n> -        die(FAILED_RUN, \"pack-refs\");\n> +        return error(FAILED_RUN, \"pack-refs\");\n>  if (cfg->prune_reflogs && maintenance_task_reflog_expire(opts, cfg))\n> -        die(FAILED_RUN, \"reflog\");\n> +        return error(FAILED_RUN, \"reflog\");\n> +    return 0;\n> }\n> \n> int cmd_gc(int argc,\n> @@ -965,7 +957,8 @@ int cmd_gc(int argc,\n>          goto out;\n>      }\n> \n> -        gc_before_repack(&opts, &cfg); /* dies on failure */\n> +        if (gc_before_repack(&opts, &cfg) < 0)\n> +            exit(127);\n\nIf 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!\n\nWhat 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.\n\n>      delete_tempfile(&pidfile);\n> \n>      /*\n> @@ -995,7 +988,8 @@ int cmd_gc(int argc,\n>      free(path);\n>  }\n> \n> -    gc_before_repack(&opts, &cfg);\n> +    if (opts.detach <= 0)\n> +        gc_before_repack(&opts, &cfg);\n> \n>  if (!repository_format_precious_objects) {\n>      struct child_process repack_cmd = CHILD_PROCESS_INIT;\n> \n> --\n> 2.49.0.1266.g31b7d2e469.dirty\n"},{"id":"519242","messageId":"aDm7KdKrjhjDToqH@pks.im","threadId":"63549","inReplyTo":"6EE3E4F8-16DB-45AB-AD67-FADE12FBFBBD@gmail.com","subject":"Re: [PATCH 10/11] builtin/gc: avoid global state in `gc_before_repack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-30T14:05:29Z","receivedAt":"2025-05-30T14:05:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 30, 2025 at 08:56:36AM -0400, Ben Knoble wrote:\n> > @@ -965,7 +957,8 @@ int cmd_gc(int argc,\n> >          goto out;\n> >      }\n> > \n> > -        gc_before_repack(&opts, &cfg); /* dies on failure */\n> > +        if (gc_before_repack(&opts, &cfg) < 0)\n> > +            exit(127);\n> \n> If I (a relative novice to this part of the code) am reading\n> correctly, we trade an implicit die in a private helper for explicit\n> exit in a « main » function, which I find much easier to reason about.\n> Nice!\n> \n> What I don’t see (being away from the rest of the source at the\n> moment) is where 127 comes from. I don’t intend a crusade against\n> magic numbers :) and I’ve certainly seen enough exit-codes of 127 to\n> guess what this means, but reading only the patch the number does\n> appear out of thin air.\n\nThe funny thing is that 127 isn't even correct -- it should be 128.\nMaybe we can adapt `die_builtin()` so that it knows to not write\nanything when the first argument is a NULL pointer?\n\nPatrick\n"}]}