patchread-cache: add GIT_TEST_INDEX_VERSION support

> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
> ---
> 
> Notes:
>     Base Ref: v2.19.0
>     Web-Diff: https://github.com/benpeart/git/commit/52e733e2ce
>     Checkout: git fetch https://github.com/benpeart/git git-test-index-version-v1 && git checkout 52e733e2ce
> 
>  read-cache.c | 47 +++++++++++++++++++++++++++++++++--------------
>  t/README     |  6 +++++-
>  2 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index 7b1354d759..d140ce9989 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1570,26 +1570,45 @@ static unsigned int get_index_format_default(void)
>  	char *envversion = getenv("GIT_INDEX_VERSION");
>  	char *endp;
>  	int value;
> -	unsigned int version = INDEX_FORMAT_DEFAULT;
> +	unsigned int version = -1;
> +
> +	if (envversion) {
> +		version = strtoul(envversion, &endp, 10);
> +		if (*endp ||
> +			version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> +			warning(_("GIT_INDEX_VERSION set, but the value is invalid.\n"
> +				"Using version %i"), INDEX_FORMAT_DEFAULT);
> +			version = INDEX_FORMAT_DEFAULT;
> +		}
> +	}
>  
> -	if (!envversion) {
> -		if (!git_config_get_int("index.version", &value))
> +	if (version == -1) {
> +		if (!git_config_get_int("index.version", &value)) {
>  			version = value;
> -		if (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> -			warning(_("index.version set, but the value is invalid.\n"
> -				  "Using version %i"), INDEX_FORMAT_DEFAULT);
> -			return INDEX_FORMAT_DEFAULT;
> +			if (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> +				warning(_("index.version set, but the value is invalid.\n"
> +					"Using version %i"), INDEX_FORMAT_DEFAULT);
> +				version = INDEX_FORMAT_DEFAULT;
> +			}
>  		}
> -		return version;
>  	}
>  
> -	version = strtoul(envversion, &endp, 10);
> -	if (*endp ||
> -	    version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> -		warning(_("GIT_INDEX_VERSION set, but the value is invalid.\n"
> -			  "Using version %i"), INDEX_FORMAT_DEFAULT);
> -		version = INDEX_FORMAT_DEFAULT;
> +	if (version == -1) {
> +		envversion = getenv("GIT_TEST_INDEX_VERSION");
> +		if (envversion) {
> +			version = strtoul(envversion, &endp, 10);
> +			if (*endp ||
> +				version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {
> +				warning(_("GIT_TEST_INDEX_VERSION set, but the value is invalid.\n"
> +					"Using version %i"), INDEX_FORMAT_DEFAULT);
> +				version = INDEX_FORMAT_DEFAULT;
> +			}
> +		}
>  	}
> +
> +	if (version == -1)
> +		version = INDEX_FORMAT_DEFAULT;
> +
>  	return version;
>  }
>  
> 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
>  ------------
>  
> 
> base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670
> -- 
> 2.18.0.windows.1
> 

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

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

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

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

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
>> 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).
> 
> I am not enthused by "you are using an old variable---we fail your
> build/test".  The assumption is that people let config.mak laying
> around regardless of how new/old the vintage of Git they are
> building and testing.  I do not think you'd want to adjust your
> config.mak as you switch between 'maint' and 'next.
> 
> I think it is OK to make it error only if the old one is set without
> the new one.  Then people can have _both_ set to the same value
> during the period in which the topic sails through pu down to next
> down to master, after seeing an failure once while building and
> testing 'pu'.
> 

>> 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.
> 
> Yeah, even though these three were posted as independent changes,
> their additions to t/README inevitably conflicted with each other.
> 
> 

>> I think it is OK to make it error only if the old one is set without
>> the new one.  Then people can have _both_ set to the same value
>> during the period in which the topic sails through pu down to next
>> down to master, after seeing an failure once while building and
>> testing 'pu'.
>>
>
> I'll make it a warning if they are both set so that people are
> reminded to remove the old variable and an error if only the old one
> is set so people know to update their environment.

> On Wed, Sep 12 2018, Ben Peart wrote:
>
>> -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.
>
> If re-rolled maybe better as a leading "fix a typo" patch.