Re: [PATCH v2 1/7] t1300: test "set all" mode with value_regex

From: Emily Shaffer <emilyshaffer@google.com>
Date: Nov 23, 2020, 19:37 UTC
Message-ID: <20201123193714.GA499823@google.com>
In-Reply-To: <ea3099719c867a5a475468ddb6741bbcd713dccc.1606147507.git.gitgitgadget@gmail.com>

On Mon, Nov 23, 2020 at 04:05:01PM +0000, Derrick Stolee via GitGitGadget wrote:

> Add a test that demonstrates these options. Break from the existing
> pattern in t1300-config.sh to use 'git config --file=<file>' instead of
> modifying .git/config directly. Also use 'git config --file=<file>
> --list' for config state comparison instead of the config file format.

I'd normally say "but *why* do you want to do that?" but I think it's actually pretty clear in the diff based on the naming of the config files (and clever - I like it). I do think pointing out that you're breaking from existing style is useful since the existing style isn't contained in the diff alone.

Show 5 quoted lines

> +test_expect_success 'set all config with value_regex' '
> +	git config --file=initial abc.key one &&
> +
> +	cp initial config &&
> +	git config --file=config abc.key two a+ &&

Make sure that if the value_regex misses existing keys, it can still add normally.

Show 9 quoted lines

> +	git config --file=config --list >actual &&
> +	cat >expect <<-\EOF &&
> +	abc.key=one
> +	abc.key=two
> +	EOF
> +	test_cmp expect actual &&
> +
> +	test_must_fail git config --file=config abc.key three o+ 2>err &&
> +	test_i18ngrep "has multiple values" err &&

Try to change the value of the config, but it's a multiconfig already. Ok.

> +	git config --file=config abc.key three a+ &&

But /a+/ doesn't match any configs that already exist, so this one gets added fine.

Show 9 quoted lines

> +	git config --file=config --list >actual &&
> +	cat >expect <<-\EOF &&
> +	abc.key=one
> +	abc.key=two
> +	abc.key=three
> +	EOF
> +	test_cmp expect actual &&
> +
> +	cp initial config &&

Get back the config before the last exercise (with just 'one').

> +	git config --file=config abc.key three o+ &&

And if the value_regex matches exactly one config, then replace that one. I could see this case being a little bit more compelling if the value_regex was doing some lifting, e.g.

abc.key=nil abc.key=one

git config abc.key two o+

abc.key=nil abc.key=two

This criteria was kind of implied in #2 in your commit message - does it work?

Show 6 quoted lines

> +	git config --file=config --list >actual &&
> +	cat >expect <<-\EOF &&
> +	abc.key=three
> +	EOF
> +	test_cmp expect actual
> +'

Anyway, it's a pain, but I'd like to see either comments describing which case each one is, or individual tests.

 - Emily