| Message ID | pull.27.v2.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add GIT_TEST_MULTI_PACK_INDEX environment variable | expand |
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
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.
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