{"thread":{"id":"49507","subject":"[PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable","startedAt":"2018-10-08T15:17:05Z","lastAt":"2018-10-22T01:41:37Z","messageCount":14,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"359831","messageId":"pull.27.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":null,"subject":"[PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-08T15:17:02Z","receivedAt":"2018-10-08T15:17:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"To increase coverage of the multi-pack-index feature, add a\nGIT_TEST_MULTI_PACK_INDEX environment variable similar to other GIT_TEST_*\nvariables.\n\nAfter creating the environment variable and running the test suite with it\nenabled, I found a few bugs in the multi-pack-index implementation. These\nare handled by the first two patches.\n\nI have set up a CI build on Azure Pipelines [1] that runs the test suite\nwith a few optional features enabled, including GIT_TEST_MULTI_PACK_INDEX\nand GIT_TEST_COMMIT_GRAPH. I'll use this to watch the features and ensure\nthey work well with the rest of the ongoing work. Eventually, we can add\nthese variables to the Travis CI scripts.\n\n[1] https://git.visualstudio.com/git/_build?definitionId=4\n\nDerrick Stolee (3):\n  midx: fix broken free() in close_midx()\n  midx: close multi-pack-index on repack\n  multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX\n\n builtin/repack.c            |  8 ++++++++\n midx.c                      | 17 +++++++++++++----\n midx.h                      |  4 ++++\n t/README                    |  4 ++++\n t/t5310-pack-bitmaps.sh     |  1 +\n t/t5319-multi-pack-index.sh |  2 +-\n t/t9300-fast-import.sh      |  2 +-\n 7 files changed, 32 insertions(+), 6 deletions(-)\n\n\nbase-commit: f84b9b09d40408cf91bbc500d9f190a7866c3e0f\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-27%2Fderrickstolee%2Fmidx-test%2Fupstream-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-27/derrickstolee/midx-test/upstream-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/27\n-- \ngitgitgadget\n"},{"id":"359832","messageId":"9fcbbe336dffb16b7cb5f4de0163404a81597af1.1539011820.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.git.gitgitgadget@gmail.com","subject":"[PATCH 1/3] midx: fix broken free() in close_midx()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-08T15:17:03Z","receivedAt":"2018-10-08T15:17:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen closing a multi-pack-index, we intend to close each pack-file\nand free the struct packed_git that represents it. However, this\nline was previously freeing the array of pointers, not the\npointer itself. This leads to a double-free issue.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex f3e8dbc108..999717b96f 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -190,7 +190,7 @@ static void close_midx(struct multi_pack_index *m)\n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tif (m->packs[i]) {\n \t\t\tclose_pack(m->packs[i]);\n-\t\t\tfree(m->packs);\n+\t\t\tfree(m->packs[i]);\n \t\t}\n \t}\n \tFREE_AND_NULL(m->packs);\n-- \ngitgitgadget\n\n"},{"id":"359833","messageId":"725ebadc92a91469eed089eb501b705c2dd2c627.1539011820.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.git.gitgitgadget@gmail.com","subject":"[PATCH 2/3] midx: close multi-pack-index on repack","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-08T15:17:05Z","receivedAt":"2018-10-08T15:17:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen repacking, we may remove pack-files. This invalidates the\nmulti-pack-index (if it exists). Previously, we removed the\nmulti-pack-index file before removing any pack-file. In some cases,\nthe repack command may load the multi-pack-index into memory. This\nmay lead to later in-memory references to the non-existent pack-\nfiles.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/repack.c | 4 ++++\n midx.c           | 6 +++++-\n midx.h           | 2 ++\n 3 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c6a7943d5c..7925bb976e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -432,6 +432,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!midx_cleared) {\n \t\t\t\t/* if we move a packfile, it will invalidated the midx */\n+\t\t\t\tif (the_repository->objects) {\n+\t\t\t\t\tclose_midx(the_repository->objects->multi_pack_index);\n+\t\t\t\t\tthe_repository->objects->multi_pack_index = NULL;\n+\t\t\t\t}\n \t\t\t\tclear_midx_file(get_object_directory());\n \t\t\t\tmidx_cleared = 1;\n \t\t\t}\ndiff --git a/midx.c b/midx.c\nindex 999717b96f..fe8532a9d1 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -180,9 +180,13 @@ cleanup_fail:\n \treturn NULL;\n }\n \n-static void close_midx(struct multi_pack_index *m)\n+void close_midx(struct multi_pack_index *m)\n {\n \tuint32_t i;\n+\n+\tif (!m)\n+\t\treturn;\n+\n \tmunmap((unsigned char *)m->data, m->data_len);\n \tclose(m->fd);\n \tm->fd = -1;\ndiff --git a/midx.h b/midx.h\nindex a210f1af2a..af6b5cb58f 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -44,4 +44,6 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i\n int write_midx_file(const char *object_dir);\n void clear_midx_file(const char *object_dir);\n \n+void close_midx(struct multi_pack_index *m);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"359834","messageId":"04e3e9108266e322a3db827355689c887a9cae8c.1539011820.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.git.gitgitgadget@gmail.com","subject":"[PATCH 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-08T15:17:06Z","receivedAt":"2018-10-08T15:17:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe multi-pack-index feature is tested in isolation by\nt5319-multi-pack-index.sh, but there are many more interesting\nscenarios in the test suite surrounding pack-file data shapes\nand interactions. Since the multi-pack-index is an optional\ndata structure, it does not make sense to include it by default\nin those tests.\n\nInstead, add a new GIT_TEST_MULTI_PACK_INDEX environment variable\nthat enables core.multiPackIndex and writes a multi-pack-index\nafter each 'git repack' command. This adds extra test coverage\nwhen needed.\n\nThere are a few spots in the test suite that need to react to this\nchange:\n\n* t5319-multi-pack-index.sh: there is a test that checks that\n  'git repack' deletes the multi-pack-index. Disable the environment\n  variable to ensure this still happens.\n\n* t5310-pack-bitmaps.sh: One test moves a pack-file from the object\n  directory to an alternate. This breaks the multi-pack-index, so\n  delete the multi-pack-index at this point, if it exists.\n\n* t9300-fast-import.sh: One test verifies the number of files in\n  the .git/objects/pack directory is exactly 8. Exclude the\n  multi-pack-index from this count so it is still 8 in all cases.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/repack.c            | 4 ++++\n midx.c                      | 9 +++++++--\n midx.h                      | 2 ++\n t/README                    | 4 ++++\n t/t5310-pack-bitmaps.sh     | 1 +\n t/t5319-multi-pack-index.sh | 2 +-\n t/t9300-fast-import.sh      | 2 +-\n 7 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 7925bb976e..418442bfe2 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -558,6 +558,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (!no_update_server_info)\n \t\tupdate_server_info(0);\n \tremove_temporary_files();\n+\n+\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\n+\t\twrite_midx_file(get_object_directory());\n+\n \tstring_list_clear(&names, 0);\n \tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_packs, 0);\ndiff --git a/midx.c b/midx.c\nindex fe8532a9d1..aeafb58fa3 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -338,9 +338,14 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i\n \tstruct multi_pack_index *m;\n \tstruct multi_pack_index *m_search;\n \tint config_value;\n+\tstatic int env_value = -1;\n \n-\tif (repo_config_get_bool(r, \"core.multipackindex\", &config_value) ||\n-\t    !config_value)\n+\tif (env_value < 0)\n+\t\tenv_value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);\n+\n+\tif (!env_value &&\n+\t    (repo_config_get_bool(r, \"core.multipackindex\", &config_value) ||\n+\t    !config_value))\n \t\treturn 0;\n \n \tfor (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next)\ndiff --git a/midx.h b/midx.h\nindex af6b5cb58f..bec8f73d28 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -3,6 +3,8 @@\n \n #include \"repository.h\"\n \n+#define GIT_TEST_MULTI_PACK_INDEX \"GIT_TEST_MULTI_PACK_INDEX\"\n+\n struct multi_pack_index {\n \tstruct multi_pack_index *next;\n \ndiff --git a/t/README b/t/README\nindex 3ea6c85460..9d0277c338 100644\n--- a/t/README\n+++ b/t/README\n@@ -327,6 +327,10 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to\n be written after every 'git commit' command, and overrides the\n 'core.commitGraph' setting to true.\n \n+GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-\n+index to be written after every 'git repack' command, and overrides the\n+'core.multiPackIndex' setting to true.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 1be3459c5b..82d7f7f6a5 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -191,6 +191,7 @@ test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pa\n \n test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '\n \tmv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ &&\n+\trm -f .git/objects/pack/multi-pack-index &&\n \ttest_when_finished \"mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/\" &&\n \techo HEAD | git pack-objects --local --stdout --revs >3b.pack &&\n \tgit index-pack 3b.pack &&\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 6f56b38674..4024ff9a39 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -152,7 +152,7 @@ compare_results_with_midx \"twelve packs\"\n \n test_expect_success 'repack removes multi-pack-index' '\n \ttest_path_is_file $objdir/pack/multi-pack-index &&\n-\tgit repack -adf &&\n+\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&\n \ttest_path_is_missing $objdir/pack/multi-pack-index\n '\n \ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 40fe7e4976..59a13b6a77 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1558,7 +1558,7 @@ test_expect_success 'O: blank lines not necessary after other commands' '\n \tINPUT_END\n \n \tgit fast-import <input &&\n-\ttest 8 = $(find .git/objects/pack -type f | wc -l) &&\n+\ttest 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) &&\n \ttest $(git rev-parse refs/tags/O3-2nd) = $(git rev-parse O3^) &&\n \tgit log --reverse --pretty=oneline O3 | sed s/^.*z// >actual &&\n \ttest_cmp expect actual\n-- \ngitgitgadget\n"},{"id":"359909","messageId":"xmqq8t37qyip.fsf@gitster-ct.c.googlers.com","threadId":"49507","inReplyTo":"9fcbbe336dffb16b7cb5f4de0163404a81597af1.1539011820.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] midx: fix broken free() in close_midx()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-09T09:07:10Z","receivedAt":"2018-10-09T09:07:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> When closing a multi-pack-index, we intend to close each pack-file\n> and free the struct packed_git that represents it. However, this\n> line was previously freeing the array of pointers, not the\n> pointer itself. This leads to a double-free issue.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  midx.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/midx.c b/midx.c\n> index f3e8dbc108..999717b96f 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -190,7 +190,7 @@ static void close_midx(struct multi_pack_index *m)\n>  \tfor (i = 0; i < m->num_packs; i++) {\n>  \t\tif (m->packs[i]) {\n>  \t\t\tclose_pack(m->packs[i]);\n> -\t\t\tfree(m->packs);\n> +\t\t\tfree(m->packs[i]);\n>  \t\t}\n>  \t}\n>  \tFREE_AND_NULL(m->packs);\n\nYup, kinda obvious when we view it with the post context.\n"},{"id":"359910","messageId":"xmqq4ldvqyci.fsf@gitster-ct.c.googlers.com","threadId":"49507","inReplyTo":"725ebadc92a91469eed089eb501b705c2dd2c627.1539011820.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] midx: close multi-pack-index on repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-09T09:10:53Z","receivedAt":"2018-10-09T09:10:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index c6a7943d5c..7925bb976e 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -432,6 +432,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \t\t\tif (!midx_cleared) {\n>  \t\t\t\t/* if we move a packfile, it will invalidated the midx */\n> +\t\t\t\tif (the_repository->objects) {\n> +\t\t\t\t\tclose_midx(the_repository->objects->multi_pack_index);\n> +\t\t\t\t\tthe_repository->objects->multi_pack_index = NULL;\n> +\t\t\t\t}\n>  \t\t\t\tclear_midx_file(get_object_directory());\n>  \t\t\t\tmidx_cleared = 1;\n\nIt somehow looks like a bit of layering violation, doesn't it?  When\nwe are clearing a midx file, don't we always want to do this as well?\n"},{"id":"359923","messageId":"8b5dbe3d-b382-bf48-b524-d9e8a074ac4d@gmail.com","threadId":"49507","inReplyTo":"xmqq4ldvqyci.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] midx: close multi-pack-index on repack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-09T14:11:32Z","receivedAt":"2018-10-09T14:11:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/9/2018 5:10 AM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> diff --git a/builtin/repack.c b/builtin/repack.c\n>> index c6a7943d5c..7925bb976e 100644\n>> --- a/builtin/repack.c\n>> +++ b/builtin/repack.c\n>> @@ -432,6 +432,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>>   \n>>   \t\t\tif (!midx_cleared) {\n>>   \t\t\t\t/* if we move a packfile, it will invalidated the midx */\n>> +\t\t\t\tif (the_repository->objects) {\n>> +\t\t\t\t\tclose_midx(the_repository->objects->multi_pack_index);\n>> +\t\t\t\t\tthe_repository->objects->multi_pack_index = NULL;\n>> +\t\t\t\t}\n>>   \t\t\t\tclear_midx_file(get_object_directory());\n>>   \t\t\t\tmidx_cleared = 1;\n> It somehow looks like a bit of layering violation, doesn't it?  When\n> we are clearing a midx file, don't we always want to do this as well?\n\nYou're right. It did feel a bit wrong. In v2, I'll replace this commit \nwith a refactor of clear_midx_file() to do that. One tricky part is that \nwe need to clear the file even if the in-memory struct hasn't been \ninitialized, but I think passing a repository will suffice for that.\n\nCC Stefan: Is there a plan to make get_object_directory() take a \nrepository parameter?\n\nThanks,\n\n-Stolee\n\n"},{"id":"359931","messageId":"CAGZ79kZHEco4KJXzGDfVTtp9ad1EqQdHCe4Y9yVMGGiAECjUQQ@mail.gmail.com","threadId":"49507","inReplyTo":"8b5dbe3d-b382-bf48-b524-d9e8a074ac4d@gmail.com","subject":"Re: [PATCH 2/3] midx: close multi-pack-index on repack","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-09T18:15:15Z","receivedAt":"2018-10-09T18:15:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 9, 2018 at 7:11 AM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> CC Stefan: Is there a plan to make get_object_directory() take a\n> repository parameter?\n\nNot an immediate plan. Regarding the large refactorings I am mostly\nwaiting for nd/the-index to stabilize (which may be stable enough by\nnow) before proceeding.  Specifically 2abf350385 (revision.c: remove\nimplicit dependency on the_index, 2018-09-21) is a missing piece that\nI want to build on.\n\nMy next step is to look into making use of the refactorings that we did\nover the last year, so I'd rather look into more submodule code again.\n\nTo come back to your question:\n    git grep get_object_directory |wc -l\n    30\n    git grep \"objects->objectdir\" |wc -l\n    11\n2 out of the 11 matches are from the implementation of get_object_directory\n\nSo either we'd actually teach get_object_directory to take a repository\nand inspect the 11 occurrences of direct access to objects->objectdir\nor we'd view get_object_directory() as a fancy wrapper, that we might want\nto drop eventually.\n"},{"id":"360325","messageId":"pull.27.v2.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-12T17:34:16Z","receivedAt":"2018-10-12T17:34:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"To increase coverage of the multi-pack-index feature, add a\nGIT_TEST_MULTI_PACK_INDEX environment variable similar to other GIT_TEST_*\nvariables.\n\nAfter creating the environment variable and running the test suite with it\nenabled, I found a few bugs in the multi-pack-index implementation. These\nare handled by the first two patches.\n\nI have set up a CI build on Azure Pipelines [1] that runs the test suite\nwith a few optional features enabled, including GIT_TEST_MULTI_PACK_INDEX\nand GIT_TEST_COMMIT_GRAPH. I'll use this to watch the features and ensure\nthey work well with the rest of the ongoing work. Eventually, we can add\nthese variables to the Travis CI scripts.\n\n[1] https://git.visualstudio.com/git/_build?definitionId=4\n\nDerrick Stolee (3):\n  midx: fix broken free() in close_midx()\n  midx: close multi-pack-index on repack\n  multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX\n\n builtin/repack.c            |  7 +++++--\n midx.c                      | 26 ++++++++++++++++++++------\n midx.h                      |  6 +++++-\n t/README                    |  4 ++++\n t/t5310-pack-bitmaps.sh     |  1 +\n t/t5319-multi-pack-index.sh |  2 +-\n t/t9300-fast-import.sh      |  2 +-\n 7 files changed, 37 insertions(+), 11 deletions(-)\n\n\nbase-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-27%2Fderrickstolee%2Fmidx-test%2Fupstream-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-27/derrickstolee/midx-test/upstream-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/27\n\nRange-diff vs v1:\n\n 1:  9fcbbe336d = 1:  8bd672fe26 midx: fix broken free() in close_midx()\n 2:  725ebadc92 ! 2:  2d8f26679d midx: close multi-pack-index on repack\n     @@ -15,16 +15,15 @@\n      --- a/builtin/repack.c\n      +++ b/builtin/repack.c\n      @@\n     + \t\t\tchar *fname, *fname_old;\n       \n       \t\t\tif (!midx_cleared) {\n     - \t\t\t\t/* if we move a packfile, it will invalidated the midx */\n     -+\t\t\t\tif (the_repository->objects) {\n     -+\t\t\t\t\tclose_midx(the_repository->objects->multi_pack_index);\n     -+\t\t\t\t\tthe_repository->objects->multi_pack_index = NULL;\n     -+\t\t\t\t}\n     - \t\t\t\tclear_midx_file(get_object_directory());\n     +-\t\t\t\t/* if we move a packfile, it will invalidated the midx */\n     +-\t\t\t\tclear_midx_file(get_object_directory());\n     ++\t\t\t\tclear_midx_file(the_repository);\n       \t\t\t\tmidx_cleared = 1;\n       \t\t\t}\n     + \n      \n      diff --git a/midx.c b/midx.c\n      --- a/midx.c\n     @@ -44,13 +43,34 @@\n       \tmunmap((unsigned char *)m->data, m->data_len);\n       \tclose(m->fd);\n       \tm->fd = -1;\n     +@@\n     + \treturn 0;\n     + }\n     + \n     +-void clear_midx_file(const char *object_dir)\n     ++void clear_midx_file(struct repository *r)\n     + {\n     +-\tchar *midx = get_midx_filename(object_dir);\n     ++\tchar *midx = get_midx_filename(r->objects->objectdir);\n     ++\n     ++\tif (r->objects && r->objects->multi_pack_index) {\n     ++\t\tclose_midx(r->objects->multi_pack_index);\n     ++\t\tr->objects->multi_pack_index = NULL;\n     ++\t}\n     + \n     + \tif (remove_path(midx)) {\n     + \t\tUNLEAK(midx);\n      \n      diff --git a/midx.h b/midx.h\n      --- a/midx.h\n      +++ b/midx.h\n      @@\n     + int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);\n     + \n       int write_midx_file(const char *object_dir);\n     - void clear_midx_file(const char *object_dir);\n     +-void clear_midx_file(const char *object_dir);\n     ++void clear_midx_file(struct repository *r);\n     + int verify_midx_file(const char *object_dir);\n       \n      +void close_midx(struct multi_pack_index *m);\n      +\n 3:  04e3e91082 = 3:  57c64e814c multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX\n\n-- \ngitgitgadget\n"},{"id":"360326","messageId":"8bd672fe26574972043bc7e041e693c3e17d20a1.1539365654.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] midx: fix broken free() in close_midx()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-12T17:34:17Z","receivedAt":"2018-10-12T17:34:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen closing a multi-pack-index, we intend to close each pack-file\nand free the struct packed_git that represents it. However, this\nline was previously freeing the array of pointers, not the\npointer itself. This leads to a double-free issue.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 713d6f9dde..bf1f511862 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -186,7 +186,7 @@ static void close_midx(struct multi_pack_index *m)\n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tif (m->packs[i]) {\n \t\t\tclose_pack(m->packs[i]);\n-\t\t\tfree(m->packs);\n+\t\t\tfree(m->packs[i]);\n \t\t}\n \t}\n \tFREE_AND_NULL(m->packs);\n-- \ngitgitgadget\n\n"},{"id":"360327","messageId":"57c64e814cc5c5a453dcfd1d84c1efba1a4ef964.1539365654.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-12T17:34:20Z","receivedAt":"2018-10-12T17:34:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe multi-pack-index feature is tested in isolation by\nt5319-multi-pack-index.sh, but there are many more interesting\nscenarios in the test suite surrounding pack-file data shapes\nand interactions. Since the multi-pack-index is an optional\ndata structure, it does not make sense to include it by default\nin those tests.\n\nInstead, add a new GIT_TEST_MULTI_PACK_INDEX environment variable\nthat enables core.multiPackIndex and writes a multi-pack-index\nafter each 'git repack' command. This adds extra test coverage\nwhen needed.\n\nThere are a few spots in the test suite that need to react to this\nchange:\n\n* t5319-multi-pack-index.sh: there is a test that checks that\n  'git repack' deletes the multi-pack-index. Disable the environment\n  variable to ensure this still happens.\n\n* t5310-pack-bitmaps.sh: One test moves a pack-file from the object\n  directory to an alternate. This breaks the multi-pack-index, so\n  delete the multi-pack-index at this point, if it exists.\n\n* t9300-fast-import.sh: One test verifies the number of files in\n  the .git/objects/pack directory is exactly 8. Exclude the\n  multi-pack-index from this count so it is still 8 in all cases.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/repack.c            | 4 ++++\n midx.c                      | 9 +++++++--\n midx.h                      | 2 ++\n t/README                    | 4 ++++\n t/t5310-pack-bitmaps.sh     | 1 +\n t/t5319-multi-pack-index.sh | 2 +-\n t/t9300-fast-import.sh      | 2 +-\n 7 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 44965cbaa3..26dcccdafc 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -553,6 +553,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (!no_update_server_info)\n \t\tupdate_server_info(0);\n \tremove_temporary_files();\n+\n+\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))\n+\t\twrite_midx_file(get_object_directory());\n+\n \tstring_list_clear(&names, 0);\n \tstring_list_clear(&rollback, 0);\n \tstring_list_clear(&existing_packs, 0);\ndiff --git a/midx.c b/midx.c\nindex 22247a30ab..02b2211e31 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -335,9 +335,14 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i\n \tstruct multi_pack_index *m;\n \tstruct multi_pack_index *m_search;\n \tint config_value;\n+\tstatic int env_value = -1;\n \n-\tif (repo_config_get_bool(r, \"core.multipackindex\", &config_value) ||\n-\t    !config_value)\n+\tif (env_value < 0)\n+\t\tenv_value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);\n+\n+\tif (!env_value &&\n+\t    (repo_config_get_bool(r, \"core.multipackindex\", &config_value) ||\n+\t    !config_value))\n \t\treturn 0;\n \n \tfor (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next)\ndiff --git a/midx.h b/midx.h\nindex 0f68bccdd5..f2bb7e681c 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -3,6 +3,8 @@\n \n #include \"repository.h\"\n \n+#define GIT_TEST_MULTI_PACK_INDEX \"GIT_TEST_MULTI_PACK_INDEX\"\n+\n struct multi_pack_index {\n \tstruct multi_pack_index *next;\n \ndiff --git a/t/README b/t/README\nindex 5e48a043ce..9bfdd3004c 100644\n--- a/t/README\n+++ b/t/README\n@@ -327,6 +327,10 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to\n be written after every 'git commit' command, and overrides the\n 'core.commitGraph' setting to true.\n \n+GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-\n+index to be written after every 'git repack' command, and overrides the\n+'core.multiPackIndex' setting to true.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 1be3459c5b..82d7f7f6a5 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -191,6 +191,7 @@ test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pa\n \n test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '\n \tmv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ &&\n+\trm -f .git/objects/pack/multi-pack-index &&\n \ttest_when_finished \"mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/\" &&\n \techo HEAD | git pack-objects --local --stdout --revs >3b.pack &&\n \tgit index-pack 3b.pack &&\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd8e841b81..70926b5bc0 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -271,7 +271,7 @@ test_expect_success 'git-fsck incorrect offset' '\n \n test_expect_success 'repack removes multi-pack-index' '\n \ttest_path_is_file $objdir/pack/multi-pack-index &&\n-\tgit repack -adf &&\n+\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&\n \ttest_path_is_missing $objdir/pack/multi-pack-index\n '\n \ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 40fe7e4976..59a13b6a77 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -1558,7 +1558,7 @@ test_expect_success 'O: blank lines not necessary after other commands' '\n \tINPUT_END\n \n \tgit fast-import <input &&\n-\ttest 8 = $(find .git/objects/pack -type f | wc -l) &&\n+\ttest 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) &&\n \ttest $(git rev-parse refs/tags/O3-2nd) = $(git rev-parse O3^) &&\n \tgit log --reverse --pretty=oneline O3 | sed s/^.*z// >actual &&\n \ttest_cmp expect actual\n-- \ngitgitgadget\n"},{"id":"360328","messageId":"2d8f26679d4ba18d7a080a5e6be993198f9b1da2.1539365654.git.gitgitgadget@gmail.com","threadId":"49507","inReplyTo":"pull.27.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] midx: close multi-pack-index on repack","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-12T17:34:19Z","receivedAt":"2018-10-12T17:34:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nWhen repacking, we may remove pack-files. This invalidates the\nmulti-pack-index (if it exists). Previously, we removed the\nmulti-pack-index file before removing any pack-file. In some cases,\nthe repack command may load the multi-pack-index into memory. This\nmay lead to later in-memory references to the non-existent pack-\nfiles.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/repack.c |  3 +--\n midx.c           | 15 ++++++++++++---\n midx.h           |  4 +++-\n 3 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c6a7943d5c..44965cbaa3 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -431,8 +431,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tchar *fname, *fname_old;\n \n \t\t\tif (!midx_cleared) {\n-\t\t\t\t/* if we move a packfile, it will invalidated the midx */\n-\t\t\t\tclear_midx_file(get_object_directory());\n+\t\t\t\tclear_midx_file(the_repository);\n \t\t\t\tmidx_cleared = 1;\n \t\t\t}\n \ndiff --git a/midx.c b/midx.c\nindex bf1f511862..22247a30ab 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -176,9 +176,13 @@ cleanup_fail:\n \treturn NULL;\n }\n \n-static void close_midx(struct multi_pack_index *m)\n+void close_midx(struct multi_pack_index *m)\n {\n \tuint32_t i;\n+\n+\tif (!m)\n+\t\treturn;\n+\n \tmunmap((unsigned char *)m->data, m->data_len);\n \tclose(m->fd);\n \tm->fd = -1;\n@@ -914,9 +918,14 @@ cleanup:\n \treturn 0;\n }\n \n-void clear_midx_file(const char *object_dir)\n+void clear_midx_file(struct repository *r)\n {\n-\tchar *midx = get_midx_filename(object_dir);\n+\tchar *midx = get_midx_filename(r->objects->objectdir);\n+\n+\tif (r->objects && r->objects->multi_pack_index) {\n+\t\tclose_midx(r->objects->multi_pack_index);\n+\t\tr->objects->multi_pack_index = NULL;\n+\t}\n \n \tif (remove_path(midx)) {\n \t\tUNLEAK(midx);\ndiff --git a/midx.h b/midx.h\nindex ce80b91c68..0f68bccdd5 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -42,7 +42,9 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name);\n int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);\n \n int write_midx_file(const char *object_dir);\n-void clear_midx_file(const char *object_dir);\n+void clear_midx_file(struct repository *r);\n int verify_midx_file(const char *object_dir);\n \n+void close_midx(struct multi_pack_index *m);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"360329","messageId":"38aff549-9582-9879-9b3d-81cf5b1c3e24@gmail.com","threadId":"49507","inReplyTo":"pull.27.v2.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-12T17:41:02Z","receivedAt":"2018-10-12T17:41:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/12/2018 1:34 PM, Derrick Stolee via GitGitGadget wrote:\n> To increase coverage of the multi-pack-index feature, add a\n> GIT_TEST_MULTI_PACK_INDEX environment variable similar to other GIT_TEST_*\n> variables.\n>\n> After creating the environment variable and running the test suite with it\n> enabled, I found a few bugs in the multi-pack-index implementation. These\n> are handled by the first two patches.\n>\n> I have set up a CI build on Azure Pipelines [1] that runs the test suite\n> with a few optional features enabled, including GIT_TEST_MULTI_PACK_INDEX\n> and GIT_TEST_COMMIT_GRAPH. I'll use this to watch the features and ensure\n> they work well with the rest of the ongoing work. Eventually, we can add\n> these variables to the Travis CI scripts.\n>\n> [1] https://git.visualstudio.com/git/_build?definitionId=4\n>\n> Derrick Stolee (3):\n>    midx: fix broken free() in close_midx()\n>    midx: close multi-pack-index on repack\n>    multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX\n>\n>   builtin/repack.c            |  7 +++++--\n>   midx.c                      | 26 ++++++++++++++++++++------\n>   midx.h                      |  6 +++++-\n>   t/README                    |  4 ++++\n>   t/t5310-pack-bitmaps.sh     |  1 +\n>   t/t5319-multi-pack-index.sh |  2 +-\n>   t/t9300-fast-import.sh      |  2 +-\n>   7 files changed, 37 insertions(+), 11 deletions(-)\n>\n>\n> base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf\nI should explicitly mention that this base commit is different as \notherwise I will conflict with ds/multi-pack-verify with the new \nprototype in midx.h.\n\nThanks,\n-Stolee\n"},{"id":"361106","messageId":"xmqqy3aqycz8.fsf@gitster-ct.c.googlers.com","threadId":"49507","inReplyTo":"38aff549-9582-9879-9b3d-81cf5b1c3e24@gmail.com","subject":"Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T01:41:31Z","receivedAt":"2018-10-22T01:41:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n>> base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf\n> I should explicitly mention that this base commit is different as\n> otherwise I will conflict with ds/multi-pack-verify with the new\n> prototype in midx.h.\n\nThere indeed is a tiny textual conflict, and in this case it may not\nmatter that much, but please make it a habit to refrain from doing\nsuch a rebase in general.  It makes it impossible to compare the new\nround in the same context that the old round was inspected and has\nbeen tested, unless such a textual conflict avoidance is undone.\n\nA good rule of thumb is to build on the same base, attempt a trial\nmerge to 'master' (and 'next' and 'pu' if you are inclined to), and\nsee how bad a conflict you get.  And if the conflict is something\nyou can trivially resolve and the resolution would bring the code to\nthe same state as you would get if you rebased, then you are better\noff not rebasing and let the maintainer deal with the merge.  You\ncannot control what other contributor would do to the code while you\nare working on it, so having to resolve these tiny textual conflicts\nis not \"an unnecessary added burden\" to me (having to backport to\nsee the new round in the same context as the old round is, though).\n\nOf course, if you truly depend on some recent addition that happend\nsince your old base, please do not hesitate to rebase.\n\nThanks.\n"}]}