Add GIT_TEST_MULTI_PACK_INDEX environment variable

To increase coverage of the multi-pack-index feature, add a GIT_TEST_MULTI_PACK_INDEX environment variable similar to other GIT_TEST_* variables.

After creating the environment variable and running the test suite with it enabled, I found a few bugs in the multi-pack-index implementation. These are handled by the first two patches.

I have set up a CI build on Azure Pipelines [1] that runs the test suite with a few optional features enabled, including GIT_TEST_MULTI_PACK_INDEX and GIT_TEST_COMMIT_GRAPH. I'll use this to watch the features and ensure they work well with the rest of the ongoing work. Eventually, we can add these variables to the Travis CI scripts.

[1] https://git.visualstudio.com/git/_build?definitionId=4

Derrick Stolee (3):
midx: fix broken free() in close_midx()
midx: close multi-pack-index on repack
multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX

 builtin/repack.c            |  8 ++++++++
 midx.c                      | 17 +++++++++++++----
 midx.h                      |  4 ++++
 t/README                    |  4 ++++
 t/t5310-pack-bitmaps.sh     |  1 +
 t/t5319-multi-pack-index.sh |  2 +-
 t/t9300-fast-import.sh      |  2 +-
 7 files changed, 32 insertions(+), 6 deletions(-)

base-commit: f84b9b09d40408cf91bbc500d9f190a7866c3e0f
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-27%2Fderrickstolee%2Fmidx-test%2Fupstream-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-27/derrickstolee/midx-test/upstream-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/27

gitgitgadget

From: Derrick Stolee <[email protected]>

When closing a multi-pack-index, we intend to close each pack-file and free the struct packed_git that represents it. However, this line was previously freeing the array of pointers, not the pointer itself. This leads to a double-free issue.

---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index f3e8dbc108..999717b96f 100644
@@ -190,7 +190,7 @@ static void close_midx(struct multi_pack_index *m)
 	for (i = 0; i < m->num_packs; i++) {
 		if (m->packs[i]) {
 			close_pack(m->packs[i]);
-			free(m->packs);
+			free(m->packs[i]);
 		}
 	}
 	FREE_AND_NULL(m->packs);

gitgitgadget

From: Derrick Stolee <[email protected]>

When repacking, we may remove pack-files. This invalidates the multi-pack-index (if it exists). Previously, we removed the multi-pack-index file before removing any pack-file. In some cases, the repack command may load the multi-pack-index into memory. This may lead to later in-memory references to the non-existent pack- files.

---
 builtin/repack.c | 4 ++++
 midx.c           | 6 +++++-
 midx.h           | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)

index c6a7943d5c..7925bb976e 100644
@@ -432,6 +432,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 			if (!midx_cleared) {
 				/* if we move a packfile, it will invalidated the midx */
+				if (the_repository->objects) {
+					close_midx(the_repository->objects->multi_pack_index);
+					the_repository->objects->multi_pack_index = NULL;
+				}
 				clear_midx_file(get_object_directory());
 				midx_cleared = 1;
 			}

index 999717b96f..fe8532a9d1 100644
@@ -180,9 +180,13 @@ cleanup_fail:
 	return NULL;
 }
 
