{"thread":{"id":"49335","subject":"[PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","startedAt":"2018-09-12T21:26:10Z","lastAt":"2018-09-14T16:20:31Z","messageCount":10,"participants":["Ben Peart","Thomas Gummerer","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"358010","messageId":"20180912212544.33624-1-benpeart@microsoft.com","threadId":"49335","inReplyTo":null,"subject":"[PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Ben Peart","fromEmail":"benpeart@microsoft.com","sentAt":"2018-09-12T21:26:04Z","receivedAt":"2018-09-12T21:26:10Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Teach get_index_format_default() to support running the test suite\nwith specific index versions.  In particular, this enables the test suite\nto be run using index version 4 which is not the default so gets less testing.\n\nSigned-off-by: Ben Peart <Ben.Peart@microsoft.com>\n---\n\nNotes:\n    Base Ref: v2.19.0\n    Web-Diff: https://github.com/benpeart/git/commit/52e733e2ce\n    Checkout: git fetch https://github.com/benpeart/git git-test-index-version-v1 && git checkout 52e733e2ce\n\n read-cache.c | 47 +++++++++++++++++++++++++++++++++--------------\n t/README     |  6 +++++-\n 2 files changed, 38 insertions(+), 15 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 7b1354d759..d140ce9989 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1570,26 +1570,45 @@ static unsigned int get_index_format_default(void)\n \tchar *envversion = getenv(\"GIT_INDEX_VERSION\");\n \tchar *endp;\n \tint value;\n-\tunsigned int version = INDEX_FORMAT_DEFAULT;\n+\tunsigned int version = -1;\n+\n+\tif (envversion) {\n+\t\tversion = strtoul(envversion, &endp, 10);\n+\t\tif (*endp ||\n+\t\t\tversion < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n+\t\t\twarning(_(\"GIT_INDEX_VERSION set, but the value is invalid.\\n\"\n+\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n+\t\t\tversion = INDEX_FORMAT_DEFAULT;\n+\t\t}\n+\t}\n \n-\tif (!envversion) {\n-\t\tif (!git_config_get_int(\"index.version\", &value))\n+\tif (version == -1) {\n+\t\tif (!git_config_get_int(\"index.version\", &value)) {\n \t\t\tversion = value;\n-\t\tif (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n-\t\t\twarning(_(\"index.version set, but the value is invalid.\\n\"\n-\t\t\t\t  \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n-\t\t\treturn INDEX_FORMAT_DEFAULT;\n+\t\t\tif (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n+\t\t\t\twarning(_(\"index.version set, but the value is invalid.\\n\"\n+\t\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n+\t\t\t\tversion = INDEX_FORMAT_DEFAULT;\n+\t\t\t}\n \t\t}\n-\t\treturn version;\n \t}\n \n-\tversion = strtoul(envversion, &endp, 10);\n-\tif (*endp ||\n-\t    version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n-\t\twarning(_(\"GIT_INDEX_VERSION set, but the value is invalid.\\n\"\n-\t\t\t  \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n-\t\tversion = INDEX_FORMAT_DEFAULT;\n+\tif (version == -1) {\n+\t\tenvversion = getenv(\"GIT_TEST_INDEX_VERSION\");\n+\t\tif (envversion) {\n+\t\t\tversion = strtoul(envversion, &endp, 10);\n+\t\t\tif (*endp ||\n+\t\t\t\tversion < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n+\t\t\t\twarning(_(\"GIT_TEST_INDEX_VERSION set, but the value is invalid.\\n\"\n+\t\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n+\t\t\t\tversion = INDEX_FORMAT_DEFAULT;\n+\t\t\t}\n+\t\t}\n \t}\n+\n+\tif (version == -1)\n+\t\tversion = INDEX_FORMAT_DEFAULT;\n+\n \treturn version;\n }\n \ndiff --git a/t/README b/t/README\nindex 9028b47d92..f872638a78 100644\n--- a/t/README\n+++ b/t/README\n@@ -315,10 +315,14 @@ packs on demand. This normally only happens when the object size is\n over 2GB. This variable forces the code path on any object larger than\n <n> bytes.\n \n-GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n+GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n path where deltas larger than this limit require extra memory\n allocation for bookkeeping.\n \n+GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n+for the index version specified.  Can be set to any valid version\n+but the non-default version 4 is probably the most beneficial.\n+\n Naming Tests\n ------------\n \n\nbase-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670\n-- \n2.18.0.windows.1\n\n"},{"id":"358018","messageId":"20180912223101.GA1719@hank.intra.tgummerer.com","threadId":"49335","inReplyTo":"20180912212544.33624-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-12T22:31:01Z","receivedAt":"2018-09-12T22:31:07Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/12, Ben Peart wrote:\n> Teach get_index_format_default() to support running the test suite\n> with specific index versions.  In particular, this enables the test suite\n> to be run using index version 4 which is not the default so gets less testing.\n\nI found this commit message slightly misleading.  Running the test\nsuite with specific index versions is already supported, by defining\nTEST_GIT_INDEX_VERSION in 'config.mak'.  What we're doing here is\nintroduce an additional environment variable that can also be used to\nset the index format in tests.\n\nEven setting TEST_GIT_INDEX_VERSION=4 in the environment does run the\ntest suite with index-v4.  Admittedly the name is a bit strange\ncompared to our usual GIT_TEST_* environment variable names, and it\nshould probably be documented better (it's only documented in the\nMakefile currently), but I'm not sure we should introduce another\nenvironment variable for this purpose?\n\n> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>\n> ---\n> \n> Notes:\n>     Base Ref: v2.19.0\n>     Web-Diff: https://github.com/benpeart/git/commit/52e733e2ce\n>     Checkout: git fetch https://github.com/benpeart/git git-test-index-version-v1 && git checkout 52e733e2ce\n> \n>  read-cache.c | 47 +++++++++++++++++++++++++++++++++--------------\n>  t/README     |  6 +++++-\n>  2 files changed, 38 insertions(+), 15 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 7b1354d759..d140ce9989 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1570,26 +1570,45 @@ static unsigned int get_index_format_default(void)\n>  \tchar *envversion = getenv(\"GIT_INDEX_VERSION\");\n>  \tchar *endp;\n>  \tint value;\n> -\tunsigned int version = INDEX_FORMAT_DEFAULT;\n> +\tunsigned int version = -1;\n> +\n> +\tif (envversion) {\n> +\t\tversion = strtoul(envversion, &endp, 10);\n> +\t\tif (*endp ||\n> +\t\t\tversion < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n> +\t\t\twarning(_(\"GIT_INDEX_VERSION set, but the value is invalid.\\n\"\n> +\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n> +\t\t\tversion = INDEX_FORMAT_DEFAULT;\n> +\t\t}\n> +\t}\n>  \n> -\tif (!envversion) {\n> -\t\tif (!git_config_get_int(\"index.version\", &value))\n> +\tif (version == -1) {\n> +\t\tif (!git_config_get_int(\"index.version\", &value)) {\n>  \t\t\tversion = value;\n> -\t\tif (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n> -\t\t\twarning(_(\"index.version set, but the value is invalid.\\n\"\n> -\t\t\t\t  \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n> -\t\t\treturn INDEX_FORMAT_DEFAULT;\n> +\t\t\tif (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n> +\t\t\t\twarning(_(\"index.version set, but the value is invalid.\\n\"\n> +\t\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n> +\t\t\t\tversion = INDEX_FORMAT_DEFAULT;\n> +\t\t\t}\n>  \t\t}\n> -\t\treturn version;\n>  \t}\n>  \n> -\tversion = strtoul(envversion, &endp, 10);\n> -\tif (*endp ||\n> -\t    version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n> -\t\twarning(_(\"GIT_INDEX_VERSION set, but the value is invalid.\\n\"\n> -\t\t\t  \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n> -\t\tversion = INDEX_FORMAT_DEFAULT;\n> +\tif (version == -1) {\n> +\t\tenvversion = getenv(\"GIT_TEST_INDEX_VERSION\");\n> +\t\tif (envversion) {\n> +\t\t\tversion = strtoul(envversion, &endp, 10);\n> +\t\t\tif (*endp ||\n> +\t\t\t\tversion < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n> +\t\t\t\twarning(_(\"GIT_TEST_INDEX_VERSION set, but the value is invalid.\\n\"\n> +\t\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n> +\t\t\t\tversion = INDEX_FORMAT_DEFAULT;\n> +\t\t\t}\n> +\t\t}\n>  \t}\n> +\n> +\tif (version == -1)\n> +\t\tversion = INDEX_FORMAT_DEFAULT;\n> +\n>  \treturn version;\n>  }\n>  \n> diff --git a/t/README b/t/README\n> index 9028b47d92..f872638a78 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -315,10 +315,14 @@ packs on demand. This normally only happens when the object size is\n>  over 2GB. This variable forces the code path on any object larger than\n>  <n> bytes.\n>  \n> -GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n> +GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n>  path where deltas larger than this limit require extra memory\n>  allocation for bookkeeping.\n>  \n> +GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n> +for the index version specified.  Can be set to any valid version\n> +but the non-default version 4 is probably the most beneficial.\n> +\n>  Naming Tests\n>  ------------\n>  \n> \n> base-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670\n> -- \n> 2.18.0.windows.1\n> \n"},{"id":"358047","messageId":"4f8b4664-6108-db7f-cb40-2bc55d9edece@gmail.com","threadId":"49335","inReplyTo":"20180912223101.GA1719@hank.intra.tgummerer.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-09-13T14:07:34Z","receivedAt":"2018-09-13T14:07:42Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 9/12/2018 6:31 PM, Thomas Gummerer wrote:\n> On 09/12, Ben Peart wrote:\n>> Teach get_index_format_default() to support running the test suite\n>> with specific index versions.  In particular, this enables the test suite\n>> to be run using index version 4 which is not the default so gets less testing.\n> \n> I found this commit message slightly misleading.  Running the test\n> suite with specific index versions is already supported, by defining\n> TEST_GIT_INDEX_VERSION in 'config.mak'.  What we're doing here is\n> introduce an additional environment variable that can also be used to\n> set the index format in tests.\n> \n> Even setting TEST_GIT_INDEX_VERSION=4 in the environment does run the\n> test suite with index-v4.  Admittedly the name is a bit strange\n> compared to our usual GIT_TEST_* environment variable names, and it\n> should probably be documented better (it's only documented in the\n> Makefile currently), but I'm not sure we should introduce another\n> environment variable for this purpose?\n\nTEST_GIT_INDEX_VERSION enables the testing I was looking for but you're \nright, it isn't well documented and the atypical naming and \nimplementation don't help either.\n\nI checked the documentation and code but didn't see any way to test the \nV4 index code path so wrote this patch.  I wonder if we can improve the \ndiscoverability of TEST_GIT_INDEX_VERSION through better naming and \ndocumentation.\n\nHow about this as an alternative?\n\n\n\n\ndiff --git a/Makefile b/Makefile\nindex 5a969f5830..9e84ef02f7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -400,7 +400,7 @@ all::\n  # (defaults to \"man\") if you want to have a different default when\n  # \"git help\" is called without a parameter specifying the format.\n  #\n-# Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite\n+# Define GIT_TEST_INDEX_VERSION to 2, 3 or 4 to run the test suite\n  # with a different indexfile format version.  If it isn't set the index\n  # file format used is index-v[23].\n  #\n@@ -2599,8 +2599,8 @@ endif\n  ifdef GIT_INTEROP_MAKE_OPTS\n         @echo GIT_INTEROP_MAKE_OPTS=\\''$(subst ','\\'',$(subst \n','\\'',$(GIT_INTEROP_MAKE_OPTS)))'\\' >>$@+\n  endif\n-ifdef TEST_GIT_INDEX_VERSION\n-       @echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst \n','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@+\n+ifdef GIT_TEST_INDEX_VERSION\n+       @echo GIT_TEST_INDEX_VERSION=\\''$(subst ','\\'',$(subst \n','\\'',$(GIT_TEST_INDEX_VERSION)))'\\' >>$@+\n\n  endif\n         @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 44288cbb59..31698c01c4 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -134,9 +134,9 @@ export EDITOR\n  GIT_TRACE_BARE=1\n  export GIT_TRACE_BARE\n\n-if test -n \"${TEST_GIT_INDEX_VERSION:+isset}\"\n+if test -n \"${GIT_TEST_INDEX_VERSION:+isset}\"\n  then\n-       GIT_INDEX_VERSION=\"$TEST_GIT_INDEX_VERSION\"\n+       GIT_INDEX_VERSION=\"$GIT_TEST_INDEX_VERSION\"\n         export GIT_INDEX_VERSION\n  fi\n\ndiff --git a/t/README b/t/README\nindex 9028b47d92..f872638a78 100644\n--- a/t/README\n+++ b/t/README\n@@ -315,10 +315,14 @@ packs on demand. This normally only happens when \nthe object size is\n   over 2GB. This variable forces the code path on any object larger than\n   <n> bytes.\n\n-GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n+GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n   path where deltas larger than this limit require extra memory\n   allocation for bookkeeping.\n\n+GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n+for the index version specified.  Can be set to any valid version\n+but the non-default version 4 is probably the most beneficial.\n+\n   Naming Tests\n   ------------\n\n"},{"id":"358082","messageId":"20180913181730.33472-1-benpeart@microsoft.com","threadId":"49335","inReplyTo":"20180912212544.33624-1-benpeart@microsoft.com","subject":"[PATCH v2] read-cache: update TEST_GIT_INDEX_VERSION support","fromName":"Ben Peart","fromEmail":"benpeart@microsoft.com","sentAt":"2018-09-13T18:17:48Z","receivedAt":"2018-09-13T18:17:54Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Rename TEST_GIT_INDEX_VERSION to GIT_TEST_INDEX_VERSION for consistency with\nthe other GIT_TEST_ special setups and properly document its use.\n\nSigned-off-by: Ben Peart <Ben.Peart@microsoft.com>\n---\n\nNotes:\n    Base Ref: v2.19.0\n    Web-Diff: https://github.com/benpeart/git/commit/e26ccb9004\n    Checkout: git fetch https://github.com/benpeart/git git-test-index-version-v2 && git checkout e26ccb9004\n    \n    ### Interdiff (v1..v2):\n    \n    diff --git a/Makefile b/Makefile\n    index 5a969f5830..9e84ef02f7 100644\n    --- a/Makefile\n    +++ b/Makefile\n    @@ -400,7 +400,7 @@ all::\n     # (defaults to \"man\") if you want to have a different default when\n     # \"git help\" is called without a parameter specifying the format.\n     #\n    -# Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite\n    +# Define GIT_TEST_INDEX_VERSION to 2, 3 or 4 to run the test suite\n     # with a different indexfile format version.  If it isn't set the index\n     # file format used is index-v[23].\n     #\n    @@ -2599,8 +2599,8 @@ endif\n     ifdef GIT_INTEROP_MAKE_OPTS\n     \t@echo GIT_INTEROP_MAKE_OPTS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_INTEROP_MAKE_OPTS)))'\\' >>$@+\n     endif\n    -ifdef TEST_GIT_INDEX_VERSION\n    -\t@echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@+\n    +ifdef GIT_TEST_INDEX_VERSION\n    +\t@echo GIT_TEST_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_INDEX_VERSION)))'\\' >>$@+\n     endif\n     \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n    \n    diff --git a/read-cache.c b/read-cache.c\n    index d140ce9989..7b1354d759 100644\n    --- a/read-cache.c\n    +++ b/read-cache.c\n    @@ -1570,45 +1570,26 @@ static unsigned int get_index_format_default(void)\n     \tchar *envversion = getenv(\"GIT_INDEX_VERSION\");\n     \tchar *endp;\n     \tint value;\n    -\tunsigned int version = -1;\n    +\tunsigned int version = INDEX_FORMAT_DEFAULT;\n    \n    -\tif (envversion) {\n    -\t\tversion = strtoul(envversion, &endp, 10);\n    -\t\tif (*endp ||\n    -\t\t\tversion < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n    -\t\t\twarning(_(\"GIT_INDEX_VERSION set, but the value is invalid.\\n\"\n    -\t\t\t\t\"Using version %i\"), INDEX_FORMAT_DEFAULT);\n    -\t\t\tversion = INDEX_FORMAT_DEFAULT;\n    -\t\t}\n    -\t}\n    -\n    -\tif (version == -1) {\n    -\t\tif (!git_config_get_int(\"index.version\", &value)) {\n    +\tif (!envversion) {\n    +\t\tif (!git_config_get_int(\"index.version\", &value))\n     \t\t\tversion = value;\n     \t\tif (version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n     \t\t\twarning(_(\"index.version set, but the value is invalid.\\n\"\n     \t\t\t\t  \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n    -\t\t\t\tversion = INDEX_FORMAT_DEFAULT;\n    -\t\t\t}\n    +\t\t\treturn INDEX_FORMAT_DEFAULT;\n     \t\t}\n    +\t\treturn version;\n     \t}\n    \n    -\tif (version == -1) {\n    -\t\tenvversion = getenv(\"GIT_TEST_INDEX_VERSION\");\n    -\t\tif (envversion) {\n     \tversion = strtoul(envversion, &endp, 10);\n     \tif (*endp ||\n     \t    version < INDEX_FORMAT_LB || INDEX_FORMAT_UB < version) {\n    -\t\t\t\twarning(_(\"GIT_TEST_INDEX_VERSION set, but the value is invalid.\\n\"\n    +\t\twarning(_(\"GIT_INDEX_VERSION set, but the value is invalid.\\n\"\n     \t\t\t  \"Using version %i\"), INDEX_FORMAT_DEFAULT);\n     \t\tversion = INDEX_FORMAT_DEFAULT;\n     \t}\n    -\t\t}\n    -\t}\n    -\n    -\tif (version == -1)\n    -\t\tversion = INDEX_FORMAT_DEFAULT;\n    -\n     \treturn version;\n     }\n    \n    diff --git a/t/README b/t/README\n    index f872638a78..fb6359b17b 100644\n    --- a/t/README\n    +++ b/t/README\n    @@ -320,8 +320,7 @@ path where deltas larger than this limit require extra memory\n     allocation for bookkeeping.\n    \n     GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n    -for the index version specified.  Can be set to any valid version\n    -but the non-default version 4 is probably the most beneficial.\n    +for the index version specified.  Can be set to any valid version.\n    \n     Naming Tests\n     ------------\n    diff --git a/t/test-lib.sh b/t/test-lib.sh\n    index 44288cbb59..31698c01c4 100644\n    --- a/t/test-lib.sh\n    +++ b/t/test-lib.sh\n    @@ -134,9 +134,9 @@ export EDITOR\n     GIT_TRACE_BARE=1\n     export GIT_TRACE_BARE\n    \n    -if test -n \"${TEST_GIT_INDEX_VERSION:+isset}\"\n    +if test -n \"${GIT_TEST_INDEX_VERSION:+isset}\"\n     then\n    -\tGIT_INDEX_VERSION=\"$TEST_GIT_INDEX_VERSION\"\n    +\tGIT_INDEX_VERSION=\"$GIT_TEST_INDEX_VERSION\"\n     \texport GIT_INDEX_VERSION\n     fi\n    \n    ### Patches\n\n Makefile      | 6 +++---\n t/README      | 5 ++++-\n t/test-lib.sh | 4 ++--\n 3 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 5a969f5830..9e84ef02f7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -400,7 +400,7 @@ all::\n # (defaults to \"man\") if you want to have a different default when\n # \"git help\" is called without a parameter specifying the format.\n #\n-# Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite\n+# Define GIT_TEST_INDEX_VERSION to 2, 3 or 4 to run the test suite\n # with a different indexfile format version.  If it isn't set the index\n # file format used is index-v[23].\n #\n@@ -2599,8 +2599,8 @@ endif\n ifdef GIT_INTEROP_MAKE_OPTS\n \t@echo GIT_INTEROP_MAKE_OPTS=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_INTEROP_MAKE_OPTS)))'\\' >>$@+\n endif\n-ifdef TEST_GIT_INDEX_VERSION\n-\t@echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@+\n+ifdef GIT_TEST_INDEX_VERSION\n+\t@echo GIT_TEST_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_TEST_INDEX_VERSION)))'\\' >>$@+\n endif\n \t@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n \ndiff --git a/t/README b/t/README\nindex 9028b47d92..fb6359b17b 100644\n--- a/t/README\n+++ b/t/README\n@@ -315,10 +315,13 @@ packs on demand. This normally only happens when the object size is\n over 2GB. This variable forces the code path on any object larger than\n <n> bytes.\n \n-GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n+GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n path where deltas larger than this limit require extra memory\n allocation for bookkeeping.\n \n+GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n+for the index version specified.  Can be set to any valid version.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 44288cbb59..31698c01c4 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -134,9 +134,9 @@ export EDITOR\n GIT_TRACE_BARE=1\n export GIT_TRACE_BARE\n \n-if test -n \"${TEST_GIT_INDEX_VERSION:+isset}\"\n+if test -n \"${GIT_TEST_INDEX_VERSION:+isset}\"\n then\n-\tGIT_INDEX_VERSION=\"$TEST_GIT_INDEX_VERSION\"\n+\tGIT_INDEX_VERSION=\"$GIT_TEST_INDEX_VERSION\"\n \texport GIT_INDEX_VERSION\n fi\n \n\nbase-commit: 1d4361b0f344188ab5eec6dcea01f61a3a3a1670\n-- \n2.18.0.windows.1\n\n"},{"id":"358084","messageId":"87in39kzc3.fsf@evledraar.gmail.com","threadId":"49335","inReplyTo":"20180912212544.33624-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-13T18:49:00Z","receivedAt":"2018-09-13T18:49:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 12 2018, Ben Peart wrote:\n\n> -GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n> +GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n>  path where deltas larger than this limit require extra memory\n>  allocation for bookkeeping.\n\nIf re-rolled maybe better as a leading \"fix a typo\" patch.\n\nI was reading this in a mobile client earlier and couldn't see at a\nglance (small screen+wrapping) what the change was here, so that sort of\nnitpicking isn't purely theoretical :)\n\n> +GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n> +for the index version specified.  Can be set to any valid version\n> +but the non-default version 4 is probably the most beneficial.\n> +\n\nI'm not familiar with this and haven't dug, so I don't know this: are\nany values of 1..4 OK? 0..4? Would be better to say that here...\n"},{"id":"358091","messageId":"xmqq36udf70k.fsf@gitster-ct.c.googlers.com","threadId":"49335","inReplyTo":"87in39kzc3.fsf@evledraar.gmail.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T20:59:39Z","receivedAt":"2018-09-13T20:59:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Wed, Sep 12 2018, Ben Peart wrote:\n>\n>> -GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n>> +GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n>>  path where deltas larger than this limit require extra memory\n>>  allocation for bookkeeping.\n>\n> If re-rolled maybe better as a leading \"fix a typo\" patch.\n\nYeah, an easy to miss change that is unrelated to the topic is\nbetter made as a separate patch.\n\n"},{"id":"358095","messageId":"20180913215616.GD1719@hank.intra.tgummerer.com","threadId":"49335","inReplyTo":"4f8b4664-6108-db7f-cb40-2bc55d9edece@gmail.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-09-13T21:56:16Z","receivedAt":"2018-09-13T21:56:21Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 09/13, Ben Peart wrote:\n> \n> \n> On 9/12/2018 6:31 PM, Thomas Gummerer wrote:\n> > On 09/12, Ben Peart wrote:\n> > > Teach get_index_format_default() to support running the test suite\n> > > with specific index versions.  In particular, this enables the test suite\n> > > to be run using index version 4 which is not the default so gets less testing.\n> > \n> > I found this commit message slightly misleading.  Running the test\n> > suite with specific index versions is already supported, by defining\n> > TEST_GIT_INDEX_VERSION in 'config.mak'.  What we're doing here is\n> > introduce an additional environment variable that can also be used to\n> > set the index format in tests.\n> > \n> > Even setting TEST_GIT_INDEX_VERSION=4 in the environment does run the\n> > test suite with index-v4.  Admittedly the name is a bit strange\n> > compared to our usual GIT_TEST_* environment variable names, and it\n> > should probably be documented better (it's only documented in the\n> > Makefile currently), but I'm not sure we should introduce another\n> > environment variable for this purpose?\n> \n> TEST_GIT_INDEX_VERSION enables the testing I was looking for but you're\n> right, it isn't well documented and the atypical naming and implementation\n> don't help either.\n> \n> I checked the documentation and code but didn't see any way to test the V4\n> index code path so wrote this patch.  I wonder if we can improve the\n> discoverability of TEST_GIT_INDEX_VERSION through better naming and\n> documentation.\n> \n> How about this as an alternative?\n\nThanks, I do think this is a good idea.  I do however share Ævar's\nconcern in https://public-inbox.org/git/87h8itkz2h.fsf@evledraar.gmail.com/.\nI have TEST_GIT_INDEX_VERSION=4 set in my config.mak since quite a\nlong time, and had I missed this thread, I would all of a sudden not\nrun the test suite with index format 4 anymore without any notice.\n\nI think the suggestion of erroring out if TEST_GIT_INDEX_VERSION is\nset would be useful in this case (and probably others in which you're\nrenaming these variables).  Not sure how many people it would affect\n(and most of those would probably read the mailing list), but it's not\na big change either.\n\nBtw, I think it would be nice to have all these renaming/documenting\nvariables for the test suite patches in one series, so they are easier\nto look at with more context.\n\n> \n> diff --git a/Makefile b/Makefile\n> index 5a969f5830..9e84ef02f7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -400,7 +400,7 @@ all::\n>  # (defaults to \"man\") if you want to have a different default when\n>  # \"git help\" is called without a parameter specifying the format.\n>  #\n> -# Define TEST_GIT_INDEX_VERSION to 2, 3 or 4 to run the test suite\n> +# Define GIT_TEST_INDEX_VERSION to 2, 3 or 4 to run the test suite\n>  # with a different indexfile format version.  If it isn't set the index\n>  # file format used is index-v[23].\n>  #\n> @@ -2599,8 +2599,8 @@ endif\n>  ifdef GIT_INTEROP_MAKE_OPTS\n>         @echo GIT_INTEROP_MAKE_OPTS=\\''$(subst ','\\'',$(subst\n> ','\\'',$(GIT_INTEROP_MAKE_OPTS)))'\\' >>$@+\n>  endif\n> -ifdef TEST_GIT_INDEX_VERSION\n> -       @echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst\n> ','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@+\n> +ifdef GIT_TEST_INDEX_VERSION\n> +       @echo GIT_TEST_INDEX_VERSION=\\''$(subst ','\\'',$(subst\n> ','\\'',$(GIT_TEST_INDEX_VERSION)))'\\' >>$@+\n> \n>  endif\n>         @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 44288cbb59..31698c01c4 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -134,9 +134,9 @@ export EDITOR\n>  GIT_TRACE_BARE=1\n>  export GIT_TRACE_BARE\n> \n> -if test -n \"${TEST_GIT_INDEX_VERSION:+isset}\"\n> +if test -n \"${GIT_TEST_INDEX_VERSION:+isset}\"\n>  then\n> -       GIT_INDEX_VERSION=\"$TEST_GIT_INDEX_VERSION\"\n> +       GIT_INDEX_VERSION=\"$GIT_TEST_INDEX_VERSION\"\n>         export GIT_INDEX_VERSION\n>  fi\n> \n> diff --git a/t/README b/t/README\n> index 9028b47d92..f872638a78 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -315,10 +315,14 @@ packs on demand. This normally only happens when the\n> object size is\n>   over 2GB. This variable forces the code path on any object larger than\n>   <n> bytes.\n> \n> -GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncomon pack-objects code\n> +GIT_TEST_OE_DELTA_SIZE=<n> exercises the uncommon pack-objects code\n>   path where deltas larger than this limit require extra memory\n>   allocation for bookkeeping.\n> \n> +GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path\n> +for the index version specified.  Can be set to any valid version\n> +but the non-default version 4 is probably the most beneficial.\n> +\n>   Naming Tests\n>   ------------\n> \n"},{"id":"358097","messageId":"xmqqh8itdp9n.fsf@gitster-ct.c.googlers.com","threadId":"49335","inReplyTo":"20180913215616.GD1719@hank.intra.tgummerer.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-13T22:08:20Z","receivedAt":"2018-09-13T22:08:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Thanks, I do think this is a good idea.  I do however share Ævar's\n> concern in https://public-inbox.org/git/87h8itkz2h.fsf@evledraar.gmail.com/.\n> I have TEST_GIT_INDEX_VERSION=4 set in my config.mak since quite a\n> long time, and had I missed this thread, I would all of a sudden not\n> run the test suite with index format 4 anymore without any notice.\n>\n> I think the suggestion of erroring out if TEST_GIT_INDEX_VERSION is\n> set would be useful in this case (and probably others in which you're\n> renaming these variables).\n\nI am not enthused by \"you are using an old variable---we fail your\nbuild/test\".  The assumption is that people let config.mak laying\naround regardless of how new/old the vintage of Git they are\nbuilding and testing.  I do not think you'd want to adjust your\nconfig.mak as you switch between 'maint' and 'next.\n\nI think it is OK to make it error only if the old one is set without\nthe new one.  Then people can have _both_ set to the same value\nduring the period in which the topic sails through pu down to next\ndown to master, after seeing an failure once while building and\ntesting 'pu'.\n\n> Btw, I think it would be nice to have all these renaming/documenting\n> variables for the test suite patches in one series, so they are easier\n> to look at with more context.\n\nYeah, even though these three were posted as independent changes,\ntheir additions to t/README inevitably conflicted with each other.\n\n\n\n"},{"id":"358119","messageId":"0a3baaf6-f71b-e8f9-7445-8bf2b5d3513e@gmail.com","threadId":"49335","inReplyTo":"xmqqh8itdp9n.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-09-14T13:50:05Z","receivedAt":"2018-09-14T13:50:12Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 9/13/2018 6:08 PM, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n>> Thanks, I do think this is a good idea.  I do however share Ævar's\n>> concern in https://public-inbox.org/git/87h8itkz2h.fsf@evledraar.gmail.com/.\n>> I have TEST_GIT_INDEX_VERSION=4 set in my config.mak since quite a\n>> long time, and had I missed this thread, I would all of a sudden not\n>> run the test suite with index format 4 anymore without any notice.\n>>\n>> I think the suggestion of erroring out if TEST_GIT_INDEX_VERSION is\n>> set would be useful in this case (and probably others in which you're\n>> renaming these variables).\n> \n> I am not enthused by \"you are using an old variable---we fail your\n> build/test\".  The assumption is that people let config.mak laying\n> around regardless of how new/old the vintage of Git they are\n> building and testing.  I do not think you'd want to adjust your\n> config.mak as you switch between 'maint' and 'next.\n> \n> I think it is OK to make it error only if the old one is set without\n> the new one.  Then people can have _both_ set to the same value\n> during the period in which the topic sails through pu down to next\n> down to master, after seeing an failure once while building and\n> testing 'pu'.\n> \n\nI'll make it a warning if they are both set so that people are reminded \nto remove the old variable and an error if only the old one is set so \npeople know to update their environment.\n\n>> Btw, I think it would be nice to have all these renaming/documenting\n>> variables for the test suite patches in one series, so they are easier\n>> to look at with more context.\n> \n> Yeah, even though these three were posted as independent changes,\n> their additions to t/README inevitably conflicted with each other.\n> \n> \n\nI'll combine all these into a single patch series.  It grew as I\ndiscovered more that needed to be updated.\n"},{"id":"358132","messageId":"xmqqzhwkcaph.fsf@gitster-ct.c.googlers.com","threadId":"49335","inReplyTo":"0a3baaf6-f71b-e8f9-7445-8bf2b5d3513e@gmail.com","subject":"Re: [PATCH v1] read-cache: add GIT_TEST_INDEX_VERSION support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-14T16:20:26Z","receivedAt":"2018-09-14T16:20:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart <peartben@gmail.com> writes:\n\n>> I think it is OK to make it error only if the old one is set without\n>> the new one.  Then people can have _both_ set to the same value\n>> during the period in which the topic sails through pu down to next\n>> down to master, after seeing an failure once while building and\n>> testing 'pu'.\n>>\n>\n> I'll make it a warning if they are both set so that people are\n> reminded to remove the old variable and an error if only the old one\n> is set so people know to update their environment.\n\nAfter the topic graduates to a stable release (or two), it would be\na good addition to encourage people to clean things up, of course,\nbut doing so too early by warning when they are both set is not a\ngood idea, I would think.  It would bring us back to \"the user has\nto futz with config.mak every time switching between 'maint' and\n'next'\".\n\n> I'll combine all these into a single patch series.  It grew as I\n> discovered more that needed to be updated.\n\nThanks. \n\n I didn't mind it too much to have them as independent patches.  It\nat least helped me forcing myself to give closer look at them ;-)\n\n"}]}