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

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

[Patrick Steinhardt]

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

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(-)

-- 
2.31.1


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

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

[Patrick Steinhardt]

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

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 <avarab@gmail.com>
Signed-of-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 3a9c44987f..ba5c8e9d98 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -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
-- 
2.31.1


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

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

[Patrick Steinhardt]

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

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 <ps@pks.im>
---
 git.c             |  8 ++++++++
 t/t1300-config.sh | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index b53e665671..ad365c05c7 100644
--- a/git.c
+++ b/git.c
@@ -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")) {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..18803f9953 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -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 &&
-- 
2.31.1


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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 19 2021, Patrick Steinhardt wrote:

> 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 <ps@pks.im>

This whole series LGTM.

>  git.c             |  8 ++++++++
>  t/t1300-config.sh | 15 ++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/git.c b/git.c
> index b53e665671..ad365c05c7 100644
> --- a/git.c
> +++ b/git.c
> @@ -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.

> +				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")) {
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index e0dd5d65ce..18803f9953 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -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 &&

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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 19 2021, Patrick Steinhardt wrote:

> +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/.

>  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 &&

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

[Jeff King]

On Tue, Apr 20, 2021 at 12:52:43PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Mitigate this inconsistency by accepting both syntaxes and add tests to
> > verify both work.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> 
> This whole series LGTM.

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

> > +		} 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

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

[Patrick Steinhardt]

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

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' '
-- 
2.31.1


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

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

[Patrick Steinhardt]

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

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 <avarab@gmail.com>
Signed-of-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 3a9c44987f..ba5c8e9d98 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -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
-- 
2.31.1


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

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

[Patrick Steinhardt]

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

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 <ps@pks.im>
---
 git.c             |  8 ++++++++
 t/t1300-config.sh | 15 ++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index b53e665671..ad365c05c7 100644
--- a/git.c
+++ b/git.c
@@ -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")) {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ce..ad4e6d0cfc 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -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 &&
-- 
2.31.1


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

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

[Jeff King]

On Thu, Apr 29, 2021 at 02:55:25PM +0200, Patrick Steinhardt wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Apr 29, 2021 at 02:55:25PM +0200, Patrick Steinhardt wrote:
>
>> 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.