Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Philip Oakley <philipoakley@iee.email>
Subject: Re: [PATCH v2 2/2] config: allow specifying config entries via envvar pairs
Date: Wed, 25 Nov 2020 10:00:59 +0100
Message-ID: <87360xq08k.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <97740ada840a1e2f151003e695de9f2efa5a7e62.1606214397.git.ps@pks.im>


On Tue, Nov 24 2020, Patrick Steinhardt wrote:

...some more feedback.

> +GIT_CONFIG_COUNT,GIT_CONFIG_KEY_<n>,GIT_CONFIG_VALUE_<n>::
> +	If GIT_CONFIG_COUNT is set to a positive number, all environment pairs
> +	GIT_CONFIG_KEY_<n> and GIT_CONFIG_VALUE_<n> up to that number will be
> +	added to the process's runtime configuration. The config pairs are
> +	zero-indexed. Any missing key or value is treated as an error. An empty
> +	GIT_CONFIG_COUNT is treated the same as GIT_CONFIG_COUNT=0, namely no
> +	pairs are processed. Config entries set this way have command scope,
> +	but will be overridden by any explicit options passed via `git -c`.

Perhaps work in some/all of some version of these:

 - There's also a GIT_CONFIG_PARAMETERS variable, which is considered
   internal to Git itself. Users are expected to set these.

   --> I.e. even if we're not going to support some format for
   --> GIT_CONFIG_PARAMETERS document what it is.

 - This is analogous to the pre-receive `GIT_PUSH_OPTION_*` variables
   (see linkgit:githooks[5]), but unlike those the `-c` option to
   linkgit:git(1) does not set `GIT_CONFIG_*`.

 - Saying "command scope" here I think is wrong/misleading. If I didn't
   know how this worked I'd expect the first git process to see it to
   delete it from the env, so e.g. the "fetch" command would see it, but
   not the "gc" it spawned (different commands). Maybe just say "the
   scope of these is as with other GIT_* environment variables, they'll
   be inherited by subprocesses".

> diff --git a/cache.h b/cache.h
> index c0072d43b1..8a36146337 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -472,6 +472,7 @@ static inline enum object_type object_type(unsigned int mode)
>  #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR"
>  #define CONFIG_ENVIRONMENT "GIT_CONFIG"
>  #define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS"
> +#define CONFIG_COUNT_ENVIRONMENT "GIT_CONFIG_COUNT"

I was wondering if this shouldn't be "GIT_CONFIG_KEY_COUNT" to be
consistent with the push options environment, but on a closer look we
have:

 - GIT_CONFIG_COUNT
 - GIT_CONFIG_KEY_N
 - GIT_CONFIG_VALUE_N
 - GIT_PUSH_OPTION_COUNT
 - GIT_PUSH_OPTION_N

So I guess that makes sense & is consistent since we'd like to split the
key-value here to save the user the effort of figuring out which "="
they should split on.

> -	if (!env)
> -		return 0;
> -

Re the indent question to make the diff more readable question Junio
had: could set some "do we have this or that" variables here to not
reindent the existing code, but maybe not worth the effort...

