Re: [PATCH 1/2] config: return positive from git_config_parse_key()

[Jonathan Tan <jonathantanmy@google.com>]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Glen Choo <chooglen@google.com>
> 
> git_config_parse_key() returns #define-d error codes, but negated. This
> negation is merely a convenience to other parts of config.c that don't
> bother inspecting the return value before passing it along. But:
> 
> a) There's no good reason why those callers couldn't negate the value
>    themselves.
> 
> b) In other callers, this value eventually gets fed to exit(3), and
>    those callers need to sanitize the negative value (and they sometimes
>    do so lossily, by overriding the return value with
>    CONFIG_INVALID_KEY).
> 
> c) We want to move that into a separate library, and returning only
>    negative values no longer makes as much sense.

I'm not sure if we ever concluded that functions returning errors should
return positive integers, but in this case I think it makes sense. We
can document what's returned as being the same as what's documented in
the config manpage.

The negative return was as early as when the function was first
introduced in b09c53a3e3 (Sanity-check config variable names, 2011-01-
30), but there's no indication there as to why the author chose negative
values.

> Change git_config_parse_key() to return positive values instead, and
> adjust callers accordingly. Callers that sanitize the negative sign for
> exit(3) now pass the return value opaquely, fixing a bug where "git
> config <key with no section or name>" results in a different exit code
> depending on whether we are setting or getting config.

Can you be more precise as to which bug is being fixed? (I think
somewhere, a 1 is returned when it should be a 2.)

> Callers that
> wanted to pass along a negative value now negate the return value
> themselves.

OK.

> diff --git a/builtin/config.c b/builtin/config.c
> index 1c75cbc43df..8a2840f0a8c 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -362,8 +362,7 @@ static int get_value(const char *key_, const char *regex_, unsigned flags)
>  			goto free_strings;
>  		}
>  	} else {
> -		if (git_config_parse_key(key_, &key, NULL)) {
> -			ret = CONFIG_INVALID_KEY;
> +		if ((ret = git_config_parse_key(key_, &key, NULL))) {
>  			goto free_strings;
>  		}
>  	}

Ah, here, the return value was sanitized in such a way that it lost
information. The change makes sense.

Besides the callers modified in this patch, there is another caller
config_parse_pair() in config.c, but that just checks whether the return
value is 0, so it remaining unmodified is fine.

> diff --git a/config.h b/config.h
> index 6332d749047..40966cb6828 100644
> --- a/config.h
> +++ b/config.h
> @@ -23,7 +23,7 @@
>  
>  struct object_id;
>  
> -/* git_config_parse_key() returns these negated: */
> +/* git_config_parse_key() returns these: */
>  #define CONFIG_INVALID_KEY 1
>  #define CONFIG_NO_SECTION_OR_NAME 2

Should these be turned into an enum? Also, it might be worth adding that
these match the return values as documented in the manpage.

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 387d336c91f..3202b0f8843 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2590,4 +2590,20 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
>  	grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
>  '
>  
> +# Exit codes
> +test_expect_success '--get with bad key' '

Rather than put an "exit codes" title, maybe embed that in the test
description.

> +	# Also exits with 1 if the value is not found

I don't understand this comment - what's the difference between a bad
key and a value not being found? And if there's a difference, could we
test both?

> +	test_expect_code 1 git config --get "bad.name\n" 2>err &&
> +	grep "error: invalid key" err &&
> +	test_expect_code 2 git config --get "bad." 2>err &&
> +	grep "error: key does not contain variable name" err
> +'
> +
> +test_expect_success 'set with bad key' '
> +	test_expect_code 1 git config "bad.name\n" var 2>err &&
> +	grep "error: invalid key" err &&
> +	test_expect_code 2 git config "bad." var 2>err &&
> +	grep "error: key does not contain variable name" err
> +'

Makes sense.

From a libification perspective, I'm not sure that using positive values
to indicate error is an advantage over negative values, but it makes
sense in this particular context to have the return values match the
manpage exactly, since that is part of the benefit of this function. So
I think this patch is worth getting in by itself.