Re: [PATCH v2 6/8] maintenance: add ability to pass config options

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2022, 06:57 UTC
Message-ID: <xmqqsfs7fn6v.fsf@gitster.g>
In-Reply-To: <cfa6dca8ef4a64b6233e3b7b6021571447fc5ba9.1645719218.git.gitgitgadget@gmail.com>

"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	if (config_opts)
> +		for (i = 0; i<config_opts->nr; i++)

Style.  SP on both sides of '<'.

> +			strvec_pushl(&maint.args, "-c", config_opts->v[i], NULL);

>  	strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL);
>  	strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet");

It is unclear if it is generally a good idea to pass hardcoded set of configuration variables to begin with, but provided if it makes sense [*], the implementation seems OK.

	Side note.  And this is a big *IF*, as we can see in all
	other helper functions in run-commands.h, nobody has such a
	privision.  If supporting such a "feature" makes sense, we
	probably would need to do so with a common interface that
	can be used across run_command() API, not with an ad-hoc
	interface that is only usable with run_auto_maintenance(),
	which may look somewhat similar to how we have a common way
	to pass set of environment variables.

Show 17 quoted lines

> diff --git a/run-command.h b/run-command.h
> index 07bed6c31b4..24021abd41f 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -222,8 +222,11 @@ int run_command(struct child_process *);
>  
>  /*
>   * Trigger an auto-gc
> + *
> + * config_opts is an optional list of additional config options to
> + * pass to the maintenance process in the form "some.option=value".
>   */
> -int run_auto_maintenance(int quiet);
> +int run_auto_maintenance(int quiet, const struct strvec *config_opts);
>  
>  #define RUN_COMMAND_NO_STDIN		(1<<0)
>  #define RUN_GIT_CMD			(1<<1)