-static void close_midx(struct multi_pack_index *m)
+void close_midx(struct multi_pack_index *m)
 {
 	uint32_t i;
+
+	if (!m)
+		return;
+
 	munmap((unsigned char *)m->data, m->data_len);
 	close(m->fd);
 	m->fd = -1;

index a210f1af2a..af6b5cb58f 100644
@@ -44,4 +44,6 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
 int write_midx_file(const char *object_dir);
 void clear_midx_file(const char *object_dir);
 
+void close_midx(struct multi_pack_index *m);
+
 #endif

gitgitgadget

From: Derrick Stolee <[email protected]>

The multi-pack-index feature is tested in isolation by t5319-multi-pack-index.sh, but there are many more interesting scenarios in the test suite surrounding pack-file data shapes and interactions. Since the multi-pack-index is an optional data structure, it does not make sense to include it by default in those tests.

Instead, add a new GIT_TEST_MULTI_PACK_INDEX environment variable that enables core.multiPackIndex and writes a multi-pack-index after each 'git repack' command. This adds extra test coverage when needed.

There are a few spots in the test suite that need to react to this change:

* t5319-multi-pack-index.sh: there is a test that checks that 'git repack' deletes the multi-pack-index. Disable the environment variable to ensure this still happens.

* t5310-pack-bitmaps.sh: One test moves a pack-file from the object directory to an alternate. This breaks the multi-pack-index, so delete the multi-pack-index at this point, if it exists.

* t9300-fast-import.sh: One test verifies the number of files in the .git/objects/pack directory is exactly 8. Exclude the multi-pack-index from this count so it is still 8 in all cases.

---
 builtin/repack.c            | 4 ++++
 midx.c                      | 9 +++++++--
 midx.h                      | 2 ++
 t/README                    | 4 ++++
 t/t5310-pack-bitmaps.sh     | 1 +
 t/t5319-multi-pack-index.sh | 2 +-
 t/t9300-fast-import.sh      | 2 +-
 7 files changed, 20 insertions(+), 4 deletions(-)

index 7925bb976e..418442bfe2 100644
@@ -558,6 +558,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (!no_update_server_info)
 		update_server_info(0);
 	remove_temporary_files();
+
+	if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))
+		write_midx_file(get_object_directory());
+
 	string_list_clear(&names, 0);
 	string_list_clear(&rollback, 0);
 	string_list_clear(&existing_packs, 0);

index fe8532a9d1..aeafb58fa3 100644
@@ -338,9 +338,14 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
 	struct multi_pack_index *m;
 	struct multi_pack_index *m_search;
 	int config_value;
+	static int env_value = -1;
 
-	if (repo_config_get_bool(r, "core.multipackindex", &config_value) ||
-	    !config_value)
+	if (env_value < 0)
+		env_value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);
+
+	if (!env_value &&
+	    (repo_config_get_bool(r, "core.multipackindex", &config_value) ||
+	    !config_value))
 		return 0;
 
 	for (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next)

index af6b5cb58f..bec8f73d28 100644
@@ -3,6 +3,8 @@
 
 #include "repository.h"
 
+#define GIT_TEST_MULTI_PACK_INDEX "GIT_TEST_MULTI_PACK_INDEX"
+
 struct multi_pack_index {
 	struct multi_pack_index *next;
 

index 3ea6c85460..9d0277c338 100644
@@ -327,6 +327,10 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to
 be written after every 'git commit' command, and overrides the
 'core.commitGraph' setting to true.
 
+GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
+index to be written after every 'git repack' command, and overrides the
+'core.multiPackIndex' setting to true.
+
 Naming Tests
 ------------
 

index 1be3459c5b..82d7f7f6a5 100755
@@ -191,6 +191,7 @@ test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pa
 
 test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
 	mv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ &&
+	rm -f .git/objects/pack/multi-pack-index &&
 	test_when_finished "mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/" &&
 	echo HEAD | git pack-objects --local --stdout --revs >3b.pack &&
 	git index-pack 3b.pack &&

index 6f56b38674..4024ff9a39 100755
@@ -152,7 +152,7 @@ compare_results_with_midx "twelve packs"
 
 test_expect_success 'repack removes multi-pack-index' '
 	test_path_is_file $objdir/pack/multi-pack-index &&
-	git repack -adf &&
+	GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&
 	test_path_is_missing $objdir/pack/multi-pack-index
 '
 

index 40fe7e4976..59a13b6a77 100755
@@ -1558,7 +1558,7 @@ test_expect_success 'O: blank lines not necessary after other commands' '
 	INPUT_END
 
 	git fast-import <input &&
-	test 8 = $(find .git/objects/pack -type f | wc -l) &&
+	test 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) &&
 	test $(git rev-parse refs/tags/O3-2nd) = $(git rev-parse O3^) &&
 	git log --reverse --pretty=oneline O3 | sed s/^.*z// >actual &&
 	test_cmp expect actual

gitgitgadget

You're right. It did feel a bit wrong. In v2, I'll replace this commit with a refactor of clear_midx_file() to do that. One tricky part is that we need to clear the file even if the in-memory struct hasn't been initialized, but I think passing a repository will suffice for that.

CC Stefan: Is there a plan to make get_object_directory() take a repository parameter?

Thanks,

-Stolee

