threads / patch / 49507

patch, 3 partsAdd GIT_TEST_MULTI_PACK_INDEX environment variable

Subject: [PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable

## tl;dr

14 messages between Oct 8, 2018 and Oct 22, 2018. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Derrick Stolee via GitGitGadget· Oct 8, 2018, 15:17 UTC · lore

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
Derrick Stolee via GitGitGadget· Oct 8, 2018, 15:17 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 1/3] midx: fix broken free() in close_midx()

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to midx.c +1 −1
diff --git a/midx.c b/midx.c
index f3e8dbc108..999717b96f 100644
--- a/midx.c
+++ b/midx.c
@@ -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
Junio C Hamano· Oct 9, 2018, 09:07 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 1/3] midx: fix broken free() in close_midx()

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 25 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  midx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/midx.c b/midx.c
> index f3e8dbc108..999717b96f 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -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);
Yup, kinda obvious when we view it with the post context.
Derrick Stolee via GitGitGadget· Oct 8, 2018, 15:17 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 2/3] midx: close multi-pack-index on repack

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/repack.c | 4 ++++
 midx.c           | 6 +++++-
 midx.h           | 2 ++
 3 files changed, 11 insertions(+), 1 deletion(-)
Show changes to 3 files +11 −1

builtin/repack.c, midx.c, midx.h

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5c..7925bb976e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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;
 			}
diff --git a/midx.c b/midx.c
index 999717b96f..fe8532a9d1 100644
--- a/midx.c
+++ b/midx.c
@@ -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;
diff --git a/midx.h b/midx.h
index a210f1af2a..af6b5cb58f 100644
--- a/midx.h
+++ b/midx.h
@@ -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
Junio C Hamano· Oct 9, 2018, 09:10 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 2/3] midx: close multi-pack-index on repack

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 14 quoted lines
> diff --git a/builtin/repack.c b/builtin/repack.c
> index c6a7943d5c..7925bb976e 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -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;

It somehow looks like a bit of layering violation, doesn't it? When we are clearing a midx file, don't we always want to do this as well?

Derrick Stolee· Oct 9, 2018, 14:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] midx: close multi-pack-index on repack

On 10/9/2018 5:10 AM, Junio C Hamano wrote:
Show 18 quoted lines
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index c6a7943d5c..7925bb976e 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -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;
> It somehow looks like a bit of layering violation, doesn't it?  When
> we are clearing a midx file, don't we always want to do this as well?

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
Stefan Beller· Oct 9, 2018, 18:15 UTC · re: Derrick Stolee · lore

Re: [PATCH 2/3] midx: close multi-pack-index on repack

On Tue, Oct 9, 2018 at 7:11 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> CC Stefan: Is there a plan to make get_object_directory() take a
> repository parameter?

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.

Derrick Stolee via GitGitGadget· Oct 8, 2018, 15:17 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX

From: Derrick Stolee <dstolee@microsoft.com>

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.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(-)
Show changes to 7 files +20 −4

builtin/repack.c, midx.c, midx.h, t/README, t/t5310-pack-bitmaps.sh, t/t5319-multi-pack-index.sh, t/t9300-fast-import.sh

diff --git a/builtin/repack.c b/builtin/repack.c
index 7925bb976e..418442bfe2 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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);
diff --git a/midx.c b/midx.c
index fe8532a9d1..aeafb58fa3 100644
--- a/midx.c
+++ b/midx.c
@@ -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)
diff --git a/midx.h b/midx.h
index af6b5cb58f..bec8f73d28 100644
--- a/midx.h
+++ b/midx.h
@@ -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;
 
diff --git a/t/README b/t/README
index 3ea6c85460..9d0277c338 100644
--- a/t/README
+++ b/t/README
@@ -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
 ------------
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 1be3459c5b..82d7f7f6a5 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -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 &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 6f56b38674..4024ff9a39 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -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
 '
 
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 40fe7e4976..59a13b6a77 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -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
Derrick Stolee via GitGitGadget· Oct 12, 2018, 17:34 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 0/3] 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            |  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
Derrick Stolee via GitGitGadget· Oct 12, 2018, 17:34 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 1/3] midx: fix broken free() in close_midx()

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to midx.c +1 −1
diff --git a/midx.c b/midx.c
index 713d6f9dde..bf1f511862 100644
--- a/midx.c
+++ b/midx.c
@@ -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
Derrick Stolee via GitGitGadget· Oct 12, 2018, 17:34 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX

From: Derrick Stolee <dstolee@microsoft.com>

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.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(-)
Show changes to 7 files +20 −4

builtin/repack.c, midx.c, midx.h, t/README, t/t5310-pack-bitmaps.sh, t/t5319-multi-pack-index.sh, t/t9300-fast-import.sh

diff --git a/builtin/repack.c b/builtin/repack.c
index 44965cbaa3..26dcccdafc 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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);
diff --git a/midx.c b/midx.c
index 22247a30ab..02b2211e31 100644
--- a/midx.c
+++ b/midx.c
@@ -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)
diff --git a/midx.h b/midx.h
index 0f68bccdd5..f2bb7e681c 100644
--- a/midx.h
+++ b/midx.h
@@ -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;
 
diff --git a/t/README b/t/README
index 5e48a043ce..9bfdd3004c 100644
--- a/t/README
+++ b/t/README
@@ -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
 ------------
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 1be3459c5b..82d7f7f6a5 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -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 &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index bd8e841b81..70926b5bc0 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -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
 '
 
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 40fe7e4976..59a13b6a77 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -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
Derrick Stolee via GitGitGadget· Oct 12, 2018, 17:34 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 2/3] midx: close multi-pack-index on repack

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/repack.c |  3 +--
 midx.c           | 15 ++++++++++++---
 midx.h           |  4 +++-
 3 files changed, 16 insertions(+), 6 deletions(-)
Show changes to 3 files +16 −6

builtin/repack.c, midx.c, midx.h

diff --git a/builtin/repack.c b/builtin/repack.c
index c6a7943d5c..44965cbaa3 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -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;
 			}
 
diff --git a/midx.c b/midx.c
index bf1f511862..22247a30ab 100644
--- a/midx.c
+++ b/midx.c
@@ -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);
diff --git a/midx.h b/midx.h
index ce80b91c68..0f68bccdd5 100644
--- a/midx.h
+++ b/midx.h
@@ -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
Derrick Stolee· Oct 12, 2018, 17:41 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable

On 10/12/2018 1:34 PM, Derrick Stolee via GitGitGadget wrote:
Show 32 quoted lines
> 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

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

Junio C Hamano· Oct 22, 2018, 01:41 UTC · re: Derrick Stolee · lore

Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable

Derrick Stolee <stolee@gmail.com> writes:
>> base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
> 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.

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.

← back to recent threads