[PATCH] buitin_config: return postitive status in get_value

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

On Sat, Jul 28, 2012 at 6:42 PM, Nikolai Vladimirov
[off-list ref] 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

On 28 July 2012 15:59, Nguyen Thai Ngoc Duy [off-list ref] 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.

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