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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 15, 2024 at 03:22:04PM +1000, James Liu wrote:
> 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.

Yes, this is being handled by "parse-options.c". But With the `no-`
prefix it won't leave the argument unset, but will explicitly unset it.
So if the argument was already set to some value X, that value would be
unset when parsing the `no-` variant.

Patrick