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

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 12, 2021, 18:17 UTC
Message-ID: <X/3n1pbZuNOIfmSr@nand.local>
In-Reply-To: <077f1a42-f37a-c7b4-b05f-fba424ac7f17@gmail.com>

On Tue, Jan 12, 2021 at 12:39:37PM -0500, Derrick Stolee wrote:

Show 18 quoted lines

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

Show 15 quoted lines

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