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>

Taylor Blau committed Sep 1, 2021 at 16:34 UTC f57a739691e2efa1d71851cd725154a5a760d8f4
6 files changed +51 -34
Documentation/git-multi-pack-index.txt
+2
@@ -23,6 +23,8 @@ OPTIONS
23 Use given directory for the location of Git objects. We check
24 `<dir>/packs/multi-pack-index` for the current MIDX file, and
25 `<dir>/packs` for the pack-files to index.
26 ++
27 +`<dir>` must be an alternate of the current repository.
28
29 --[no-]progress::
30 Turn progress on/off explicitly. If neither is specified, progress is
builtin/commit-graph.c
-22
@@ -43,28 +43,6 @@ static struct opts_commit_graph {
43 int enable_changed_paths;
44 } opts;
45
46 -static struct object_directory *find_odb(struct repository *r,
47 - const char *obj_dir)
48 -{
49 - struct object_directory *odb;
50 - char *obj_dir_real = real_pathdup(obj_dir, 1);
51 - struct strbuf odb_path_real = STRBUF_INIT;
52 -
53 - prepare_alt_odb(r);
54 - for (odb = r->objects->odb; odb; odb = odb->next) {
55 - strbuf_realpath(&odb_path_real, odb->path, 1);
56 - if (!strcmp(obj_dir_real, odb_path_real.buf))
57 - break;
58 - }
59 -
60 - free(obj_dir_real);
61 - strbuf_release(&odb_path_real);
62 -
63 - if (!odb)
64 - die(_("could not find object directory matching %s"), obj_dir);
65 - return odb;
66 -}
67 -
46 static int graph_verify(int argc, const char **argv)
47 {
48 struct commit_graph *graph = NULL;
midx.c
+18 -11
@@ -893,7 +893,7 @@ static int midx_checksum_valid(struct multi_pack_index *m)
893 return hashfile_checksum_valid(m->data, m->data_len);
894 }
895
896 -static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,
896 +static int write_midx_internal(const char *object_dir,
897 struct string_list *packs_to_drop,
898 const char *preferred_pack_name,
899 unsigned flags)
@@ -904,20 +904,26 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *
904 struct hashfile *f = NULL;
905 struct lock_file lk;
906 struct write_midx_context ctx = { 0 };
907 + struct multi_pack_index *cur;
908 int pack_name_concat_len = 0;
909 int dropped_packs = 0;
910 int result = 0;
911 struct chunkfile *cf;
912
913 + /* Ensure the given object_dir is local, or a known alternate. */
914 + find_odb(the_repository, object_dir);
915 +
916 midx_name = get_midx_filename(object_dir);
917 if (safe_create_leading_directories(midx_name))
918 die_errno(_("unable to create leading directories of %s"),
919 midx_name);
920
917 - if (m)
918 - ctx.m = m;
919 - else
920 - ctx.m = load_multi_pack_index(object_dir, 1);
921 + for (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) {
922 + if (!strcmp(object_dir, cur->object_dir)) {
923 + ctx.m = cur;
924 + break;
925 + }
926 + }
927
928 if (ctx.m && !midx_checksum_valid(ctx.m)) {
929 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 *
1125 f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
1126
1127 if (ctx.m)
1122 - close_midx(ctx.m);
1128 + close_object_store(the_repository->objects);
1129
1130 if (ctx.nr - dropped_packs == 0) {
1131 error(_("no pack files to index."));
@@ -1182,8 +1188,7 @@ int write_midx_file(const char *object_dir,
1188 const char *preferred_pack_name,
1189 unsigned flags)
1190 {
1185 - return write_midx_internal(object_dir, NULL, NULL, preferred_pack_name,
1186 - flags);
1191 + return write_midx_internal(object_dir, NULL, preferred_pack_name, flags);
1192 }
1193
1194 struct clear_midx_data {
@@ -1461,8 +1466,10 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
1466
1467 free(count);
1468
1464 - if (packs_to_drop.nr)
1465 - result = write_midx_internal(object_dir, m, &packs_to_drop, NULL, flags);
1469 + if (packs_to_drop.nr) {
1470 + result = write_midx_internal(object_dir, &packs_to_drop, NULL, flags);
1471 + m = NULL;
1472 + }
1473
1474 string_list_clear(&packs_to_drop, 0);
1475 return result;
@@ -1651,7 +1658,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
1658 goto cleanup;
1659 }
1660
1654 - result = write_midx_internal(object_dir, m, NULL, NULL, flags);
1661 + result = write_midx_internal(object_dir, NULL, NULL, flags);
1662 m = NULL;
1663
1664 cleanup:
object-file.c
+21
@@ -820,6 +820,27 @@ out:
820 return ref_git;
821 }
822
823 +struct object_directory *find_odb(struct repository *r, const char *obj_dir)
824 +{
825 + struct object_directory *odb;
826 + char *obj_dir_real = real_pathdup(obj_dir, 1);
827 + struct strbuf odb_path_real = STRBUF_INIT;
828 +
829 + prepare_alt_odb(r);
830 + for (odb = r->objects->odb; odb; odb = odb->next) {
831 + strbuf_realpath(&odb_path_real, odb->path, 1);
832 + if (!strcmp(obj_dir_real, odb_path_real.buf))
833 + break;
834 + }
835 +
836 + free(obj_dir_real);
837 + strbuf_release(&odb_path_real);
838 +
839 + if (!odb)
840 + die(_("could not find object directory matching %s"), obj_dir);
841 + return odb;
842 +}
843 +
844 static void fill_alternate_refs_command(struct child_process *cmd,
845 const char *repo_path)
846 {
object-store.h
+1
@@ -38,6 +38,7 @@ KHASH_INIT(odb_path_map, const char * /* key: odb_path */,
38
39 void prepare_alt_odb(struct repository *r);
40 char *compute_alternate_path(const char *path, struct strbuf *err);
41 +struct object_directory *find_odb(struct repository *r, const char *obj_dir);
42 typedef int alt_odb_fn(struct object_directory *, void *);
43 int foreach_alt_odb(alt_odb_fn, void*);
44 typedef void alternate_ref_fn(const struct object_id *oid, void *);
t/t5319-multi-pack-index.sh
+9 -1
@@ -582,7 +582,15 @@ test_expect_success 'force some 64-bit offsets with pack-objects' '
582 idx64=objects64/pack/test-64-$pack64.idx &&
583 chmod u+w $idx64 &&
584 corrupt_data $idx64 $(test_oid idxoff) "\02" &&
585 - midx64=$(git multi-pack-index --object-dir=objects64 write) &&
585 + # objects64 is not a real repository, but can serve as an alternate
586 + # anyway so we can write a MIDX into it
587 + git init repo &&
588 + test_when_finished "rm -fr repo" &&
589 + (
590 + cd repo &&
591 + ( cd ../objects64 && pwd ) >.git/objects/info/alternates &&
592 + midx64=$(git multi-pack-index --object-dir=../objects64 write)
593 + ) &&
594 midx_read_expect 1 63 5 objects64 " large-offsets"
595 '
596