Re: [PATCH v5 03/10] config API: add and use a "git_config_get()" family of functions

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> We could have changed git_configset_get_value_multi() (and then
> git_config_get_value() etc.) to accept a "NULL" as a "dest" for all
> callers, but let's avoid changing the behavior of existing API
> users. Having an "unused" value that we throw away internal to
> config.c is cheap.
>
> A "NULL as optional dest" pattern is also more fragile, as the intent
> of the caller might be misinterpreted if he were to accidentally pass
> "NULL", e.g. when "dest" is passed in from another function.

Okay, I think I can buy this argument. In other words,
git_config_get_value() is only used to put the value in "*dest", so
"dest = NULL" is an error. This is by design, because it defends against
callers who are using it wrongly. If it accepted "NULL" to mean 'dest
will be ignored', we're creating possible hard-to-spot bugs because we
no longer error out early.

> This still leaves various inconsistencies and clobbering or ignoring
> of the return value in place. E.g here we're modifying
> configset_add_value(), but ever since it was added in [2] we've been
> ignoring its "int" return value, but as we're changing the
> configset_find_element() it uses, let's have it faithfully ferry that
> "ret" along.
>
> Let's also use the "RESULT_MUST_BE_USED" macro introduced in [3] to
> assert that we're checking the return value of
> configset_find_element().
>
> We're leaving the same change to configset_add_value() for some future
> series. Once we start paying attention to its return value we'd need
> to ferry it up as deep as do_config_from(), and would need to make
> least read_{,very_}early_config() and git_protected_config() return an
> "int" instead of "void". Let's leave that for now, and focus on
> the *_get_*() functions.
>
> In a subsequent commit we'll fix the other *_get_*() functions to so
> that they'll ferry our underlying "ret" along, rather than normalizing
> it to a "return 1". But as an intermediate step to that we'll need to
> fix git_configset_get_value_multi() to return "int", and that change
> itself is smaller because of this change to migrate some callers away
> from the *_value_multi() API.

I haven't read ahead, but on first impression this sounds like it might
be too intrusive for a series whose goal is to clean up
*_get_value_multi().

> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> index 4be1ab1147c..7def7053e1c 100755
> --- a/t/t1308-config-set.sh
> +++ b/t/t1308-config-set.sh
> @@ -58,6 +58,8 @@ test_expect_success 'setup default config' '
>  		skin = false
>  		nose = 1
>  		horns
> +	[value]
> +		less
>  	EOF
>  '
>  
> @@ -116,6 +118,45 @@ test_expect_success 'find value with the highest priority' '
>  	check_config get_value case.baz "hask"
>  '
>  
> +test_expect_success 'return value for an existing key' '
> +	test-tool config get lamb.chop >out 2>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err
> +'
> +
> +test_expect_success 'return value for value-less key' '
> +	test-tool config get value.less >out 2>err &&
> +	test_must_be_empty out &&
> +	test_must_be_empty err
> +'
> +
> +test_expect_success 'return value for a missing key' '
> +	cat >expect <<-\EOF &&
> +	Value not found for "missing.key"
> +	EOF
> +	test_expect_code 1 test-tool config get missing.key >actual 2>err &&
> +	test_cmp actual expect &&
> +	test_must_be_empty err
> +'
> +
> +test_expect_success 'return value for a bad key: CONFIG_INVALID_KEY' '
> +	cat >expect <<-\EOF &&
> +	Key "fails.iskeychar.-" is invalid
> +	EOF
> +	test_expect_code 1 test-tool config get fails.iskeychar.- >actual 2>err &&
> +	test_cmp actual expect &&
> +	test_must_be_empty out
> +'
> +
> +test_expect_success 'return value for a bad key: CONFIG_NO_SECTION_OR_NAME' '
> +	cat >expect <<-\EOF &&
> +	Key "keynosection" has no section
> +	EOF
> +	test_expect_code 1 test-tool config get keynosection >actual 2>err &&
> +	test_cmp actual expect &&
> +	test_must_be_empty out
> +'
> +

No real comments on the changes themselves. The added test coverage in
this version is quite nice.