[PATCH v1 0/4] Cleanup pass on special test setups

[PATCH v1 0/4] Cleanup pass on special test setups

[Ben Peart]

As documented in t/README, the whole test suite can be run to test some
special features that cannot be easily covered by a few specific test cases.

Not all of these that exist in the code have been named consistantly and
documented in r/README which leads to a discoverability problem.  Update
several of these variables to follow the same naming pattern and document
them properly.

To facilitate transitioning from the old names to the new names, add logic
in t/test-lib.sh to give an error when the old variable is set to let people
know they need to update their environment to use the new variable. If the
new variable is also set, just give a warning so they can eventually remove
the old variable.

Base Ref: v2.19.0
Web-Diff: https://github.com/benpeart/git/commit/eff73d737e
Checkout: git fetch https://github.com/benpeart/git git-test-cleanup-v1 && git checkout eff73d737e

Ben Peart (4):
  correct typo/spelling error in t/README
  fsmonitor: update GIT_TEST_FSMONITOR support
  read-cache: update TEST_GIT_INDEX_VERSION support
  preload-index: update GIT_FORCE_PRELOAD_TEST support

 Makefile                    |  6 +++---
 config.c                    |  2 +-
 preload-index.c             |  3 ++-
 t/README                    | 13 ++++++++++++-
 t/t1700-split-index.sh      |  2 +-
 t/t7519-status-fsmonitor.sh |  6 +++---
 t/test-lib.sh               | 37 +++++++++++++++++++++++++++++++++++--
 7 files changed, 57 insertions(+), 12 deletions(-)


base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670
-- 
2.18.0.windows.1

[PATCH v1 1/4] correct typo/spelling error in t/README

[Ben Peart]

Correct a spelling error in the documentation for GIT_TEST_OE_DELTA_SIZE

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 t/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 9028b47d92..56a417439c 100644
--- a/t/README
+++ b/t/README
@@ -315,7 +315,7 @@ 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.
 
-- 
2.18.0.windows.1

[PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

[Ben Peart]

Rename GIT_FSMONITOR_TEST to GIT_TEST_FSMONITOR for consistency with the
other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give an error when the old variable is set to
let people know they need to update their environment to use the new
variable. If the new variable is also set, just give a warning so they can
eventually remove the old variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 config.c                    |  2 +-
 t/README                    |  4 ++++
 t/t1700-split-index.sh      |  2 +-
 t/t7519-status-fsmonitor.sh |  2 +-
 t/test-lib.sh               | 11 +++++++++++
 5 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 3461993f0a..3555c63f28 100644
--- a/config.c
+++ b/config.c
@@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void)
 int git_config_get_fsmonitor(void)
 {
 	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
-		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
 
 	if (core_fsmonitor && !*core_fsmonitor)
 		core_fsmonitor = NULL;
diff --git a/t/README b/t/README
index 56a417439c..47165f7eab 100644
--- a/t/README
+++ b/t/README
@@ -319,6 +319,10 @@ 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_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
+code path for utilizing a file system monitor to speed up detecting
+new or changed files.
+
 Naming Tests
 ------------
 
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index b3b4d83eaf..f6a856f24c 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -6,7 +6,7 @@ test_description='split index mode tests'
 
 # We need total control of index splitting here
 sane_unset GIT_TEST_SPLIT_INDEX
-sane_unset GIT_FSMONITOR_TEST
+sane_unset GIT_TEST_FSMONITOR
 
 test_expect_success 'enable split index' '
 	git config splitIndex.maxPercentChange 100 &&
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 756beb0d8e..d77012ea6d 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -8,7 +8,7 @@ test_description='git status with file system watcher'
 # To run the entire git test suite using fsmonitor:
 #
 # copy t/t7519/fsmonitor-all to a location in your path and then set
-# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests.
+# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.
 #
 
 # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 44288cbb59..0ef111d808 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -140,6 +140,17 @@ then
 	export GIT_INDEX_VERSION
 fi
 
+if test -n "$GIT_FSMONITOR_TEST"
+then
+	if test -n "$GIT_TEST_FSMONITOR"
+	then
+		echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
+	else
+		echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
+		exit 1
+	fi
+fi
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
-- 
2.18.0.windows.1

Re: [PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

[Junio C Hamano]

Ben Peart <benpeart@microsoft.com> writes:

> +if test -n "$GIT_FSMONITOR_TEST"
> +then
> +	if test -n "$GIT_TEST_FSMONITOR"
> +	then
> +		echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
> +	else
> +		echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
> +		exit 1
> +	fi
> +fi

I would have expected that, because we are now doing multiple pairs
of variables in a single series, we would add a helper function that
can be called like so:

	check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR

in the earliest step.  Perhaps something like this.

check_var_migration () {
	old_name=$1 new_name=$2
	eval "old_isset=\${${old_name}:+isset}"
	eval "new_isset=\${${new_name}:+isset}"
	case "$old_isset,$new_isset" in
	isset,)
		echo >&2 "error: $old_name is now $new_name"
		exit 1 ;;
	isset,isset)
		# enable this, once $old_name no longer is valid anywhere
		# echo >&2 "warning: $old_name is now $new_name"
		# echo >&2 "hint: remove $old_name"
		;;
	esac
}

Re: [PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ben Peart <benpeart@microsoft.com> writes:
>
>> +if test -n "$GIT_FSMONITOR_TEST"
>> +then
>> +	if test -n "$GIT_TEST_FSMONITOR"
>> +	then
>> +		echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
>> +	else
>> +		echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
>> +		exit 1
>> +	fi
>> +fi
>
> I would have expected that, because we are now doing multiple pairs
> of variables in a single series, we would add a helper function that
> can be called like so:
>
> 	check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
>
> in the earliest step.  Perhaps something like this.
>
> check_var_migration () {
> 	old_name=$1 new_name=$2
> 	eval "old_isset=\${${old_name}:+isset}"
> 	eval "new_isset=\${${new_name}:+isset}"
> 	case "$old_isset,$new_isset" in
> 	isset,)
> 		echo >&2 "error: $old_name is now $new_name"
> 		exit 1 ;;
> 	isset,isset)
> 		# enable this, once $old_name no longer is valid anywhere
> 		# echo >&2 "warning: $old_name is now $new_name"
> 		# echo >&2 "hint: remove $old_name"
> 		;;
> 	esac
> }

Alternatively, we could do this, to warn and then migrate the value
given to the old variable automatically to the new variable and let
the test proceed.

check_var_migration () {
	old_name=$1 new_name=$2
	eval "old_isset=\${${old_name}:+isset}"
	eval "new_isset=\${${new_name}:+isset}"
	case "$old_isset,$new_isset" in
	isset,)
		echo >&2 "warning: $old_name is now $new_name"
		echo >&2 "hint: set $new_name too during the transition period"
		eval "$new_name=\$$old_name"
		;;
	isset,isset)
		# do this later
		# echo >&2 "warning: $old_name is now $new_name"
		# echo >&2 "hint: remove $old_name"
		;;
	esac
}