Not an immediate plan. Regarding the large refactorings I am mostly waiting for nd/the-index to stabilize (which may be stable enough by now) before proceeding. Specifically 2abf350385 (revision.c: remove implicit dependency on the_index, 2018-09-21) is a missing piece that I want to build on.

My next step is to look into making use of the refactorings that we did over the last year, so I'd rather look into more submodule code again.

To come back to your question:

    git grep get_object_directory |wc -l
    30
    git grep "objects->objectdir" |wc -l
    11

2 out of the 11 matches are from the implementation of get_object_directory

So either we'd actually teach get_object_directory to take a repository and inspect the 11 occurrences of direct access to objects->objectdir or we'd view get_object_directory() as a fancy wrapper, that we might want to drop eventually.

To increase coverage of the multi-pack-index feature, add a GIT_TEST_MULTI_PACK_INDEX environment variable similar to other GIT_TEST_* variables.

After creating the environment variable and running the test suite with it enabled, I found a few bugs in the multi-pack-index implementation. These are handled by the first two patches.

I have set up a CI build on Azure Pipelines [1] that runs the test suite with a few optional features enabled, including GIT_TEST_MULTI_PACK_INDEX and GIT_TEST_COMMIT_GRAPH. I'll use this to watch the features and ensure they work well with the rest of the ongoing work. Eventually, we can add these variables to the Travis CI scripts.

[1] https://git.visualstudio.com/git/_build?definitionId=4

Derrick Stolee (3):
midx: fix broken free() in close_midx()
midx: close multi-pack-index on repack
multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX

 builtin/repack.c            |  7 +++++--
 midx.c                      | 26 ++++++++++++++++++++------
 midx.h                      |  6 +++++-
 t/README                    |  4 ++++
 t/t5310-pack-bitmaps.sh     |  1 +
 t/t5319-multi-pack-index.sh |  2 +-
 t/t9300-fast-import.sh      |  2 +-
 7 files changed, 37 insertions(+), 11 deletions(-)

base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-27%2Fderrickstolee%2Fmidx-test%2Fupstream-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-27/derrickstolee/midx-test/upstream-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/27

