{"thread":{"id":"38867","subject":"[PATCH] t1700: make test pass with index-v4","startedAt":"2015-03-20T15:09:24Z","lastAt":"2015-03-24T21:51:04Z","messageCount":18,"participants":["Thomas Gummerer","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"258131","messageId":"1426864165-7334-1-git-send-email-t.gummerer@gmail.com","threadId":"38867","inReplyTo":null,"subject":"[PATCH] t1700: make test pass with index-v4","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T15:09:24Z","receivedAt":"2015-03-20T15:09:24Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The different index versions have different sha-1 checksums. Those\nchecksums are checked in t1700, which makes it fail when run with index\nv4. Fix it.\n\nSigned-off-by: Thomas Gummerer \n---\n t/t1700-split-index.sh | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 94fb473..92f7298 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX\n test_expect_success 'enable split index' '\n \tgit update-index --split-index &&\n \ttest-dump-split-index .git/index >actual &&\n+\tindexversion=$(test-index-version <.git/index) &&\n+\tif test \"$indexversion\" = \"4\"\n+\tthen\n+\t\town=432ef4b63f32193984f339431fd50ca796493569\n+\t\tbase=508851a7f0dfa8691e9f69c7f055865389012491\n+\telse\n+\t\town=8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n+\t\tbase=39d890139ee5356c7ef572216cebcd27aa41f9df\n+\tfi &&\n \tcat >expect <actual &&\n \tcat >expect < writes:\n\n> The different index versions have different sha-1 checksums. Those\n> checksums are checked in t1700, which makes it fail when run with index\n> v4. Fix it.\n\nI am more interested to see how you managed to use index v4 in the\ntests be described next to \"when run with index v4\". I thought we\nwere controling these things fairly tightly (e.g. we disable hooks,\nmove $HOME to avoid getting affected by your personal settings,\netc.).\n\nThanks.\n\n\n> Signed-off-by: Thomas Gummerer \n> ---\n> t/t1700-split-index.sh | 15 ++++++++++++---\n> 1 file changed, 12 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 94fb473..92f7298 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX\n> test_expect_success 'enable split index' '\n> \tgit update-index --split-index &&\n> \ttest-dump-split-index .git/index >actual &&\n> +\tindexversion=$(test-index-version <.git/index) &&\n> +\tif test \"$indexversion\" = \"4\"\n> +\tthen\n> +\t\town=432ef4b63f32193984f339431fd50ca796493569\n> +\t\tbase=508851a7f0dfa8691e9f69c7f055865389012491\n> +\telse\n> +\t\town=8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> +\t\tbase=39d890139ee5356c7ef572216cebcd27aa41f9df\n> +\tfi &&\n> \tcat >expect < -own 8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> +own $own\n> +base $base\n> replacements:\n> deletions:\n> EOF\n> @@ -30,7 +39,7 @@ EOF\n> \n> \ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n> \tcat >expect < -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> +base $base\n> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tone\n> replacements:\n> deletions:\n"},{"id":"258140","messageId":"20150320173750.GA6545@hank","threadId":"38867","inReplyTo":"xmqqsiczfuqa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t1700: make test pass with index-v4","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T17:37:50Z","receivedAt":"2015-03-20T17:37:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n>\n> > The different index versions have different sha-1 checksums. Those\n> > checksums are checked in t1700, which makes it fail when run with index\n> > v4. Fix it.\n>\n> I am more interested to see how you managed to use index v4 in the\n> tests be described next to \"when run with index v4\". I thought we\n> were controling these things fairly tightly (e.g. we disable hooks,\n> move $HOME to avoid getting affected by your personal settings,\n> etc.).\n\nThe tests can be run with index-v4 by setting TEST_GIT_INDEX_VERSION\nin config.mak. This configuration was introduced in 5d9fc88 test-lib:\nallow setting the index format version.\n\n> Thanks.\n>\n>\n> > Signed-off-by: Thomas Gummerer \n> > ---\n> > t/t1700-split-index.sh | 15 ++++++++++++---\n> > 1 file changed, 12 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> > index 94fb473..92f7298 100755\n> > --- a/t/t1700-split-index.sh\n> > +++ b/t/t1700-split-index.sh\n> > @@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX\n> > test_expect_success 'enable split index' '\n> > \tgit update-index --split-index &&\n> > \ttest-dump-split-index .git/index >actual &&\n> > +\tindexversion=$(test-index-version <.git/index) &&\n> > +\tif test \"$indexversion\" = \"4\"\n> > +\tthen\n> > +\t\town=432ef4b63f32193984f339431fd50ca796493569\n> > +\t\tbase=508851a7f0dfa8691e9f69c7f055865389012491\n> > +\telse\n> > +\t\town=8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> > +\t\tbase=39d890139ee5356c7ef572216cebcd27aa41f9df\n> > +\tfi &&\n> > \tcat >expect < > -own 8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> > -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> > +own $own\n> > +base $base\n> > replacements:\n> > deletions:\n> > EOF\n> > @@ -30,7 +39,7 @@ EOF\n> >\n> > \ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n> > \tcat >expect < > -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> > +base $base\n> > 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tone\n> > replacements:\n> > deletions:\n\n--\nThomas Gummerer\n"},{"id":"258150","messageId":"xmqqtwxfee6a.fsf@gitster.dls.corp.google.com","threadId":"38867","inReplyTo":"20150320173750.GA6545@hank","subject":"Re: [PATCH] t1700: make test pass with index-v4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-20T18:06:53Z","receivedAt":"2015-03-20T18:06:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> On 03/20, Junio C Hamano wrote:\n>> Thomas Gummerer writes:\n>>\n>> > The different index versions have different sha-1 checksums. Those\n>> > checksums are checked in t1700, which makes it fail when run with index\n>> > v4. Fix it.\n>>\n>> I am more interested to see how you managed to use index v4 in the\n>> tests be described next to \"when run with index v4\". I thought we\n>> were controling these things fairly tightly (e.g. we disable hooks,\n>> move $HOME to avoid getting affected by your personal settings,\n>> etc.).\n>\n> The tests can be run with index-v4 by setting TEST_GIT_INDEX_VERSION\n> in config.mak. This configuration was introduced in 5d9fc88 test-lib:\n> allow setting the index format version.\n\nAn updated patch to mention \"when run with TEST_GIT_INDEX_VERSION=4\"\nin the log message was what I was asking for ;-)\n"},{"id":"258152","messageId":"1426875630-17481-1-git-send-email-t.gummerer@gmail.com","threadId":"38867","inReplyTo":"xmqqtwxfee6a.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] t1700: make test pass with index-v4","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T18:20:30Z","receivedAt":"2015-03-20T18:20:30Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The different index versions have different sha-1 checksums. Those\nchecksums are checked in t1700, which makes it fail when the test suite\nis run with TEST_GIT_INDEX_VERSION=4. Fix it.\n\nSigned-off-by: Thomas Gummerer \n---\n> An updated patch to mention \"when run with TEST_GIT_INDEX_VERSION=4\"\n> in the log message was what I was asking for ;-)\n\nSorry I didn't catch that. Here it is.\n\t\t\t\t\t \n\n t/t1700-split-index.sh | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 94fb473..92f7298 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX\n test_expect_success 'enable split index' '\n \tgit update-index --split-index &&\n \ttest-dump-split-index .git/index >actual &&\n+\tindexversion=$(test-index-version <.git/index) &&\n+\tif test \"$indexversion\" = \"4\"\n+\tthen\n+\t\town=432ef4b63f32193984f339431fd50ca796493569\n+\t\tbase=508851a7f0dfa8691e9f69c7f055865389012491\n+\telse\n+\t\town=8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n+\t\tbase=39d890139ee5356c7ef572216cebcd27aa41f9df\n+\tfi &&\n \tcat >expect <actual &&\n \tcat >expect < writes:\n\n> The different index versions have different sha-1 checksums. Those\n> checksums are checked in t1700, which makes it fail when the test suite\n> is run with TEST_GIT_INDEX_VERSION=4. Fix it.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n>> An updated patch to mention \"when run with TEST_GIT_INDEX_VERSION=4\"\n>> in the log message was what I was asking for ;-)\n>\n> Sorry I didn't catch that. Here it is.\n\nThanks. Is this the only one that fails with v4?\n\n> \t\t\t\t\t \n>\n> t/t1700-split-index.sh | 15 ++++++++++++---\n> 1 file changed, 12 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> index 94fb473..92f7298 100755\n> --- a/t/t1700-split-index.sh\n> +++ b/t/t1700-split-index.sh\n> @@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX\n> test_expect_success 'enable split index' '\n> \tgit update-index --split-index &&\n> \ttest-dump-split-index .git/index >actual &&\n> +\tindexversion=$(test-index-version <.git/index) &&\n> +\tif test \"$indexversion\" = \"4\"\n> +\tthen\n> +\t\town=432ef4b63f32193984f339431fd50ca796493569\n> +\t\tbase=508851a7f0dfa8691e9f69c7f055865389012491\n> +\telse\n> +\t\town=8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> +\t\tbase=39d890139ee5356c7ef572216cebcd27aa41f9df\n> +\tfi &&\n> \tcat >expect < -own 8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> +own $own\n> +base $base\n> replacements:\n> deletions:\n> EOF\n> @@ -30,7 +39,7 @@ EOF\n> \n> \ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n> \tcat >expect < -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> +base $base\n> 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tone\n> replacements:\n> deletions:\n"},{"id":"258182","messageId":"20150320195943.GB6545@hank","threadId":"38867","inReplyTo":"xmqqd243e9xe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] t1700: make test pass with index-v4","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T19:59:43Z","receivedAt":"2015-03-20T19:59:43Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n>\n> > The different index versions have different sha-1 checksums. Those\n> > checksums are checked in t1700, which makes it fail when the test suite\n> > is run with TEST_GIT_INDEX_VERSION=4. Fix it.\n> >\n> > Signed-off-by: Thomas Gummerer \n> > ---\n> >> An updated patch to mention \"when run with TEST_GIT_INDEX_VERSION=4\"\n> >> in the log message was what I was asking for ;-)\n> >\n> > Sorry I didn't catch that. Here it is.\n>\n> Thanks. Is this the only one that fails with v4?\n\nYes, with this patch the test suite passes with TEST_GIT_INDEX_VERSION\nset.\n\nI've now tried to set the GIT_TEST_SPLIT_INDEX environment variable\nand run the tests, and quite a few seem to fail for both v3 and v4.\nI'm looking into that now, but that seems to be unrelated to index v4,\nand I'm not sure yet if I'm testing the right thing.\n\n> >\n> >\n> > t/t1700-split-index.sh | 15 ++++++++++++---\n> > 1 file changed, 12 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\n> > index 94fb473..92f7298 100755\n> > --- a/t/t1700-split-index.sh\n> > +++ b/t/t1700-split-index.sh\n> > @@ -10,9 +10,18 @@ sane_unset GIT_TEST_SPLIT_INDEX\n> > test_expect_success 'enable split index' '\n> > \tgit update-index --split-index &&\n> > \ttest-dump-split-index .git/index >actual &&\n> > +\tindexversion=$(test-index-version <.git/index) &&\n> > +\tif test \"$indexversion\" = \"4\"\n> > +\tthen\n> > +\t\town=432ef4b63f32193984f339431fd50ca796493569\n> > +\t\tbase=508851a7f0dfa8691e9f69c7f055865389012491\n> > +\telse\n> > +\t\town=8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> > +\t\tbase=39d890139ee5356c7ef572216cebcd27aa41f9df\n> > +\tfi &&\n> > \tcat >expect < > -own 8299b0bcd1ac364e5f1d7768efb62fa2da79a339\n> > -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> > +own $own\n> > +base $base\n> > replacements:\n> > deletions:\n> > EOF\n> > @@ -30,7 +39,7 @@ EOF\n> >\n> > \ttest-dump-split-index .git/index | sed \"/^own/d\" >actual &&\n> > \tcat >expect < > -base 39d890139ee5356c7ef572216cebcd27aa41f9df\n> > +base $base\n> > 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0\tone\n> > replacements:\n> > deletions:\n\n--\nThomas Gummerer\n"},{"id":"258190","messageId":"1426887794-9655-1-git-send-email-t.gummerer@gmail.com","threadId":"38867","inReplyTo":"20150320195943.GB6545@hank","subject":"[PATCH 0/2] fix test suite with split index","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T21:43:12Z","receivedAt":"2015-03-20T21:43:12Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"> I've now tried to set the GIT_TEST_SPLIT_INDEX environment variable\n> and run the tests, and quite a few seem to fail for both v3 and v4.\n> I'm looking into that now, but that seems to be unrelated to index v4,\n> and I'm not sure yet if I'm testing the right thing.\n\nSo now I've tracked it down, 15999d0 read_index_from(): catch out of\norder entries when reading an index file breaks reading split index\nfiles in some cases.\n\nThe first patch allows setting TEST_GIT_TEST_SPLIT_INDEX in config.mak\nto make running the test suite with split index easier, the second\npatch fixes the issue. With the patch I previously sent, the test\nsuite now passes with split-index enabled for both index v3 and v4.\n\t \n\nThomas Gummerer (2):\n test-lib: allow using split index in the test suite\n read-cache: fix reading of split index\n\n Makefile | 6 ++++++\n read-cache.c | 42 +++++++++++++++++++++++-------------------\n t/test-lib.sh | 6 ++++++\n 3 files changed, 35 insertions(+), 19 deletions(-)\n\n-- \n2.1.0.264.g0463184.dirty\n"},{"id":"258189","messageId":"1426887794-9655-2-git-send-email-t.gummerer@gmail.com","threadId":"38867","inReplyTo":"1426887794-9655-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 1/2] test-lib: allow using split index in the test suite","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T21:43:13Z","receivedAt":"2015-03-20T21:43:13Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Allow adding a TEST_GIT_TEST_SPLIT_INDEX variable to config.mak to run\nthe test suite with split index enabled.\n\nSigned-off-by: Thomas Gummerer \n---\n Makefile | 6 ++++++\n t/test-lib.sh | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 44f1dd1..55e558a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -339,6 +339,9 @@ all::\n # with a different indexfile format version. If it isn't set the index\n # file format used is index-v[23].\n #\n+# Define TEST_GIT_TEST_SPLIT_INDEX to 1 to run the test suite with split\n+# index enabled.\n+#\n # Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not\n # return NULL when it receives a bogus time_t.\n #\n@@ -2129,6 +2132,9 @@ endif\n ifdef TEST_GIT_INDEX_VERSION\n \t@echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@\n endif\n+ifdef TEST_GIT_TEST_SPLIT_INDEX\n+\t@echo TEST_GIT_TEST_SPLIT_INDEX=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_TEST_SPLIT_INDEX)))'\\' >>$@\n+endif\n \n ### Detect Python interpreter path changes\n ifndef NO_PYTHON\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c096778..477f253 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -119,6 +119,12 @@ then\n \texport GIT_INDEX_VERSION\n fi\n \n+if test -n \"${TEST_GIT_TEST_SPLIT_INDEX:+isset}\"\n+then\n+\tGIT_TEST_SPLIT_INDEX=\"$TEST_GIT_TEST_SPLIT_INDEX\"\n+\texport GIT_TEST_SPLIT_INDEX\n+fi\n+\n # Add libc MALLOC and MALLOC_PERTURB test\n # only if we are not executing the test with valgrind\n if expr \" $GIT_TEST_OPTS \" : \".* --valgrind \" >/dev/null ||\n-- \n2.1.0.264.g0463184.dirty\n"},{"id":"258191","messageId":"1426887794-9655-3-git-send-email-t.gummerer@gmail.com","threadId":"38867","inReplyTo":"1426887794-9655-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 2/2] read-cache: fix reading of split index","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T21:43:14Z","receivedAt":"2015-03-20T21:43:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The split index extension uses ewah bitmaps to mark index entries as\ndeleted, instead of removing them from the index directly. This can\nresult in an on-disk index, in which entries of stage #0 and higher\nstages appear, which are removed later when the index bases are merged.\n\n15999d0 read_index_from(): catch out of order entries when reading an\nindex file introduces a check which checks if the entries are in order\nafter each index entry is read in do_read_index. This check may however\nfail when a split index is read.\n\nFix this by moving checking the index after we know there is no split\nindex or after the split index bases are successfully merged instead.\n\nSigned-off-by: Thomas Gummerer \n---\n read-cache.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 8d71860..1bf78a4 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1486,18 +1486,25 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n \treturn ce;\n }\n \n-static void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)\n-{\n-\tint name_compare = strcmp(ce->name, next_ce->name);\n-\tif (0 < name_compare)\n-\t\tdie(\"unordered stage entries in index\");\n-\tif (!name_compare) {\n-\t\tif (!ce_stage(ce))\n-\t\t\tdie(\"multiple stage entries for merged file '%s'\",\n-\t\t\t\tce->name);\n-\t\tif (ce_stage(ce) > ce_stage(next_ce))\n-\t\t\tdie(\"unordered stage entries for '%s'\",\n-\t\t\t\tce->name);\n+static void check_ce_order(struct index_state *istate)\n+{\n+\tunsigned int i;\n+\n+\tfor (i = 1; i < istate->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = istate->cache[i - 1];\n+\t\tstruct cache_entry *next_ce = istate->cache[i];\n+\t\tint name_compare = strcmp(ce->name, next_ce->name);\n+\n+\t\tif (0 < name_compare)\n+\t\t\tdie(\"unordered stage entries in index\");\n+\t\tif (!name_compare) {\n+\t\t\tif (!ce_stage(ce))\n+\t\t\t\tdie(\"multiple stage entries for merged file '%s'\",\n+\t\t\t\t ce->name);\n+\t\t\tif (ce_stage(ce) > ce_stage(next_ce))\n+\t\t\t\tdie(\"unordered stage entries for '%s'\",\n+\t\t\t\t ce->name);\n+\t\t}\n \t}\n }\n \n@@ -1562,9 +1569,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n-\t\tif (i > 0)\n-\t\t\tcheck_ce_order(istate->cache[i - 1], ce);\n-\n \t\tsrc_offset += consumed;\n \t}\n \tstrbuf_release(&previous_name_buf);\n@@ -1608,11 +1612,10 @@ int read_index_from(struct index_state *istate, const char *path)\n \n \tret = do_read_index(istate, path, 0);\n \tsplit_index = istate->split_index;\n-\tif (!split_index)\n-\t\treturn ret;\n-\n-\tif (is_null_sha1(split_index->base_sha1))\n+\tif (!split_index || is_null_sha1(split_index->base_sha1)) {\n+\t\tcheck_ce_order(istate);\n \t\treturn ret;\n+\t}\n \n \tif (split_index->base)\n \t\tdiscard_index(split_index->base);\n@@ -1628,6 +1631,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t\t\t sha1_to_hex(split_index->base_sha1)),\n \t\t sha1_to_hex(split_index->base->sha1));\n \tmerge_base_index(istate);\n+\tcheck_ce_order(istate);\n \treturn ret;\n }\n \n-- \n2.1.0.264.g0463184.dirty\n"},{"id":"258192","messageId":"xmqqvbhvcp14.fsf@gitster.dls.corp.google.com","threadId":"38867","inReplyTo":"1426887794-9655-2-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 1/2] test-lib: allow using split index in the test suite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-20T21:55:19Z","receivedAt":"2015-03-20T21:55:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> Allow adding a TEST_GIT_TEST_SPLIT_INDEX variable to config.mak to run\n> the test suite with split index enabled.\n>\n> Signed-off-by: Thomas Gummerer \n\n> ---\n\nHmm, it is not wrong per-se, but would it be too much trouble to do\n\n GIT_TEST_SPLIT_INDEX=YesPlease make test\n\nor is this doing something a lot more than that?\n\n> Makefile | 6 ++++++\n> t/test-lib.sh | 6 ++++++\n> 2 files changed, 12 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index 44f1dd1..55e558a 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -339,6 +339,9 @@ all::\n> # with a different indexfile format version. If it isn't set the index\n> # file format used is index-v[23].\n> #\n> +# Define TEST_GIT_TEST_SPLIT_INDEX to 1 to run the test suite with split\n> +# index enabled.\n> +#\n> # Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not\n> # return NULL when it receives a bogus time_t.\n> #\n> @@ -2129,6 +2132,9 @@ endif\n> ifdef TEST_GIT_INDEX_VERSION\n> \t@echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@\n> endif\n> +ifdef TEST_GIT_TEST_SPLIT_INDEX\n> +\t@echo TEST_GIT_TEST_SPLIT_INDEX=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_TEST_SPLIT_INDEX)))'\\' >>$@\n> +endif\n> \n> ### Detect Python interpreter path changes\n> ifndef NO_PYTHON\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index c096778..477f253 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -119,6 +119,12 @@ then\n> \texport GIT_INDEX_VERSION\n> fi\n> \n> +if test -n \"${TEST_GIT_TEST_SPLIT_INDEX:+isset}\"\n> +then\n> +\tGIT_TEST_SPLIT_INDEX=\"$TEST_GIT_TEST_SPLIT_INDEX\"\n> +\texport GIT_TEST_SPLIT_INDEX\n> +fi\n> +\n> # Add libc MALLOC and MALLOC_PERTURB test\n> # only if we are not executing the test with valgrind\n> if expr \" $GIT_TEST_OPTS \" : \".* --valgrind \" >/dev/null ||\n"},{"id":"258195","messageId":"20150320221239.GC6545@hank","threadId":"38867","inReplyTo":"xmqqvbhvcp14.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] test-lib: allow using split index in the test suite","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-20T22:12:39Z","receivedAt":"2015-03-20T22:12:39Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/20, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n>\n> > Allow adding a TEST_GIT_TEST_SPLIT_INDEX variable to config.mak to run\n> > the test suite with split index enabled.\n> >\n> > Signed-off-by: Thomas Gummerer \n>\n> > ---\n>\n> Hmm, it is not wrong per-se, but would it be too much trouble to do\n>\n> GIT_TEST_SPLIT_INDEX=YesPlease make test\n>\n> or is this doing something a lot more than that?\n\nNo that would work as well, I just thought of it as analogous to\nTEST_GIT_INDEX_VERSION and it might be more convenient to set it once\nand keep it in the config.mak for some people, to check that split\nindex doesn't break. In any case I do not feel strongly about this.\n\n\n> > Makefile | 6 ++++++\n> > t/test-lib.sh | 6 ++++++\n> > 2 files changed, 12 insertions(+)\n> >\n> > diff --git a/Makefile b/Makefile\n> > index 44f1dd1..55e558a 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -339,6 +339,9 @@ all::\n> > # with a different indexfile format version. If it isn't set the index\n> > # file format used is index-v[23].\n> > #\n> > +# Define TEST_GIT_TEST_SPLIT_INDEX to 1 to run the test suite with split\n> > +# index enabled.\n> > +#\n> > # Define GMTIME_UNRELIABLE_ERRORS if your gmtime() function does not\n> > # return NULL when it receives a bogus time_t.\n> > #\n> > @@ -2129,6 +2132,9 @@ endif\n> > ifdef TEST_GIT_INDEX_VERSION\n> > \t@echo TEST_GIT_INDEX_VERSION=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_INDEX_VERSION)))'\\' >>$@\n> > endif\n> > +ifdef TEST_GIT_TEST_SPLIT_INDEX\n> > +\t@echo TEST_GIT_TEST_SPLIT_INDEX=\\''$(subst ','\\'',$(subst ','\\'',$(TEST_GIT_TEST_SPLIT_INDEX)))'\\' >>$@\n> > +endif\n> >\n> > ### Detect Python interpreter path changes\n> > ifndef NO_PYTHON\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index c096778..477f253 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -119,6 +119,12 @@ then\n> > \texport GIT_INDEX_VERSION\n> > fi\n> >\n> > +if test -n \"${TEST_GIT_TEST_SPLIT_INDEX:+isset}\"\n> > +then\n> > +\tGIT_TEST_SPLIT_INDEX=\"$TEST_GIT_TEST_SPLIT_INDEX\"\n> > +\texport GIT_TEST_SPLIT_INDEX\n> > +fi\n> > +\n> > # Add libc MALLOC and MALLOC_PERTURB test\n> > # only if we are not executing the test with valgrind\n> > if expr \" $GIT_TEST_OPTS \" : \".* --valgrind \" >/dev/null ||\n\n--\nThomas Gummerer\n"},{"id":"258407","messageId":"CACsJy8CYi+hYu8zwOy=m7zZk3-8fr+Jq9uT4kEf8fLCOcjHJzw@mail.gmail.com","threadId":"38867","inReplyTo":"1426887794-9655-3-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix reading of split index","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-24T13:02:36Z","receivedAt":"2015-03-24T13:02:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer wrote:\n> The split index extension uses ewah bitmaps to mark index entries as\n> deleted, instead of removing them from the index directly. This can\n> result in an on-disk index, in which entries of stage #0 and higher\n> stages appear, which are removed later when the index bases are merged.\n>\n> 15999d0 read_index_from(): catch out of order entries when reading an\n> index file introduces a check which checks if the entries are in order\n> after each index entry is read in do_read_index. This check may however\n> fail when a split index is read.\n>\n> Fix this by moving checking the index after we know there is no split\n> index or after the split index bases are successfully merged instead.\n\nThank you for catching this. I was about to write \"would be nice to\npoint out what tests fail so the reviewer has easier time trying\nthemselves\", but whoa.. quite a few of them!\n\nMay I suggest a slight modification. Even though stage info is messed\nup before the index is merged, I think we should still check that both\nfront and base indexes have all the names sorted correctly (and even\nstronger, the base index should never contain staged entries). If\nsorting order is messed up, it could lead to other problems. So\ninstead of removing the test from do_read_index(), perhaps add a flag\nin check_ce_order() to optionally detect the stage problem, but\nprint/do nothing, only set a flag so the caller know there may be a\nproblem. In the two new call sites you added, we still call the new\ncheck_ce_order() again to make sure everything is in order. In the\ncall site where split index is not active, if the previous\ncheck_ce_order() call from inside do_read_index() says \"everything is\nok\", we could even skip the check.\n-- \nDuy\n"},{"id":"258436","messageId":"1427216429-15569-1-git-send-email-t.gummerer@gmail.com","threadId":"38867","inReplyTo":"CACsJy8CYi+hYu8zwOy=m7zZk3-8fr+Jq9uT4kEf8fLCOcjHJzw@mail.gmail.com","subject":"[PATCH] read-cache: tighten checks for do_read_index","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-24T17:00:29Z","receivedAt":"2015-03-24T17:00:29Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"03f15a7 read-cache: fix reading of split index moved the checks for the\ncorrect order of index entries out of do_read_index. This loosens the\nchecks more than necessary. Re-introduce the checks for the order, but\ndon't error out when we have multiple stage-0 entries in the index.\nReturn a flag for the caller instead, if we have multiple stage-0\nentries and let the caller decide if we need to error out.\n\nSigned-off-by: Thomas Gummerer \n---\n\nThis is a patch on top of my previous patch, as that one has already\nbeen merged to next.\n\n cache.h | 2 +-\n read-cache.c | 54 ++++++++++++++++++++++++++++++++-----------------\n test-dump-split-index.c | 2 +-\n 3 files changed, 37 insertions(+), 21 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e7b24a2..3eaa258 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -487,7 +487,7 @@ struct lock_file;\n extern int read_index(struct index_state *);\n extern int read_index_preload(struct index_state *, const struct pathspec *pathspec);\n extern int do_read_index(struct index_state *istate, const char *path,\n-\t\t\t int must_exist); /* for testting only! */\n+\t\t\t int must_exist, int *multiple_stage_entries); /* for testting only! */\n extern int read_index_from(struct index_state *, const char *path);\n extern int is_index_unborn(struct index_state *);\n extern int read_index_unmerged(struct index_state *);\ndiff --git a/read-cache.c b/read-cache.c\nindex 36ff89f..2ba67ce 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1488,30 +1488,39 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,\n \treturn ce;\n }\n \n-static void check_ce_order(struct index_state *istate)\n+static int check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce,\n+\t\t\t int gentle_multiple_stage)\n {\n-\tunsigned int i;\n-\n-\tfor (i = 1; i < istate->cache_nr; i++) {\n-\t\tstruct cache_entry *ce = istate->cache[i - 1];\n-\t\tstruct cache_entry *next_ce = istate->cache[i];\n-\t\tint name_compare = strcmp(ce->name, next_ce->name);\n+\tint name_compare = strcmp(ce->name, next_ce->name);\n \n-\t\tif (0 < name_compare)\n-\t\t\tdie(\"unordered stage entries in index\");\n-\t\tif (!name_compare) {\n-\t\t\tif (!ce_stage(ce))\n+\tif (0 < name_compare)\n+\t\tdie(\"unordered stage entries in index\");\n+\tif (!name_compare) {\n+\t\tif (!ce_stage(ce)) {\n+\t\t\tif (gentle_multiple_stage)\n+\t\t\t\treturn 1;\n+\t\t\telse\n \t\t\t\tdie(\"multiple stage entries for merged file '%s'\",\n \t\t\t\t ce->name);\n-\t\t\tif (ce_stage(ce) > ce_stage(next_ce))\n-\t\t\t\tdie(\"unordered stage entries for '%s'\",\n-\t\t\t\t ce->name);\n \t\t}\n+\t\tif (ce_stage(ce) > ce_stage(next_ce))\n+\t\t\tdie(\"unordered stage entries for '%s'\",\n+\t\t\t ce->name);\n \t}\n+\treturn 0;\n+}\n+\n+static void check_istate_order(struct index_state *istate)\n+{\n+\tunsigned int i;\n+\n+\tfor (i = 1; i < istate->cache_nr; i++)\n+\t\tcheck_ce_order(istate->cache[i - 1], istate->cache[i], 0);\n }\n \n /* remember to discard_cache() before reading a different cache! */\n-int do_read_index(struct index_state *istate, const char *path, int must_exist)\n+int do_read_index(struct index_state *istate, const char *path, int must_exist,\n+\t\t int *multiple_stage_entries)\n {\n \tint fd, i;\n \tstruct stat st;\n@@ -1571,6 +1580,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \t\tce = create_from_disk(disk_ce, &consumed, previous_name);\n \t\tset_index_entry(istate, i, ce);\n \n+\t\tif (i > 0)\n+\t\t\tif (check_ce_order(istate->cache[i - 1], ce, 1) > 0 &&\n+\t\t\t multiple_stage_entries)\n+\t\t\t\t*multiple_stage_entries |= 1;\n+\n \t\tsrc_offset += consumed;\n \t}\n \tstrbuf_release(&previous_name_buf);\n@@ -1607,15 +1621,17 @@ int read_index_from(struct index_state *istate, const char *path)\n {\n \tstruct split_index *split_index;\n \tint ret;\n+\tint multiple_stage_entries = 0;\n \n \t/* istate->initialized covers both .git/index and .git/sharedindex.xxx */\n \tif (istate->initialized)\n \t\treturn istate->cache_nr;\n \n-\tret = do_read_index(istate, path, 0);\n+\tret = do_read_index(istate, path, 0, &multiple_stage_entries);\n \tsplit_index = istate->split_index;\n \tif (!split_index || is_null_sha1(split_index->base_sha1)) {\n-\t\tcheck_ce_order(istate);\n+\t\tif (multiple_stage_entries)\n+\t\t\tcheck_istate_order(istate);\n \t\treturn ret;\n \t}\n \n@@ -1625,7 +1641,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\tsplit_index->base = xcalloc(1, sizeof(*split_index->base));\n \tret = do_read_index(split_index->base,\n \t\t\t git_path(\"sharedindex.%s\",\n-\t\t\t\t sha1_to_hex(split_index->base_sha1)), 1);\n+\t\t\t\t sha1_to_hex(split_index->base_sha1)), 1, NULL);\n \tif (hashcmp(split_index->base_sha1, split_index->base->sha1))\n \t\tdie(\"broken index, expect %s in %s, got %s\",\n \t\t sha1_to_hex(split_index->base_sha1),\n@@ -1633,7 +1649,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \t\t\t\t sha1_to_hex(split_index->base_sha1)),\n \t\t sha1_to_hex(split_index->base->sha1));\n \tmerge_base_index(istate);\n-\tcheck_ce_order(istate);\n+\tcheck_istate_order(istate);\n \treturn ret;\n }\n \ndiff --git a/test-dump-split-index.c b/test-dump-split-index.c\nindex 9cf3112..fc9ced7 100644\n--- a/test-dump-split-index.c\n+++ b/test-dump-split-index.c\n@@ -12,7 +12,7 @@ int main(int ac, char **av)\n \tstruct split_index *si;\n \tint i;\n \n-\tdo_read_index(&the_index, av[1], 1);\n+\tdo_read_index(&the_index, av[1], 1, NULL);\n \tprintf(\"own %s\\n\", sha1_to_hex(the_index.sha1));\n \tsi = the_index.split_index;\n \tif (!si) {\n-- \n2.1.0.264.g0463184.dirty\n"},{"id":"258423","messageId":"20150324170755.GA2006@hank","threadId":"38867","inReplyTo":"CACsJy8CYi+hYu8zwOy=m7zZk3-8fr+Jq9uT4kEf8fLCOcjHJzw@mail.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix reading of split index","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-24T17:07:55Z","receivedAt":"2015-03-24T17:07:55Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Duy Nguyen wrote:\n> On Sat, Mar 21, 2015 at 4:43 AM, Thomas Gummerer wrote:\n> > The split index extension uses ewah bitmaps to mark index entries as\n> > deleted, instead of removing them from the index directly. This can\n> > result in an on-disk index, in which entries of stage #0 and higher\n> > stages appear, which are removed later when the index bases are merged.\n> >\n> > 15999d0 read_index_from(): catch out of order entries when reading an\n> > index file introduces a check which checks if the entries are in order\n> > after each index entry is read in do_read_index. This check may however\n> > fail when a split index is read.\n> >\n> > Fix this by moving checking the index after we know there is no split\n> > index or after the split index bases are successfully merged instead.\n>\n> Thank you for catching this. I was about to write \"would be nice to\n> point out what tests fail so the reviewer has easier time trying\n> themselves\", but whoa.. quite a few of them!\n>\n> May I suggest a slight modification. Even though stage info is messed\n> up before the index is merged, I think we should still check that both\n> front and base indexes have all the names sorted correctly (and even\n> stronger, the base index should never contain staged entries). If\n\nHmm I just tried adding another check for that, but the base index\ndoes seem to include staged entries sometimes.\n\nI've tried with this, but there are quite a few test failures. For\nexample in t3600-rm.sh test #52 fails, and test-dump-split-index shows\nthe submodule with stages 1, 2 and 3 in the index.\n\nown 74cd8e14a8fcc5df52e5c47a3ba0c30b29e5075a\nbase 0ff6ae43b1caa039c2a6262f07678b88314a5b4f\n160000 6daff6d0fc4a9299deb0a51881e14cdbda16b88d 1\tsubmod\n160000 ee8321115a919c0607236124af886df2c9f16e2f 2\tsubmod\n160000 f3abce3ddcc2d68a8c113bd16767deeb376276f9 3\tsubmod\nreplacements:\ndeletions: 3\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 2ba67ce..b502290 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1528,6 +1528,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist,\n struct cache_header *hdr;\n void *mmap;\n size_t mmap_size;\n+ int fully_merged = 1;\n struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;\n\n if (istate->initialized)\n@@ -1580,6 +1581,10 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist,\n ce = create_from_disk(disk_ce, &consumed, previous_name);\n set_index_entry(istate, i, ce);\n\n+ if (ce_stage(ce)) {\n+ fully_merged = 0;\n+ }\n+\n if (i > 0)\n if (check_ce_order(istate->cache[i - 1], ce, 1) > 0 &&\n multiple_stage_entries)\n@@ -1610,6 +1615,8 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist,\n src_offset += extsize;\n }\n munmap(mmap, mmap_size);\n+ if (!fully_merged && istate->split_index)\n+ die(\"base index cannot contain staged entries\");\n return istate->cache_nr;\n\n unmap:\n\n\n> sorting order is messed up, it could lead to other problems. So\n> instead of removing the test from do_read_index(), perhaps add a flag\n> in check_ce_order() to optionally detect the stage problem, but\n> print/do nothing, only set a flag so the caller know there may be a\n> problem. In the two new call sites you added, we still call the new\n> check_ce_order() again to make sure everything is in order. In the\n> call site where split index is not active, if the previous\n> check_ce_order() call from inside do_read_index() says \"everything is\n> ok\", we could even skip the check.\n> --\n> Duy\n\n--\nThomas Gummerer\n"},{"id":"258425","messageId":"xmqqpp7y71ox.fsf@gitster.dls.corp.google.com","threadId":"38867","inReplyTo":"CACsJy8CYi+hYu8zwOy=m7zZk3-8fr+Jq9uT4kEf8fLCOcjHJzw@mail.gmail.com","subject":"Re: [PATCH 2/2] read-cache: fix reading of split index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T17:19:42Z","receivedAt":"2015-03-24T17:19:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Thank you for catching this. I was about to write \"would be nice to\n> point out what tests fail so the reviewer has easier time trying\n> themselves\", but whoa.. quite a few of them!\n>\n> May I suggest a slight modification. Even though stage info is messed\n> up before the index is merged, I think we should still check that both\n> front and base indexes have all the names sorted correctly (and even\n> stronger, the base index should never contain staged entries).\n\nI smell a slight layering violation here, though. As far as the\ncode to check the validity of the index is concerned, it is only\nabout the in-core index other code uses at runtime, and how that\nin-core index is prepared, and most importantly, what is recorded in\nthe istate before it gets ready to be used by other code, is not its\nconcern. The state immediately after the base index is read but\nbefore it gets fixed up by the split-index code can have quirks\nspecific to how split-index code does things and it is perfectly OK\nif it does not pass the check for the final shape.\n\nThe above does not change if the current split-index code happens to\npromise certain properties in that intermediate state. It is fine\nif the split-index codepath wants to add its own validator to the\nintermediate state for added robustness, but the rules for the\nintermediate state and the rules for the final state can be\ndifferent, and from the maintainability's point of view, it is\nbetter if we keep the validator for the final-shape oblivious to\nwhat split-index does.\n"},{"id":"258452","messageId":"xmqq384u5be7.fsf@gitster.dls.corp.google.com","threadId":"38867","inReplyTo":"1427216429-15569-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH] read-cache: tighten checks for do_read_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T21:33:04Z","receivedAt":"2015-03-24T21:33:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> 03f15a7 read-cache: fix reading of split index moved the checks for the\n> correct order of index entries out of do_read_index. This loosens the\n> checks more than necessary. Re-introduce the checks for the order, but\n> don't error out when we have multiple stage-0 entries in the index.\n> Return a flag for the caller instead, if we have multiple stage-0\n> entries and let the caller decide if we need to error out.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n>\n> This is a patch on top of my previous patch, as that one has already\n> been merged to next.\n\nI am not convinced that this is a good change in the first place.\n\nThe original before your fix was wrong exactly because it was too\ntightly tied to the implementation of the index file format where\nthere was only one file whose contents must be sorted, and that is\nwhy it was a broken check in a new world with split-index. And your\nfix in 'next' is the right fix---it makes the verification happen\nonly on the result is given to the caller for its consumption.\n\nIt may be true that entries may have to be sorted in a certain order\nwhen reading the original index file format and also reading some\nsteps in reading the split-index, but that merely happens to be an\nimprementation detail of the two format currently supported, and as\nwe improve these formats (or even introduce yet another one) in the\nlonger term, this patch would re-introduce the same issue your\nearlier fix corrected, wouldn't it?\n"},{"id":"258453","messageId":"20150324215104.GB2006@hank","threadId":"38867","inReplyTo":"xmqq384u5be7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] read-cache: tighten checks for do_read_index","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-24T21:51:04Z","receivedAt":"2015-03-24T21:51:04Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n>\n> > 03f15a7 read-cache: fix reading of split index moved the checks for the\n> > correct order of index entries out of do_read_index. This loosens the\n> > checks more than necessary. Re-introduce the checks for the order, but\n> > don't error out when we have multiple stage-0 entries in the index.\n> > Return a flag for the caller instead, if we have multiple stage-0\n> > entries and let the caller decide if we need to error out.\n> >\n> > Signed-off-by: Thomas Gummerer \n> > ---\n> >\n> > This is a patch on top of my previous patch, as that one has already\n> > been merged to next.\n>\n> I am not convinced that this is a good change in the first place.\n>\n> The original before your fix was wrong exactly because it was too\n> tightly tied to the implementation of the index file format where\n> there was only one file whose contents must be sorted, and that is\n> why it was a broken check in a new world with split-index. And your\n> fix in 'next' is the right fix---it makes the verification happen\n> only on the result is given to the caller for its consumption.\n>\n> It may be true that entries may have to be sorted in a certain order\n> when reading the original index file format and also reading some\n> steps in reading the split-index, but that merely happens to be an\n> imprementation detail of the two format currently supported, and as\n> we improve these formats (or even introduce yet another one) in the\n> longer term, this patch would re-introduce the same issue your\n> earlier fix corrected, wouldn't it?\n\nYes, after looking at it again I completely agree. Sorry for the noise.\n"}]}