git: support separate arg for `--config-env`'s value

PATCH Superseded Patrick Steinhardt 4 participants 11 messages Apr 19, 2021 – Apr 30, 2021 Documentation

lore

Share & tools

lore

Patrick Steinhardt

author

2021-04-19 15:55 UTC #0 lore raw

cover git: support separate arg for `--config-env`'s value

Hi,

As pointed out by Ævar, the synopsis in the git(1) manpage was misdocumenting the syntax of the --config-env option. This patch series fixes it and also addresses the shortcoming that while most of the other options support both --git-dir=foo and --git-dir foo, --config-env only supported the former format.

I'm happy to drop the latter patch if it's generally seen as something that's not needed, but Ævar and Peff got a point in that it feels inconsistent with the other options.

Patrick

Patrick Steinhardt (2):
git.txt: fix synopsis of --config-env missing the equals sign
git: support separate arg for --config-env's value

 Documentation/git.txt |  2 +-
 git.c                 |  8 ++++++++
 t/t1300-config.sh     | 15 ++++++++++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

Signature · 1 line

2.31.1

Patrick Steinhardt

author

2021-04-19 15:55 UTC #1 lore raw

PATCH 1/2 git.txt: fix synopsis of `--config-env` missing the equals sign

When executing git -h, then the --config-env documentation rightly lists the option as requiring an equals between the option and its
argument: this is the only currently supported format. But the git(1) manpage incorrectly lists the option as taking a space in between.

Fix the issue by adding the missing space.

Reported-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-of-by Patrick Steinhardt <[email protected]>

---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/git.txt +1 -1

index 3a9c44987f..ba5c8e9d98 100644
@@ -13,7 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
-    [--super-prefix=<path>] [--config-env <name>=<envvar>]
+    [--super-prefix=<path>] [--config-env=<name>=<envvar>]
     <command> [<args>]
 
 DESCRIPTION

Signature · 1 line

2.31.1

Patrick Steinhardt

author

2021-04-19 15:55 UTC #2 lore raw

PATCH 2/2 git: support separate arg for `--config-env`'s value

In reply to Patrick Steinhardt

While not documented as such, many of the top-level options like --git-dir and --work-tree support two syntaxes: they accept both an equals sign between option and its value, and they do support option and value as two separate arguments. The recently added --config-env option only supports the syntax with an equals sign.

Mitigate this inconsistency by accepting both syntaxes and add tests to verify both work.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 git.c             |  8 ++++++++
 t/t1300-config.sh | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

git.c +8 -0

index b53e665671..ad365c05c7 100644
@@ -255,6 +255,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			git_config_push_parameter((*argv)[1]);
 			(*argv)++;
 			(*argc)--;
