Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support

[Thomas Gummerer <t.gummerer@gmail.com>]

On 09/13, Ben Peart wrote:
> 
> 
> On 9/12/2018 6:31 PM, Thomas Gummerer wrote:
> > On 09/12, Ben Peart wrote:
> > > Teach get_index_format_default() to support running the test suite
> > > with specific index versions.  In particular, this enables the test suite
> > > to be run using index version 4 which is not the default so gets less testing.
> > 
> > I found this commit message slightly misleading.  Running the test
> > suite with specific index versions is already supported, by defining
> > TEST_GIT_INDEX_VERSION in 'config.mak'.  What we're doing here is
> > introduce an additional environment variable that can also be used to
> > set the index format in tests.
> > 
> > Even setting TEST_GIT_INDEX_VERSION=4 in the environment does run the
> > test suite with index-v4.  Admittedly the name is a bit strange
> > compared to our usual GIT_TEST_* environment variable names, and it
> > should probably be documented better (it's only documented in the
> > Makefile currently), but I'm not sure we should introduce another
> > environment variable for this purpose?
> 
> TEST_GIT_INDEX_VERSION enables the testing I was looking for but you're
> right, it isn't well documented and the atypical naming and implementation
> don't help either.
> 
> I checked the documentation and code but didn't see any way to test the V4
> index code path so wrote this patch.  I wonder if we can improve the
> discoverability of TEST_GIT_INDEX_VERSION through better naming and
> documentation.
> 
> How about this as an alternative?

Thanks, I do think this is a good idea.  I do however share Ævar's
concern in https://public-inbox.org/git/87h8itkz2h.fsf@evledraar.gmail.com/.
I have TEST_GIT_INDEX_VERSION=4 set in my config.mak since quite a
long time, and had I missed this thread, I would all of a sudden not
run the test suite with index format 4 anymore without any notice.

I think the suggestion of erroring out if TEST_GIT_INDEX_VERSION is
set would be useful in this case (and probably others in which you're
renaming these variables).  Not sure how many people it would affect
(and most of those would probably read the mailing list), but it's not
a big change either.

Btw, I think it would be nice to have all these renaming/documenting
variables for the test suite patches in one series, so they are easier
to look at with more context.

> 
> diff --git a/Makefile b/Makefile
> index 5a969f5830..9e84ef02f7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -400,7 +400,7 @@ all::
>  # (defaults to "man") if you want to have a different default when
>  # "git help" is called without a parameter specifying the format.
>  #
> -# Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite
> +# Define GIT_TEST_INDEX_VERSION to 2, 3 or 4 to run the test suite
>  # with a different indexfile format version.  If it isn't set the index
>  # file format used is index-v[23].
>  #
> @@ -2599,8 +2599,8 @@ endif
>  ifdef GIT_INTEROP_MAKE_OPTS
>         @echo GIT_INTEROP_MAKE_OPTS=\''$(subst ','\'',$(subst
> ','\'',$(GIT_INTEROP_MAKE_OPTS)))'\' >>$@+
>  endif
> -ifdef TEST_GIT_INDEX_VERSION
> -       @echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst
> ','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@+
> +ifdef GIT_TEST_INDEX_VERSION
> +       @echo GIT_TEST_INDEX_VERSION=\''$(subst ','\'',$(subst
> ','\'',$(GIT_TEST_INDEX_VERSION)))'\' >>$@+
> 
>  endif
>         @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 44288cbb59..31698c01c4 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -134,9 +134,9 @@ export EDITOR
>  GIT_TRACE_BARE=1
>  export GIT_TRACE_BARE
> 
> -if test -n "${TEST_GIT_INDEX_VERSION:+isset}"
> +if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
>  then
> -       GIT_INDEX_VERSION="$TEST_GIT_INDEX_VERSION"
> +       GIT_INDEX_VERSION="$GIT_TEST_INDEX_VERSION"
>         export GIT_INDEX_VERSION
>  fi
> 
> diff --git a/t/README b/t/README
> index 9028b47d92..f872638a78 100644
> --- a/t/README
> +++ b/t/README
> @@ -315,10 +315,14 @@ packs on demand. This normally only happens when the
> object size is
>   over 2GB. This variable forces the code path on any object larger than
>   <n> bytes.
> 
> -GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code
> +GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code
>   path where deltas larger than this limit require extra memory
>   allocation for bookkeeping.
> 
> +GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
> +for the index version specified.  Can be set to any valid version
> +but the non-default version 4 is probably the most beneficial.
> +
>   Naming Tests
>   ------------
>