[PATCH] buitin_config: return postitive status in get_value

[PATCH] buitin_config: return postitive status in get_value

[Nikolai Vladimirov]

Returning -1 instead of 1 results in wrong exit status(255) since
the output of get_value is passed to exit().

'git config missing_section' should now return proper exit status = 1,
as specified by the git config documentation.

Signed-off-by: Nikolai Vladimirov <nikolay@vladimiroff.com>
---
 builtin/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..c262cbb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -160,7 +160,7 @@ static int show_config(const char *key_, const char *value_, void *cb)
 
 static int get_value(const char *key_, const char *regex_)
 {
-	int ret = -1;
+	int ret = 1;
 	char *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
-- 
1.7.11.2

Re: [PATCH] buitin_config: return postitive status in get_value

[Nguyen Thai Ngoc Duy]

On Sat, Jul 28, 2012 at 6:42 PM, Nikolai Vladimirov
<nikolay@vladimiroff.com> wrote:
> Returning -1 instead of 1 results in wrong exit status(255) since
> the output of get_value is passed to exit().
>
> 'git config missing_section' should now return proper exit status = 1,
> as specified by the git config documentation.

I'm curious. Why is -1 (or 255) wrong? It was introduced in the first
version of get_value in 4ddba79 (git-config-set: add more options -
2005-11-20). Back then it returned -1 when there's regex compile error
to distinguish with 0 and 1 (but git-config-set.txt in the same commit
did not get update about exit code). Maybe we should update document
instead of the code.
-- 
Duy

Re: [PATCH] buitin_config: return postitive status in get_value

[Nikolay Vladimirov]

On 28 July 2012 15:59, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> I'm curious. Why is -1 (or 255) wrong? It was introduced in the first
> version of get_value in 4ddba79 (git-config-set: add more options -
> 2005-11-20). Back then it returned -1 when there's regex compile error
> to distinguish with 0 and 1 (but git-config-set.txt in the same commit
> did not get update about exit code). Maybe we should update document
> instead of the code.
> --
> Duy

That sounds great.
But the behavior now seems kind of strange, or maybe I'm missing something:
# git config foobar; echo $?
error: key does not contain a section: foobar
255

# git config foobar.info; echo $?
1

git version 1.7.11.2

I would generally expect the both to behave the same way.

Re: [PATCH] buitin_config: return postitive status in get_value

[Nguyen Thai Ngoc Duy]

On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote:
> But the behavior now seems kind of strange, or maybe I'm missing something:
> # git config foobar; echo $?
> error: key does not contain a section: foobar
> 255
> 
> # git config foobar.info; echo $?
> 1
> 
> git version 1.7.11.2
> 
> I would generally expect the both to behave the same way.

Then the following patch may be better because it leaves other cases
untouched (I'm not saying that we should or should not do it though)

-- 8< --
diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..d048ebf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -199,8 +199,10 @@ static int get_value(const char *key_, const char *regex_)
 			goto free_strings;
 		}
 	} else {
-		if (git_config_parse_key(key_, &key, NULL))
+		if (git_config_parse_key(key_, &key, NULL)) {
+			ret = 1;
 			goto free_strings;
+		}
 	}
 
 	if (regex_) {
-- 8< --

-- 
Duy

Re: [PATCH] buitin_config: return postitive status in get_value

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote:
>> But the behavior now seems kind of strange, or maybe I'm missing something:
>> # git config foobar; echo $?
>> error: key does not contain a section: foobar
>> 255
>> 
>> # git config foobar.info; echo $?
>> 1
>> 
>> git version 1.7.11.2
>> 
>> I would generally expect the both to behave the same way.
>
> Then the following patch may be better because it leaves other cases
> untouched (I'm not saying that we should or should not do it though)

I personally think that the documentation unnecessarily exposes the
useless value assignment of the exit codes (the use of different
exit codes was done merely to aid debugging the git-config command
itself) by describing the then-current set of conditions, and should
be reduced to say "0 for success, non-zero for any error".

But if we really want to follow that "documented" subset of possible
conditions, I agree that your version to return "1" in this case,
together with a change to initialize "ret" to "7" and document it as
"all other errors (ret=7), would make more sense.  Other conditions
that have been added since that partial enumeration of the exit code
was done are regexp errors, which I think will get -1 from the same
function.

>
> -- 8< --
> diff --git a/builtin/config.c b/builtin/config.c
> index 8cd08da..d048ebf 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -199,8 +199,10 @@ static int get_value(const char *key_, const char *regex_)
>  			goto free_strings;
>  		}
>  	} else {
> -		if (git_config_parse_key(key_, &key, NULL))
> +		if (git_config_parse_key(key_, &key, NULL)) {
> +			ret = 1;
>  			goto free_strings;
> +		}
>  	}
>  
>  	if (regex_) {
> -- 8< --

Re: [PATCH] buitin_config: return postitive status in get_value

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I personally think that the documentation unnecessarily exposes the
> useless value assignment of the exit codes (the use of different
> exit codes was done merely to aid debugging the git-config command
> itself) by describing the then-current set of conditions, and should
> be reduced to say "0 for success, non-zero for any error".
>
> But if we really want to follow that "documented" subset of possible
> conditions, I agree that your version to return "1" in this case,
> together with a change to initialize "ret" to "7" and document it as
> "all other errors (ret=7), would make more sense.  Other conditions
> that have been added since that partial enumeration of the exit code
> was done are regexp errors, which I think will get -1 from the same
> function.

IOW, something like this.

 Documentation/git-config.txt | 18 ++++++++++--------
 builtin/config.c             |  8 ++++++--
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2d6ef32..b071d65 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -54,19 +54,21 @@ configuration file by default, and options '--system', '--global',
 '--file <filename>' can be used to tell the command to write to
 that location (you can say '--local' but that is the default).
 
-This command will fail (with exit code ret) if:
+This command will fail with non-zero status if:
 
+. The section name or key name is invalid (ret=1),
+. No section name or key name was provided (ret=2),
 . 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),