Re: [PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

[Junio C Hamano]

Ben Peart <benpeart@microsoft.com> writes:

> diff --git a/config.c b/config.c
> index 3461993f0a..3555c63f28 100644
> --- a/config.c
> +++ b/config.c
> @@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void)
>  int git_config_get_fsmonitor(void)
>  {
>  	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
> -		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
> +		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");

Sorry for not noticing earlier, but unlike 4/4 that changed
getenv(VAR) to git_env_bool(VAR, 0) "while at it", this leaves it to
getenv(VAR), meaning "if it is set to any non-empty string, it is
true".  Is there a reason for this discrepancy?

I _think_ the renaming should be done without getting mixed with
other changes like the git_env_bool() done in 4/4.  The idea to use
git_env_bool() in stead of getenv() may be a good one, but then we
should consistently do so when appropriate, and that would make a
fine theme for another topic.

Re: [PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

[Ben Peart]

On 9/14/2018 1:15 PM, Junio C Hamano wrote:
> Ben Peart <benpeart@microsoft.com> writes:
> 
>> diff --git a/config.c b/config.c
>> index 3461993f0a..3555c63f28 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void)
>>   int git_config_get_fsmonitor(void)
>>   {
>>   	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
>> -		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
>> +		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
> 
> Sorry for not noticing earlier, but unlike 4/4 that changed
> getenv(VAR) to git_env_bool(VAR, 0) "while at it", this leaves it to
> getenv(VAR), meaning "if it is set to any non-empty string, it is
> true".  Is there a reason for this discrepancy?
> 

The difference here is that core.fsmonitor isn't a boolean value.  It is 
a string to a command that is executed so it can't be moved over to 
get_env_bool().

> I _think_ the renaming should be done without getting mixed with
> other changes like the git_env_bool() done in 4/4.  The idea to use
> git_env_bool() in stead of getenv() may be a good one, but then we
> should consistently do so when appropriate, and that would make a
> fine theme for another topic.
> 

Re: [PATCH v1 2/4] fsmonitor: update GIT_TEST_FSMONITOR support

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> The difference here is that core.fsmonitor isn't a boolean value.  It
> is a string to a command that is executed so it can't be moved over to
> get_env_bool().

Ah, of course ;-)

Then please take the following as a review comment for 4/4; checking
if each getenv(VAR) should or should not become git_env_bool() and
updating them should be done as a separate change for variables
whether they are being renamed or not in this series.

>> I _think_ the renaming should be done without getting mixed with
>> other changes like the git_env_bool() done in 4/4.  The idea to use
>> git_env_bool() in stead of getenv() may be a good one, but then we
>> should consistently do so when appropriate, and that would make a
>> fine theme for another topic.
>>

[PATCH v2 0/5] Cleanup pass on special test setups

[Ben Peart]

Changes this round are to use Junio's more elegant script to test and warn
about using old variables and munging which changes are in which commit.

Base Ref: v2.19.0
Web-Diff: https://github.com/benpeart/git/commit/79d62d39e4
Checkout: git fetch https://github.com/benpeart/git git-test-cleanup-v2 && git checkout 79d62d39e4


### Interdiff (v1..v2):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3f447b8ddc..17a56f44ad 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -140,38 +140,27 @@ then
 	export GIT_INDEX_VERSION
 fi
 
-if test -n "$TEST_GIT_INDEX_VERSION"
-then
-	if test -n "$GIT_TEST_INDEX_VERSION"
-	then
-		echo "warning: the TEST_GIT_INDEX_VERSION variable has been renamed to GIT_TEST_INDEX_VERSION"
-	else
-		echo "error: the TEST_GIT_INDEX_VERSION variable has been renamed to GIT_TEST_INDEX_VERSION"
-		exit 1
-	fi
-fi
-
-if test -n "$GIT_FSMONITOR_TEST"
-then
-	if test -n "$GIT_TEST_FSMONITOR"
-	then
-		echo "warning: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
-	else
-		echo "error: the GIT_FSMONITOR_TEST variable has been renamed to GIT_TEST_FSMONITOR"
-		exit 1
-	fi
-fi
+check_var_migration () {
+	old_name=$1 new_name=$2
+	eval "old_isset=\${${old_name}:+isset}"
+	eval "new_isset=\${${new_name}:+isset}"
+	case "$old_isset,$new_isset" in
+	isset,)
+		echo >&2 "warning: $old_name is now $new_name"
+		echo >&2 "hint: set $new_name too during the transition period"
+		eval "$new_name=\$$old_name"
+		;;
+	isset,isset)
+		# do this later
+		# echo >&2 "warning: $old_name is now $new_name"
+		# echo >&2 "hint: remove $old_name"
+		;;
+	esac
+}
 
-if test -n "$GIT_FORCE_PRELOAD_TEST"
-then
-	if test -n "$GIT_TEST_PRELOAD_INDEX"
-	then
-		echo "warning: the GIT_FORCE_PRELOAD_TEST variable has been renamed to GIT_TEST_PRELOAD_INDEX"
-	else
-		echo "error: the GIT_FORCE_PRELOAD_TEST variable has been renamed to GIT_TEST_PRELOAD_INDEX"
-		exit 1
-	fi
-fi
+check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
+check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
+check_var_migration GIT_FORCE_PRELOAD_TEST GIT_TEST_PRELOAD_INDEX
 
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind


### Patches

Ben Peart (5):
  correct typo/spelling error in t/README
  preload-index: teach GIT_FORCE_PRELOAD_TEST to take a boolean
  fsmonitor: update GIT_TEST_FSMONITOR support
  read-cache: update TEST_GIT_INDEX_VERSION support
  preload-index: update GIT_FORCE_PRELOAD_TEST support

 Makefile                    |  6 +++---
 config.c                    |  2 +-
 preload-index.c             |  3 ++-
 t/README                    | 13 ++++++++++++-
 t/t1700-split-index.sh      |  2 +-
 t/t7519-status-fsmonitor.sh |  6 +++---
 t/test-lib.sh               | 26 ++++++++++++++++++++++++--
 7 files changed, 46 insertions(+), 12 deletions(-)


base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670
-- 
2.18.0.windows.1

[PATCH v2 1/5] correct typo/spelling error in t/README

[Ben Peart]

Correct a spelling error in the documentation for GIT_TEST_OE_DELTA_SIZE

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 t/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 9028b47d92..56a417439c 100644
--- a/t/README
+++ b/t/README
@@ -315,7 +315,7 @@ 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.
 
-- 
2.18.0.windows.1

Re: [PATCH v2 1/5] correct typo/spelling error in t/README

[Jonathan Nieder]

Hi,

Ben Peart wrote:

> Subject: correct typo/spelling error in t/README

nit: what is the difference between a typo/spelling error and another
kind of spelling error?  Maybe this could be something like

	t/README: correct spelling of "uncommon"

which makes it crystal clear what the patch will do.