Range-diff vs v1:

 1:  9fcbbe336d = 1:  8bd672fe26 midx: fix broken free() in close_midx()
 2:  725ebadc92 ! 2:  2d8f26679d midx: close multi-pack-index on repack
     @@ -15,16 +15,15 @@
      --- a/builtin/repack.c
      +++ b/builtin/repack.c
      @@
     + 			char *fname, *fname_old;
       
       			if (!midx_cleared) {
     - 				/* if we move a packfile, it will invalidated the midx */
     -+				if (the_repository->objects) {
     -+					close_midx(the_repository->objects->multi_pack_index);
     -+					the_repository->objects->multi_pack_index = NULL;
     -+				}
     - 				clear_midx_file(get_object_directory());
     +-				/* if we move a packfile, it will invalidated the midx */
     +-				clear_midx_file(get_object_directory());
     ++				clear_midx_file(the_repository);
       				midx_cleared = 1;
       			}
     + 
      
      diff --git a/midx.c b/midx.c
      --- a/midx.c
     @@ -44,13 +43,34 @@
       	munmap((unsigned char *)m->data, m->data_len);
       	close(m->fd);
       	m->fd = -1;
     +@@
     + 	return 0;
     + }
     + 
     +-void clear_midx_file(const char *object_dir)
     ++void clear_midx_file(struct repository *r)
     + {
     +-	char *midx = get_midx_filename(object_dir);
     ++	char *midx = get_midx_filename(r->objects->objectdir);
     ++
     ++	if (r->objects && r->objects->multi_pack_index) {
     ++		close_midx(r->objects->multi_pack_index);
     ++		r->objects->multi_pack_index = NULL;
     ++	}
     + 
     + 	if (remove_path(midx)) {
     + 		UNLEAK(midx);
      
      diff --git a/midx.h b/midx.h
      --- a/midx.h
      +++ b/midx.h
      @@
     + int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);
     + 
       int write_midx_file(const char *object_dir);
     - void clear_midx_file(const char *object_dir);
     +-void clear_midx_file(const char *object_dir);
     ++void clear_midx_file(struct repository *r);
     + int verify_midx_file(const char *object_dir);
       
      +void close_midx(struct multi_pack_index *m);
      +

3: 04e3e91082 = 3: 57c64e814c multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX

gitgitgadget

From: Derrick Stolee <[email protected]>

When closing a multi-pack-index, we intend to close each pack-file and free the struct packed_git that represents it. However, this line was previously freeing the array of pointers, not the pointer itself. This leads to a double-free issue.

---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 713d6f9dde..bf1f511862 100644
@@ -186,7 +186,7 @@ static void close_midx(struct multi_pack_index *m)
 	for (i = 0; i < m->num_packs; i++) {
 		if (m->packs[i]) {
 			close_pack(m->packs[i]);
-			free(m->packs);
+			free(m->packs[i]);
 		}
 	}
 	FREE_AND_NULL(m->packs);

gitgitgadget

From: Derrick Stolee <[email protected]>

The multi-pack-index feature is tested in isolation by t5319-multi-pack-index.sh, but there are many more interesting scenarios in the test suite surrounding pack-file data shapes and interactions. Since the multi-pack-index is an optional data structure, it does not make sense to include it by default in those tests.

Instead, add a new GIT_TEST_MULTI_PACK_INDEX environment variable that enables core.multiPackIndex and writes a multi-pack-index after each 'git repack' command. This adds extra test coverage when needed.

There are a few spots in the test suite that need to react to this change:

* t5319-multi-pack-index.sh: there is a test that checks that 'git repack' deletes the multi-pack-index. Disable the environment variable to ensure this still happens.

* t5310-pack-bitmaps.sh: One test moves a pack-file from the object directory to an alternate. This breaks the multi-pack-index, so delete the multi-pack-index at this point, if it exists.

* t9300-fast-import.sh: One test verifies the number of files in the .git/objects/pack directory is exactly 8. Exclude the multi-pack-index from this count so it is still 8 in all cases.

---
 builtin/repack.c            | 4 ++++
 midx.c                      | 9 +++++++--
 midx.h                      | 2 ++
 t/README                    | 4 ++++
 t/t5310-pack-bitmaps.sh     | 1 +
 t/t5319-multi-pack-index.sh | 2 +-
 t/t9300-fast-import.sh      | 2 +-
 7 files changed, 20 insertions(+), 4 deletions(-)

index 44965cbaa3..26dcccdafc 100644
@@ -553,6 +553,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (!no_update_server_info)
 		update_server_info(0);
 	remove_temporary_files();
+
+	if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))
+		write_midx_file(get_object_directory());
+
 	string_list_clear(&names, 0);
 	string_list_clear(&rollback, 0);
 	string_list_clear(&existing_packs, 0);

index 22247a30ab..02b2211e31 100644
@@ -335,9 +335,14 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i
 	struct multi_pack_index *m;
 	struct multi_pack_index *m_search;
 	int config_value;
+	static int env_value = -1;
 
-	if (repo_config_get_bool(r, "core.multipackindex", &config_value) ||
-	    !config_value)
+	if (env_value < 0)
+		env_value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);
+
+	if (!env_value &&
+	    (repo_config_get_bool(r, "core.multipackindex", &config_value) ||
+	    !config_value))
 		return 0;
 
 	for (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next)

index 0f68bccdd5..f2bb7e681c 100644
@@ -3,6 +3,8 @@
 
 #include "repository.h"
 
