Re: [PATCH v5 5/5] setup.c: create `discovery.bare`

[Glen Choo <chooglen@google.com>]

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

> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@google.com>
>
>> diff --git a/t/t0035-discovery-bare.sh b/t/t0035-discovery-bare.sh
>> new file mode 100755
>> index 00000000000..0b345d361e6
>> --- /dev/null
>> +++ b/t/t0035-discovery-bare.sh
>> @@ -0,0 +1,68 @@
>> +#!/bin/sh
>> +
>> +test_description='verify discovery.bare checks'
>> +
>
> You're missing a:
>
> 	TEST_PASSES_SANITIZE_LEAK=true
>
> Above this line:
>
>> +. ./test-lib.sh
>
> Which tells us that this new test doesn't leak (yay!)

Ah, thanks! Hooray.

>> +expect_accepted () {
>> +	git "$@" rev-parse --git-dir
>> +}
>
> I think we can do away with this helper, we use the argument support
> once, and for the rest we can inline the trivial command...

That is true, having fewer test helpers can be a good idea. Though in
this case, the helper wins out slightly (IMO at least) because of the 
readability/refactoring benefit.

>> +
>> +expect_rejected () {
>> +	test_must_fail git "$@" rev-parse --git-dir 2>err &&
>> +	grep "discovery.bare" err
>
> grep -F ?
>
> This helper is less trivial, but more obvious would be a "run command
> and assirt xyz about the output" helper, see
> e.g. test_stdout_line_count.

This takes precedent from t0033, which does the same "run command and
grep the result". And just as I typed this out, I remembered that
t0033's corresponding test helper was made more specific in f62563988f
(t0033-safe-directory: check the error message without matching the
trash dir, 2022-04-27), because just grep-ing for the config variable
masked some errors.

It turns out the same thing is happening in the last test - I forgot
that "-c" doesn't unset the variable (it sets the value to ''), and the
test_must_fail passes because we fail to parse "discovery.bare", _not_
because we forbade the repo.

So besides -F, I think the only change here would be to grep on the
specific "cannot use bare repository" message (instead of grepping for
"discovery.bare").

>> +test_expect_success 'discovery.bare unset' '
>> +	(
>> +		cd outer-repo/bare-repo &&
>> +		expect_accepted
>> +	)
>
> Also: Odd to use a sub-shell when the helper takes -C...
>
>> +'
>> +
>> +test_expect_success 'discovery.bare=always' '
>> +	git config --global discovery.bare always &&
>> +	(
>> +		cd outer-repo/bare-repo &&
>> +		expect_accepted
>> +	)
>> +'
>> +
>> +test_expect_success 'discovery.bare=never' '
>> +	git config --global discovery.bare never &&
>> +	(
>> +		cd outer-repo/bare-repo &&
>> +		expect_rejected
>> +	)
>
> ...ditto...

Ok, I'll drop the sub-shell.

>
>> +'
>> +
>> +test_expect_success 'discovery.bare in the repository' '
>> +	(
>> +		cd outer-repo/bare-repo &&
>> +		# Temporarily set discovery.bare=always, otherwise git
>> +		# config fails with "fatal: not in a git directory"
>> +		# (like safe.directory)
>> +		git config --global discovery.bare always &&
>> +		git config discovery.bare always &&
>> +		git config --global discovery.bare never &&
>> +		expect_rejected
>> +	)
>
> Drop the sub-shell and use test_config?

Oh, I was so focused on t0033 that I hadn't realized that we had
test_config_global. Thanks :)

>> +'
>> +
>> +test_expect_success 'discovery.bare on the command line' '
>> +	git config --global discovery.bare never &&
>> +	(
>> +		cd outer-repo/bare-repo &&
>> +		expect_accepted -c discovery.bare=always &&
>> +		expect_rejected -c discovery.bare=
>> +	)
>> +'
>> +
>> +test_done