From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 30 Jun 2022 13:20:01 GMT
Subject: Re: [PATCH v5 5/5] setup.c: create `discovery.bare`
Message-ID: <220630.864k02e14e.gmgdl@evledraar.gmail.com>
In-Reply-To: <14411512783fd4e2cdcc8513690377b29262f6b8.1656354994.git.gitgitgadget@gmail.com>


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!)

> +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...

> +
> +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.


> +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...


> +'
> +
> +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?

> +'
> +
> +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