[v2,0/3] Add GIT_TEST_MULTI_PACK_INDEX environment variable
mbox series

Message ID pull.27.v2.git.gitgitgadget@gmail.com
Headers show
Series
  • Add GIT_TEST_MULTI_PACK_INDEX environment variable
Related show

Message

Derrick Stolee via GitGitGadget Oct. 12, 2018, 5:34 p.m. UTC
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

Comments

Derrick Stolee Oct. 12, 2018, 5:41 p.m. UTC | #1
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
Junio C Hamano Oct. 22, 2018, 1:41 a.m. UTC | #2
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.