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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 29, 2012, 06:38 UTC
Message-ID: <7vipd7vzql.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120728132923.GA31388@do>

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

Show 15 quoted lines

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

Show 19 quoted lines

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