-. you try to use an invalid regexp (ret=6), or
-. you use '--global' option without $HOME being properly set (ret=128).
+. The config file cannot be written (ret=4),
+. 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),
+. You try to use an invalid regexp (ret=6),
+. You use '--global' option without $HOME being properly set (ret=128),
+. Any other errors (ret=7).
 
 On success, the command returns the exit code 0.
 
+
 OPTIONS
 -------
 
diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..2318308 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -160,7 +160,7 @@ static int show_config(const char *key_, const char *value_, void *cb)
 
 static int get_value(const char *key_, const char *regex_)
 {
-	int ret = -1;
+	int ret = 7;
 	char *global = NULL, *xdg = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 	struct config_include_data inc = CONFIG_INCLUDE_INIT;
@@ -196,11 +196,14 @@ static int get_value(const char *key_, const char *regex_)
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
 			free(key);
+			ret = 6;
 			goto free_strings;
 		}
 	} else {
-		if (git_config_parse_key(key_, &key, NULL))
+		if (git_config_parse_key(key_, &key, NULL)) {
+			ret = 1;
 			goto free_strings;
+		}
 	}
 
 	if (regex_) {
@@ -212,6 +215,7 @@ static int get_value(const char *key_, const char *regex_)
 		regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(regexp, regex_, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid pattern: %s\n", regex_);
+			ret = 6;
 			goto free_strings;
 		}
 	}

Re: [PATCH] buitin_config: return postitive status in get_value

[Jeff King]

On Sun, Jul 29, 2012 at 01:43:21PM -0700, Junio C Hamano wrote:

> > But if we really want to follow that "documented" subset of possible
> > conditions, I agree that your version to return "1" in this case,
> > together with a change to initialize "ret" to "7" and document it as
> > "all other errors (ret=7), would make more sense.  Other conditions
> > that have been added since that partial enumeration of the exit code
> > was done are regexp errors, which I think will get -1 from the same
> > function.
> 
> IOW, something like this.
> 
>  Documentation/git-config.txt | 18 ++++++++++--------
>  builtin/config.c             |  8 ++++++--
>  2 files changed, 16 insertions(+), 10 deletions(-)

This looks right to me. Even if we are missing an error case, it is
certainly going in the right direction.

-Peff

Re: [PATCH] buitin_config: return postitive status in get_value

[Nguyen Thai Ngoc Duy]

On Mon, Jul 30, 2012 at 3:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> +. You use '--global' option without $HOME being properly set (ret=128),
> +. Any other errors (ret=7).

To be pedantic, ret=128 is a result of die() and not setting $HOME is
just one of them. There's also ret=129 for usage(), which is not
documented. So maybe we can make it clear that we document some exit
codes, but not all of them.

> -This command will fail (with exit code ret) if:
> +This command will fail with non-zero status if:

"This command will fail with non-zero status. Some important exit codes are:"
-- 
Duy

Re: [PATCH] buitin_config: return postitive status in get_value

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Mon, Jul 30, 2012 at 3:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +. You use '--global' option without $HOME being properly set (ret=128),
>> +. Any other errors (ret=7).
>
> To be pedantic, ret=128 is a result of die() and not setting $HOME is
> just one of them. There's also ret=129 for usage(), which is not
> documented. So maybe we can make it clear that we document some exit
> codes, but not all of them.
>
>> -This command will fail (with exit code ret) if:
>> +This command will fail with non-zero status if:
>
> "This command will fail with non-zero status. Some important exit codes are:"

Sounds good; will squash in.

Re: [PATCH] buitin_config: return postitive status in get_value

[Jeff King]

On Sat, Jul 28, 2012 at 11:38:10PM -0700, Junio C Hamano wrote:

> > Then the following patch may be better because it leaves other cases
> > untouched (I'm not saying that we should or should not do it though)
> 
> I personally think that the documentation unnecessarily exposes the
> useless value assignment of the exit codes (the use of different
> exit codes was done merely to aid debugging the git-config command
> itself) by describing the then-current set of conditions, and should
> be reduced to say "0 for success, non-zero for any error".

We use ret=5 in the test suite to say "unset this variable, but it's OK if
it wasn't set in the first place" but still fail on error. The only
other one I can imagine that would be useful is "you tried to get a
variable but it did not exist, and there was no other error". Which is
probably what ret=1 is attempting to do, though it also encompasses
syntactically bogus keys.

-Peff