| Message ID | 97740ada840a1e2f151003e695de9f2efa5a7e62.1606214397.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config: allow specifying config entries via envvar pairs | expand |
Patrick Steinhardt <ps@pks.im> writes: > +GIT_CONFIG_COUNT,GIT_CONFIG_KEY_<n>,GIT_CONFIG_VALUE_<n>:: I think we write a header with multiple/related items like this instead: GIT_CONFIG_COUNT:: GIT_CONFIG_KEY_<n>:: GIT_CONFIG_VALUE_<n>:: See how -f/--file is marked up in an earlier part of the same file. > + 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`. > + > See also <<FILES>>. Doesn't this <<FILES>> refer to GIT_CONFIG and GIT_CONFIG_NOSYSTEM that are described earlier? It certainly looks out of place to see it after the KEY/VALUE thing. > + 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); Didn't we got bitten by number of times that the string returned by getenv() are not necessarily nonvolatile depending on platforms? I think the result of getenv() would need to be xstrdup'ed. cf. 6776a84d (diff: ensure correct lifetime of external_diff_cmd, 2019-01-11) > + 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; With re-indentation this patch does, it is a bit hard to see the correspondence between common lines in preimage and postimage, but I think the patch adds the support of the new style environments before the existing support of the GIT_CONFIG_DATA, but when there is no compelling reason not to, new code should be added near the bottom, not before the existing code, in the function. Otherwise, this part of the patch looks OK to me. Thanks.
On Tue, Nov 24, 2020 at 07:39:48PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > +GIT_CONFIG_COUNT,GIT_CONFIG_KEY_<n>,GIT_CONFIG_VALUE_<n>:: > > I think we write a header with multiple/related items like this > instead: > > GIT_CONFIG_COUNT:: > GIT_CONFIG_KEY_<n>:: > GIT_CONFIG_VALUE_<n>:: > > See how -f/--file is marked up in an earlier part of the same file. Ah, thanks. I wondered how to format these but didn't spot other examples. > > + 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`. > > + > > See also <<FILES>>. > > Doesn't this <<FILES>> refer to GIT_CONFIG and GIT_CONFIG_NOSYSTEM > that are described earlier? It certainly looks out of place to see > it after the KEY/VALUE thing. Right, my fault. > > + 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); > > Didn't we got bitten by number of times that the string returned by > getenv() are not necessarily nonvolatile depending on platforms? I > think the result of getenv() would need to be xstrdup'ed. > > cf. 6776a84d (diff: ensure correct lifetime of external_diff_cmd, > 2019-01-11) We did, but do we have to in this case? There is no interleaving calls to getenv(3P), so we don't depend on at least $n getenv(3P) calls succeeding without clobbering old values. It's true that it could be that any other caller in the callchain clobbers the value, but as far as I can see none does. Anyway, I'm not opposed to changing this if you think it to be necessary. > > + 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; > > With re-indentation this patch does, it is a bit hard to see the > correspondence between common lines in preimage and postimage, but I > think the patch adds the support of the new style environments > before the existing support of the GIT_CONFIG_DATA, but when there > is no compelling reason not to, new code should be added near the > bottom, not before the existing code, in the function. > > Otherwise, this part of the patch looks OK to me. > > Thanks. It is required as this is what sets precedence of GIT_CONFIG_PARAMETERS and thus `git -c` over GIT_CONFIG_COUNT. It's easy enough to split this into two patches though, with a first refactoring which does the indentation and a second one which adds the new code. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> > + 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); >> >> Didn't we got bitten by number of times that the string returned by >> getenv() are not necessarily nonvolatile depending on platforms? I >> think the result of getenv() would need to be xstrdup'ed. >> >> cf. 6776a84d (diff: ensure correct lifetime of external_diff_cmd, >> 2019-01-11) > > We did, but do we have to in this case? There is no interleaving calls > to getenv(3P), so we don't depend on at least $n getenv(3P) calls > succeeding without clobbering old values. It's true that it could be > that any other caller in the callchain clobbers the value, but as far as > I can see none does. Doesn't the code expect "key" will stay valid even after another call to getenv() grabs "value"? > It is required as this is what sets precedence of GIT_CONFIG_PARAMETERS > and thus `git -c` over GIT_CONFIG_COUNT. OK, that is what the "will be overridden by any explicit options" was about. Perhaps that deserves an in-code comment, something like /* * process GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_N pairs * first, to be overridden by GIT_CONFIG_PARAMETERS * inherited from parent Git processes' "git -c var=val" * later */ before we check GIT_CONFIG_COUNT and loop over the new style environment variables. Thanks.
On Tue, Nov 24, 2020 at 11:41:34PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > >> > + 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); > >> > >> Didn't we got bitten by number of times that the string returned by > >> getenv() are not necessarily nonvolatile depending on platforms? I > >> think the result of getenv() would need to be xstrdup'ed. > >> > >> cf. 6776a84d (diff: ensure correct lifetime of external_diff_cmd, > >> 2019-01-11) > > > > We did, but do we have to in this case? There is no interleaving calls > > to getenv(3P), so we don't depend on at least $n getenv(3P) calls > > succeeding without clobbering old values. It's true that it could be > > that any other caller in the callchain clobbers the value, but as far as > > I can see none does. > > Doesn't the code expect "key" will stay valid even after another > call to getenv() grabs "value"? Oh, right. No idea what I was thinking there. > > It is required as this is what sets precedence of GIT_CONFIG_PARAMETERS > > and thus `git -c` over GIT_CONFIG_COUNT. > > OK, that is what the "will be overridden by any explicit options" > was about. Perhaps that deserves an in-code comment, something like > > /* > * process GIT_CONFIG_KEY_N/GIT_CONFIG_VALUE_N pairs > * first, to be overridden by GIT_CONFIG_PARAMETERS > * inherited from parent Git processes' "git -c var=val" > * later > */ > > before we check GIT_CONFIG_COUNT and loop over the new style > environment variables. > > Thanks. Will do, thanks! Patrick
On Tue, Nov 24 2020, Patrick Steinhardt wrote: > +test_expect_success 'command line overrides environment config' ' > + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=pair.one GIT_CONFIG_VALUE_0=value \ > + git -c pair.one=override config pair.one >actual && > + cat >expect <<-EOF && > + override > + EOF > + test_cmp expect actual > +' > + Maybe a test to see which one of this new-style key-value thing v.s. GIT_CONFIG_PARAMETERS wins? Helps if/when we ever refactor this to at least see the behavior of the purely internal thing changed.
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
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. "are", or "are not"? I think it is the latter, and if so I agree that it is a good thing to say here or somewhere nearby. > --> I.e. even if we're not going to support some format for > --> GIT_CONFIG_PARAMETERS document what it is. My preference is to keep it an implementation detail, especially if we were to be adding this new thing as a documented feature, so documenting it beyond its existence and nature is counterproductive. > - 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_*`. I am slightly negative about this. It would be an irrelevant noise to readers who are interested in environment variables that affect how "git config" works (which is what this section is about). Also for those who want to learn about GIT_PUSH_OPTION variable, I do not think they would look for it in "git config" documentation and check its ENVIRONMENT section. It would be much more likely for them to look for them in the documentation for receive-pack or push (and then redirected to githooks doc). > - 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". OK. > 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... I was leaving a clue for those who want to futz with "diff" algorithm that this change can be a good test case for their improvement. I didn't mean that as a suggestion to help "diff" produce a better result by twisting code. We should not tweak our code to please "git show" output. Tweaking code to please "cat/less $file" output is very much welcome, though. Thanks.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 7573160f21..84ae9b51a3 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -335,6 +335,15 @@ GIT_CONFIG_NOSYSTEM:: Whether to skip reading settings from the system-wide $(prefix)/etc/gitconfig file. See linkgit:git[1] for details. +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`. + See also <<FILES>>. 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" #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH" #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES" #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS" diff --git a/config.c b/config.c index 3281b1374e..5da1ae16c9 100644 --- a/config.c +++ b/config.c @@ -484,38 +484,82 @@ int git_config_parse_parameter(const char *text, int git_config_from_parameters(config_fn_t fn, void *data) { - const char *env = getenv(CONFIG_DATA_ENVIRONMENT); + const char *env; + struct strbuf envvar = STRBUF_INIT; int ret = 0; - char *envw; + char *envw = NULL; const char **argv = NULL; - int nr = 0, alloc = 0; int i; struct config_source source; - if (!env) - return 0; - memset(&source, 0, sizeof(source)); source.prev = cf; source.origin_type = CONFIG_ORIGIN_CMDLINE; cf = &source; - /* sq_dequote will write over it */ - envw = xstrdup(env); + env = getenv(CONFIG_COUNT_ENVIRONMENT); + if (env) { + unsigned long count; + char *endp; - 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' ' + GIT_CONFIG_COUNT=2 \ + GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="foo" \ + GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="bar" \ + git config --get-regexp "pair.*" >actual && + cat >expect <<-EOF && + pair.one foo + pair.two bar + EOF + test_cmp expect actual +' + +test_expect_success 'git config ignores pairs without count' ' + test_must_fail env GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \ + git config pair.one 2>error && + test_must_be_empty error +' + +test_expect_success 'git config ignores pairs with zero count' ' + test_must_fail env \ + GIT_CONFIG_COUNT=0 \ + GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \ + git config pair.one +' + +test_expect_success 'git config ignores pairs exceeding count' ' + GIT_CONFIG_COUNT=1 \ + GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \ + GIT_CONFIG_KEY_1="pair.two" GIT_CONFIG_VALUE_1="value" \ + git config --get-regexp "pair.*" >actual && + cat >expect <<-EOF && + pair.one value + EOF + test_cmp expect actual +' + +test_expect_success 'git config ignores pairs with zero count' ' + test_must_fail env \ + GIT_CONFIG_COUNT=0 GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \ + git config pair.one >error && + test_must_be_empty error +' + +test_expect_success 'git config ignores pairs with empty count' ' + test_must_fail env \ + GIT_CONFIG_COUNT= GIT_CONFIG_KEY_0="pair.one" GIT_CONFIG_VALUE_0="value" \ + git config pair.one >error && + test_must_be_empty error +' + +test_expect_success 'git config fails with invalid count' ' + test_must_fail env GIT_CONFIG_COUNT=10a git config --list 2>error && + test_i18ngrep "bogus count" error && + test_must_fail env GIT_CONFIG_COUNT=9999999999999999 git config --list 2>error && + test_i18ngrep "too many entries" error +' + +test_expect_success 'git config fails with missing config key' ' + test_must_fail env GIT_CONFIG_COUNT=1 GIT_CONFIG_VALUE_0="value" \ + git config --list 2>error && + test_i18ngrep "missing config key" error +' + +test_expect_success 'git config fails with missing config value' ' + test_must_fail env GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0="pair.one" \ + git config --list 2>error && + test_i18ngrep "missing config value" error +' + +test_expect_success 'git config fails with invalid config pair key' ' + test_must_fail env GIT_CONFIG_COUNT=1 \ + GIT_CONFIG_KEY_0= GIT_CONFIG_VALUE_0=value \ + git config --list && + test_must_fail env GIT_CONFIG_COUNT=1 \ + GIT_CONFIG_KEY_0=missing-section GIT_CONFIG_VALUE_0=value \ + git config --list +' + +test_expect_success 'environment overrides config file' ' + test_when_finished "rm -f .git/config" && + cat >.git/config <<-EOF && + [pair] + one = value + EOF + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=pair.one GIT_CONFIG_VALUE_0=override \ + git config pair.one >actual && + cat >expect <<-EOF && + override + EOF + test_cmp expect actual +' + +test_expect_success 'command line overrides environment config' ' + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=pair.one GIT_CONFIG_VALUE_0=value \ + git -c pair.one=override config pair.one >actual && + cat >expect <<-EOF && + override + EOF + test_cmp expect actual +' + test_expect_success 'git config --edit works' ' git config -f tmp test.value no && echo test.value=yes >expect && @@ -1661,9 +1762,11 @@ test_expect_success '--show-origin with --list' ' file:.git/config user.override=local file:.git/config include.path=../include/relative.include file:.git/../include/relative.include user.relative=include + command line: user.environ=true command line: user.cmdline=true EOF - git -c user.cmdline=true config --list --show-origin >output && + GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=user.environ GIT_CONFIG_VALUE_0=true\ + git -c user.cmdline=true config --list --show-origin >output && test_cmp expect output '
While we currently have the `GIT_CONFIG_PARAMETERS` environment variable which can be used to pass runtime configuration data to git processes, it's an internal implementation detail and not supposed to be used by end users. Next to being for internal use only, this way of passing config entries has a major downside: the config keys need to be parsed as they contain both key and value in a single variable. As such, it is left to the user to escape any potentially harmful characters in the value, which is quite hard to do if values are controlled by a third party. This commit thus adds a new way of adding config entries via the environment which gets rid of this shortcoming. If the user passes the `GIT_CONFIG_COUNT=$n` environment variable, Git will parse environment variable pairs `GIT_CONFIG_KEY_$i` and `GIT_CONFIG_VALUE_$i` for each `i` in `[0,n)`. While the same can be achieved with `git -c <name>=<value>`, one may wish to not do so for potentially sensitive information. E.g. if one wants to set `http.extraHeader` to contain an authentication token, doing so via `-c` would trivially leak those credentials via e.g. ps(1), which typically also shows command arguments. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-config.txt | 9 +++ cache.h | 1 + config.c | 72 +++++++++++++++++++----- environment.c | 1 + t/t1300-config.sh | 105 ++++++++++++++++++++++++++++++++++- 5 files changed, 173 insertions(+), 15 deletions(-)