> Correct a spelling error in the documentation for GIT_TEST_OE_DELTA_SIZE

The commit message should consist of complete sentences, so this is
missing a period.  Alternatively, I think it would be fine to omit the
sentence altogether.

> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
> ---
>  t/README | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This appears to be the only usage of uncomon in the code base.  Thanks
for fixing it.

With or without the commit message tweaks mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

[PATCH v2 2/5] preload-index: teach GIT_FORCE_PRELOAD_TEST to take a boolean

[Ben Peart]

Teach GIT_FORCE_PRELOAD_TEST to take a boolean to turn on or off this test
feature instead of simply testing for existance.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 preload-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/preload-index.c b/preload-index.c
index 71cd2437a3..0a4e2933bb 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -5,6 +5,7 @@
 #include "pathspec.h"
 #include "dir.h"
 #include "fsmonitor.h"
+#include "config.h"
 
 #ifdef NO_PTHREADS
 static void preload_index(struct index_state *index,
@@ -84,7 +85,7 @@ static void preload_index(struct index_state *index,
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
-	if ((index->cache_nr > 1) && (threads < 2) && getenv("GIT_FORCE_PRELOAD_TEST"))
+	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_FORCE_PRELOAD_TEST", 0))
 		threads = 2;
 	if (threads < 2)
 		return;
-- 
2.18.0.windows.1

Re: [PATCH v2 2/5] preload-index: teach GIT_FORCE_PRELOAD_TEST to take a boolean

[Jonathan Nieder]

Ben Peart wrote:

> Subject: preload-index: teach GIT_FORCE_PRELOAD_TEST to take a boolean

Reading this subject line alone (e.g. in "git log --oneline" output),
it's not obvious to me what this patch will do.

What behavior change does it make / what will it make newly possible?

Maybe something like:

	preload-index: use git_env_bool() not getenv() for customization

	GIT_FORCE_PRELOAD_TEST is only checked for presence by using getenv().
	Use git_env_bool() instead so that GIT_FORCE_PRELOAD_TEST=false can
	work as expected.

> Teach GIT_FORCE_PRELOAD_TEST to take a boolean to turn on or off this test
> feature instead of simply testing for existance.
>
> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
> ---
>  preload-index.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Can you say a little about how this came up?  Was it just noticed
while reading the code, or did it come up in practice?

I wonder if a more useful knob would be a GIT_FORCE_PRELOAD_THREADS
setting, but that's orthogonal to this change.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2 2/5] preload-index: teach GIT_FORCE_PRELOAD_TEST to take a boolean

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Maybe something like:
>
> 	preload-index: use git_env_bool() not getenv() for customization
>
> 	GIT_FORCE_PRELOAD_TEST is only checked for presence by using getenv().
> 	Use git_env_bool() instead so that GIT_FORCE_PRELOAD_TEST=false can
> 	work as expected.

That is much better description.  Also

	$ cd t && GIT_FORCE_PRELOAD_TEST=t ./t0000-basic.sh

would have allowed us to enable the feature in the older world, but
I suspect it would instead fail the test, saying 't is not a bool
nor int'.

So strictly speaking, it is a backward incompatible change.  I am
not sure if I like it.

>> Teach GIT_FORCE_PRELOAD_TEST to take a boolean to turn on or off this test
>> feature instead of simply testing for existance.

s/existance/existence/?

[PATCH v2 3/5] fsmonitor: update GIT_TEST_FSMONITOR support

[Ben Peart]

Rename GIT_FSMONITOR_TEST to GIT_TEST_FSMONITOR for consistency with the
other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give a warning when the old variable is set to
let people know they need to update their environment to use the new
variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 config.c                    |  2 +-
 t/README                    |  4 ++++
 t/t1700-split-index.sh      |  2 +-
 t/t7519-status-fsmonitor.sh |  2 +-
 t/test-lib.sh               | 20 ++++++++++++++++++++
 5 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 3461993f0a..3555c63f28 100644
--- a/config.c
+++ b/config.c
@@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void)
 int git_config_get_fsmonitor(void)
 {
 	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
-		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
 
 	if (core_fsmonitor && !*core_fsmonitor)
 		core_fsmonitor = NULL;
diff --git a/t/README b/t/README
index 56a417439c..47165f7eab 100644
--- a/t/README
+++ b/t/README
@@ -319,6 +319,10 @@ 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_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
+code path for utilizing a file system monitor to speed up detecting
+new or changed files.
+
 Naming Tests
 ------------
 
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index b3b4d83eaf..f6a856f24c 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -6,7 +6,7 @@ test_description='split index mode tests'
 
 # We need total control of index splitting here
 sane_unset GIT_TEST_SPLIT_INDEX
-sane_unset GIT_FSMONITOR_TEST
+sane_unset GIT_TEST_FSMONITOR
 
 test_expect_success 'enable split index' '
 	git config splitIndex.maxPercentChange 100 &&
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 756beb0d8e..d77012ea6d 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -8,7 +8,7 @@ test_description='git status with file system watcher'
 # To run the entire git test suite using fsmonitor:
 #
 # copy t/t7519/fsmonitor-all to a location in your path and then set
-# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests.
+# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.
 #
 
 # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 44288cbb59..653688c067 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -140,6 +140,26 @@ then
 	export GIT_INDEX_VERSION
 fi
 
+check_var_migration () {
+	old_name=$1 new_name=$2
+	eval "old_isset=\${${old_name}:+isset}"
+	eval "new_isset=\${${new_name}:+isset}"
+	case "$old_isset,$new_isset" in
+	isset,)
+		echo >&2 "warning: $old_name is now $new_name"
+		echo >&2 "hint: set $new_name too during the transition period"
+		eval "$new_name=\$$old_name"
+		;;
+	isset,isset)
+		# do this later
+		# echo >&2 "warning: $old_name is now $new_name"
+		# echo >&2 "hint: remove $old_name"
+		;;
+	esac
+}
+
+check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
-- 
2.18.0.windows.1

[PATCH v2 4/5] read-cache: update TEST_GIT_INDEX_VERSION support

[Ben Peart]

Rename TEST_GIT_INDEX_VERSION to GIT_TEST_INDEX_VERSION for consistency with
the other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give a warning when the old variable is set to
let people know they need to update their environment to use the new
variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 Makefile      | 6 +++---
 t/README      | 4 ++++
 t/test-lib.sh | 5 +++--
 3 files changed, 10 insertions(+), 5 deletions(-)

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/README b/t/README
index 47165f7eab..9b13f6d12e 100644
--- a/t/README
+++ b/t/README
@@ -323,6 +323,10 @@ GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
 code path for utilizing a file system monitor to speed up detecting
 new or changed files.
 
+GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
+for the index version specified.  Can be set to any valid version
+(currently 2, 3, or 4).
+
 Naming Tests
 ------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 653688c067..397eb71578 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
 
@@ -159,6 +159,7 @@ check_var_migration () {
 }
 
 check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
