Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> On Mon, Jun 27 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@xxxxxxxxxx>
>
>> 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
