Re: [PATCH v2 0/2] config: allow specifying config entries via envvar pairs

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

On Wed, Nov 25, 2020 at 05:41:14AM -0500, Jeff King wrote:
> On Tue, Nov 24, 2020 at 11:50:46AM +0100, Patrick Steinhardt wrote:
> 
> >     - I've changed priorities. The envvars are treated as command-level
> >       and as such override all values configured in files. But any
> >       explicit `git -c key=value` will now override these envvars.
> 
> That ordering makes sense. Those get passed through the environment,
> too, but at some point there is a process where your new ones are in the
> environment and the "-c" ones are on the command-line.
> 
> I do still think that a "--config-env" option solves your problem in a
> much simpler way (especially in terms of interface we expose to users
> that we'll be locked into forever). I sketched out the solution below if
> it's of interest (and I'd be happy to polish it up, or hand it off to
> you if so). But if you're unconvinced, I'll stop mentioning it.
> 
> diff --git a/config.c b/config.c
> index 8f324ed3a6..d8cf6a5d6b 100644
> --- a/config.c
> +++ b/config.c
> @@ -345,6 +345,27 @@ void git_config_push_parameter(const char *text)
>  	strbuf_release(&env);
>  }
>  
> +void git_config_push_env(const char *spec)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *env_name;
> +	const char *env_value;
> +
> +	env_name = strchr(spec, '=');
> +	if (!env_name)
> +		return; /* die or warn? */
> +	env_name++;
> +
> +	env_value = getenv(env_name);
> +	if (!env_value)
> +		return; /* die or warn? */
> +
> +	strbuf_add(&buf, spec, env_name - spec);
> +	strbuf_addstr(&buf, env_value);
> +	git_config_push_parameter(buf.buf);
> +	strbuf_release(&buf);
> +}

I realize that you say it's yet unpolished, but doesn't this have
parsing issues? The first strchr(3P) probably needs to be a strrchr(3P)
to correctly parse `includeIf./home/foo/=repo.path=MY_PATH_ENV`. But
we'd also have to handle shell quoting for the user, don't we?

Anyway, I'd be happy to adopt is as part of the series if we care
enough. For now I'll send out the current state I have though.

Patrick

>  static inline int iskeychar(int c)
>  {
>  	return isalnum(c) || c == '-';
> diff --git a/config.h b/config.h
> index 91cdfbfb41..d05651c96c 100644
> --- a/config.h
> +++ b/config.h
> @@ -138,6 +138,7 @@ int git_config_from_mem(config_fn_t fn,
>  int git_config_from_blob_oid(config_fn_t fn, const char *name,
>  			     const struct object_id *oid, void *data);
>  void git_config_push_parameter(const char *text);
> +void git_config_push_env(const char *spec);
>  int git_config_from_parameters(config_fn_t fn, void *data);
>  void read_early_config(config_fn_t cb, void *data);
>  void read_very_early_config(config_fn_t cb, void *data);
> diff --git a/git.c b/git.c
> index 4b7bd77b80..342f2fb0c9 100644
> --- a/git.c
> +++ b/git.c
> @@ -254,6 +254,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			git_config_push_parameter((*argv)[1]);
>  			(*argv)++;
>  			(*argc)--;
> +		} else if (skip_prefix(cmd, "--config-env=", &cmd)) {
> +			git_config_push_env(cmd);
>  		} else if (!strcmp(cmd, "--literal-pathspecs")) {
>  			setenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, "1", 1);
>  			if (envchanged)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]