git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
@ 2018-09-12 21:26 Ben Peart
  2018-09-12 22:31 ` Thomas Gummerer
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Ben Peart @ 2018-09-12 21:26 UTC (permalink / raw)
  To: git@vger.kernel.org; +Cc: gitster@pobox.com, Ben Peart, Ben Peart

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.

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-12 21:26 [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ben Peart
@ 2018-09-12 22:31 ` Thomas Gummerer
  2018-09-13 14:07   ` Ben Peart
  2018-09-13 18:17 ` [PATCH v2] read-cache: update TEST_GIT_INDEX_VERSION support Ben Peart
  2018-09-13 18:49 ` [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 10+ messages in thread
From: Thomas Gummerer @ 2018-09-12 22:31 UTC (permalink / raw)
  To: Ben Peart; +Cc: git@vger.kernel.org, gitster@pobox.com, Ben Peart

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?

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-12 22:31 ` Thomas Gummerer
@ 2018-09-13 14:07   ` Ben Peart
  2018-09-13 21:56     ` Thomas Gummerer
  0 siblings, 1 reply; 10+ messages in thread
From: Ben Peart @ 2018-09-13 14:07 UTC (permalink / raw)
  To: Thomas Gummerer, Ben Peart
  Cc: git@vger.kernel.org, gitster@pobox.com, Ben Peart



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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH v2] read-cache: update TEST_GIT_INDEX_VERSION support
  2018-09-12 21:26 [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ben Peart
  2018-09-12 22:31 ` Thomas Gummerer
@ 2018-09-13 18:17 ` Ben Peart
  2018-09-13 18:49 ` [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 10+ messages in thread
From: Ben Peart @ 2018-09-13 18:17 UTC (permalink / raw)
  To: Ben Peart; +Cc: Ben Peart, git@vger.kernel.org, gitster@pobox.com

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

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

Notes:
    Base Ref: v2.19.0
    Web-Diff: https://github.com/benpeart/git/commit/e26ccb9004
    Checkout: git fetch https://github.com/benpeart/git git-test-index-version-v2 && git checkout e26ccb9004
    
    ### Interdiff (v1..v2):
    
    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/read-cache.c b/read-cache.c
    index d140ce9989..7b1354d759 100644
    --- a/read-cache.c
    +++ b/read-cache.c
    @@ -1570,45 +1570,26 @@ static unsigned int get_index_format_default(void)
     	char *envversion = getenv("GIT_INDEX_VERSION");
     	char *endp;
     	int value;
    -	unsigned int version = -1;
    +	unsigned int version = INDEX_FORMAT_DEFAULT;
    
    -	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 (version == -1) {
    -		if (!git_config_get_int("index.version", &value)) {
    +	if (!envversion) {
    +		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);
    -				version = INDEX_FORMAT_DEFAULT;
    -			}
    +			return INDEX_FORMAT_DEFAULT;
     		}
    +		return version;
     	}
    
    -	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"
    +		warning(_("GIT_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 f872638a78..fb6359b17b 100644
    --- a/t/README
    +++ b/t/README
    @@ -320,8 +320,7 @@ 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.
    +for the index version specified.  Can be set to any valid version.
    
     Naming Tests
     ------------
    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
    
    ### Patches

 Makefile      | 6 +++---
 t/README      | 5 ++++-
 t/test-lib.sh | 4 ++--
 3 files changed, 9 insertions(+), 6 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 9028b47d92..fb6359b17b 100644
--- a/t/README
+++ b/t/README
@@ -315,10 +315,13 @@ 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.
+
 Naming Tests
 ------------
 
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
 

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-12 21:26 [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ben Peart
  2018-09-12 22:31 ` Thomas Gummerer
  2018-09-13 18:17 ` [PATCH v2] read-cache: update TEST_GIT_INDEX_VERSION support Ben Peart
@ 2018-09-13 18:49 ` Ævar Arnfjörð Bjarmason
  2018-09-13 20:59   ` Junio C Hamano
  2 siblings, 1 reply; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-09-13 18:49 UTC (permalink / raw)
  To: Ben Peart; +Cc: git@vger.kernel.org, gitster@pobox.com, Ben Peart


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.

I was reading this in a mobile client earlier and couldn't see at a
glance (small screen+wrapping) what the change was here, so that sort of
nitpicking isn't purely theoretical :)

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

I'm not familiar with this and haven't dug, so I don't know this: are
any values of 1..4 OK? 0..4? Would be better to say that here...

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-13 18:49 ` [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ævar Arnfjörð Bjarmason
@ 2018-09-13 20:59   ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-09-13 20:59 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Ben Peart, git@vger.kernel.org, Ben Peart

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Yeah, an easy to miss change that is unrelated to the topic is
better made as a separate patch.


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-13 14:07   ` Ben Peart
@ 2018-09-13 21:56     ` Thomas Gummerer
  2018-09-13 22:08       ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gummerer @ 2018-09-13 21:56 UTC (permalink / raw)
  To: Ben Peart; +Cc: Ben Peart, git@vger.kernel.org, gitster@pobox.com, Ben Peart

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-13 21:56     ` Thomas Gummerer
@ 2018-09-13 22:08       ` Junio C Hamano
  2018-09-14 13:50         ` Ben Peart
  0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2018-09-13 22:08 UTC (permalink / raw)
  To: Thomas Gummerer; +Cc: Ben Peart, Ben Peart, git@vger.kernel.org, Ben Peart

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.




^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-13 22:08       ` Junio C Hamano
@ 2018-09-14 13:50         ` Ben Peart
  2018-09-14 16:20           ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Ben Peart @ 2018-09-14 13:50 UTC (permalink / raw)
  To: Junio C Hamano, Thomas Gummerer; +Cc: Ben Peart, git@vger.kernel.org, Ben Peart



On 9/13/2018 6:08 PM, Junio C Hamano wrote:
> 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'.
> 

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.

>> 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'll combine all these into a single patch series.  It grew as I
discovered more that needed to be updated.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support
  2018-09-14 13:50         ` Ben Peart
@ 2018-09-14 16:20           ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-09-14 16:20 UTC (permalink / raw)
  To: Ben Peart; +Cc: Thomas Gummerer, Ben Peart, git@vger.kernel.org, Ben Peart

Ben Peart <peartben@gmail.com> writes:

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

After the topic graduates to a stable release (or two), it would be
a good addition to encourage people to clean things up, of course,
but doing so too early by warning when they are both set is not a
good idea, I would think.  It would bring us back to "the user has
to futz with config.mak every time switching between 'maint' and
'next'".

> I'll combine all these into a single patch series.  It grew as I
> discovered more that needed to be updated.

Thanks. 

 I didn't mind it too much to have them as independent patches.  It
at least helped me forcing myself to give closer look at them ;-)


^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-14 16:20 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-12 21:26 [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ben Peart
2018-09-12 22:31 ` Thomas Gummerer
2018-09-13 14:07   ` Ben Peart
2018-09-13 21:56     ` Thomas Gummerer
2018-09-13 22:08       ` Junio C Hamano
2018-09-14 13:50         ` Ben Peart
2018-09-14 16:20           ` Junio C Hamano
2018-09-13 18:17 ` [PATCH v2] read-cache: update TEST_GIT_INDEX_VERSION support Ben Peart
2018-09-13 18:49 ` [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support Ævar Arnfjörð Bjarmason
2018-09-13 20:59   ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).