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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jan 12, 2021 at 12:39:37PM -0500, Derrick Stolee wrote:
> 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.

I agree that this wouldn't work; the second parameter is the default
value, not a substitute for "false".

It _would_ work currently, since there aren't any tests in this series
that explicitly

    export GIT_TEST_WRITE_REF_INDEX=false

t5325 does 'sane_unset GIT_TEST_WRITE_REV_INDEX', which accomplishes
the same thing without setting a value. If we instead used the export
form, this would break, so I think the implementation is more robust
as-is.

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

FWIW, I'm not a huge fan of the indirection either, but I think that its
use here is warranted, since it is checked twice (in index-pack, and
pack-objects).

It _could_ be checked lower in the call stack, but I tried this when
developing this series and it ended up being far messier than what is
presented here. The trickiness is with the extra verification mode
(which ensures that an existing '.rev' file was written correctly), and
having to indicate that at each caller.

> Thanks,
> -Stolee

Thanks,
Taylor