{"thread":{"id":"51007","subject":"[PATCH 0/2] Multi-pack-index: Fix \"too many file descriptors\" bug","startedAt":"2019-04-29T16:18:58Z","lastAt":"2019-05-08T14:22:12Z","messageCount":5,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"374647","messageId":"pull.182.git.gitgitgadget@gmail.com","threadId":"51007","inReplyTo":null,"subject":"[PATCH 0/2] Multi-pack-index: Fix \"too many file descriptors\" bug","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T16:18:54Z","receivedAt":"2019-04-29T16:18:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Thanks to Jeff H for finding the problem with the multi-pack-index regarding\nmany packs. Specifically: if we open too many packs, the close_one_pack()\nmethod cannot find the packs from the multi-pack-index to close.\n\nJeff already fixed the problem explicitly in 'git multi-pack-index verify'\nwhich would hit this issue 100% of the time we had 2000+ packs. This issue\ncould still happen in 'git rev-list --all --objects' if there are enough\npacks containing commits and trees. This series fixes the issue.\n\nThe basic solution is to add packs from the multi-pack-index into the\npacked_git struct as they are opened. To avoid performance issues, add a \nmulti_pack_index bit to the packed_git struct. Midx-aware algorithms can\nthen ignore those packs.\n\nThere was a very subtle issue that happens during a 'git repack': we clear\nthe multi-pack-index after possibly reading some packs from it, thus leaving\nsome packs in the packed_git struct but having a NULL multi_pack_index in\nthe object store. This informs the change to close_midx().\n\nI'm based on a recent 'master' commit that contains the following three\nbranches due to nearby changes causing conflicts if I pick only Jeff's\nchange as a base:\n\njh/midx-verify-too-many-packs jk/server-info-rabbit-hole\nbc/hash-transition-16\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n  midx: pass a repository pointer\n  midx: add packs to packed_git linked list\n\n builtin/multi-pack-index.c |  2 +-\n builtin/pack-objects.c     |  2 +-\n midx.c                     | 42 +++++++++++++++++++++++++-------------\n midx.h                     |  7 ++++---\n object-store.h             |  9 ++------\n packfile.c                 | 30 ++++++++-------------------\n sha1-name.c                |  6 ++++++\n 7 files changed, 51 insertions(+), 47 deletions(-)\n\n\nbase-commit: 83232e38648b51abbcbdb56c94632b6906cc85a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-182%2Fderrickstolee%2Fmany-packs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-182/derrickstolee/many-packs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/182\n-- \ngitgitgadget\n"},{"id":"374648","messageId":"3b6324df4d26f47058a714abe47b9df7811c383e.1556554734.git.gitgitgadget@gmail.com","threadId":"51007","inReplyTo":"pull.182.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] midx: pass a repository pointer","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T16:18:55Z","receivedAt":"2019-04-29T16:19:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nMuch of the multi-pack-index code focuses on the multi_pack_index\nstruct, and so we only pass a pointer to the current one. However,\nwe will insert a dependency on the packed_git linked list in a\nfuture change, so we will need a repository reference. Inserting\nthese parameters is a significant enough change to split out.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/multi-pack-index.c |  2 +-\n builtin/pack-objects.c     |  2 +-\n midx.c                     | 22 ++++++++++++++--------\n midx.h                     |  7 ++++---\n packfile.c                 |  4 ++--\n 5 files changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex ae6e476ad5..72dfd3dadc 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -46,7 +46,7 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \tif (!strcmp(argv[0], \"write\"))\n \t\treturn write_midx_file(opts.object_dir);\n \tif (!strcmp(argv[0], \"verify\"))\n-\t\treturn verify_midx_file(opts.object_dir);\n+\t\treturn verify_midx_file(the_repository, opts.object_dir);\n \n \tdie(_(\"unrecognized verb: %s\"), argv[0]);\n }\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2d9a3bdc9d..e606b9ef03 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1078,7 +1078,7 @@ static int want_object_in_pack(const struct object_id *oid,\n \n \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n \t\tstruct pack_entry e;\n-\t\tif (fill_midx_entry(oid, &e, m)) {\n+\t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n \t\t\tstruct packed_git *p = e.p;\n \t\t\toff_t offset;\n \ndiff --git a/midx.c b/midx.c\nindex d5d2e9522f..8b8faec35a 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -201,7 +201,7 @@ void close_midx(struct multi_pack_index *m)\n \tFREE_AND_NULL(m->pack_names);\n }\n \n-int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id)\n+int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id)\n {\n \tstruct strbuf pack_name = STRBUF_INIT;\n \n@@ -261,7 +261,10 @@ static uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n \treturn get_be32(m->chunk_object_offsets + pos * MIDX_CHUNK_OFFSET_WIDTH);\n }\n \n-static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *e, uint32_t pos)\n+static int nth_midxed_pack_entry(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t struct pack_entry *e,\n+\t\t\t\t uint32_t pos)\n {\n \tuint32_t pack_int_id;\n \tstruct packed_git *p;\n@@ -271,7 +274,7 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n \n \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n \n-\tif (prepare_midx_pack(m, pack_int_id))\n+\tif (prepare_midx_pack(r, m, pack_int_id))\n \t\tdie(_(\"error preparing packfile from multi-pack-index\"));\n \tp = m->packs[pack_int_id];\n \n@@ -301,14 +304,17 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n \treturn 1;\n }\n \n-int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m)\n+int fill_midx_entry(struct repository * r,\n+\t\t    const struct object_id *oid,\n+\t\t    struct pack_entry *e,\n+\t\t    struct multi_pack_index *m)\n {\n \tuint32_t pos;\n \n \tif (!bsearch_midx(oid, m, &pos))\n \t\treturn 0;\n \n-\treturn nth_midxed_pack_entry(m, e, pos);\n+\treturn nth_midxed_pack_entry(r, m, e, pos);\n }\n \n /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n@@ -1020,7 +1026,7 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)\n \t\t\tdisplay_progress(progress, _n); \\\n \t} while (0)\n \n-int verify_midx_file(const char *object_dir)\n+int verify_midx_file(struct repository *r, const char *object_dir)\n {\n \tstruct pair_pos_vs_id *pairs = NULL;\n \tuint32_t i;\n@@ -1034,7 +1040,7 @@ int verify_midx_file(const char *object_dir)\n \tprogress = start_progress(_(\"Looking for referenced packfiles\"),\n \t\t\t\t  m->num_packs);\n \tfor (i = 0; i < m->num_packs; i++) {\n-\t\tif (prepare_midx_pack(m, i))\n+\t\tif (prepare_midx_pack(r, m, i))\n \t\t\tmidx_report(\"failed to load pack in position %d\", i);\n \n \t\tdisplay_progress(progress, i + 1);\n@@ -1099,7 +1105,7 @@ int verify_midx_file(const char *object_dir)\n \n \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n \n-\t\tif (!fill_midx_entry(&oid, &e, m)) {\n+\t\tif (!fill_midx_entry(r, &oid, &e, m)) {\n \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n \t\t\t\t    pairs[i].pos, oid_to_hex(&oid));\n \t\t\tcontinue;\ndiff --git a/midx.h b/midx.h\nindex 26dd042d63..3eb29731f2 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -5,6 +5,7 @@\n \n struct object_id;\n struct pack_entry;\n+struct repository;\n \n #define GIT_TEST_MULTI_PACK_INDEX \"GIT_TEST_MULTI_PACK_INDEX\"\n \n@@ -37,18 +38,18 @@ struct multi_pack_index {\n };\n \n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);\n-int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id);\n+int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\n int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result);\n struct object_id *nth_midxed_object_oid(struct object_id *oid,\n \t\t\t\t\tstruct multi_pack_index *m,\n \t\t\t\t\tuint32_t n);\n-int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m);\n+int fill_midx_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m);\n int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name);\n int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);\n \n int write_midx_file(const char *object_dir);\n void clear_midx_file(struct repository *r);\n-int verify_midx_file(const char *object_dir);\n+int verify_midx_file(struct repository *r, const char *object_dir);\n \n void close_midx(struct multi_pack_index *m);\n \ndiff --git a/packfile.c b/packfile.c\nindex cdf6b6ec34..7b94a14726 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1035,7 +1035,7 @@ struct packed_git *get_all_packs(struct repository *r)\n \t\tfor (m = r->objects->multi_pack_index; m; m = m->next) {\n \t\t\tuint32_t i;\n \t\t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\t\tif (!prepare_midx_pack(m, i)) {\n+\t\t\t\tif (!prepare_midx_pack(r, m, i)) {\n \t\t\t\t\tm->packs[i]->next = p;\n \t\t\t\t\tp = m->packs[i];\n \t\t\t\t}\n@@ -1998,7 +1998,7 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \t\treturn 0;\n \n \tfor (m = r->objects->multi_pack_index; m; m = m->next) {\n-\t\tif (fill_midx_entry(oid, e, m))\n+\t\tif (fill_midx_entry(r, oid, e, m))\n \t\t\treturn 1;\n \t}\n \n-- \ngitgitgadget\n\n"},{"id":"374649","messageId":"6db8c5dce7073583a9bf3ab7bd7e4f9d74c015c4.1556554734.git.gitgitgadget@gmail.com","threadId":"51007","inReplyTo":"pull.182.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] midx: add packs to packed_git linked list","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-29T16:18:56Z","receivedAt":"2019-04-29T16:19:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <dstolee@microsoft.com>\n\nThe multi-pack-index allows searching for objects across multiple\npacks using one object list. The original design gains many of\nthese performance benefits by keeping the packs in the\nmulti-pack-index out of the packed_git list.\n\nUnfortunately, this has one major drawback. If the multi-pack-index\ncovers thousands of packs, and a command loads many of those packs,\nthen we can hit the limit for open file descriptors. The\nclose_one_pack() method is used to limit this resource, but it\nonly looks at the packed_git list, and uses an LRU cache to prevent\nthrashing.\n\nInstead of complicating this close_one_pack() logic to include\ndirect references to the multi-pack-index, simply add the packs\nopened by the multi-pack-index to the packed_git list. This\nimmediately solves the file-descriptor limit problem, but requires\nsome extra steps to avoid performance issues or other problems:\n\n1. Create a multi_pack_index bit in the packed_git struct that is\n   one if and only if the pack was loaded from a multi-pack-index.\n\n2. Skip packs with the multi_pack_index bit when doing object\n   lookups and abbreviations. These algorithms already check the\n   multi-pack-index before the packed_git struct. This has a very\n   small performance hit, as we need to walk more packed_git\n   structs. This is acceptable, since these operations run binary\n   search on the other packs, so this walk-and-ignore logic is\n   very fast by comparison.\n\n3. When closing a multi-pack-index file, do not close its packs,\n   as those packs will be closed using close_all_packs(). In some\n   cases, such as 'git repack', we run 'close_midx()' without also\n   closing the packs, so we need to un-set the multi_pack_index bit\n   in those packs. This is necessary, and caught by running\n   t6501-freshen-objects.sh with GIT_TEST_MULTI_PACK_INDEX=1.\n\nTo manually test this change, I inserted trace2 logging into\nclose_pack_fd() and set pack_max_fds to 10, then ran 'git rev-list\n--all --objects' on a copy of the Git repo with 300+ pack-files and\na multi-pack-index. The logs verified the packs are closed as\nwe read them beyond the file descriptor limit.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c         | 20 ++++++++++++++------\n object-store.h |  9 ++-------\n packfile.c     | 28 ++++++++--------------------\n sha1-name.c    |  6 ++++++\n 4 files changed, 30 insertions(+), 33 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 8b8faec35a..e7e1fe4d65 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -192,10 +192,8 @@ void close_midx(struct multi_pack_index *m)\n \tm->fd = -1;\n \n \tfor (i = 0; i < m->num_packs; i++) {\n-\t\tif (m->packs[i]) {\n-\t\t\tclose_pack(m->packs[i]);\n-\t\t\tfree(m->packs[i]);\n-\t\t}\n+\t\tif (m->packs[i])\n+\t\t\tm->packs[i]->multi_pack_index = 0;\n \t}\n \tFREE_AND_NULL(m->packs);\n \tFREE_AND_NULL(m->pack_names);\n@@ -204,6 +202,7 @@ void close_midx(struct multi_pack_index *m)\n int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id)\n {\n \tstruct strbuf pack_name = STRBUF_INIT;\n+\tstruct packed_git *p;\n \n \tif (pack_int_id >= m->num_packs)\n \t\tdie(_(\"bad pack-int-id: %u (%u total packs)\"),\n@@ -215,9 +214,18 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t\n \tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n \t\t    m->pack_names[pack_int_id]);\n \n-\tm->packs[pack_int_id] = add_packed_git(pack_name.buf, pack_name.len, m->local);\n+\tp = add_packed_git(pack_name.buf, pack_name.len, m->local);\n \tstrbuf_release(&pack_name);\n-\treturn !m->packs[pack_int_id];\n+\n+\tif (!p)\n+\t\treturn 1;\n+\n+\tp->multi_pack_index = 1;\n+\tm->packs[pack_int_id] = p;\n+\tinstall_packed_git(r, p);\n+\tlist_add_tail(&p->mru, &r->objects->packed_git_mru);\n+\n+\treturn 0;\n }\n \n int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result)\ndiff --git a/object-store.h b/object-store.h\nindex b086f5ecdb..7acbc7fffe 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -76,7 +76,8 @@ struct packed_git {\n \t\t pack_keep_in_core:1,\n \t\t freshened:1,\n \t\t do_not_close:1,\n-\t\t pack_promisor:1;\n+\t\t pack_promisor:1,\n+\t\t multi_pack_index:1;\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \tstruct revindex_entry *revindex;\n \t/* something like \".git/objects/pack/xxxxx.pack\" */\n@@ -128,12 +129,6 @@ struct raw_object_store {\n \t/* A most-recently-used ordered version of the packed_git list. */\n \tstruct list_head packed_git_mru;\n \n-\t/*\n-\t * A linked list containing all packfiles, starting with those\n-\t * contained in the multi_pack_index.\n-\t */\n-\tstruct packed_git *all_packs;\n-\n \t/*\n \t * A fast, rough count of the number of objects in the repository.\n \t * These two fields are not meant for direct access. Use\ndiff --git a/packfile.c b/packfile.c\nindex 7b94a14726..060de420d1 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -994,8 +994,6 @@ static void prepare_packed_git(struct repository *r)\n \t}\n \trearrange_packed_git(r);\n \n-\tr->objects->all_packs = NULL;\n-\n \tprepare_packed_git_mru(r);\n \tr->objects->packed_git_initialized = 1;\n }\n@@ -1026,26 +1024,16 @@ struct multi_pack_index *get_multi_pack_index(struct repository *r)\n \n struct packed_git *get_all_packs(struct repository *r)\n {\n-\tprepare_packed_git(r);\n-\n-\tif (!r->objects->all_packs) {\n-\t\tstruct packed_git *p = r->objects->packed_git;\n-\t\tstruct multi_pack_index *m;\n-\n-\t\tfor (m = r->objects->multi_pack_index; m; m = m->next) {\n-\t\t\tuint32_t i;\n-\t\t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\t\tif (!prepare_midx_pack(r, m, i)) {\n-\t\t\t\t\tm->packs[i]->next = p;\n-\t\t\t\t\tp = m->packs[i];\n-\t\t\t\t}\n-\t\t\t}\n-\t\t}\n+\tstruct multi_pack_index *m;\n \n-\t\tr->objects->all_packs = p;\n+\tprepare_packed_git(r);\n+\tfor (m = r->objects->multi_pack_index; m; m = m->next) {\n+\t\tuint32_t i;\n+\t\tfor (i = 0; i < m->num_packs; i++)\n+\t\t\tprepare_midx_pack(r, m, i);\n \t}\n \n-\treturn r->objects->all_packs;\n+\treturn r->objects->packed_git;\n }\n \n struct list_head *get_packed_git_mru(struct repository *r)\n@@ -2004,7 +1992,7 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n \n \tlist_for_each(pos, &r->objects->packed_git_mru) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n-\t\tif (fill_pack_entry(oid, e, p)) {\n+\t\tif (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {\n \t\t\tlist_move(&p->mru, &r->objects->packed_git_mru);\n \t\t\treturn 1;\n \t\t}\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 07c71a7567..42ac1c5bb6 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -157,6 +157,9 @@ static void unique_in_pack(struct packed_git *p,\n \tuint32_t num, i, first = 0;\n \tconst struct object_id *current = NULL;\n \n+\tif (p->multi_pack_index)\n+\t\treturn;\n+\n \tif (open_pack_index(p) || !p->num_objects)\n \t\treturn;\n \n@@ -589,6 +592,9 @@ static void find_abbrev_len_for_pack(struct packed_git *p,\n \tstruct object_id oid;\n \tconst struct object_id *mad_oid;\n \n+\tif (p->multi_pack_index)\n+\t\treturn;\n+\n \tif (open_pack_index(p) || !p->num_objects)\n \t\treturn;\n \n-- \ngitgitgadget\n"},{"id":"374999","messageId":"xmqq8svimzrz.fsf@gitster-ct.c.googlers.com","threadId":"51007","inReplyTo":"3b6324df4d26f47058a714abe47b9df7811c383e.1556554734.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] midx: pass a repository pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T08:31:28Z","receivedAt":"2019-05-07T08:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <dstolee@microsoft.com>\n>\n> Much of the multi-pack-index code focuses on the multi_pack_index\n> struct, and so we only pass a pointer to the current one. However,\n> we will insert a dependency on the packed_git linked list in a\n> future change, so we will need a repository reference. Inserting\n> these parameters is a significant enough change to split out.\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n\nThis is a good move in the loger term, but not a happy thing to do\nwhile your \"expire\" topic is still in flight, as the impact from\nupdating the signature of prepare_midx_pack() and friends will break\nnew callers in expire_midx_packs() etc.\n\nI am tempted to queue this and eject ds/midx-expire-repack for now,\nwhile checking how that topic would look like when rebased on top of\nthese two patches.  We'll see.\n\nThanks.\n\n>  builtin/multi-pack-index.c |  2 +-\n>  builtin/pack-objects.c     |  2 +-\n>  midx.c                     | 22 ++++++++++++++--------\n>  midx.h                     |  7 ++++---\n>  packfile.c                 |  4 ++--\n>  5 files changed, 22 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index ae6e476ad5..72dfd3dadc 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -46,7 +46,7 @@ int cmd_multi_pack_index(int argc, const char **argv,\n>  \tif (!strcmp(argv[0], \"write\"))\n>  \t\treturn write_midx_file(opts.object_dir);\n>  \tif (!strcmp(argv[0], \"verify\"))\n> -\t\treturn verify_midx_file(opts.object_dir);\n> +\t\treturn verify_midx_file(the_repository, opts.object_dir);\n>  \n>  \tdie(_(\"unrecognized verb: %s\"), argv[0]);\n>  }\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 2d9a3bdc9d..e606b9ef03 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1078,7 +1078,7 @@ static int want_object_in_pack(const struct object_id *oid,\n>  \n>  \tfor (m = get_multi_pack_index(the_repository); m; m = m->next) {\n>  \t\tstruct pack_entry e;\n> -\t\tif (fill_midx_entry(oid, &e, m)) {\n> +\t\tif (fill_midx_entry(the_repository, oid, &e, m)) {\n>  \t\t\tstruct packed_git *p = e.p;\n>  \t\t\toff_t offset;\n>  \n> diff --git a/midx.c b/midx.c\n> index d5d2e9522f..8b8faec35a 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -201,7 +201,7 @@ void close_midx(struct multi_pack_index *m)\n>  \tFREE_AND_NULL(m->pack_names);\n>  }\n>  \n> -int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id)\n> +int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id)\n>  {\n>  \tstruct strbuf pack_name = STRBUF_INIT;\n>  \n> @@ -261,7 +261,10 @@ static uint32_t nth_midxed_pack_int_id(struct multi_pack_index *m, uint32_t pos)\n>  \treturn get_be32(m->chunk_object_offsets + pos * MIDX_CHUNK_OFFSET_WIDTH);\n>  }\n>  \n> -static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *e, uint32_t pos)\n> +static int nth_midxed_pack_entry(struct repository *r,\n> +\t\t\t\t struct multi_pack_index *m,\n> +\t\t\t\t struct pack_entry *e,\n> +\t\t\t\t uint32_t pos)\n>  {\n>  \tuint32_t pack_int_id;\n>  \tstruct packed_git *p;\n> @@ -271,7 +274,7 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n>  \n>  \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n>  \n> -\tif (prepare_midx_pack(m, pack_int_id))\n> +\tif (prepare_midx_pack(r, m, pack_int_id))\n>  \t\tdie(_(\"error preparing packfile from multi-pack-index\"));\n>  \tp = m->packs[pack_int_id];\n>  \n> @@ -301,14 +304,17 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n>  \treturn 1;\n>  }\n>  \n> -int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m)\n> +int fill_midx_entry(struct repository * r,\n> +\t\t    const struct object_id *oid,\n> +\t\t    struct pack_entry *e,\n> +\t\t    struct multi_pack_index *m)\n>  {\n>  \tuint32_t pos;\n>  \n>  \tif (!bsearch_midx(oid, m, &pos))\n>  \t\treturn 0;\n>  \n> -\treturn nth_midxed_pack_entry(m, e, pos);\n> +\treturn nth_midxed_pack_entry(r, m, e, pos);\n>  }\n>  \n>  /* Match \"foo.idx\" against either \"foo.pack\" _or_ \"foo.idx\". */\n> @@ -1020,7 +1026,7 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)\n>  \t\t\tdisplay_progress(progress, _n); \\\n>  \t} while (0)\n>  \n> -int verify_midx_file(const char *object_dir)\n> +int verify_midx_file(struct repository *r, const char *object_dir)\n>  {\n>  \tstruct pair_pos_vs_id *pairs = NULL;\n>  \tuint32_t i;\n> @@ -1034,7 +1040,7 @@ int verify_midx_file(const char *object_dir)\n>  \tprogress = start_progress(_(\"Looking for referenced packfiles\"),\n>  \t\t\t\t  m->num_packs);\n>  \tfor (i = 0; i < m->num_packs; i++) {\n> -\t\tif (prepare_midx_pack(m, i))\n> +\t\tif (prepare_midx_pack(r, m, i))\n>  \t\t\tmidx_report(\"failed to load pack in position %d\", i);\n>  \n>  \t\tdisplay_progress(progress, i + 1);\n> @@ -1099,7 +1105,7 @@ int verify_midx_file(const char *object_dir)\n>  \n>  \t\tnth_midxed_object_oid(&oid, m, pairs[i].pos);\n>  \n> -\t\tif (!fill_midx_entry(&oid, &e, m)) {\n> +\t\tif (!fill_midx_entry(r, &oid, &e, m)) {\n>  \t\t\tmidx_report(_(\"failed to load pack entry for oid[%d] = %s\"),\n>  \t\t\t\t    pairs[i].pos, oid_to_hex(&oid));\n>  \t\t\tcontinue;\n> diff --git a/midx.h b/midx.h\n> index 26dd042d63..3eb29731f2 100644\n> --- a/midx.h\n> +++ b/midx.h\n> @@ -5,6 +5,7 @@\n>  \n>  struct object_id;\n>  struct pack_entry;\n> +struct repository;\n>  \n>  #define GIT_TEST_MULTI_PACK_INDEX \"GIT_TEST_MULTI_PACK_INDEX\"\n>  \n> @@ -37,18 +38,18 @@ struct multi_pack_index {\n>  };\n>  \n>  struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);\n> -int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id);\n> +int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\n>  int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result);\n>  struct object_id *nth_midxed_object_oid(struct object_id *oid,\n>  \t\t\t\t\tstruct multi_pack_index *m,\n>  \t\t\t\t\tuint32_t n);\n> -int fill_midx_entry(const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m);\n> +int fill_midx_entry(struct repository *r, const struct object_id *oid, struct pack_entry *e, struct multi_pack_index *m);\n>  int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name);\n>  int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local);\n>  \n>  int write_midx_file(const char *object_dir);\n>  void clear_midx_file(struct repository *r);\n> -int verify_midx_file(const char *object_dir);\n> +int verify_midx_file(struct repository *r, const char *object_dir);\n>  \n>  void close_midx(struct multi_pack_index *m);\n>  \n> diff --git a/packfile.c b/packfile.c\n> index cdf6b6ec34..7b94a14726 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -1035,7 +1035,7 @@ struct packed_git *get_all_packs(struct repository *r)\n>  \t\tfor (m = r->objects->multi_pack_index; m; m = m->next) {\n>  \t\t\tuint32_t i;\n>  \t\t\tfor (i = 0; i < m->num_packs; i++) {\n> -\t\t\t\tif (!prepare_midx_pack(m, i)) {\n> +\t\t\t\tif (!prepare_midx_pack(r, m, i)) {\n>  \t\t\t\t\tm->packs[i]->next = p;\n>  \t\t\t\t\tp = m->packs[i];\n>  \t\t\t\t}\n> @@ -1998,7 +1998,7 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa\n>  \t\treturn 0;\n>  \n>  \tfor (m = r->objects->multi_pack_index; m; m = m->next) {\n> -\t\tif (fill_midx_entry(oid, e, m))\n> +\t\tif (fill_midx_entry(r, oid, e, m))\n>  \t\t\treturn 1;\n>  \t}\n"},{"id":"375142","messageId":"1733843e-f0e4-0e25-468b-64727f6855d5@gmail.com","threadId":"51007","inReplyTo":"xmqq8svimzrz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] midx: pass a repository pointer","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-08T14:22:08Z","receivedAt":"2019-05-08T14:22:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/7/2019 4:31 AM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <dstolee@microsoft.com>\n>>\n>> Much of the multi-pack-index code focuses on the multi_pack_index\n>> struct, and so we only pass a pointer to the current one. However,\n>> we will insert a dependency on the packed_git linked list in a\n>> future change, so we will need a repository reference. Inserting\n>> these parameters is a significant enough change to split out.\n>>\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n> \n> This is a good move in the loger term, but not a happy thing to do\n> while your \"expire\" topic is still in flight, as the impact from\n> updating the signature of prepare_midx_pack() and friends will break\n> new callers in expire_midx_packs() etc.\n> \n> I am tempted to queue this and eject ds/midx-expire-repack for now,\n> while checking how that topic would look like when rebased on top of\n> these two patches.  We'll see.\n\nSorry for the noise. I consider this series a higher priority, as it\nfixes a problem with the feature in existing releases.\n\nThanks,\n-Stolee\n"}]}