From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 02 Oct 2026 14:11:38 GMT
Subject: Re: [PATCH v9 3/4] var: accept more than one variable
Message-ID: <cda2dd01-9dab-4842-86af-9b77767dfc52@gmail.com>
In-Reply-To: <20260926162048.30853-4-andrewpleeter@gmail.com>

Hi Andrew

This all looks fine, though we typically avoid using test_env() because 
it introduces a hidden subshell. I've left a couple of suggestions 
below, but unless there is another reason to re-roll I wouldn't worry 
too much.

On 26/09/2026 17:20, Andrew Pleeter wrote:

> +test_expect_success 'variable without a value is omitted but is not an error' '
> +	test_tick &&
> +	cat >expect <<-EOF &&
> +	GIT_AUTHOR_IDENT=$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE
> +	GIT_COMMITTER_IDENT=$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +	EOF
> +	test_env GIT_CONFIG_GLOBAL= \
> +		git var GIT_AUTHOR_IDENT GIT_CONFIG_GLOBAL GIT_COMMITTER_IDENT >actual &&

There is no need to use test_env here, "GIT_CONFIG_GLOBAL= git var ..." 
is all that's needed.

> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'a single variable without a value still exits with 1' '
> +	test_env GIT_CONFIG_GLOBAL= test_expect_code 1 git var GIT_CONFIG_GLOBAL >out &&

Here test_env is also not needed, "env GIT_CONFIG_GLOBAL= 
test_expect_code 1 git var ..." would be our typical style.

Thanks

Phillip


> +	test_must_be_empty out
> +'
> +
> +test_expect_success 'unknown variable is a usage error' '
> +	test_must_fail git var GIT_AUTHOR_IDENT NO_SUCH_VARIABLE 2>err &&
> +	test_grep usage err
> +'
> +
>   test_done