> -	if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
> -		ret = error(_("bogus format in %s"), CONFIG_DATA_ENVIRONMENT);
> -		goto out;
> +		count = strtoul(env, &endp, 10);
> +		if (*endp) {
> +			ret = error(_("bogus count in %s"), CONFIG_COUNT_ENVIRONMENT);
> +			goto out;
> +		}
> +		if (count > INT_MAX) {
> +			ret = error(_("too many entries in %s"), CONFIG_COUNT_ENVIRONMENT);
> +			goto out;
> +		}
> +
> +		for (i = 0; i < count; i++) {
> +			const char *key, *value;
> +
> +			strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
> +			key = getenv(envvar.buf);
> +			if (!key) {
> +				ret = error(_("missing config key %s"), envvar.buf);
> +				goto out;
> +			}
> +			strbuf_reset(&envvar);
> +
> +			strbuf_addf(&envvar, "GIT_CONFIG_VALUE_%d", i);
> +			value = getenv(envvar.buf);
> +			if (!value) {
> +				ret = error(_("missing config value %s"), envvar.buf);
> +				goto out;
> +			}
> +			strbuf_reset(&envvar);
> +
> +			if (config_parse_pair(key, value, fn, data) < 0) {
> +				ret = -1;
> +				goto out;
> +			}
> +		}
>  	}
>  
> -	for (i = 0; i < nr; i++) {
> -		if (git_config_parse_parameter(argv[i], fn, data) < 0) {
> -			ret = -1;
> +	env = getenv(CONFIG_DATA_ENVIRONMENT);
> +	if (env) {
> +		int nr = 0, alloc = 0;
> +
> +		/* sq_dequote will write over it */
> +		envw = xstrdup(env);
> +
> +		if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
> +			ret = error(_("bogus format in %s"), CONFIG_DATA_ENVIRONMENT);
>  			goto out;
>  		}
> +
> +		for (i = 0; i < nr; i++) {
> +			if (git_config_parse_parameter(argv[i], fn, data) < 0) {
> +				ret = -1;
> +				goto out;
> +			}
> +		}
>  	}
>  
>  out:
> +	strbuf_release(&envvar);
>  	free(argv);
>  	free(envw);
>  	cf = source.prev;
> diff --git a/environment.c b/environment.c
> index bb518c61cd..e94eca92f3 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -116,6 +116,7 @@ const char * const local_repo_env[] = {
>  	ALTERNATE_DB_ENVIRONMENT,
>  	CONFIG_ENVIRONMENT,
>  	CONFIG_DATA_ENVIRONMENT,
> +	CONFIG_COUNT_ENVIRONMENT,
>  	DB_ENVIRONMENT,
>  	GIT_DIR_ENVIRONMENT,
>  	GIT_WORK_TREE_ENVIRONMENT,
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 825d9a184f..8c90cca79d 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1316,6 +1316,107 @@ test_expect_success 'detect bogus GIT_CONFIG_PARAMETERS' '
>  		git config --get-regexp "env.*"
>  '
>  
> +test_expect_success 'git config handles environment config pairs' '

I was wondering if the patch would keep the current
GIT_CONFIG_PARAMETERS or replace it entirely with the new facility.

On the one hand it would make sense to just replace
GIT_CONFIG_PARAMETERS, we could make this code loop over the new values.

On the other hand, and this is an edge case I hadn't considered before,
any change to the semantics of GIT_CONFIG_PARAMETERS means that e.g. a
fetch->gc spawning would break in the face of a concurrent OS update to
/usr/bin/git, since "fetch" and "gc" might be of differing versions

  parent reply index

Thread overview: 116+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-24 10:50 [PATCH v2 0/2] " Patrick Steinhardt
2020-11-24 10:50 ` [PATCH v2 1/2] config: extract function to parse config pairs Patrick Steinhardt
2020-11-24 10:50 ` [PATCH v2 2/2] config: allow specifying config entries via envvar pairs Patrick Steinhardt
2020-11-25  3:39   ` Junio C Hamano
2020-11-25  7:06     ` Patrick Steinhardt
2020-11-25  7:41       ` Junio C Hamano
2020-11-25  7:57         ` Patrick Steinhardt
2020-11-25  8:47   ` Ævar Arnfjörð Bjarmason
2020-11-25  9:00   ` Ævar Arnfjörð Bjarmason [this message]
2020-11-25 19:50     ` Junio C Hamano
2020-11-25 10:41 ` [PATCH v2 0/2] " Jeff King
2020-11-25 13:16   ` Patrick Steinhardt
2020-11-26  0:36     ` Jeff King
2020-11-25 20:28   ` Junio C Hamano
2020-11-25 22:47   ` brian m. carlson
2020-11-26  6:31     ` Patrick Steinhardt
2020-12-01  9:47   ` Patrick Steinhardt
2020-12-01 11:30     ` Jeff King
2020-12-01  9:55 ` [PATCH v3 0/4] " Patrick Steinhardt
2020-12-01  9:55   ` [PATCH v3 1/4] environment: make `getenv_safe()` non-static Patrick Steinhardt
2020-12-01  9:56   ` [PATCH v3 2/4] config: extract function to parse config pairs Patrick Steinhardt
2020-12-01  9:56   ` [PATCH v3 3/4] config: refactor parsing of GIT_CONFIG_PARAMETERS Patrick Steinhardt
2020-12-01  9:56   ` [PATCH v3 4/4] config: allow specifying config entries via envvar pairs Patrick Steinhardt
2020-12-09 11:52 ` [PATCH v4 0/6] config: allow specifying config entries via env Patrick Steinhardt
2020-12-09 11:52   ` [PATCH v4 1/6] git: add `--super-prefix` to usage string Patrick Steinhardt
2020-12-09 11:52   ` [PATCH v4 2/6] config: add new way to pass config via `--config-env` Patrick Steinhardt
2020-12-09 14:40     ` Ævar Arnfjörð Bjarmason
2020-12-09 16:24       ` Jeff King
2020-12-11 13:24         ` Patrick Steinhardt
2020-12-11 14:21           ` Jeff King
2020-12-11 14:54             ` Patrick Steinhardt
2020-12-11 16:10               ` Jeff King
2020-12-09 16:10     ` Jeff King
2020-12-09 16:12       ` [PATCH 1/3] quote: make sq_dequote_step() a public function Jeff King
2020-12-09 16:17       ` [PATCH 2/3] config: parse more robust format in GIT_CONFIG_PARAMETERS Jeff King
2020-12-09 16:20       ` [PATCH 3/3] config: store "git -c" variables using more robust format Jeff King
2020-12-09 16:34         ` Jeff King
2020-12-10 20:55         ` Ævar Arnfjörð Bjarmason
2020-12-10 21:49           ` Junio C Hamano
2020-12-11 13:21           ` Jeff King
2020-12-10  0:00       ` [PATCH v4 2/6] config: add new way to pass config via `--config-env` Junio C Hamano
2020-12-10  0:09         ` Jeff King
2020-12-10  0:57           ` Junio C Hamano
2020-12-11 13:24       ` Patrick Steinhardt
2020-12-11 14:20         ` Jeff King
2020-12-09 11:52   ` [PATCH v4 3/6] environment: make `getenv_safe()` non-static Patrick Steinhardt
2020-12-09 11:52   ` [PATCH v4 4/6] config: extract function to parse config pairs Patrick Steinhardt
2020-12-09 13:12     ` Ævar Arnfjörð Bjarmason
2020-12-09 11:52   ` [PATCH v4 5/6] config: refactor parsing of GIT_CONFIG_PARAMETERS Patrick Steinhardt
2020-12-09 11:52   ` [PATCH v4 6/6] config: allow specifying config entries via envvar pairs Patrick Steinhardt
2020-12-09 15:29   ` [PATCH v4 0/6] config: allow specifying config entries via env Ævar Arnfjörð Bjarmason
2020-12-11 13:35     ` Patrick Steinhardt
2020-12-11 14:27       ` Jeff King
2020-12-11 14:42         ` Jeff King
2020-12-11 14:58           ` Patrick Steinhardt
2020-12-11 14:47         ` Patrick Steinhardt
2020-12-11 15:21           ` Ævar Arnfjörð Bjarmason
2020-12-11 16:02           ` Jeff King
2020-12-16  7:52 ` [PATCH v5 0/8] " Patrick Steinhardt
2020-12-16  7:52   ` [PATCH v5 1/8] git: add `--super-prefix` to usage string Patrick Steinhardt
2020-12-16  7:52   ` [PATCH v5 2/8] config: add new way to pass config via `--config-env` Patrick Steinhardt
2020-12-23 21:35     ` Junio C Hamano
2020-12-16  7:54   ` [PATCH v5 4/8] config: extract function to parse config pairs Patrick Steinhardt
2020-12-16  7:54   ` [PATCH v5 7/8] environment: make `getenv_safe()` a public function Patrick Steinhardt
2020-12-16  7:54   ` [PATCH v5 8/8] config: allow specifying config entries via envvar pairs Patrick Steinhardt
2020-12-23 21:14     ` Junio C Hamano
2020-12-23 21:55       ` Junio C Hamano
2021-01-06 10:28         ` Patrick Steinhardt
2021-01-06 21:07           ` Junio C Hamano
2020-12-16  7:56   ` [PATCH v5 3/8] quote: make sq_dequote_step() a public function Patrick Steinhardt
2020-12-16  7:56   ` [PATCH v5 5/8] config: store "git -c" variables using more robust format Patrick Steinhardt
2020-12-16  7:57   ` [PATCH v5 6/8] config: parse more robust format in GIT_CONFIG_PARAMETERS Patrick Steinhardt
2020-12-16 20:01     ` Phillip Wood
2021-01-07  6:36 ` [PATCH v6 0/8] config: allow specifying config entries via env Patrick Steinhardt
2021-01-07  6:36   ` [PATCH v6 1/8] git: add `--super-prefix` to usage string Patrick Steinhardt
2021-01-07  6:36   ` [PATCH v6 2/8] config: add new way to pass config via `--config-env` Patrick Steinhardt
2021-01-10 20:29     ` Simon Ruderich
2021-01-11  0:29       ` Junio C Hamano
2021-01-11  8:24         ` Patrick Steinhardt
2021-01-07  6:36   ` [PATCH v6 3/8] quote: make sq_dequote_step() a public function Patrick Steinhardt
2021-01-07  6:37   ` [PATCH v6 4/8] config: extract function to parse config pairs Patrick Steinhardt
2021-01-07  6:37   ` [PATCH v6 5/8] config: store "git -c" variables using more robust format Patrick Steinhardt
2021-01-07  6:37   ` [PATCH v6 6/8] config: parse more robust format in GIT_CONFIG_PARAMETERS Patrick Steinhardt
2021-01-07  6:37   ` [PATCH v6 7/8] environment: make `getenv_safe()` a public function Patrick Steinhardt
2021-01-07  6:37   ` [PATCH v6 8/8] config: allow specifying config entries via envvar pairs Patrick Steinhardt
2021-01-11  8:36 ` [PATCH v7 0/8] " Patrick Steinhardt
2021-01-11  8:36   ` [PATCH v7 1/8] git: add `--super-prefix` to usage string Patrick Steinhardt
2021-01-11  8:36   ` [PATCH v7 2/8] config: add new way to pass config via `--config-env` Patrick Steinhardt
2021-01-11 22:34     ` Junio C Hamano
2021-01-11  8:36   ` [PATCH v7 3/8] quote: make sq_dequote_step() a public function Patrick Steinhardt
2021-01-11  8:36   ` [PATCH v7 4/8] config: extract function to parse config pairs Patrick Steinhardt
2021-01-11  8:37   ` [PATCH v7 5/8] config: store "git -c" variables using more robust format Patrick Steinhardt
2021-01-11  8:37   ` [PATCH v7 6/8] config: parse more robust format in GIT_CONFIG_PARAMETERS Patrick Steinhardt
2021-01-11  8:37   ` [PATCH v7 7/8] environment: make `getenv_safe()` a public function Patrick Steinhardt
2021-01-11  8:37   ` [PATCH v7 8/8] config: allow specifying config entries via envvar pairs Patrick Steinhardt
2021-01-12 12:26 ` [PATCH v8 0/8] " Patrick Steinhardt
2021-01-12 12:26   ` [PATCH v8 1/8] git: add `--super-prefix` to usage string Patrick Steinhardt
2021-01-12 12:26   ` [PATCH v8 2/8] config: add new way to pass config via `--config-env` Patrick Steinhardt
2021-04-16 15:40     ` Ævar Arnfjörð Bjarmason
2021-04-17  8:38       ` Jeff King
2021-04-19 15:28         ` Patrick Steinhardt
2021-04-20 11:01           ` Ævar Arnfjörð Bjarmason
2021-04-20 10:59         ` Ævar Arnfjörð Bjarmason
2021-04-23 10:05           ` Jeff King
2021-05-19 11:36             ` Ævar Arnfjörð Bjarmason
2021-01-12 12:26   ` [PATCH v8 3/8] quote: make sq_dequote_step() a public function Patrick Steinhardt
2021-01-12 12:26   ` [PATCH v8 4/8] config: extract function to parse config pairs Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v8 5/8] config: store "git -c" variables using more robust format Patrick Steinhardt
2021-01-15 19:16     ` Jeff King
2021-01-20  6:29       ` Patrick Steinhardt
2021-01-20  6:55         ` Junio C Hamano
2021-01-20  7:42           ` Patrick Steinhardt
2021-01-20 22:28             ` Junio C Hamano
2021-01-12 12:27   ` [PATCH v8 6/8] config: parse more robust format in GIT_CONFIG_PARAMETERS Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v8 7/8] environment: make `getenv_safe()` a public function Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v8 8/8] config: allow specifying config entries via envvar pairs Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87360xq08k.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=philipoakley@iee.email \
    --cc=ps@pks.im \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git