+		} else if (!strcmp(cmd, "--config-env")) {
+			if (*argc < 2) {
+				fprintf(stderr, _("no config key given for --config-env\n" ));
+				usage(git_usage_string);
+			}
+			git_config_push_env((*argv)[1]);
+			(*argv)++;
+			(*argc)--;
 		} else if (skip_prefix(cmd, "--config-env=", &cmd)) {
 			git_config_push_env(cmd);
 		} else if (!strcmp(cmd, "--literal-pathspecs")) {

t/t1300-config.sh +14 -1

index e0dd5d65ce..18803f9953 100755
@@ -1374,16 +1374,29 @@ test_expect_success 'git --config-env=key=envvar support' '
 	cat >expect <<-\EOF &&
 	value
 	value
+	value
+	value
+	false
 	false
 	EOF
 	{
 		ENVVAR=value git --config-env=core.name=ENVVAR config core.name &&
+		ENVVAR=value git --config-env core.name=ENVVAR config core.name &&
 		ENVVAR=value git --config-env=foo.CamelCase=ENVVAR config foo.camelcase &&
-		ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag
+		ENVVAR=value git --config-env foo.CamelCase=ENVVAR config foo.camelcase &&
+		ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag &&
+		ENVVAR= git --config-env foo.flag=ENVVAR config --bool foo.flag
 	} >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'git --config-env with missing value' '
+	test_must_fail env ENVVAR=value git --config-env 2>error &&
+	test_i18ngrep "no config key given for --config-env" error &&
+	test_must_fail env ENVVAR=value git --config-env config core.name 2>error &&
+	test_i18ngrep "invalid config format: config" error
+'
+
 test_expect_success 'git --config-env fails with invalid parameters' '
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&

Signature · 1 line

2.31.1

Ævar Arnfjörð Bjarmason

2021-04-20 10:52 UTC #3 lore raw

Quoted 10 lines · Patrick Steinhardt

While not documented as such, many of the top-level options like --git-dir and --work-tree support two syntaxes: they accept both an equals sign between option and its value, and they do support option and value as two separate arguments. The recently added --config-env option only supports the syntax with an equals sign.
Mitigate this inconsistency by accepting both syntaxes and add tests to verify both work.
Signed-off-by Patrick Steinhardt <[email protected]>

This whole series LGTM.

Quoted 9 lines

 git.c             |  8 ++++++++
 t/t1300-config.sh | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

git.c +0 -0

index b53e665671..ad365c05c7 100644

git.c static int handle_options(const char ***argv, int *argc, int *envchanged) · line 260

Quoted 6 lines

git.c +3 -0

@@ -255,6 +255,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			git_config_push_parameter((*argv)[1]);
 			(*argv)++;
 			(*argc)--;
+		} else if (!strcmp(cmd, "--config-env")) {
+			if (*argc < 2) {
+				fprintf(stderr, _("no config key given for --config-env\n" ));

I found this use of fprintf() slightly odd, why not error(), but then went back and read the function and saw that it has N number of "fprintf(stderr, [...])" already.

That could probably all be converted to error(), but better to be consistent for now.

Quoted 43 lines

git.c +5 -0

+				usage(git_usage_string);
+			}
+			git_config_push_env((*argv)[1]);
+			(*argv)++;
+			(*argc)--;
 		} else if (skip_prefix(cmd, "--config-env=", &cmd)) {
 			git_config_push_env(cmd);
 		} else if (!strcmp(cmd, "--literal-pathspecs")) {

t/t1300-config.sh +14 -1

index e0dd5d65ce..18803f9953 100755
@@ -1374,16 +1374,29 @@ test_expect_success 'git --config-env=key=envvar support' '
 	cat >expect <<-\EOF &&
 	value
 	value
+	value
+	value
+	false
 	false
 	EOF
 	{
 		ENVVAR=value git --config-env=core.name=ENVVAR config core.name &&
+		ENVVAR=value git --config-env core.name=ENVVAR config core.name &&
 		ENVVAR=value git --config-env=foo.CamelCase=ENVVAR config foo.camelcase &&
-		ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag
+		ENVVAR=value git --config-env foo.CamelCase=ENVVAR config foo.camelcase &&
+		ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag &&
+		ENVVAR= git --config-env foo.flag=ENVVAR config --bool foo.flag
 	} >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'git --config-env with missing value' '
+	test_must_fail env ENVVAR=value git --config-env 2>error &&
+	test_i18ngrep "no config key given for --config-env" error &&
+	test_must_fail env ENVVAR=value git --config-env config core.name 2>error &&
+	test_i18ngrep "invalid config format: config" error
+'
+
 test_expect_success 'git --config-env fails with invalid parameters' '
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&

Ævar Arnfjörð Bjarmason

2021-04-20 11:24 UTC #5 lore raw

In reply to Patrick Steinhardt

t/t1300-config.sh test_expect_success 'git --config-env=key=envvar support' ' · line 1399

Quoted 7 lines · Patrick Steinhardt

t/t1300-config.sh +7 -0

+test_expect_success 'git --config-env with missing value' '
+	test_must_fail env ENVVAR=value git --config-env 2>error &&
+	test_i18ngrep "no config key given for --config-env" error &&
+	test_must_fail env ENVVAR=value git --config-env config core.name 2>error &&
+	test_i18ngrep "invalid config format: config" error
+'
+

Nit: (not spotted on the first reading): s/test_i18ngrep/grep/.

Quoted 3 lines

test_expect_success 'git --config-env fails with invalid parameters' '
t/t1300-config.sh +0 -0
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&

Jeff King

2021-04-23 10:07 UTC #4 lore raw

In reply to Ævar Arnfjörð Bjarmason

Quoted 6 lines · Ævar Arnfjörð Bjarmason

Quoted 4 lines
Mitigate this inconsistency by accepting both syntaxes and add tests to verify both work.
Signed-off-by Patrick Steinhardt <[email protected]>
This whole series LGTM.

Me too. Thanks for finding it (and Patrick for fixing it).

Quoted 11 lines

git.c static int handle_options(const char ***argv, int *argc, int *envchanged) · line 260
Quoted 3 lines
git.c +3 -0
+		} else if (!strcmp(cmd, "--config-env")) {
+			if (*argc < 2) {
+				fprintf(stderr, _("no config key given for --config-env\n" ));
I found this use of fprintf() slightly odd, why not error(), but then went back and read the function and saw that it has N number of "fprintf(stderr, [...])" already.
That could probably all be converted to error(), but better to be consistent for now.

Yep. Likewise the weird extra space at the end of the string.

-Peff

Patrick Steinhardt

author

2021-04-29 12:53 UTC #7 lore raw

PATCH v2 1/2 git.txt: fix synopsis of `--config-env` missing the equals sign

In reply to Patrick Steinhardt

Fix the issue by adding the missing space.

Reported-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-of-by Patrick Steinhardt <[email protected]>

---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Documentation/git.txt +1 -1

index 3a9c44987f..ba5c8e9d98 100644
@@ -13,7 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
-    [--super-prefix=<path>] [--config-env <name>=<envvar>]
+    [--super-prefix=<path>] [--config-env=<name>=<envvar>]
     <command> [<args>]
 
 DESCRIPTION

Signature · 1 line

2.31.1

Patrick Steinhardt

author

2021-04-29 12:53 UTC #6 lore raw

v2 cover git: support separate arg for `--config-env`'s value

In reply to Patrick Steinhardt

Hi,

this is the second version of this series fixing inconsistencies with the --config-env parameter. It's only got a single change compared to v1, which is to replace test_i18ngrep with grep as pointed out by Ævar.

Patrick

Patrick Steinhardt (2):
git.txt: fix synopsis of --config-env missing the equals sign
git: support separate arg for --config-env's value

 Documentation/git.txt |  2 +-
 git.c                 |  8 ++++++++
 t/t1300-config.sh     | 15 ++++++++++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

Range-diff against v1:

1:  08049f3b10 = 1:  08049f3b10 git.txt: fix synopsis of `--config-env` missing the equals sign
2:  d52db89bc2 ! 2:  5264fb6fa7 git: support separate arg for `--config-env`'s value
    @@ t/t1300-config.sh: test_expect_success 'git --config-env=key=envvar support' '
      
     +test_expect_success 'git --config-env with missing value' '
     +	test_must_fail env ENVVAR=value git --config-env 2>error &&
    -+	test_i18ngrep "no config key given for --config-env" error &&
    ++	grep "no config key given for --config-env" error &&
     +	test_must_fail env ENVVAR=value git --config-env config core.name 2>error &&
    -+	test_i18ngrep "invalid config format: config" error
    ++	grep "invalid config format: config" error
     +'
     +
      test_expect_success 'git --config-env fails with invalid parameters' '

Signature · 1 line

2.31.1

Patrick Steinhardt

author

2021-04-29 12:53 UTC #8 lore raw

PATCH v2 2/2 git: support separate arg for `--config-env`'s value

Mitigate this inconsistency by accepting both syntaxes and add tests to verify both work.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 git.c             |  8 ++++++++
 t/t1300-config.sh | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

git.c +8 -0

index b53e665671..ad365c05c7 100644
@@ -255,6 +255,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			git_config_push_parameter((*argv)[1]);
 			(*argv)++;
 			(*argc)--;
+		} else if (!strcmp(cmd, "--config-env")) {
+			if (*argc < 2) {
+				fprintf(stderr, _("no config key given for --config-env\n" ));
+				usage(git_usage_string);
+			}
+			git_config_push_env((*argv)[1]);
+			(*argv)++;
+			(*argc)--;
 		} else if (skip_prefix(cmd, "--config-env=", &cmd)) {
 			git_config_push_env(cmd);
 		} else if (!strcmp(cmd, "--literal-pathspecs")) {

t/t1300-config.sh +14 -1

index e0dd5d65ce..ad4e6d0cfc 100755
@@ -1374,16 +1374,29 @@ test_expect_success 'git --config-env=key=envvar support' '
 	cat >expect <<-\EOF &&
 	value
 	value
+	value
+	value
+	false
 	false
 	EOF
 	{
 		ENVVAR=value git --config-env=core.name=ENVVAR config core.name &&
+		ENVVAR=value git --config-env core.name=ENVVAR config core.name &&
 		ENVVAR=value git --config-env=foo.CamelCase=ENVVAR config foo.camelcase &&
-		ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag
+		ENVVAR=value git --config-env foo.CamelCase=ENVVAR config foo.camelcase &&
+		ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag &&
+		ENVVAR= git --config-env foo.flag=ENVVAR config --bool foo.flag
 	} >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'git --config-env with missing value' '
+	test_must_fail env ENVVAR=value git --config-env 2>error &&
+	grep "no config key given for --config-env" error &&
+	test_must_fail env ENVVAR=value git --config-env config core.name 2>error &&
+	grep "invalid config format: config" error
+'
+
 test_expect_success 'git --config-env fails with invalid parameters' '
 	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
 	test_i18ngrep "invalid config format: foo.flag" error &&

Signature · 1 line

2.31.1

Jeff King

2021-04-29 20:04 UTC #9 lore raw

In reply to Patrick Steinhardt

Quoted 4 lines · Patrick Steinhardt

this is the second version of this series fixing inconsistencies with the --config-env parameter. It's only got a single change compared to v1, which is to replace test_i18ngrep with grep as pointed out by Ævar.

Thanks, this version looks good to me.

-Peff

Junio C Hamano

2021-04-30 00:47 UTC #10 lore raw

Quoted 10 lines · Jeff King

Quoted 4 lines · Patrick Steinhardt
this is the second version of this series fixing inconsistencies with the --config-env parameter. It's only got a single change compared to v1, which is to replace test_i18ngrep with grep as pointed out by Ævar.
Thanks, this version looks good to me.
-Peff

Thanks, will replace.