* Re: git config --get-urlmatch does not set exit code 1 when no match is found
2016-02-28 10:45 ` John Keeping
@ 2016-02-28 11:52 ` John Keeping
2016-02-28 11:54 ` [PATCH 0/3] " John Keeping
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: John Keeping @ 2016-02-28 11:52 UTC (permalink / raw)
To: Guilherme; +Cc: git@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found
2016-02-28 10:45 ` John Keeping
2016-02-28 11:52 ` John Keeping
@ 2016-02-28 11:54 ` John Keeping
2016-02-28 20:01 ` Junio C Hamano
2016-02-28 11:54 ` [PATCH 1/3] config: fail if --get-urlmatch finds no value John Keeping
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: John Keeping @ 2016-02-28 11:54 UTC (permalink / raw)
To: git; +Cc: John Keeping, Guilherme
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found
2016-02-28 11:54 ` [PATCH 0/3] " John Keeping
@ 2016-02-28 20:01 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2016-02-28 20:01 UTC (permalink / raw)
To: John Keeping; +Cc: git, Guilherme
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(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] config: fail if --get-urlmatch finds no value
2016-02-28 10:45 ` John Keeping
2016-02-28 11:52 ` John Keeping
2016-02-28 11:54 ` [PATCH 0/3] " John Keeping
@ 2016-02-28 11:54 ` John Keeping
2016-02-28 11:54 ` [PATCH 2/3] Documentation/git-config: use bulleted list for exit codes John Keeping
2016-02-28 11:54 ` [PATCH 3/3] Documentation/git-config: fix --get-all description John Keeping
4 siblings, 0 replies; 11+ messages in thread
From: John Keeping @ 2016-02-28 11:54 UTC (permalink / raw)
To: git; +Cc: John Keeping, Guilherme
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/3] Documentation/git-config: use bulleted list for exit codes
2016-02-28 10:45 ` John Keeping
` (2 preceding siblings ...)
2016-02-28 11:54 ` [PATCH 1/3] config: fail if --get-urlmatch finds no value John Keeping
@ 2016-02-28 11:54 ` John Keeping
2016-02-28 11:54 ` [PATCH 3/3] Documentation/git-config: fix --get-all description John Keeping
4 siblings, 0 replies; 11+ messages in thread
From: John Keeping @ 2016-02-28 11:54 UTC (permalink / raw)
To: git; +Cc: John Keeping, Guilherme
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/3] Documentation/git-config: fix --get-all description
2016-02-28 10:45 ` John Keeping
` (3 preceding siblings ...)
2016-02-28 11:54 ` [PATCH 2/3] Documentation/git-config: use bulleted list for exit codes John Keeping
@ 2016-02-28 11:54 ` John Keeping
4 siblings, 0 replies; 11+ messages in thread
From: John Keeping @ 2016-02-28 11:54 UTC (permalink / raw)
To: git; +Cc: John Keeping, Guilherme
--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
^ permalink raw reply related [flat|nested] 11+ messages in thread