+check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
 
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
-- 
2.18.0.windows.1

Re: [PATCH v2 4/5] read-cache: update TEST_GIT_INDEX_VERSION support

[Junio C Hamano]

Ben Peart <benpeart@microsoft.com> writes:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 653688c067..397eb71578 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

Is this done a bit before ...

> @@ -159,6 +159,7 @@ check_var_migration () {
>  }
>  
>  check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
> +check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION

... this has a chance to kick in to say things like "Whoa you have
TEST_GIT_INDEX_VERSION that is an old spelling of
GIT_TEST_INDEX_VERSION", isn't it?

>  # Add libc MALLOC and MALLOC_PERTURB test
>  # only if we are not executing the test with valgrind

Re: [PATCH v2 4/5] read-cache: update TEST_GIT_INDEX_VERSION support

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ben Peart <benpeart@microsoft.com> writes:
>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 653688c067..397eb71578 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
>
> Is this done a bit before ...
>
>> @@ -159,6 +159,7 @@ check_var_migration () {
>>  }
>>  
>>  check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
>> +check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
>
> ... this has a chance to kick in to say things like "Whoa you have
> TEST_GIT_INDEX_VERSION that is an old spelling of
> GIT_TEST_INDEX_VERSION", isn't it?

So, the obvious fix would look like the patch below.

One problem with warning is that

	$ TEST_GIT_INDEX_VERSION=4 sh ./t0000-basic.sh

(or any other depreated variable set without its modern counterpart
set) would fail due to extra output produced to the standard error
stream.

 t/test-lib.sh | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 17a56f44ad..8ef86e05a3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -134,12 +134,6 @@ export EDITOR
 GIT_TRACE_BARE=1
 export GIT_TRACE_BARE
 
