Re: [PATCH v9 2/2] test-config: add tests for the config_set API

[Junio C Hamano <gitster@pobox.com>]

Tanay Abhra <tanayabh@gmail.com> writes:

> Expose the `config_set` C API as a set of simple commands in order to
> facilitate testing. Add tests for the `config_set` API as well as for
> `git_config_get_*()` family for the usual config files.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  .gitignore            |   1 +
>  Makefile              |   1 +
>  t/t1308-config-set.sh | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  test-config.c         | 140 +++++++++++++++++++++++++++++++++
>  4 files changed, 354 insertions(+)
>  create mode 100755 t/t1308-config-set.sh
>  create mode 100644 test-config.c
>
> diff --git a/.gitignore b/.gitignore
> index 42294e5..7677533 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -177,6 +177,7 @@
>  /gitweb/static/gitweb.min.*
>  /test-chmtime
>  /test-ctype
> +/test-config
>  /test-date
>  /test-delta
>  /test-dump-cache-tree
> diff --git a/Makefile b/Makefile
> index b92418d..e070eb8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
>  
>  TEST_PROGRAMS_NEED_X += test-chmtime
>  TEST_PROGRAMS_NEED_X += test-ctype
> +TEST_PROGRAMS_NEED_X += test-config
>  TEST_PROGRAMS_NEED_X += test-date
>  TEST_PROGRAMS_NEED_X += test-delta
>  TEST_PROGRAMS_NEED_X += test-dump-cache-tree
> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> new file mode 100755
> index 0000000..94085eb
> --- /dev/null
> +++ b/t/t1308-config-set.sh
> @@ -0,0 +1,212 @@
> +#!/bin/sh
> +
> +test_description='Test git config-set API in different settings'
> +
> +. ./test-lib.sh
> +
> +# 'check_config get_* section.key value' verifies that the entry for
> +# section.key is 'value'
> +check_config () {
> +	case "$1" in
> +	expect_code)
> +		if [ "$#" -lt 5 ];
> +		then

Spell out "test" and drop unnecessary semicolon, i.e.

	if test $# -lt 5
        then

> +			>expect
> +		else
> +			printf "%s\n" "$5" >expect

The other "expecting success" side of this function allows to expect
more than one line of output, but this one only allows you to expect
at most one line?  Why?

> +		fi &&
> +		test_expect_code "$2" test-config "$3" "$4" >actual
> +		;;
> +	*)
> +		op=$1 key=$2 &&
> +		shift &&
> +		shift &&
> +		printf "%s\n" "$@" >expect &&
> +		test-config "$op" "$key" >actual
> +		;;
> +	esac &&
> +	test_cmp expect actual

Perhaps you meant to say something like this instead?

        if test "$1" = expect_code
        then
        	expect_code="$2" && shift && shift
	else
        	expect_code=0
	fi &&
	op=$1 key=$2 && shift && shift
        if test $# != 0
        then
        	printf "%s\n" "$@"
	fi >expect &&
        test_expect_code $expet_code test-config "$op" "$key" >actual &&
	test_cmp expect actual

I dunno.

> +test_expect_success 'setup default config' '
> +	cat >.git/config <<\EOF

So the default .git/config that was prepared by "git init" is
discarded and replaced with this?  Shouldn't it be

	cat >>.git/config <<\EOF

instead?

> +test_expect_success 'find multiple values' '
> +	cat >expect <<-\EOF &&
> +	sam
> +	bat
> +	hask
> +	EOF
> +	test-config get_value_multi "case.baz">actual &&
> +	test_cmp expect actual
> +'

Hmmm, wasn't the whole point of the helper to allow us to make
things like the above into a one-liner, perhaps like this?

      check_config get_value_multi case.baz sam bat hask

I suspect the same applies to most if not all uses of test-config
in the remainder of this patch.