Re: [PATCH 7/8] t: support GIT_TEST_WRITE_REV_INDEX

From: Derrick Stolee <stolee@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Taylor Blau" <me@ttaylorr.com>
Cc: git@vger.kernel.org, peff@peff.net, jrnieder@gmail.com
Subject: Re: [PATCH 7/8] t: support GIT_TEST_WRITE_REV_INDEX
Date: Tue, 12 Jan 2021 12:39:37 -0500	[thread overview]
Message-ID: <077f1a42-f37a-c7b4-b05f-fba424ac7f17@gmail.com> (raw)
In-Reply-To: <875z425b17.fsf@evledraar.gmail.com>

On 1/12/2021 12:18 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jan 08 2021, Taylor Blau wrote:
>> -	rev_index = !!(opts.flags & (WRITE_REV_VERIFY | WRITE_REV));
>> +	if (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))
>> +		rev_index = 1;
>> +	else
>> +		rev_index = !!(opts.flags & (WRITE_REV_VERIFY | WRITE_REV));
> 
> Why not make an explicit GIT_TEST_WRITE_REV_INDEX=false meaningful? It's
> also sometimes handy to turn these off in the tests.
> 
>     rev_index = git_env_bool("GIT_TEST_WRITE_REV_INDEX",
>     	!!(opts.flags & (WRITE_REV_VERIFY | WRITE_REV)));

This will cause tests that explicitly request a rev-index to fail when
GIT_TEST_WRITE_REF_INDEX=false. I'm not sure that's a good pattern to
follow.

>> +#define GIT_TEST_WRITE_REV_INDEX "GIT_TEST_WRITE_REV_INDEX"
> 
> Micro style nit: FWIW I'm not a fan of this macro->string indirection a
> few GIT_TEST_* names have had since 859fdc0c3c (commit-graph: define
> GIT_TEST_COMMIT_GRAPH, 2018-08-29).
> 
> Most of them just use git_env_bool("GIT_TEST_[...]") which IMO makes it
> easier to eyeball a "git grep"

In the case of GIT_TEST_COMMIT_GRAPH, there are multiple places that
check the environment variable, and it is probably best to have the
strings consistent through a macro.

For something like GIT_TEST_WRITE_REV_INDEX, this macro is less
important because it is checked only once.

Thanks,
-Stolee