+#define GIT_TEST_MULTI_PACK_INDEX "GIT_TEST_MULTI_PACK_INDEX"
+
 struct multi_pack_index {
 	struct multi_pack_index *next;
 

index 5e48a043ce..9bfdd3004c 100644
@@ -327,6 +327,10 @@ GIT_TEST_COMMIT_GRAPH=<boolean>, when true, forces the commit-graph to
 be written after every 'git commit' command, and overrides the
 'core.commitGraph' setting to true.
 
+GIT_TEST_MULTI_PACK_INDEX=<boolean>, when true, forces the multi-pack-
+index to be written after every 'git repack' command, and overrides the
+'core.multiPackIndex' setting to true.
+
 Naming Tests
 ------------
 

index 1be3459c5b..82d7f7f6a5 100755
@@ -191,6 +191,7 @@ test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pa
 
 test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
 	mv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ &&
+	rm -f .git/objects/pack/multi-pack-index &&
 	test_when_finished "mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/" &&
 	echo HEAD | git pack-objects --local --stdout --revs >3b.pack &&
 	git index-pack 3b.pack &&

index bd8e841b81..70926b5bc0 100755
@@ -271,7 +271,7 @@ test_expect_success 'git-fsck incorrect offset' '
 
 test_expect_success 'repack removes multi-pack-index' '
 	test_path_is_file $objdir/pack/multi-pack-index &&
-	git repack -adf &&
+	GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&
 	test_path_is_missing $objdir/pack/multi-pack-index
 '
 

index 40fe7e4976..59a13b6a77 100755
@@ -1558,7 +1558,7 @@ test_expect_success 'O: blank lines not necessary after other commands' '
 	INPUT_END
 
 	git fast-import <input &&
-	test 8 = $(find .git/objects/pack -type f | wc -l) &&
+	test 8 = $(find .git/objects/pack -type f | grep -v multi-pack-index | wc -l) &&
 	test $(git rev-parse refs/tags/O3-2nd) = $(git rev-parse O3^) &&
 	git log --reverse --pretty=oneline O3 | sed s/^.*z// >actual &&
 	test_cmp expect actual

gitgitgadget

From: Derrick Stolee <[email protected]>

When repacking, we may remove pack-files. This invalidates the multi-pack-index (if it exists). Previously, we removed the multi-pack-index file before removing any pack-file. In some cases, the repack command may load the multi-pack-index into memory. This may lead to later in-memory references to the non-existent pack- files.

---
 builtin/repack.c |  3 +--
 midx.c           | 15 ++++++++++++---
 midx.h           |  4 +++-
 3 files changed, 16 insertions(+), 6 deletions(-)

index c6a7943d5c..44965cbaa3 100644
@@ -431,8 +431,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 			char *fname, *fname_old;
 
 			if (!midx_cleared) {
-				/* if we move a packfile, it will invalidated the midx */
-				clear_midx_file(get_object_directory());
+				clear_midx_file(the_repository);
 				midx_cleared = 1;
 			}
 

index bf1f511862..22247a30ab 100644
@@ -176,9 +176,13 @@ cleanup_fail:
 	return NULL;
 }
 
-static void close_midx(struct multi_pack_index *m)
+void close_midx(struct multi_pack_index *m)
 {
 	uint32_t i;
+
+	if (!m)
+		return;
+
 	munmap((unsigned char *)m->data, m->data_len);
 	close(m->fd);
 	m->fd = -1;

@@ -914,9 +918,14 @@ cleanup:
 	return 0;
 }
 
-void clear_midx_file(const char *object_dir)
+void clear_midx_file(struct repository *r)
 {
-	char *midx = get_midx_filename(object_dir);
+	char *midx = get_midx_filename(r->objects->objectdir);
+
+	if (r->objects && r->objects->multi_pack_index) {
+		close_midx(r->objects->multi_pack_index);
+		r->objects->multi_pack_index = NULL;
+	}
 
 	if (remove_path(midx)) {
 		UNLEAK(midx);

index ce80b91c68..0f68bccdd5 100644
@@ -42,7 +42,9 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name);
 int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);
 
 int write_midx_file(const char *object_dir);
-void clear_midx_file(const char *object_dir);
+void clear_midx_file(struct repository *r);
 int verify_midx_file(const char *object_dir);
 
+void close_midx(struct multi_pack_index *m);
+
 #endif

gitgitgadget

I should explicitly mention that this base commit is different as otherwise I will conflict with ds/multi-pack-verify with the new prototype in midx.h.

Thanks,
-Stolee

There indeed is a tiny textual conflict, and in this case it may not matter that much, but please make it a habit to refrain from doing such a rebase in general. It makes it impossible to compare the new round in the same context that the old round was inspected and has been tested, unless such a textual conflict avoidance is undone.

A good rule of thumb is to build on the same base, attempt a trial merge to 'master' (and 'next' and 'pu' if you are inclined to), and see how bad a conflict you get. And if the conflict is something you can trivially resolve and the resolution would bring the code to the same state as you would get if you rebased, then you are better off not rebasing and let the maintainer deal with the merge. You cannot control what other contributor would do to the code while you are working on it, so having to resolve these tiny textual conflicts is not "an unnecessary added burden" to me (having to backport to see the new round in the same context as the old round is, though).

Of course, if you truly depend on some recent addition that happend since your old base, please do not hesitate to rebase.

Thanks.