midx: avoid opening multiple MIDXs when writing

Opening multiple instance of the same MIDX can lead to problems like two
separate packed_git structures which represent the same pack being added
to the repository's object store.

The above scenario can happen because prepare_midx_pack() checks if
`m->packs[pack_int_id]` is NULL in order to determine if a pack has been
opened and installed in the repository before. But a caller can
construct two copies of the same MIDX by calling get_multi_pack_index()
and load_multi_pack_index() since the former manipulates the
object store directly but the latter is a lower-level routine which
allocates a new MIDX for each call.

So if prepare_midx_pack() is called on multiple MIDXs with the same
pack_int_id, then that pack will be installed twice in the object
store's packed_git pointer.

This can lead to problems in, for e.g., the pack-bitmap code, which does
something like the following (in pack-bitmap.c:open_pack_bitmap()):

    struct bitmap_index *bitmap_git = ...;
    for (p = get_all_packs(r); p; p = p->next) {
      if (open_pack_bitmap_1(bitmap_git, p) == 0)
        ret = 0;
    }

which is a problem if two copies of the same pack exist in the
packed_git list because pack-bitmap.c:open_pack_bitmap_1() contains a
conditional like the following:

    if (bitmap_git->pack || bitmap_git->midx) {
      /* ignore extra bitmap file; we can only handle one */
      warning("ignoring extra bitmap file: %s", packfile->pack_name);
      close(fd);
      return -1;
    }

Avoid this scenario by not letting write_midx_internal() open a MIDX
that isn't also pointed at by the object store. So long as this is the
case, other routines should prefer to open MIDXs with
get_multi_pack_index() or reprepare_packed_git() instead of creating
instances on their own. Because get_multi_pack_index() returns
`r->object_store->multi_pack_index` if it is non-NULL, we'll only have
one instance of a MIDX open at one time, avoiding these problems.

To encourage this, drop the `struct multi_pack_index *` parameter from
`write_midx_internal()`, and rely instead on the `object_dir` to find
(or initialize) the correct MIDX instance.

Likewise, replace the call to `close_midx()` with
`close_object_store()`, since we're about to replace the MIDX with a new
one and should invalidate the object store's memory of any MIDX that
might have existed beforehand.

Note that this now forbids passing object directories that don't belong
to alternate repositories over `--object-dir`, since before we would
have happily opened a MIDX in any directory, but now restrict ourselves
to only those reachable by `r->objects->multi_pack_index` (and alternate
MIDXs that we can see by walking the `next` pointer).

As far as I can tell, supporting arbitrary directories with
`--object-dir` was a historical accident, since even the documentation
says `<alt>` when referring to the value passed to this option.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
index c9b063d31e1df8ab8a0e0d26fdf5c963ae9a1b54..0af6beb2dd137a8f3e3d0bd4ce58f5c1480a57ba 100644 (file)
--- a/Documentation/git-multi-pack-index.txt
+++ b/Documentation/git-multi-pack-index.txt
@@ -23,6 +23,8 @@ OPTIONS
        Use given directory for the location of Git objects. We check
        `<dir>/packs/multi-pack-index` for the current MIDX file, and
        `<dir>/packs` for the pack-files to index.
++
+`<dir>` must be an alternate of the current repository.
 
 --[no-]progress::
        Turn progress on/off explicitly. If neither is specified, progress is

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index cd8631522167e1c1f23da9038bf58ce87c17fa94..003eaaac5cfebc448dca184dde6183740237fdf2 100644 (file)
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -43,28 +43,6 @@ static struct opts_commit_graph {
        int enable_changed_paths;
 } opts;
 
-static struct object_directory *find_odb(struct repository *r,
-                                        const char *obj_dir)
-{
-       struct object_directory *odb;
-       char *obj_dir_real = real_pathdup(obj_dir, 1);
-       struct strbuf odb_path_real = STRBUF_INIT;
-
-       prepare_alt_odb(r);
-       for (odb = r->objects->odb; odb; odb = odb->next) {
-               strbuf_realpath(&odb_path_real, odb->path, 1);
-               if (!strcmp(obj_dir_real, odb_path_real.buf))
-                       break;
-       }
-
-       free(obj_dir_real);
-       strbuf_release(&odb_path_real);
-
-       if (!odb)
-               die(_("could not find object directory matching %s"), obj_dir);
-       return odb;
-}
-
 static int graph_verify(int argc, const char **argv)
 {
        struct commit_graph *graph = NULL;

diff --git a/midx.c b/midx.c
index e83f22b5ee908d98541410ad2d43e45558cd99e0..25906044ffd9cdeb3710e58fa7e52cdd08b7b8a2 100644 (file)
--- a/midx.c
+++ b/midx.c
@@ -893,7 +893,7 @@ static int midx_checksum_valid(struct multi_pack_index *m)
        return hashfile_checksum_valid(m->data, m->data_len);
 }
 
-static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,
+static int write_midx_internal(const char *object_dir,
                               struct string_list *packs_to_drop,
                               const char *preferred_pack_name,
                               unsigned flags)
@@ -904,20 +904,26 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
        struct hashfile *f = NULL;
        struct lock_file lk;
        struct write_midx_context ctx = { 0 };
