| Message ID | 36c2a51b13e463a4aa8e5316447336927153d99d.1610453228.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1ff21c05ba99ed2d0ade8318e3cb0c1a3f8d4b80 |
| Headers | show |
| Series | config: allow specifying config entries via envvar pairs | expand |
On Tue, Jan 12, 2021 at 01:27:01PM +0100, Patrick Steinhardt wrote: > The previous commit added a new format for $GIT_CONFIG_PARAMETERS which > is able to robustly handle subsections with "=" in them. Let's start It looks like this commit and 6 got flipped from the original ordering (it's the "previous commit" talked about here). And indeed, running the tests on the individual commits in this series shows that we fail at this step (because we are writing the new format, but the reader is too strict to accept it). That doesn't matter to the end result, of course, but it hurts later bisecting. Just flipping patches 5 and 6 makes it all work. -Peff
On Fri, Jan 15, 2021 at 02:16:46PM -0500, Jeff King wrote: > On Tue, Jan 12, 2021 at 01:27:01PM +0100, Patrick Steinhardt wrote: > > > The previous commit added a new format for $GIT_CONFIG_PARAMETERS which > > is able to robustly handle subsections with "=" in them. Let's start > > It looks like this commit and 6 got flipped from the original ordering > (it's the "previous commit" talked about here). And indeed, running the > tests on the individual commits in this series shows that we fail at > this step (because we are writing the new format, but the reader is too > strict to accept it). > > That doesn't matter to the end result, of course, but it hurts later > bisecting. Just flipping patches 5 and 6 makes it all work. > > -Peff Oops, yes. That always happens to me when I start using git-am(1). I see that the patch series has been applied to "next" already, so does it make any sense to resend with patches 5 and 6 flipped? Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Fri, Jan 15, 2021 at 02:16:46PM -0500, Jeff King wrote: >> On Tue, Jan 12, 2021 at 01:27:01PM +0100, Patrick Steinhardt wrote: >> >> > The previous commit added a new format for $GIT_CONFIG_PARAMETERS which >> > is able to robustly handle subsections with "=" in them. Let's start >> >> It looks like this commit and 6 got flipped from the original ordering >> (it's the "previous commit" talked about here). And indeed, running the >> tests on the individual commits in this series shows that we fail at >> this step (because we are writing the new format, but the reader is too >> strict to accept it). >> >> That doesn't matter to the end result, of course, but it hurts later >> bisecting. Just flipping patches 5 and 6 makes it all work. >> >> -Peff > > Oops, yes. That always happens to me when I start using git-am(1). I see > that the patch series has been applied to "next" already, so does it > make any sense to resend with patches 5 and 6 flipped? I recall saying that I'd "rebase -i" before merging it to "next". Did I forget to do so? Disecting 4ed03412 (Merge branch 'ps/config-env-pairs' into next, 2021-01-15), we see: $ git log --oneline --reverse master..4ed03412^2 | cat -n 1 b0812b6ac0 git: add `--super-prefix` to usage string 2 ce81b1da23 config: add new way to pass config via `--config-env` 3 13c44953fb quote: make sq_dequote_step() a public function 4 b342ae61b3 config: extract function to parse config pairs 5 f9dbb64fad config: parse more robust format in GIT_CONFIG_PARAMETERS 6 1ff21c05ba config: store "git -c" variables using more robust format 7 b9d147fb15 environment: make `getenv_safe()` a public function 8 d8d77153ea config: allow specifying config entries via envvar pairs The 5/8 that needs to come after 6/8 has title "store ... using more rebust format" and that is the 6th patch in the series merged to 'next'. The 6/8 that needs to come before that one was called "parse more robust format" and it now appears as the 5th patch. So it seems all is well?
On Tue, Jan 19, 2021 at 10:55:48PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > On Fri, Jan 15, 2021 at 02:16:46PM -0500, Jeff King wrote: > >> On Tue, Jan 12, 2021 at 01:27:01PM +0100, Patrick Steinhardt wrote: > >> > >> > The previous commit added a new format for $GIT_CONFIG_PARAMETERS which > >> > is able to robustly handle subsections with "=" in them. Let's start > >> > >> It looks like this commit and 6 got flipped from the original ordering > >> (it's the "previous commit" talked about here). And indeed, running the > >> tests on the individual commits in this series shows that we fail at > >> this step (because we are writing the new format, but the reader is too > >> strict to accept it). > >> > >> That doesn't matter to the end result, of course, but it hurts later > >> bisecting. Just flipping patches 5 and 6 makes it all work. > >> > >> -Peff > > > > Oops, yes. That always happens to me when I start using git-am(1). I see > > that the patch series has been applied to "next" already, so does it > > make any sense to resend with patches 5 and 6 flipped? > > I recall saying that I'd "rebase -i" before merging it to "next". > Did I forget to do so? > > Disecting 4ed03412 (Merge branch 'ps/config-env-pairs' into next, > 2021-01-15), we see: > > $ git log --oneline --reverse master..4ed03412^2 | cat -n > 1 b0812b6ac0 git: add `--super-prefix` to usage string > 2 ce81b1da23 config: add new way to pass config via `--config-env` > 3 13c44953fb quote: make sq_dequote_step() a public function > 4 b342ae61b3 config: extract function to parse config pairs > 5 f9dbb64fad config: parse more robust format in GIT_CONFIG_PARAMETERS > 6 1ff21c05ba config: store "git -c" variables using more robust format > 7 b9d147fb15 environment: make `getenv_safe()` a public function > 8 d8d77153ea config: allow specifying config entries via envvar pairs > > The 5/8 that needs to come after 6/8 has title "store ... using more > rebust format" and that is the 6th patch in the series merged to > 'next'. The 6/8 that needs to come before that one was called > "parse more robust format" and it now appears as the 5th patch. > > So it seems all is well? Indeed, I missed your message about the interactive rebase. Thanks! Patrick
Patrick Steinhardt <ps@pks.im> writes: >> The 5/8 that needs to come after 6/8 has title "store ... using more >> rebust format" and that is the 6th patch in the series merged to >> 'next'. The 6/8 that needs to come before that one was called >> "parse more robust format" and it now appears as the 5th patch. >> >> So it seems all is well? > > Indeed, I missed your message about the interactive rebase. Thanks! Thanks for contributing in the first place, and thanks for double checking. Very much appreciated.
diff --git a/config.c b/config.c index b7a8129f6c..7f7da60574 100644 --- a/config.c +++ b/config.c @@ -332,7 +332,7 @@ int git_config_include(const char *var, const char *value, void *data) return ret; } -void git_config_push_parameter(const char *text) +static void git_config_push_split_parameter(const char *key, const char *value) { struct strbuf env = STRBUF_INIT; const char *old = getenv(CONFIG_DATA_ENVIRONMENT); @@ -340,20 +340,60 @@ void git_config_push_parameter(const char *text) strbuf_addstr(&env, old); strbuf_addch(&env, ' '); } - sq_quote_buf(&env, text); + sq_quote_buf(&env, key); + strbuf_addch(&env, '='); + if (value) + sq_quote_buf(&env, value); setenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1); strbuf_release(&env); } +void git_config_push_parameter(const char *text) +{ + const char *value; + + /* + * When we see: + * + * section.subsection=with=equals.key=value + * + * we cannot tell if it means: + * + * [section "subsection=with=equals"] + * key = value + * + * or: + * + * [section] + * subsection = with=equals.key=value + * + * We parse left-to-right for the first "=", meaning we'll prefer to + * keep the value intact over the subsection. This is historical, but + * also sensible since values are more likely to contain odd or + * untrusted input than a section name. + * + * A missing equals is explicitly allowed (as a bool-only entry). + */ + value = strchr(text, '='); + if (value) { + char *key = xmemdupz(text, value - text); + git_config_push_split_parameter(key, value + 1); + free(key); + } else { + git_config_push_split_parameter(text, NULL); + } +} + void git_config_push_env(const char *spec) { - struct strbuf buf = STRBUF_INIT; + char *key; const char *env_name; const char *env_value; env_name = strrchr(spec, '='); if (!env_name) die(_("invalid config format: %s"), spec); + key = xmemdupz(spec, env_name - spec); env_name++; if (!*env_name) die(_("missing environment variable name for configuration '%.*s'"), @@ -364,10 +404,8 @@ void git_config_push_env(const char *spec) die(_("missing environment variable '%s' for configuration '%.*s'"), env_name, (int)(env_name - spec - 1), spec); - strbuf_add(&buf, spec, env_name - spec); - strbuf_addstr(&buf, env_value); - git_config_push_parameter(buf.buf); - strbuf_release(&buf); + git_config_push_split_parameter(key, env_value); + free(key); } static inline int iskeychar(int c) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 853f2509c5..25437324c1 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1364,6 +1364,14 @@ test_expect_success 'git -c and --config-env override each other' ' test_cmp expect actual ' +test_expect_success '--config-env handles keys with equals' ' + echo value=with=equals >expect && + ENVVAR=value=with=equals git \ + --config-env=section.subsection=with=equals.key=ENVVAR \ + config section.subsection=with=equals.key >actual && + test_cmp expect actual +' + test_expect_success 'git config --edit works' ' git config -f tmp test.value no && echo test.value=yes >expect &&
The previous commit added a new format for $GIT_CONFIG_PARAMETERS which is able to robustly handle subsections with "=" in them. Let's start writing the new format. Unfortunately, this does much less than you'd hope, because "git -c" itself has the same ambiguity problem! But it's still worth doing: - we've now pushed the problem from the inter-process communication into the "-c" command-line parser. This would free us up to later add an unambiguous format there (e.g., separate arguments like "git --config key value", etc). - for --config-env, the parser already disallows "=" in the environment variable name. So: git --config-env section.with=equals.key=ENVVAR will robustly set section.with=equals.key to the contents of $ENVVAR. The new test shows the improvement for --config-env. Signed-off-by: Jeff King <peff@peff.net> --- config.c | 52 ++++++++++++++++++++++++++++++++++++++++------- t/t1300-config.sh | 8 ++++++++ 2 files changed, 53 insertions(+), 7 deletions(-)