git config --get-urlmatch does not set exit code 1 when no match is found

git config --get-urlmatch does not set exit code 1 when no match is found

[Guilherme]

Hello,

My current woes are with multi-valued configuration values. More
specifically credential.helper

The documentation of git config says that when a value is not matched
it should return 1.

To reproduce make sure that credential.helper is not set.

git config --get-urlmatch credential.helper http://somedomain:1234/
echo %ERRORLEVEL%
0

git config --get credential.helper
echo %ERRORLEVEL%
1

git config --get credential.http://somedomain:1234/.helper
echo %ERRORLEVEL%
1

The documentation says that for credential.helper is not found for a
domain it should fall back to credential.helper if it is set. So I
think that all those tests above should have returned 0. Am i right?

Cheers.

Re: git config --get-urlmatch does not set exit code 1 when no match is found

[John Keeping]

On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:
> My current woes are with multi-valued configuration values. More
> specifically credential.helper
> 
> The documentation of git config says that when a value is not matched
> it should return 1.
> 
> To reproduce make sure that credential.helper is not set.
> 
> git config --get-urlmatch credential.helper http://somedomain:1234/
> echo %ERRORLEVEL%
> 0
> 
> git config --get credential.helper
> echo %ERRORLEVEL%
> 1
> 
> git config --get credential.http://somedomain:1234/.helper
> echo %ERRORLEVEL%
> 1
> 
> The documentation says that for credential.helper is not found for a
> domain it should fall back to credential.helper if it is set. So I
> think that all those tests above should have returned 0. Am i right?

It looks to me like a simple bug that --get-urlmatch doesn't return 1 if
the key isn't found, but git-config(1) isn't entirely clear.  The
overall documentation on exit codes at the end of DESCRIPTION says that
exit code 1 means:

	the section or key is invalid (ret=1)

Then the documentation for the --get option says:

	Returns error code 1 if the key was not found.

and --get-all says:

	Like get, but does not fail if the number of values for the key
	is not exactly one.

although it does return 1 if there are zero values.  --get-regexp
behaves in the same way.

Overall I think that the fact that --get-urlmatch is the outlier here
means that it should change to match the other --get* options (ignoring
--get-color and --get-colorbool which are very different).  Although I
wonder if anyone is relying on the current behaviour and will find their
workflow broken if we change this.

The documentation could also use some clarification since most of the
return codes only apply for the "set" options and in some cases this
isn't clear from the existing descriptions.

Re: git config --get-urlmatch does not set exit code 1 when no match is found

[John Keeping]

On Sun, Feb 28, 2016 at 10:45:57AM +0000, John Keeping wrote:
> On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:
> > My current woes are with multi-valued configuration values. More
> > specifically credential.helper
> > 
> > The documentation of git config says that when a value is not matched
> > it should return 1.
> > 
> > To reproduce make sure that credential.helper is not set.
> > 
> > git config --get-urlmatch credential.helper http://somedomain:1234/
> > echo %ERRORLEVEL%
> > 0
> > 
> > git config --get credential.helper
> > echo %ERRORLEVEL%
> > 1
> > 
> > git config --get credential.http://somedomain:1234/.helper
> > echo %ERRORLEVEL%
> > 1
> > 
> > The documentation says that for credential.helper is not found for a
> > domain it should fall back to credential.helper if it is set. So I
> > think that all those tests above should have returned 0. Am i right?

I misread this as "should have returned 1", which is what the text below
agrees with.

The "git config" command does not know anything about the semantics of
particular config keys.  It is purely an interface to parse and query
the config file format and it is up to the consumer to know what to do
if a key doesn't exist.

Both of the "git config --get" examples you give are behaving as
documented in git-config(1).

> It looks to me like a simple bug that --get-urlmatch doesn't return 1 if
> the key isn't found, but git-config(1) isn't entirely clear.  The
> overall documentation on exit codes at the end of DESCRIPTION says that
> exit code 1 means:
> 
> 	the section or key is invalid (ret=1)
> 
> Then the documentation for the --get option says:
> 
> 	Returns error code 1 if the key was not found.
> 
> and --get-all says:
> 
> 	Like get, but does not fail if the number of values for the key
> 	is not exactly one.
> 
> although it does return 1 if there are zero values.  --get-regexp
> behaves in the same way.
> 
> Overall I think that the fact that --get-urlmatch is the outlier here
> means that it should change to match the other --get* options (ignoring
> --get-color and --get-colorbool which are very different).  Although I
> wonder if anyone is relying on the current behaviour and will find their
> workflow broken if we change this.
> 
> The documentation could also use some clarification since most of the
> return codes only apply for the "set" options and in some cases this
> isn't clear from the existing descriptions.

[PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found

[John Keeping]

On Sun, Feb 28, 2016 at 10:45:57AM +0000, John Keeping wrote:
> It looks to me like a simple bug that --get-urlmatch doesn't return 1 if
> the key isn't found, but git-config(1) isn't entirely clear.  The
> overall documentation on exit codes at the end of DESCRIPTION says that
> exit code 1 means:
> 
>       the section or key is invalid (ret=1)
> 
> Then the documentation for the --get option says:
> 
>       Returns error code 1 if the key was not found.
> 
> and --get-all says:
> 
>       Like get, but does not fail if the number of values for the key
>       is not exactly one.
> 
> although it does return 1 if there are zero values.  --get-regexp
> behaves in the same way.
> 
> Overall I think that the fact that --get-urlmatch is the outlier here
> means that it should change to match the other --get* options (ignoring
> --get-color and --get-colorbool which are very different).  Although I
> wonder if anyone is relying on the current behaviour and will find their
> workflow broken if we change this.
> 
> The documentation could also use some clarification since most of the
> return codes only apply for the "set" options and in some cases this
> isn't clear from the existing descriptions.

Here's a series that changes the behaviour of "git config --get-urlmatch"
when no appropriate key is found as well as a couple of improvements to
the documentation while we're here.

The second two patches are independent of the first and I think they
should be picked up even if we decide the change to --get-urlmatch's
exit code is not desirable.

John Keeping (3):
  config: fail if --get-urlmatch finds no value
  Documentation/git-config: use bulleted list for exit codes
  Documentation/git-config: fix --get-all description

 Documentation/git-config.txt | 19 +++++++++----------
 builtin/config.c             |  5 ++++-
 t/t1300-repo-config.sh       |  3 +++
 3 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.7.1.503.g3cfa3ac

[PATCH 1/3] config: fail if --get-urlmatch finds no value

[John Keeping]

The --get, --get-all and --get-regexp options to git-config exit with
status 1 if the key is not found but --get-urlmatch succeeds in this
case.

Change --get-urlmatch to behave in the same way as the other --get*
options so that all four are consistent.  --get-color is a special case
because it accepts a default value to return and so should not return an
error if the key is not found.

Also clarify this behaviour in the documentation.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-config.txt | 2 +-
 builtin/config.c             | 5 ++++-
 t/t1300-repo-config.sh       | 3 +++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 153b2d8..2a04e87 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -102,7 +102,7 @@ OPTIONS
 	given URL is returned (if no such key exists, the value for
 	section.key is used as a fallback).  When given just the
 	section as name, do so for all the keys in the section and
-	list them.
+	list them.  Returns error code 1 if no value is found.
 
 --global::
 	For writing options: write to global `~/.gitconfig` file
diff --git a/builtin/config.c b/builtin/config.c
index ca9f834..1d7c6ef 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -417,6 +417,7 @@ static int urlmatch_collect_fn(const char *var, const char *value, void *cb)
 
 static int get_urlmatch(const char *var, const char *url)
 {
+	int ret;
 	char *section_tail;
 	struct string_list_item *item;
 	struct urlmatch_config config = { STRING_LIST_INIT_DUP };
@@ -443,6 +444,8 @@ static int get_urlmatch(const char *var, const char *url)
 	git_config_with_options(urlmatch_config_entry, &config,
 				&given_config_source, respect_includes);
 
+	ret = !values.nr;
+
 	for_each_string_list_item(item, &values) {
 		struct urlmatch_current_candidate_value *matched = item->util;
 		struct strbuf buf = STRBUF_INIT;
@@ -459,7 +462,7 @@ static int get_urlmatch(const char *var, const char *url)
 	free(config.url.url);
 
 	free((void *)config.section);
-	return 0;
+	return ret;
 }
 
 static char *default_user_config(void)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..89d8c47 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1148,6 +1148,9 @@ test_expect_success 'urlmatch' '
 		cookieFile = /tmp/cookie.txt
 	EOF
 
+	test_expect_code 1 git config --bool --get-urlmatch doesnt.exist https://good.example.com >actual &&
+	test_must_be_empty actual &&
+
 	echo true >expect &&
 	git config --bool --get-urlmatch http.SSLverify https://good.example.com >actual &&
 	test_cmp expect actual &&
-- 
2.7.1.503.g3cfa3ac

[PATCH 2/3] Documentation/git-config: use bulleted list for exit codes

[John Keeping]

Using a numbered list is confusing because the exit codes are not listed
in order so the numbers at the start of each line do not match the exit
codes described by the following text.  Switch to a bulleted list so
that the only number appearing on each line is the exit code described.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-config.txt | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2a04e87..e9c755f 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -58,13 +58,13 @@ that location (you can say '--local' but that is the default).
 This command will fail with non-zero status upon error.  Some exit
 codes are:
 
-. The config file is invalid (ret=3),
-. can not write to the config file (ret=4),
-. no section or name was provided (ret=2),
-. the section or key is invalid (ret=1),
-. you try to unset an option which does not exist (ret=5),
-. you try to unset/set an option for which multiple lines match (ret=5), or
-. you try to use an invalid regexp (ret=6).
+- The config file is invalid (ret=3),
+- can not write to the config file (ret=4),
+- no section or name was provided (ret=2),
+- the section or key is invalid (ret=1),
+- you try to unset an option which does not exist (ret=5),
+- you try to unset/set an option for which multiple lines match (ret=5), or
+- you try to use an invalid regexp (ret=6).
 
 On success, the command returns the exit code 0.
 
-- 
2.7.1.503.g3cfa3ac

[PATCH 3/3] Documentation/git-config: fix --get-all description

[John Keeping]

--get does not fail if a key is multi-valued, it returns the last value
as described in its documentation.  Clarify the description of --get-all
to avoid implying that --get does fail in this case.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-config.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e9c755f..6fc08e3 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -86,8 +86,7 @@ OPTIONS
 	found and the last value if multiple key values were found.
 
 --get-all::
-	Like get, but does not fail if the number of values for the key
-	is not exactly one.
+	Like get, but returns all values for a multi-valued key.
 
 --get-regexp::
 	Like --get-all, but interprets the name as a regular expression and
-- 
2.7.1.503.g3cfa3ac

Re: [PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Here's a series that changes the behaviour of "git config --get-urlmatch"
> when no appropriate key is found as well as a couple of improvements to
> the documentation while we're here.

Sounds sensible.  It does change the behaviour, but it is inevitable
that a bugfix has to change existing behaviour, so...

>
> John Keeping (3):
>   config: fail if --get-urlmatch finds no value
>   Documentation/git-config: use bulleted list for exit codes
>   Documentation/git-config: fix --get-all description
>
>  Documentation/git-config.txt | 19 +++++++++----------
>  builtin/config.c             |  5 ++++-
>  t/t1300-repo-config.sh       |  3 +++
>  3 files changed, 16 insertions(+), 11 deletions(-)

Re: git config --get-urlmatch does not set exit code 1 when no match is found

[Jeff King]

On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:

> My current woes are with multi-valued configuration values. More
> specifically credential.helper
> 
> The documentation of git config says that when a value is not matched
> it should return 1.
> 
> To reproduce make sure that credential.helper is not set.
> 
> git config --get-urlmatch credential.helper http://somedomain:1234/
> echo %ERRORLEVEL%
> 0

This isn't really addressing your question, but I should warn you that
internally, the credential code _doesn't_ use the urlmatch
infrastructure. It predates the urlmatch code, and was never converted
(so basically only http.* uses urlmatch).  I think there are some corner
cases where the two behave differently.

I'm not sure what you're using this for, but you may get surprising
results.

-Peff

Re: git config --get-urlmatch does not set exit code 1 when no match is found

[Guilherme]

@Peff Thank you for the heads up.

I'm trying to find out if there are any credential helpers configured
in the system that will be running tests. On the dedicated test
machines that is not a problem but the developer machines are.

Should I already post a pre-emptive email asking about the corner cases?

More importantly for me is if there is a case where get-url would not
show a match where git clone would. If git clone skips a configuration
that config url-match doesn't then it's not so bad.

Re: git config --get-urlmatch does not set exit code 1 when no match is found

[Jeff King]

On Mon, Feb 29, 2016 at 06:38:28PM +0530, Guilherme wrote:

> @Peff Thank you for the heads up.
> 
> I'm trying to find out if there are any credential helpers configured
> in the system that will be running tests. On the dedicated test
> machines that is not a problem but the developer machines are.

Do you need to do it in an automated way, or just once?

If manual is OK, I suspect running:

  GIT_TRACE=1 git credential </dev/null

will give you a list of what gets run. That's awfully hacky, though.
Probably a "git credential list" command would be useful. Or just
adapting it to use the urlmatch stuff.

> Should I already post a pre-emptive email asking about the corner cases?
> 
> More importantly for me is if there is a case where get-url would not
> show a match where git clone would. If git clone skips a configuration
> that config url-match doesn't then it's not so bad.

Sorry, I don't recall the details. I feel like we discussed it a little
on the list, but I can't find it now. The closest I could find is:

  http://article.gmane.org/gmane.comp.version-control.git/267895

I think the main differences would be in ordering, not in what is
selected.

-Peff