+       struct multi_pack_index *cur;
        int pack_name_concat_len = 0;
        int dropped_packs = 0;
        int result = 0;
        struct chunkfile *cf;
 
+       /* Ensure the given object_dir is local, or a known alternate. */
+       find_odb(the_repository, object_dir);
+
        midx_name = get_midx_filename(object_dir);
        if (safe_create_leading_directories(midx_name))
                die_errno(_("unable to create leading directories of %s"),
                          midx_name);
 
-       if (m)
-               ctx.m = m;
-       else
-               ctx.m = load_multi_pack_index(object_dir, 1);
+       for (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) {
+               if (!strcmp(object_dir, cur->object_dir)) {
+                       ctx.m = cur;
+                       break;
+               }
+       }
 
        if (ctx.m && !midx_checksum_valid(ctx.m)) {
                warning(_("ignoring existing multi-pack-index; checksum mismatch"));
@@ -1119,7 +1125,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
        f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 
        if (ctx.m)
-               close_midx(ctx.m);
+               close_object_store(the_repository->objects);
 
        if (ctx.nr - dropped_packs == 0) {
                error(_("no pack files to index."));
@@ -1182,8 +1188,7 @@ int write_midx_file(const char *object_dir,
                    const char *preferred_pack_name,
                    unsigned flags)
 {
-       return write_midx_internal(object_dir, NULL, NULL, preferred_pack_name,
-                                  flags);
+       return write_midx_internal(object_dir, NULL, preferred_pack_name, flags);
 }
 
 struct clear_midx_data {
@@ -1461,8 +1466,10 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
 
        free(count);
 
-       if (packs_to_drop.nr)
-               result = write_midx_internal(object_dir, m, &packs_to_drop, NULL, flags);
+       if (packs_to_drop.nr) {
+               result = write_midx_internal(object_dir, &packs_to_drop, NULL, flags);
+               m = NULL;
+       }
 
        string_list_clear(&packs_to_drop, 0);
        return result;
@@ -1651,7 +1658,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
                goto cleanup;
        }
 
-       result = write_midx_internal(object_dir, m, NULL, NULL, flags);
+       result = write_midx_internal(object_dir, NULL, NULL, flags);
        m = NULL;
 
 cleanup:

diff --git a/object-file.c b/object-file.c
index a8be8994814933e9d1afaae77edb93ec9934a3ac..a4d720b4f5c5c0075adba594064e8180219b600e 100644 (file)
--- a/object-file.c
+++ b/object-file.c
@@ -820,6 +820,27 @@ out:
        return ref_git;
 }
 
+struct object_directory *find_odb(struct repository *r, const char *obj_dir)
+{
+       struct object_directory *odb;
+       char *obj_dir_real = real_pathdup(obj_dir, 1);
+       struct strbuf odb_path_real = STRBUF_INIT;
+
+       prepare_alt_odb(r);
+       for (odb = r->objects->odb; odb; odb = odb->next) {
+               strbuf_realpath(&odb_path_real, odb->path, 1);
+               if (!strcmp(obj_dir_real, odb_path_real.buf))
+                       break;
+       }
+
+       free(obj_dir_real);
+       strbuf_release(&odb_path_real);
+
+       if (!odb)
+               die(_("could not find object directory matching %s"), obj_dir);
+       return odb;
+}
+
 static void fill_alternate_refs_command(struct child_process *cmd,
                                        const char *repo_path)
 {

diff --git a/object-store.h b/object-store.h
index d24915ced1b2dd2aedb8300741d83372d9ea53a0..250aa5f33cfa4fedb2f5c773f704691e1dda3262 100644 (file)
--- a/object-store.h
+++ b/object-store.h
@@ -38,6 +38,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,
 
 void prepare_alt_odb(struct repository *r);
 char *compute_alternate_path(const char *path, struct strbuf *err);
+struct object_directory *find_odb(struct repository *r, const char *obj_dir);
 typedef int alt_odb_fn(struct object_directory *, void *);
 int foreach_alt_odb(alt_odb_fn, void*);
 typedef void alternate_ref_fn(const struct object_id *oid, void *);

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index d7e4988f2bd3669dcb6cad6c8c4c6c941379de41..bd09c3194bd123a2b3c246ce291406067c05f98e 100755 (executable)
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -582,7 +582,15 @@ test_expect_success 'force some 64-bit offsets with pack-objects' '
        idx64=objects64/pack/test-64-$pack64.idx &&
        chmod u+w $idx64 &&
        corrupt_data $idx64 $(test_oid idxoff) "\02" &&
-       midx64=$(git multi-pack-index --object-dir=objects64 write) &&
+       # objects64 is not a real repository, but can serve as an alternate
+       # anyway so we can write a MIDX into it
+       git init repo &&
+       test_when_finished "rm -fr repo" &&
+       (
+               cd repo &&
+               ( cd ../objects64 && pwd ) >.git/objects/info/alternates &&
+               midx64=$(git multi-pack-index --object-dir=../objects64 write)
+       ) &&
        midx_read_expect 1 63 5 objects64 " large-offsets"
 '