* [PATCH 1/3] midx: fix broken free() in close_midx()
2018-10-08 15:17 [PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
@ 2018-10-08 15:17 ` Derrick Stolee via GitGitGadget
2018-10-09 9:07 ` Junio C Hamano
2018-10-08 15:17 ` [PATCH 2/3] midx: close multi-pack-index on repack Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-08 15:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
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);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/3] midx: fix broken free() in close_midx()
2018-10-08 15:17 ` [PATCH 1/3] midx: fix broken free() in close_midx() Derrick Stolee via GitGitGadget
@ 2018-10-09 9:07 ` Junio C Hamano
0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-10-09 9:07 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git, Derrick Stolee
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] midx: close multi-pack-index on repack
2018-10-08 15:17 [PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
2018-10-08 15:17 ` [PATCH 1/3] midx: fix broken free() in close_midx() Derrick Stolee via GitGitGadget
@ 2018-10-08 15:17 ` Derrick Stolee via GitGitGadget
2018-10-09 9:10 ` Junio C Hamano
2018-10-08 15:17 ` [PATCH 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
3 siblings, 1 reply; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-08 15:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
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(-)
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] midx: close multi-pack-index on repack
2018-10-08 15:17 ` [PATCH 2/3] midx: close multi-pack-index on repack Derrick Stolee via GitGitGadget
@ 2018-10-09 9:10 ` Junio C Hamano
2018-10-09 14:11 ` Derrick Stolee
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2018-10-09 9:10 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git, Derrick Stolee
"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?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] midx: close multi-pack-index on repack
2018-10-09 9:10 ` Junio C Hamano
@ 2018-10-09 14:11 ` Derrick Stolee
2018-10-09 18:15 ` Stefan Beller
0 siblings, 1 reply; 14+ messages in thread
From: Derrick Stolee @ 2018-10-09 14:11 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget
Cc: git, Derrick Stolee, Stefan Beller
On 10/9/2018 5:10 AM, Junio C Hamano wrote:
> "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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/3] midx: close multi-pack-index on repack
2018-10-09 14:11 ` Derrick Stolee
@ 2018-10-09 18:15 ` Stefan Beller
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Beller @ 2018-10-09 18:15 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Junio C Hamano, gitgitgadget, git, Derrick Stolee
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX
2018-10-08 15:17 [PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
2018-10-08 15:17 ` [PATCH 1/3] midx: fix broken free() in close_midx() Derrick Stolee via GitGitGadget
2018-10-08 15:17 ` [PATCH 2/3] midx: close multi-pack-index on repack Derrick Stolee via GitGitGadget
@ 2018-10-08 15:17 ` Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
3 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-08 15:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
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(-)
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable
2018-10-08 15:17 [PATCH 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2018-10-08 15:17 ` [PATCH 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX Derrick Stolee via GitGitGadget
@ 2018-10-12 17:34 ` Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 1/3] midx: fix broken free() in close_midx() Derrick Stolee via GitGitGadget
` (3 more replies)
3 siblings, 4 replies; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-12 17:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v2 1/3] midx: fix broken free() in close_midx()
2018-10-12 17:34 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
@ 2018-10-12 17:34 ` Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 2/3] midx: close multi-pack-index on repack Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-12 17:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
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 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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/3] midx: close multi-pack-index on repack
2018-10-12 17:34 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 1/3] midx: fix broken free() in close_midx() Derrick Stolee via GitGitGadget
@ 2018-10-12 17:34 ` Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX Derrick Stolee via GitGitGadget
2018-10-12 17:41 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee
3 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-12 17:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
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(-)
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX
2018-10-12 17:34 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 1/3] midx: fix broken free() in close_midx() Derrick Stolee via GitGitGadget
2018-10-12 17:34 ` [PATCH v2 2/3] midx: close multi-pack-index on repack Derrick Stolee via GitGitGadget
@ 2018-10-12 17:34 ` Derrick Stolee via GitGitGadget
2018-10-12 17:41 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee
3 siblings, 0 replies; 14+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-10-12 17:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Derrick Stolee
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(-)
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable
2018-10-12 17:34 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2018-10-12 17:34 ` [PATCH v2 3/3] multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX Derrick Stolee via GitGitGadget
@ 2018-10-12 17:41 ` Derrick Stolee
2018-10-22 1:41 ` Junio C Hamano
3 siblings, 1 reply; 14+ messages in thread
From: Derrick Stolee @ 2018-10-12 17:41 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git; +Cc: Junio C Hamano
On 10/12/2018 1:34 PM, Derrick Stolee via GitGitGadget wrote:
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable
2018-10-12 17:41 ` [PATCH v2 0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable Derrick Stolee
@ 2018-10-22 1:41 ` Junio C Hamano
0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-10-22 1:41 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Derrick Stolee via GitGitGadget, git
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.
^ permalink raw reply [flat|nested] 14+ messages in thread