Re: [PATCH 3/7] builtin/gc: fix leaking config values

James Liu
2024-08-15T05:22:08Z

On Tue Aug 13, 2024 at 5:17 PM AEST, Patrick Steinhardt wrote:
> Note that there is one small gotcha here with the "--prune" option. Next
> to passing a string, this option also accepts the "--no-prune" option
> that overrides the default or configured value. We thus need to discern
> between the option not having been passed by the user and the negative
> variant of it. This is done by using a simple sentinel value that lets
> us discern these cases.
>
> @@ -644,12 +673,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> struct child_process rerere_cmd = CHILD_PROCESS_INIT;
> struct maintenance_run_opts opts = {0};
> struct gc_config cfg = GC_CONFIG_INIT;
> + const char *prune_expire_sentinel = "sentinel";
> + const char *prune_expire_arg = prune_expire_sentinel;
> + int ret;
>
> struct option builtin_gc_options[] = {
> OPT__QUIET(&quiet, N_("suppress progress reporting")),
> - { OPTION_STRING, 0, "prune", &cfg.prune_expire, N_("date"),
> + { OPTION_STRING, 0, "prune", &prune_expire_arg, N_("date"),
> N_("prune unreferenced objects"),
> - PARSE_OPT_OPTARG, NULL, (intptr_t)cfg.prune_expire },
> + PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire_arg },
> OPT_BOOL(0, "cruft", &cfg.cruft_packs, N_("pack unreferenced objects separately")),
> OPT_MAGNITUDE(0, "max-cruft-size", &cfg.max_cruft_size,
> N_("with --cruft, limit the size of new cruft packs")),

I was wondering how the `no-*` options worked since they're not
explicitly defined in the `builtin_gc_options` array. I guess
they're handled internally by `parse_options()`, and if the `no-` prefix
was present, it leaves that argument unset.