-if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
-then
-	GIT_INDEX_VERSION="$GIT_TEST_INDEX_VERSION"
-	export GIT_INDEX_VERSION
-fi
-
 check_var_migration () {
 	old_name=$1 new_name=$2
 	eval "old_isset=\${${old_name}:+isset}"
@@ -162,6 +156,13 @@ check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
 check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
 check_var_migration GIT_FORCE_PRELOAD_TEST GIT_TEST_PRELOAD_INDEX
 
+# Use specific version of the index file format
+if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
+then
+	GIT_INDEX_VERSION="$GIT_TEST_INDEX_VERSION"
+	export GIT_INDEX_VERSION
+fi
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||

[PATCH v2 5/5] preload-index: update GIT_FORCE_PRELOAD_TEST support

[Ben Peart]

Rename GIT_FORCE_PRELOAD_TEST to GIT_TEST_PRELOAD_INDEX for consistency with
the other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give a warning when the old variable is set to
let people know they need to update their environment to use the new
variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 preload-index.c             | 2 +-
 t/README                    | 3 +++
 t/t7519-status-fsmonitor.sh | 4 ++--
 t/test-lib.sh               | 1 +
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 0a4e2933bb..a850e197c2 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -85,7 +85,7 @@ static void preload_index(struct index_state *index,
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
-	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_FORCE_PRELOAD_TEST", 0))
+	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
 		threads = 2;
 	if (threads < 2)
 		return;
diff --git a/t/README b/t/README
index 9b13f6d12e..5670c7aad0 100644
--- a/t/README
+++ b/t/README
@@ -327,6 +327,9 @@ GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
 for the index version specified.  Can be set to any valid version
 (currently 2, 3, or 4).
 
+GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
+by overriding the minimum number of cache entries required per thread.
+
 Naming Tests
 ------------
 
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d77012ea6d..8308d6d5b1 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -245,9 +245,9 @@ do
 		git config core.preloadIndex $preload_val &&
 		if test $preload_val = true
 		then
-			GIT_FORCE_PRELOAD_TEST=$preload_val; export GIT_FORCE_PRELOAD_TEST
+			GIT_TEST_PRELOAD_INDEX=$preload_val; export GIT_TEST_PRELOAD_INDEX
 		else
-			unset GIT_FORCE_PRELOAD_TEST
+			sane_unset GIT_TEST_PRELOAD_INDEX
 		fi
 	'
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 397eb71578..17a56f44ad 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -160,6 +160,7 @@ check_var_migration () {
 
 check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
 check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
+check_var_migration GIT_FORCE_PRELOAD_TEST GIT_TEST_PRELOAD_INDEX
 
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
-- 
2.18.0.windows.1

[PATCH v1 3/4] read-cache: update TEST_GIT_INDEX_VERSION support

[Ben Peart]

Rename TEST_GIT_INDEX_VERSION to GIT_TEST_INDEX_VERSION for consistency with
the other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give an error when the old variable is set to
let people know they need to update their environment to use the new
variable. If the new variable is also set, just give a warning so they can
eventually remove the old variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 Makefile      |  6 +++---
 t/README      |  4 ++++
 t/test-lib.sh | 15 +++++++++++++--
 3 files changed, 20 insertions(+), 5 deletions(-)

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/README b/t/README
index 47165f7eab..9b13f6d12e 100644
--- a/t/README
+++ b/t/README
@@ -323,6 +323,10 @@ GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
 code path for utilizing a file system monitor to speed up detecting
 new or changed files.
 
+GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
+for the index version specified.  Can be set to any valid version
+(currently 2, 3, or 4).
+
 Naming Tests
 ------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0ef111d808..5f5f0f4b55 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -134,12 +134,23 @@ 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
 
+if test -n "$TEST_GIT_INDEX_VERSION"
+then
+	if test -n "$GIT_TEST_INDEX_VERSION"
+	then
+		echo "warning: the TEST_GIT_INDEX_VERSION variable has been renamed to GIT_TEST_INDEX_VERSION"
+	else
+		echo "error: the TEST_GIT_INDEX_VERSION variable has been renamed to GIT_TEST_INDEX_VERSION"
+		exit 1
+	fi
+fi
+
 if test -n "$GIT_FSMONITOR_TEST"
 then
 	if test -n "$GIT_TEST_FSMONITOR"
-- 
2.18.0.windows.1

[PATCH v1 4/4] preload-index: update GIT_FORCE_PRELOAD_TEST support

[Ben Peart]

Rename GIT_FORCE_PRELOAD_TEST to GIT_TEST_PRELOAD_INDEX for consistency with
the other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give an error when the old variable is set to
let people know they need to update their environment to use the new
variable. If the new variable is also set, just give a warning so they can
eventually remove the old variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 preload-index.c             |  3 ++-
 t/README                    |  3 +++
 t/t7519-status-fsmonitor.sh |  4 ++--
 t/test-lib.sh               | 11 +++++++++++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 71cd2437a3..a850e197c2 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -5,6 +5,7 @@
 #include "pathspec.h"
 #include "dir.h"
 #include "fsmonitor.h"
+#include "config.h"
 
 #ifdef NO_PTHREADS
 static void preload_index(struct index_state *index,
@@ -84,7 +85,7 @@ static void preload_index(struct index_state *index,
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
-	if ((index->cache_nr > 1) && (threads < 2) && getenv("GIT_FORCE_PRELOAD_TEST"))
+	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
 		threads = 2;
 	if (threads < 2)
 		return;
diff --git a/t/README b/t/README
index 9b13f6d12e..5670c7aad0 100644
--- a/t/README
+++ b/t/README
@@ -327,6 +327,9 @@ GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
 for the index version specified.  Can be set to any valid version
 (currently 2, 3, or 4).
 
+GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
+by overriding the minimum number of cache entries required per thread.
+
 Naming Tests
 ------------
 
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d77012ea6d..8308d6d5b1 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -245,9 +245,9 @@ do
 		git config core.preloadIndex $preload_val &&
 		if test $preload_val = true
 		then
-			GIT_FORCE_PRELOAD_TEST=$preload_val; export GIT_FORCE_PRELOAD_TEST
+			GIT_TEST_PRELOAD_INDEX=$preload_val; export GIT_TEST_PRELOAD_INDEX
 		else
-			unset GIT_FORCE_PRELOAD_TEST
+			sane_unset GIT_TEST_PRELOAD_INDEX
 		fi
 	'
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5f5f0f4b55..3f447b8ddc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -162,6 +162,17 @@ then
 	fi
 fi
 
+if test -n "$GIT_FORCE_PRELOAD_TEST"
+then
+	if test -n "$GIT_TEST_PRELOAD_INDEX"
+	then
+		echo "warning: the GIT_FORCE_PRELOAD_TEST variable has been renamed to GIT_TEST_PRELOAD_INDEX"
+	else
+		echo "error: the GIT_FORCE_PRELOAD_TEST variable has been renamed to GIT_TEST_PRELOAD_INDEX"
+		exit 1
+	fi
+fi
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
-- 
2.18.0.windows.1

[PATCH v3 0/5] Cleanup pass on special test setups

[Ben Peart]

This round has one code change based on feedback. Other changes are just
rewording commit messages.

Base Ref: v2.19.0
Web-Diff: https://github.com/benpeart/git/commit/043246d936
Checkout: git fetch https://github.com/benpeart/git git-test-cleanup-v3 && git checkout 043246d936


### Interdiff (v2..v3):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 17a56f44ad..8ef86e05a3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -134,12 +134,6 @@ export EDITOR
 GIT_TRACE_BARE=1
 export GIT_TRACE_BARE
 
-if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
-then
-	GIT_INDEX_VERSION="$GIT_TEST_INDEX_VERSION"
-	export GIT_INDEX_VERSION
-fi
-
 check_var_migration () {
 	old_name=$1 new_name=$2
 	eval "old_isset=\${${old_name}:+isset}"
@@ -162,6 +156,13 @@ check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
 check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
 check_var_migration GIT_FORCE_PRELOAD_TEST GIT_TEST_PRELOAD_INDEX
 
+# Use specific version of the index file format
+if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
+then
+	GIT_INDEX_VERSION="$GIT_TEST_INDEX_VERSION"
+	export GIT_INDEX_VERSION
+fi
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||


### Patches

Ben Peart (5):
  t/README: correct spelling of "uncommon"
  preload-index: use git_env_bool() not getenv() for customization
  fsmonitor: update GIT_TEST_FSMONITOR support
  read-cache: update TEST_GIT_INDEX_VERSION support
  preload-index: update GIT_FORCE_PRELOAD_TEST support

 Makefile                    |  6 +++---
 config.c                    |  2 +-
 preload-index.c             |  3 ++-
 t/README                    | 13 ++++++++++++-
 t/t1700-split-index.sh      |  2 +-
 t/t7519-status-fsmonitor.sh |  6 +++---
 t/test-lib.sh               | 27 +++++++++++++++++++++++++--
 7 files changed, 47 insertions(+), 12 deletions(-)


base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670
-- 
2.18.0.windows.1

[PATCH v3 2/5] preload-index: use git_env_bool() not getenv() for customization

[Ben Peart]

GIT_FORCE_PRELOAD_TEST is only checked for presence by using getenv().
Use git_env_bool() instead so that GIT_FORCE_PRELOAD_TEST=false can
work as expected.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 preload-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/preload-index.c b/preload-index.c
index 71cd2437a3..0a4e2933bb 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -5,6 +5,7 @@
 #include "pathspec.h"
 #include "dir.h"
 #include "fsmonitor.h"
+#include "config.h"
 
 #ifdef NO_PTHREADS
 static void preload_index(struct index_state *index,
@@ -84,7 +85,7 @@ static void preload_index(struct index_state *index,
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
-	if ((index->cache_nr > 1) && (threads < 2) && getenv("GIT_FORCE_PRELOAD_TEST"))
+	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_FORCE_PRELOAD_TEST", 0))
 		threads = 2;
 	if (threads < 2)
 		return;
-- 
2.18.0.windows.1

[PATCH v3 1/5] t/README: correct spelling of "uncommon"

[Ben Peart]

Correct a spelling error in the documentation for GIT_TEST_OE_DELTA_SIZE.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 t/README | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/README b/t/README
index 9028b47d92..56a417439c 100644
--- a/t/README
+++ b/t/README
@@ -315,7 +315,7 @@ 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.
 
-- 
2.18.0.windows.1

[PATCH v3 3/5] fsmonitor: update GIT_TEST_FSMONITOR support

[Ben Peart]

Rename GIT_FSMONITOR_TEST to GIT_TEST_FSMONITOR for consistency with the
other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give a warning when the old variable is set to
let people know they need to update their environment to use the new
variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 config.c                    |  2 +-
 t/README                    |  4 ++++
 t/t1700-split-index.sh      |  2 +-
 t/t7519-status-fsmonitor.sh |  2 +-
 t/test-lib.sh               | 20 ++++++++++++++++++++
 5 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 3461993f0a..3555c63f28 100644
--- a/config.c
+++ b/config.c
@@ -2278,7 +2278,7 @@ int git_config_get_max_percent_split_change(void)
 int git_config_get_fsmonitor(void)
 {
 	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
-		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
 
 	if (core_fsmonitor && !*core_fsmonitor)
 		core_fsmonitor = NULL;
diff --git a/t/README b/t/README
index 56a417439c..47165f7eab 100644
--- a/t/README
+++ b/t/README
@@ -319,6 +319,10 @@ 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_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
+code path for utilizing a file system monitor to speed up detecting
+new or changed files.
+
 Naming Tests
 ------------
 
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index b3b4d83eaf..f6a856f24c 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -6,7 +6,7 @@ test_description='split index mode tests'
 
 # We need total control of index splitting here
 sane_unset GIT_TEST_SPLIT_INDEX
-sane_unset GIT_FSMONITOR_TEST
+sane_unset GIT_TEST_FSMONITOR
 
 test_expect_success 'enable split index' '
 	git config splitIndex.maxPercentChange 100 &&
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 756beb0d8e..d77012ea6d 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -8,7 +8,7 @@ test_description='git status with file system watcher'
 # To run the entire git test suite using fsmonitor:
 #
 # copy t/t7519/fsmonitor-all to a location in your path and then set
-# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests.
+# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.
 #
 
 # Note, after "git reset --hard HEAD" no extensions exist other than 'TREE'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 44288cbb59..653688c067 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -140,6 +140,26 @@ then
 	export GIT_INDEX_VERSION
 fi
 
+check_var_migration () {
+	old_name=$1 new_name=$2
+	eval "old_isset=\${${old_name}:+isset}"
+	eval "new_isset=\${${new_name}:+isset}"
+	case "$old_isset,$new_isset" in
+	isset,)
+		echo >&2 "warning: $old_name is now $new_name"
+		echo >&2 "hint: set $new_name too during the transition period"
+		eval "$new_name=\$$old_name"
+		;;
+	isset,isset)
+		# do this later
+		# echo >&2 "warning: $old_name is now $new_name"
+		# echo >&2 "hint: remove $old_name"
+		;;
+	esac
+}
+
+check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
+
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
-- 
2.18.0.windows.1

Re: [PATCH v3 3/5] fsmonitor: update GIT_TEST_FSMONITOR support

[SZEDER Gábor]

On Tue, Sep 18, 2018 at 11:29:35PM +0000, Ben Peart wrote:
> diff --git a/t/README b/t/README
> index 56a417439c..47165f7eab 100644
> --- a/t/README
> +++ b/t/README
> @@ -319,6 +319,10 @@ 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_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
> +code path for utilizing a file system monitor to speed up detecting
> +new or changed files.

Here you tell us to set GIT_TEST_FSMONITOR to an absolute path, and we
are good to go.

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> index 756beb0d8e..d77012ea6d 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -8,7 +8,7 @@ test_description='git status with file system watcher'
>  # To run the entire git test suite using fsmonitor:
>  #
>  # copy t/t7519/fsmonitor-all to a location in your path and then set
> -# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests.
> +# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.

But this old comment is different, suggesting copying that script to
our $PATH.

I prefer your instructions above, because it's only a single step,
and, more importantly, it won't pollute my $PATH.  I think this
comment should be updated to make the advices in both places
consistent.  Or perhaps even removed, now that all GIT_TEST variables
are documented in the same place?

Re: [PATCH v3 3/5] fsmonitor: update GIT_TEST_FSMONITOR support

[Ben Peart]

On 9/28/2018 6:01 AM, SZEDER Gábor wrote:
> On Tue, Sep 18, 2018 at 11:29:35PM +0000, Ben Peart wrote:
>> diff --git a/t/README b/t/README
>> index 56a417439c..47165f7eab 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -319,6 +319,10 @@ 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_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
>> +code path for utilizing a file system monitor to speed up detecting
>> +new or changed files.
> 
> Here you tell us to set GIT_TEST_FSMONITOR to an absolute path, and we
> are good to go.
> 
>> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
>> index 756beb0d8e..d77012ea6d 100755
>> --- a/t/t7519-status-fsmonitor.sh
>> +++ b/t/t7519-status-fsmonitor.sh
>> @@ -8,7 +8,7 @@ test_description='git status with file system watcher'
>>   # To run the entire git test suite using fsmonitor:
>>   #
>>   # copy t/t7519/fsmonitor-all to a location in your path and then set
>> -# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests.
>> +# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.
> 
> But this old comment is different, suggesting copying that script to
> our $PATH.
> 
> I prefer your instructions above, because it's only a single step,
> and, more importantly, it won't pollute my $PATH.  I think this
> comment should be updated to make the advices in both places
> consistent.  Or perhaps even removed, now that all GIT_TEST variables
> are documented in the same place?
> 

I prefer the suggestion to simply remove this text from the test script 
now that there is documentation for it in the t/README file.

Re: [PATCH v3 3/5] fsmonitor: update GIT_TEST_FSMONITOR support

[Ben Peart]

On 9/28/2018 10:21 AM, Ben Peart wrote:
> 
> 
> On 9/28/2018 6:01 AM, SZEDER Gábor wrote:
>> On Tue, Sep 18, 2018 at 11:29:35PM +0000, Ben Peart wrote:
>>> diff --git a/t/README b/t/README
>>> index 56a417439c..47165f7eab 100644
>>> --- a/t/README
>>> +++ b/t/README
>>> @@ -319,6 +319,10 @@ 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_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
>>> +code path for utilizing a file system monitor to speed up detecting
>>> +new or changed files.
>>
>> Here you tell us to set GIT_TEST_FSMONITOR to an absolute path, and we
>> are good to go.
>>
>>> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
>>> index 756beb0d8e..d77012ea6d 100755
>>> --- a/t/t7519-status-fsmonitor.sh
>>> +++ b/t/t7519-status-fsmonitor.sh
>>> @@ -8,7 +8,7 @@ test_description='git status with file system watcher'
>>>   # To run the entire git test suite using fsmonitor:
>>>   #
>>>   # copy t/t7519/fsmonitor-all to a location in your path and then set
>>> -# GIT_FSMONITOR_TEST=fsmonitor-all and run your tests.
>>> +# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.
>>
>> But this old comment is different, suggesting copying that script to
>> our $PATH.
>>
>> I prefer your instructions above, because it's only a single step,
>> and, more importantly, it won't pollute my $PATH.  I think this
>> comment should be updated to make the advices in both places
>> consistent.  Or perhaps even removed, now that all GIT_TEST variables
>> are documented in the same place?
>>
> 
> I prefer the suggestion to simply remove this text from the test script 
> now that there is documentation for it in the t/README file.

Junio, can you squash in the following patch or would you prefer I 
reroll the entire series?

Thanks,

Ben

 From 393007340dc1baf3539ab727e0a8128e7c408a27 Mon Sep 17 00:00:00 2001
From: Ben Peart <peartben@gmail.com>
Date: Fri, 28 Sep 2018 10:23:18 -0400
Subject: fixup! fsmonitor: remove outdated instructions from test

Remove the outdated instructions on how to run the test suite utilizing
fsmonitor now that it is properly documented in t/README.

Signed-off-by: Ben Peart <peartben@gmail.com>
---

Notes:
     Base Ref: git-test-cleanup-v3
     Web-Diff: https://github.com/benpeart/git/commit/393007340d
     Checkout: git fetch https://github.com/benpeart/git 
git-test-cleanup-v1 && git checkout 393007340d

  t/t7519-status-fsmonitor.sh | 7 -------
  1 file changed, 7 deletions(-)

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 8308d6d5b1..3f0dd98010 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -4,13 +4,6 @@ test_description='git status with file system watcher'

  . ./test-lib.sh

-#
-# To run the entire git test suite using fsmonitor:
-#
-# copy t/t7519/fsmonitor-all to a location in your path and then set
-# GIT_TEST_FSMONITOR=fsmonitor-all and run your tests.
-#
-
  # Note, after "git reset --hard HEAD" no extensions exist other than 
'TREE'
  # "git update-index --fsmonitor" can be used to get the extension written
  # before testing the results.

base-commit: 043246d9369fb851c5c2b922466f77fc7ef0327b
-- 
2.18.0.windows.1

Re: [PATCH v3 3/5] fsmonitor: update GIT_TEST_FSMONITOR support

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> Junio, can you squash in the following patch or would you prefer I
> reroll the entire series?

Squash it to f8cd77d5 ("fsmonitor: update GIT_TEST_FSMONITOR
support", 2018-09-18) and use the two new lines in the log message?

I can do that.

Thanks.

[PATCH v3 4/5] read-cache: update TEST_GIT_INDEX_VERSION support

[Ben Peart]

Rename TEST_GIT_INDEX_VERSION to GIT_TEST_INDEX_VERSION for consistency with
the other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give a warning when the old variable is set to
let people know they need to update their environment to use the new
variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 Makefile      |  6 +++---
 t/README      |  4 ++++
 t/test-lib.sh | 14 ++++++++------
 3 files changed, 15 insertions(+), 9 deletions(-)

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/README b/t/README
index 47165f7eab..9b13f6d12e 100644
--- a/t/README
+++ b/t/README
@@ -323,6 +323,10 @@ GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
 code path for utilizing a file system monitor to speed up detecting
 new or changed files.
 
+GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
+for the index version specified.  Can be set to any valid version
+(currently 2, 3, or 4).
+
 Naming Tests
 ------------
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 653688c067..e80c84d13c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -134,12 +134,6 @@ export EDITOR
 GIT_TRACE_BARE=1
 export GIT_TRACE_BARE
 
-if test -n "${TEST_GIT_INDEX_VERSION:+isset}"
-then
-	GIT_INDEX_VERSION="$TEST_GIT_INDEX_VERSION"
-	export GIT_INDEX_VERSION
-fi
-
 check_var_migration () {
 	old_name=$1 new_name=$2
 	eval "old_isset=\${${old_name}:+isset}"
@@ -159,6 +153,14 @@ check_var_migration () {
 }
 
 check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
+check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
+
+# Use specific version of the index file format
+if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
+then
+	GIT_INDEX_VERSION="$GIT_TEST_INDEX_VERSION"
+	export GIT_INDEX_VERSION
+fi
 
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
-- 
2.18.0.windows.1

[PATCH v3 5/5] preload-index: update GIT_FORCE_PRELOAD_TEST support

[Ben Peart]

Rename GIT_FORCE_PRELOAD_TEST to GIT_TEST_PRELOAD_INDEX for consistency with
the other GIT_TEST_ special setups and properly document its use.

Add logic in t/test-lib.sh to give a warning when the old variable is set to
let people know they need to update their environment to use the new
variable.

Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
---
 preload-index.c             | 2 +-
 t/README                    | 3 +++
 t/t7519-status-fsmonitor.sh | 4 ++--
 t/test-lib.sh               | 1 +
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/preload-index.c b/preload-index.c
index 0a4e2933bb..a850e197c2 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -85,7 +85,7 @@ static void preload_index(struct index_state *index,
 		return;
 
 	threads = index->cache_nr / THREAD_COST;
-	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_FORCE_PRELOAD_TEST", 0))
+	if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
 		threads = 2;
 	if (threads < 2)
 		return;
diff --git a/t/README b/t/README
index 9b13f6d12e..5670c7aad0 100644
--- a/t/README
+++ b/t/README
@@ -327,6 +327,9 @@ GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
 for the index version specified.  Can be set to any valid version
 (currently 2, 3, or 4).
 
+GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
+by overriding the minimum number of cache entries required per thread.
+
 Naming Tests
 ------------
 
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d77012ea6d..8308d6d5b1 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -245,9 +245,9 @@ do
 		git config core.preloadIndex $preload_val &&
 		if test $preload_val = true
 		then
-			GIT_FORCE_PRELOAD_TEST=$preload_val; export GIT_FORCE_PRELOAD_TEST
+			GIT_TEST_PRELOAD_INDEX=$preload_val; export GIT_TEST_PRELOAD_INDEX
 		else
-			unset GIT_FORCE_PRELOAD_TEST
+			sane_unset GIT_TEST_PRELOAD_INDEX
 		fi
 	'
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e80c84d13c..8ef86e05a3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -154,6 +154,7 @@ check_var_migration () {
 
 check_var_migration GIT_FSMONITOR_TEST GIT_TEST_FSMONITOR
 check_var_migration TEST_GIT_INDEX_VERSION GIT_TEST_INDEX_VERSION
+check_var_migration GIT_FORCE_PRELOAD_TEST GIT_TEST_PRELOAD_INDEX
 
 # Use specific version of the index file format
 if test -n "${GIT_TEST_INDEX_VERSION:+isset}"
-- 
2.18.0.windows.1

Re*: [PATCH v3 0/5] Cleanup pass on special test setups

[Junio C Hamano]

Ben Peart <benpeart@microsoft.com> writes:

> This round has one code change based on feedback. Other changes are just
> rewording commit messages.

Thanks.  I think the only remaining issue is what to do with the
interaction between extra/additional error message that comes from
the updates in 3/5 and the test framework selftest in t0000.

-- >8 --
Subject: t0000: do not get self-test disrupted by environment warnings

The test framework test-lib.sh itself would want to give warnings
and hints, e.g. when it sees a deprecated environment variable is in
use that we want to encourage users to migrate to another variable.

The self-test of test framework done in t0000 however do not expect
to see these warnings and hints, so depending on the settings of
environment variables, a running test may or may not produce these
messages to the standard error output, breaking the expectations of
self-test test framework does on itself.  Here is what we see:

    $ TEST_GIT_INDEX_VERSION=4 sh t0000-basic.sh -i -v
    ...
    'err' is not empty, it contains:
    warning: TEST_GIT_INDEX_VERSION is now GIT_TEST_INDEX_VERSION
    hint: set GIT_TEST_INDEX_VERSION too during the transition period
    not ok 5 - pretend we have a fully passing test suite

The following quick attempt to work it around does not work, because
some tests in t0000 do want to see expected errors from the test
framework itself.

         t/t0000-basic.sh | 2 +-
         1 file changed, 1 insertion(+), 1 deletion(-)

        diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
        index 850f651e4e..88c6ed4696 100755
        --- a/t/t0000-basic.sh
        +++ b/t/t0000-basic.sh
        @@ -88,7 +88,7 @@ _run_sub_test_lib_test_common () {
                        '

                        # Point to the t/test-lib.sh, which isn't in ../ as usual
        -		. "\$TEST_DIRECTORY"/test-lib.sh
        +		. "\$TEST_DIRECTORY"/test-lib.sh >/dev/null 2>&1
                        EOF
                        cat >>"$name.sh" &&
                        chmod +x "$name.sh" &&

There are a few possible ways to work this around:

 * We could strip the warning: and hint: unconditionally from the
   error output before the error messages are checked in the
   self-test (helper functions check_sub_test_lib_test_err and
   check_sub_test_lib_test); the problem with this approach is that
   it will make it impossible to write self-tests to ensure that
   right warnings and hints are given.

 * We could force a sane environment settings before the test helper
   _run_sub_test_lib_test_common dot-sources test-lib.sh; the
   problem with this approach is that _run_sub_test_lib_test_common
   now needs to be aware of what pairs of environment variables are
   checked in test-lib.sh using check_var_migration helper.

The final patch I came up with is probably the solution that is
least bad.  Set a variable to tell test-lib.sh that we are running
a self-test, so that various pieces in test-lib.sh can react to keep
the output stable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0000-basic.sh | 4 ++++
 t/test-lib.sh    | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 850f651e4e..52c02b7c7e 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -87,6 +87,10 @@ _run_sub_test_lib_test_common () {
 		passing metrics
 		'
 
+		# Tell the framework that we are self-testing to make sure
+		# it yields a stable result.
+		GIT_TEST_FRAMEWORK_SELFTEST=t &&
+
 		# Point to the t/test-lib.sh, which isn't in ../ as usual
 		. "\$TEST_DIRECTORY"/test-lib.sh
 		EOF
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8ef86e05a3..364a11ea25 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -135,9 +135,17 @@ GIT_TRACE_BARE=1
 export GIT_TRACE_BARE
 
 check_var_migration () {
+	# the warnings and hints given from this helper depends
+	# on end-user settings, which will disrupt the self-test
+	# done on the test framework itself.
+	case "$GIT_TEST_FRAMEWORK_SELFTEST" in
+	t)	return ;;
+	esac
+
 	old_name=$1 new_name=$2
 	eval "old_isset=\${${old_name}:+isset}"
 	eval "new_isset=\${${new_name}:+isset}"
+
 	case "$old_isset,$new_isset" in
 	isset,)
 		echo >&2 "warning: $old_name is now $new_name"

Re: Re*: [PATCH v3 0/5] Cleanup pass on special test setups

[Ben Peart]

On 9/20/2018 2:43 PM, Junio C Hamano wrote:
> Ben Peart <benpeart@microsoft.com> writes:
> 
>> This round has one code change based on feedback. Other changes are just
>> rewording commit messages.
> 
> Thanks.  I think the only remaining issue is what to do with the
> interaction between extra/additional error message that comes from
> the updates in 3/5 and the test framework selftest in t0000.
> 
> -- >8 --
> Subject: t0000: do not get self-test disrupted by environment warnings
> 
> The test framework test-lib.sh itself would want to give warnings
> and hints, e.g. when it sees a deprecated environment variable is in
> use that we want to encourage users to migrate to another variable.
> 
> The self-test of test framework done in t0000 however do not expect
> to see these warnings and hints, so depending on the settings of
> environment variables, a running test may or may not produce these
> messages to the standard error output, breaking the expectations of
> self-test test framework does on itself.  Here is what we see:
> 
>      $ TEST_GIT_INDEX_VERSION=4 sh t0000-basic.sh -i -v
>      ...
>      'err' is not empty, it contains:
>      warning: TEST_GIT_INDEX_VERSION is now GIT_TEST_INDEX_VERSION
>      hint: set GIT_TEST_INDEX_VERSION too during the transition period
>      not ok 5 - pretend we have a fully passing test suite
> 
> The following quick attempt to work it around does not work, because
> some tests in t0000 do want to see expected errors from the test
> framework itself.
> 
>           t/t0000-basic.sh | 2 +-
>           1 file changed, 1 insertion(+), 1 deletion(-)
> 
>          diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>          index 850f651e4e..88c6ed4696 100755
>          --- a/t/t0000-basic.sh
>          +++ b/t/t0000-basic.sh
>          @@ -88,7 +88,7 @@ _run_sub_test_lib_test_common () {
>                          '
> 
>                          # Point to the t/test-lib.sh, which isn't in ../ as usual
>          -		. "\$TEST_DIRECTORY"/test-lib.sh
>          +		. "\$TEST_DIRECTORY"/test-lib.sh >/dev/null 2>&1
>                          EOF
>                          cat >>"$name.sh" &&
>                          chmod +x "$name.sh" &&
> 
> There are a few possible ways to work this around:
> 
>   * We could strip the warning: and hint: unconditionally from the
>     error output before the error messages are checked in the
>     self-test (helper functions check_sub_test_lib_test_err and
>     check_sub_test_lib_test); the problem with this approach is that
>     it will make it impossible to write self-tests to ensure that
>     right warnings and hints are given.
> 
>   * We could force a sane environment settings before the test helper
>     _run_sub_test_lib_test_common dot-sources test-lib.sh; the
>     problem with this approach is that _run_sub_test_lib_test_common
>     now needs to be aware of what pairs of environment variables are
>     checked in test-lib.sh using check_var_migration helper.
> 
> The final patch I came up with is probably the solution that is
> least bad.  Set a variable to tell test-lib.sh that we are running
> a self-test, so that various pieces in test-lib.sh can react to keep
> the output stable.
> 

This looks like a reasonable compromise to me.  It's nice to give the 
migration hints to end users so they know they need to update their 
environments to reflect the required changes.  On the other hand, we 
don't want or need them to be triggered when we are running the self-test.

It would be nice to enable that automatically without the need for 
another environment variable but I couldn't think of a good way to 
accomplish that so I agree - this seems like the "least bad" solution. :-)

Thanks Junio

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   t/t0000-basic.sh | 4 ++++
>   t/test-lib.sh    | 8 ++++++++
>   2 files changed, 12 insertions(+)
> 
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 850f651e4e..52c02b7c7e 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -87,6 +87,10 @@ _run_sub_test_lib_test_common () {
>   		passing metrics
>   		'
>   
> +		# Tell the framework that we are self-testing to make sure
> +		# it yields a stable result.
> +		GIT_TEST_FRAMEWORK_SELFTEST=t &&
> +
>   		# Point to the t/test-lib.sh, which isn't in ../ as usual
>   		. "\$TEST_DIRECTORY"/test-lib.sh
>   		EOF
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 8ef86e05a3..364a11ea25 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -135,9 +135,17 @@ GIT_TRACE_BARE=1
>   export GIT_TRACE_BARE
>   
>   check_var_migration () {
> +	# the warnings and hints given from this helper depends
> +	# on end-user settings, which will disrupt the self-test
> +	# done on the test framework itself.
> +	case "$GIT_TEST_FRAMEWORK_SELFTEST" in
> +	t)	return ;;
> +	esac
> +
>   	old_name=$1 new_name=$2
>   	eval "old_isset=\${${old_name}:+isset}"
>   	eval "new_isset=\${${new_name}:+isset}"
> +
>   	case "$old_isset,$new_isset" in
>   	isset,)
>   		echo >&2 "warning: $old_name is now $new_name"
>