{"thread":{"id":"49171","subject":"[PATCH 0/9] multi-pack-index cleanups","startedAt":"2018-08-20T16:51:59Z","lastAt":"2018-08-21T14:44:06Z","messageCount":18,"participants":["Derrick Stolee","Stefan Beller","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"356095","messageId":"20180820165124.152146-1-dstolee@microsoft.com","threadId":"49171","inReplyTo":null,"subject":"[PATCH 0/9] multi-pack-index cleanups","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:51:51Z","receivedAt":"2018-08-20T16:51:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This series is based on ds/multi-pack-index and\njk/for-each-object-iteration.\n\nThe multi-pack-index indexes objects across multiple pack-files. To\nspeed up object lookups and abbreviations, we do not place the pack-\nfiles covered by the multi-pack-index into the packed_git linked list\nor the packed_git_mru list. Existing test coverage focused on typical\nuses and the main consumers of the multi-pack-index.\n\nTo better understand the implications of the multi-pack-index with\nother scenarios, I ran the test suite after adding a step to 'git repack'\nto write a multi-pack-index, and to default core.multiPackIndex to 'true'.\nThis commit is available as [1].\n\nThe following issues were discovered, and are fixed by this series:\n\n1. The multi-pack-index did not distinguish between local and non-local\n   pack-files.\n\n2. A bad packed object was not inspected by object lookups in the multi-\n   pack-index, so would loop infinitely trying to load the same object.\n\n3. 'git count-objects --verbose' would not see the objects in the multi-\n   pack-index and would report the multi-pack-index as garbage.\n\n4. If the local object directory had a multi-pack-index but an alternate\n   did not, then the multi-pack-index would be dropped.\n\n5. If the multi-pack-index covered a pack-file that was paired with a\n   reachability bitmap, then that bitmap would not be loaded.\n\nSeveral issues were resolved simply by making a new 'all_packs' list in\nthe object store and replacing get_packed_git() calls with get_all_packs()\ncalls. The all_packs list is a linked list that starts with the pack-files\nin multi-pack-indexes and then continues along the packed_git linked list.\n\nAlso: I simplified the usage reports in 'git multi-pack-index' to help\nusers who are entering parameters incorrectly.\n\n[1] https://github.com/derrickstolee/git/commit/098dd1d515b592fb165a276241d7d68d1cde0036\n    DO-NOT-MERGE: compute multi-pack-index on repack.\n    I will send this commit as a separate patch so we can see the change\n    I made and the one test I needed to fix (because it moves a pack-file,\n    thereby making the multi-pack-index invalid).\n\n[2] https://github.com/derrickstolee/git/pull/9\n    A GitHub pull request containing this series.\n\nDerrick Stolee (9):\n  multi-pack-index: provide more helpful usage info\n  multi-pack-index: store local property\n  midx: mark bad packed objects\n  midx: stop reporting garbage\n  midx: fix bug that skips midx with alternates\n  packfile: add all_packs list\n  treewide: use get_all_packs\n  midx: test a few commands that use get_all_packs\n  pack-objects: consider packs in multi-pack-index\n\n builtin/count-objects.c     |  2 +-\n builtin/fsck.c              |  4 ++--\n builtin/gc.c                |  4 ++--\n builtin/multi-pack-index.c  | 16 +++++++-------\n builtin/pack-objects.c      | 42 +++++++++++++++++++++++++++++------\n builtin/pack-redundant.c    |  4 ++--\n fast-import.c               |  4 ++--\n http-backend.c              |  4 ++--\n midx.c                      | 32 ++++++++++++++++++---------\n midx.h                      |  7 ++++--\n object-store.h              |  6 +++++\n pack-bitmap.c               |  2 +-\n pack-objects.c              |  2 +-\n packfile.c                  | 40 ++++++++++++++++++++++++++++-----\n packfile.h                  |  1 +\n server-info.c               |  4 ++--\n t/helper/test-read-midx.c   |  2 +-\n t/t5319-multi-pack-index.sh | 44 ++++++++++++++++++++++++++++++++++---\n 18 files changed, 168 insertions(+), 52 deletions(-)\n\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356096","messageId":"20180820165124.152146-2-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 1/9] multi-pack-index: provide more helpful usage info","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:51:53Z","receivedAt":"2018-08-20T16:52:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The multi-pack-index builtin has a very simple command-line\ninterface. Instead of simply reporting usage, give the user a\nhint to why the arguments failed.\n\nReported-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/multi-pack-index.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 6a7aa00cf2..2633efd95d 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -32,16 +32,16 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \t\topts.object_dir = get_object_directory();\n \n \tif (argc == 0)\n-\t\tgoto usage;\n+\t\tusage_with_options(builtin_multi_pack_index_usage,\n+\t\t\t\t   builtin_multi_pack_index_options);\n \n-\tif (!strcmp(argv[0], \"write\")) {\n-\t\tif (argc > 1)\n-\t\t\tgoto usage;\n+\tif (argc > 1) {\n+\t\tdie(_(\"too many arguments\"));\n+\t\treturn 1;\n+\t}\n \n+\tif (!strcmp(argv[0], \"write\"))\n \t\treturn write_midx_file(opts.object_dir);\n-\t}\n \n-usage:\n-\tusage_with_options(builtin_multi_pack_index_usage,\n-\t\t\t   builtin_multi_pack_index_options);\n+\tdie(_(\"unrecognized verb: %s\"), argv[0]);\n }\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356097","messageId":"20180820165124.152146-4-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 3/9] midx: mark bad packed objects","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:51:57Z","receivedAt":"2018-08-20T16:52:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"When an object fails to decompress from a pack-file, we mark the object\nas 'bad' so we can retry with a different copy of the object (if such a\ncopy exists).\n\nBefore now, the multi-pack-index did not update the bad objects list for\nthe pack-files it contains, and we did not check the bad objects list\nwhen reading an object. Now, do both.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 6824acf5f8..7fa75a37a3 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -280,6 +280,16 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n \tif (!is_pack_valid(p))\n \t\treturn 0;\n \n+\tif (p->num_bad_objects) {\n+\t\tuint32_t i;\n+\t\tstruct object_id oid;\n+\t\tnth_midxed_object_oid(&oid, m, pos);\n+\t\tfor (i = 0; i < p->num_bad_objects; i++)\n+\t\t\tif (!hashcmp(oid.hash,\n+\t\t\t\t     p->bad_object_sha1 + the_hash_algo->rawsz * i))\n+\t\t\t\treturn 0;\n+\t}\n+\n \te->offset = nth_midxed_offset(m, pos);\n \te->p = p;\n \n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356098","messageId":"20180820165124.152146-5-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 4/9] midx: stop reporting garbage","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:51:59Z","receivedAt":"2018-08-20T16:52:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"When prepare_packed_git is called with the report_garbage method\ninitialized, we report unexpected files in the objects directory\nas garbage. Stop reporting the multi-pack-index and the pack-files\nit covers as garbage.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n packfile.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 896da460ac..fe713a0242 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -820,9 +820,8 @@ static void prepare_pack(const char *full_name, size_t full_name_len,\n \tstruct packed_git *p;\n \tsize_t base_len = full_name_len;\n \n-\tif (strip_suffix_mem(full_name, &base_len, \".idx\")) {\n-\t\tif (data->m && midx_contains_pack(data->m, file_name))\n-\t\t\treturn;\n+\tif (strip_suffix_mem(full_name, &base_len, \".idx\") &&\n+\t    !(data->m && midx_contains_pack(data->m, file_name))) {\n \t\t/* Don't reopen a pack we already have. */\n \t\tfor (p = data->r->objects->packed_git; p; p = p->next) {\n \t\t\tsize_t len;\n@@ -842,6 +841,8 @@ static void prepare_pack(const char *full_name, size_t full_name_len,\n \tif (!report_garbage)\n \t\treturn;\n \n+\tif (!strcmp(file_name, \"multi-pack-index\"))\n+\t\treturn;\n \tif (ends_with(file_name, \".idx\") ||\n \t    ends_with(file_name, \".pack\") ||\n \t    ends_with(file_name, \".bitmap\") ||\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356099","messageId":"20180820165124.152146-6-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 5/9] midx: fix bug that skips midx with alternates","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:52:00Z","receivedAt":"2018-08-20T16:52:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The logic for constructing the linked list of multi-pack-indexes\nin the object store is incorrect. If the local object store has\na multi-pack-index, but an alternate does not, then the list is\ndropped.\n\nAdd tests that would have revealed this bug.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c                      | 11 ++++++-----\n t/t5319-multi-pack-index.sh | 17 +++++++++++++++++\n 2 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 7fa75a37a3..0710c4c175 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -331,7 +331,7 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name)\n \n int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local)\n {\n-\tstruct multi_pack_index *m = r->objects->multi_pack_index;\n+\tstruct multi_pack_index *m;\n \tstruct multi_pack_index *m_search;\n \tint config_value;\n \n@@ -339,14 +339,15 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i\n \t    !config_value)\n \t\treturn 0;\n \n-\tfor (m_search = m; m_search; m_search = m_search->next)\n+\tfor (m_search = r->objects->multi_pack_index; m_search; m_search = m_search->next)\n \t\tif (!strcmp(object_dir, m_search->object_dir))\n \t\t\treturn 1;\n \n-\tr->objects->multi_pack_index = load_multi_pack_index(object_dir, local);\n+\tm = load_multi_pack_index(object_dir, local);\n \n-\tif (r->objects->multi_pack_index) {\n-\t\tr->objects->multi_pack_index->next = m;\n+\tif (m) {\n+\t\tm->next = r->objects->multi_pack_index;\n+\t\tr->objects->multi_pack_index = m;\n \t\treturn 1;\n \t}\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex ae1d5d4592..4b6e2825a6 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -149,6 +149,23 @@ test_expect_success 'repack removes multi-pack-index' '\n \n compare_results_with_midx \"after repack\"\n \n+test_expect_success 'multi-pack-index and alternates' '\n+\tgit init --bare alt.git &&\n+\techo $(pwd)/alt.git/objects >.git/objects/info/alternates &&\n+\techo content1 >file1 &&\n+\taltblob=$(GIT_DIR=alt.git git hash-object -w file1) &&\n+\tgit cat-file blob $altblob &&\n+\tgit rev-list --all\n+'\n+\n+compare_results_with_midx \"with alternate (local midx)\"\n+\n+test_expect_success 'multi-pack-index in an alternate' '\n+\tmv .git/objects/pack/* alt.git/objects/pack\n+'\n+\n+compare_results_with_midx \"with alternate (remote midx)\"\n+\n \n # usage: corrupt_data <file> <pos> [<data>]\n corrupt_data () {\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356100","messageId":"20180820165124.152146-7-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 6/9] packfile: add all_packs list","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:52:02Z","receivedAt":"2018-08-20T16:52:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"If a repo contains a multi-pack-index, then the packed_git list\ndoes not contain the packfiles that are covered by the multi-pack-index.\nThis is important for doing object lookups, abbreviations, and\napproximating object count. However, there are many operations that\nreally want to iterate over all packfiles.\n\nCreate a new 'all_packs' linked list that contains this list, starting\nwith the packfiles in the multi-pack-index and then continuing along\nthe packed_git linked list.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c         |  2 +-\n midx.h         |  1 +\n object-store.h |  6 ++++++\n packfile.c     | 27 +++++++++++++++++++++++++++\n packfile.h     |  1 +\n 5 files changed, 36 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 0710c4c175..f3e8dbc108 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -197,7 +197,7 @@ static void close_midx(struct multi_pack_index *m)\n \tFREE_AND_NULL(m->pack_names);\n }\n \n-static int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id)\n+int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id)\n {\n \tstruct strbuf pack_name = STRBUF_INIT;\n \ndiff --git a/midx.h b/midx.h\nindex 8aa79f4b62..a210f1af2a 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -32,6 +32,7 @@ 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 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,\ndiff --git a/object-store.h b/object-store.h\nindex f9c57e2c26..c69d546392 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -128,6 +128,12 @@ 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 fe713a0242..adcf2e12a0 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -972,6 +972,9 @@ static void prepare_packed_git(struct repository *r)\n \t\tprepare_packed_git_one(r, alt->path, 0);\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@@ -995,6 +998,30 @@ struct multi_pack_index *get_multi_pack_index(struct repository *r)\n \treturn r->objects->multi_pack_index;\n }\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(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+\n+\t\tr->objects->all_packs = p;\n+\t}\n+\n+\treturn r->objects->all_packs;\n+}\n+\n struct list_head *get_packed_git_mru(struct repository *r)\n {\n \tprepare_packed_git(r);\ndiff --git a/packfile.h b/packfile.h\nindex 7855556686..3b90e2864c 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -51,6 +51,7 @@ extern void install_packed_git(struct repository *r, struct packed_git *pack);\n struct packed_git *get_packed_git(struct repository *r);\n struct list_head *get_packed_git_mru(struct repository *r);\n struct multi_pack_index *get_multi_pack_index(struct repository *r);\n+struct packed_git *get_all_packs(struct repository *r);\n \n /*\n  * Give a rough count of objects in the repository. This sacrifices accuracy\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356101","messageId":"20180820165124.152146-8-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 7/9] treewide: use get_all_packs","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:52:04Z","receivedAt":"2018-08-20T16:52:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"There are many places in the codebase that want to iterate over\nall packfiles known to Git. The purposes are wide-ranging, and\nthose that can take advantage of the multi-pack-index already\ndo. So, use get_all_packs() instead of get_packed_git() to be\nsure we are iterating over all packfiles.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/count-objects.c  |  2 +-\n builtin/fsck.c           |  4 ++--\n builtin/gc.c             |  4 ++--\n builtin/pack-objects.c   | 14 +++++++-------\n builtin/pack-redundant.c |  4 ++--\n fast-import.c            |  4 ++--\n http-backend.c           |  4 ++--\n pack-bitmap.c            |  2 +-\n pack-objects.c           |  2 +-\n packfile.c               |  2 +-\n server-info.c            |  4 ++--\n 11 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex d51e2ce1ec..a7cad052c6 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -123,7 +123,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf pack_buf = STRBUF_INIT;\n \t\tstruct strbuf garbage_buf = STRBUF_INIT;\n \n-\t\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\tif (!p->pack_local)\n \t\t\t\tcontinue;\n \t\t\tif (open_pack_index(p))\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex c96f3f4fcc..184d8e7f4e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -740,7 +740,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\tstruct progress *progress = NULL;\n \n \t\t\tif (show_progress) {\n-\t\t\t\tfor (p = get_packed_git(the_repository); p;\n+\t\t\t\tfor (p = get_all_packs(the_repository); p;\n \t\t\t\t     p = p->next) {\n \t\t\t\t\tif (open_pack_index(p))\n \t\t\t\t\t\tcontinue;\n@@ -749,7 +749,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \n \t\t\t\tprogress = start_progress(_(\"Checking objects\"), total);\n \t\t\t}\n-\t\t\tfor (p = get_packed_git(the_repository); p;\n+\t\t\tfor (p = get_all_packs(the_repository); p;\n \t\t\t     p = p->next) {\n \t\t\t\t/* verify gives error messages itself */\n \t\t\t\tif (verify_pack(p, fsck_obj_buffer,\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 57069442b0..2b592260e9 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -183,7 +183,7 @@ static struct packed_git *find_base_packs(struct string_list *packs,\n {\n \tstruct packed_git *p, *base = NULL;\n \n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n \t\tif (limit) {\n@@ -208,7 +208,7 @@ static int too_many_packs(void)\n \tif (gc_auto_pack_limit <= 0)\n \t\treturn 0;\n \n-\tfor (cnt = 0, p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (cnt = 0, p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n \t\tif (p->pack_keep)\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4391504a91..1a896d7810 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2784,7 +2784,7 @@ static void add_objects_in_unpacked_packs(struct rev_info *revs)\n \n \tmemset(&in_pack, 0, sizeof(in_pack));\n \n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tstruct object_id oid;\n \t\tstruct object *o;\n \n@@ -2848,7 +2848,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n \tstruct packed_git *p;\n \n \tp = (last_found != (void *)1) ? last_found :\n-\t\t\t\t\tget_packed_git(the_repository);\n+\t\t\t\t\tget_all_packs(the_repository);\n \n \twhile (p) {\n \t\tif ((!p->pack_local || p->pack_keep ||\n@@ -2858,7 +2858,7 @@ static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)\n \t\t\treturn 1;\n \t\t}\n \t\tif (p == last_found)\n-\t\t\tp = get_packed_git(the_repository);\n+\t\t\tp = get_all_packs(the_repository);\n \t\telse\n \t\t\tp = p->next;\n \t\tif (p == last_found)\n@@ -2894,7 +2894,7 @@ static void loosen_unused_packed_objects(struct rev_info *revs)\n \tuint32_t i;\n \tstruct object_id oid;\n \n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (!p->pack_local || p->pack_keep || p->pack_keep_in_core)\n \t\t\tcontinue;\n \n@@ -3041,7 +3041,7 @@ static void add_extra_kept_packs(const struct string_list *names)\n \tif (!names->nr)\n \t\treturn;\n \n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tconst char *name = basename(p->pack_name);\n \t\tint i;\n \n@@ -3314,7 +3314,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tadd_extra_kept_packs(&keep_pack_list);\n \tif (ignore_packed_keep_on_disk) {\n \t\tstruct packed_git *p;\n-\t\tfor (p = get_packed_git(the_repository); p; p = p->next)\n+\t\tfor (p = get_all_packs(the_repository); p; p = p->next)\n \t\t\tif (p->pack_local && p->pack_keep)\n \t\t\t\tbreak;\n \t\tif (!p) /* no keep-able packs found */\n@@ -3327,7 +3327,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t * it also covers non-local objects\n \t\t */\n \t\tstruct packed_git *p;\n-\t\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\tif (!p->pack_local) {\n \t\t\t\thave_non_local_packs = 1;\n \t\t\t\tbreak;\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 0494dceff7..cf9a9aabd4 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -577,7 +577,7 @@ static struct pack_list * add_pack(struct packed_git *p)\n \n static struct pack_list * add_pack_file(const char *filename)\n {\n-\tstruct packed_git *p = get_packed_git(the_repository);\n+\tstruct packed_git *p = get_all_packs(the_repository);\n \n \tif (strlen(filename) < 40)\n \t\tdie(\"Bad pack filename: %s\", filename);\n@@ -592,7 +592,7 @@ static struct pack_list * add_pack_file(const char *filename)\n \n static void load_all(void)\n {\n-\tstruct packed_git *p = get_packed_git(the_repository);\n+\tstruct packed_git *p = get_all_packs(the_repository);\n \n \twhile (p) {\n \t\tadd_pack(p);\ndiff --git a/fast-import.c b/fast-import.c\nindex 89bb0c9db3..f8c3acd3b5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1068,7 +1068,7 @@ static int store_object(\n \t\tduplicate_count_by_type[type]++;\n \t\treturn 1;\n \t} else if (find_sha1_pack(oid.hash,\n-\t\t\t\t  get_packed_git(the_repository))) {\n+\t\t\t\t  get_all_packs(the_repository))) {\n \t\te->type = type;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\n@@ -1266,7 +1266,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n \t\ttruncate_pack(&checkpoint);\n \n \t} else if (find_sha1_pack(oid.hash,\n-\t\t\t\t  get_packed_git(the_repository))) {\n+\t\t\t\t  get_all_packs(the_repository))) {\n \t\te->type = OBJ_BLOB;\n \t\te->pack_id = MAX_PACK_ID;\n \t\te->idx.offset = 1; /* just not zero! */\ndiff --git a/http-backend.c b/http-backend.c\nindex bd0442a805..5e879177ed 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -523,13 +523,13 @@ static void get_info_packs(struct strbuf *hdr, char *arg)\n \tsize_t cnt = 0;\n \n \tselect_getanyfile(hdr);\n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (p->pack_local)\n \t\t\tcnt++;\n \t}\n \n \tstrbuf_grow(&buf, cnt * 53 + 2);\n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (p->pack_local)\n \t\t\tstrbuf_addf(&buf, \"P %s\\n\", p->pack_name + objdirlen + 6);\n \t}\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex f0a1937a1c..4e50ab391f 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -335,7 +335,7 @@ static int open_pack_bitmap(struct bitmap_index *bitmap_git)\n \n \tassert(!bitmap_git->map && !bitmap_git->loaded);\n \n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n \t\t\tret = 0;\n \t}\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 92708522e7..832dcf7462 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -99,7 +99,7 @@ static void prepare_in_pack_by_idx(struct packing_data *pdata)\n \t * (i.e. in_pack_idx also zero) should return NULL.\n \t */\n \tmapping[cnt++] = NULL;\n-\tfor (p = get_packed_git(the_repository); p; p = p->next, cnt++) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next, cnt++) {\n \t\tif (cnt == nr) {\n \t\t\tfree(mapping);\n \t\t\treturn;\ndiff --git a/packfile.c b/packfile.c\nindex adcf2e12a0..cbef7033c3 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2036,7 +2036,7 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,\n \tint pack_errors = 0;\n \n \tprepare_packed_git(the_repository);\n-\tfor (p = the_repository->objects->packed_git; p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)\n \t\t\tcontinue;\n \t\tif ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&\ndiff --git a/server-info.c b/server-info.c\nindex 41050c2449..e2b2d6a27a 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -199,7 +199,7 @@ static void init_pack_info(const char *infofile, int force)\n \tobjdir = get_object_directory();\n \tobjdirlen = strlen(objdir);\n \n-\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t/* we ignore things on alternate path since they are\n \t\t * not available to the pullers in general.\n \t\t */\n@@ -209,7 +209,7 @@ static void init_pack_info(const char *infofile, int force)\n \t}\n \tnum_pack = i;\n \tinfo = xcalloc(num_pack, sizeof(struct pack_info *));\n-\tfor (i = 0, p = get_packed_git(the_repository); p; p = p->next) {\n+\tfor (i = 0, p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (!p->pack_local)\n \t\t\tcontinue;\n \t\tinfo[i] = xcalloc(1, sizeof(struct pack_info));\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356102","messageId":"20180820165124.152146-3-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 2/9] multi-pack-index: store local property","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:51:55Z","receivedAt":"2018-08-20T16:52:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"A pack-file is 'local' if it is stored within the usual object\ndirectory. If it is stored in an alternate, it is non-local.\n\nPack-files are stored using a 'pack_local' member in the packed_git\nstruct. Add a similar 'local' member to the multi_pack_index struct\nand 'local' parameters to the methods that load and prepare multi-\npack-indexes.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n midx.c                    | 11 ++++++-----\n midx.h                    |  6 ++++--\n packfile.c                |  4 ++--\n t/helper/test-read-midx.c |  2 +-\n 4 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 19b7df338e..6824acf5f8 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -37,7 +37,7 @@ static char *get_midx_filename(const char *object_dir)\n \treturn xstrfmt(\"%s/pack/multi-pack-index\", object_dir);\n }\n \n-struct multi_pack_index *load_multi_pack_index(const char *object_dir)\n+struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local)\n {\n \tstruct multi_pack_index *m = NULL;\n \tint fd;\n@@ -73,6 +73,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir)\n \tm->fd = fd;\n \tm->data = midx_map;\n \tm->data_len = midx_size;\n+\tm->local = local;\n \n \tm->signature = get_be32(m->data);\n \tif (m->signature != MIDX_SIGNATURE) {\n@@ -209,7 +210,7 @@ static int prepare_midx_pack(struct multi_pack_index *m, uint32_t pack_int_id)\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, 1);\n+\tm->packs[pack_int_id] = 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@@ -318,7 +319,7 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_name)\n \treturn 0;\n }\n \n-int prepare_multi_pack_index_one(struct repository *r, const char *object_dir)\n+int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, int local)\n {\n \tstruct multi_pack_index *m = r->objects->multi_pack_index;\n \tstruct multi_pack_index *m_search;\n@@ -332,7 +333,7 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir)\n \t\tif (!strcmp(object_dir, m_search->object_dir))\n \t\t\treturn 1;\n \n-\tr->objects->multi_pack_index = load_multi_pack_index(object_dir);\n+\tr->objects->multi_pack_index = load_multi_pack_index(object_dir, local);\n \n \tif (r->objects->multi_pack_index) {\n \t\tr->objects->multi_pack_index->next = m;\n@@ -746,7 +747,7 @@ int write_midx_file(const char *object_dir)\n \t\t\t  midx_name);\n \t}\n \n-\tpacks.m = load_multi_pack_index(object_dir);\n+\tpacks.m = load_multi_pack_index(object_dir, 1);\n \n \tpacks.nr = 0;\n \tpacks.alloc_list = packs.m ? packs.m->num_packs : 16;\ndiff --git a/midx.h b/midx.h\nindex e3b07f1586..8aa79f4b62 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -18,6 +18,8 @@ struct multi_pack_index {\n \tuint32_t num_packs;\n \tuint32_t num_objects;\n \n+\tint local;\n+\n \tconst unsigned char *chunk_pack_names;\n \tconst uint32_t *chunk_oid_fanout;\n \tconst unsigned char *chunk_oid_lookup;\n@@ -29,14 +31,14 @@ struct multi_pack_index {\n \tchar object_dir[FLEX_ARRAY];\n };\n \n-struct multi_pack_index *load_multi_pack_index(const char *object_dir);\n+struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);\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 midx_contains_pack(struct multi_pack_index *m, const char *idx_name);\n-int prepare_multi_pack_index_one(struct repository *r, const char *object_dir);\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(const char *object_dir);\ndiff --git a/packfile.c b/packfile.c\nindex 12db1a9d7d..896da460ac 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -963,11 +963,11 @@ static void prepare_packed_git(struct repository *r)\n \n \tif (r->objects->packed_git_initialized)\n \t\treturn;\n-\tprepare_multi_pack_index_one(r, r->objects->objectdir);\n+\tprepare_multi_pack_index_one(r, r->objects->objectdir, 1);\n \tprepare_packed_git_one(r, r->objects->objectdir, 1);\n \tprepare_alt_odb(r);\n \tfor (alt = r->objects->alt_odb_list; alt; alt = alt->next) {\n-\t\tprepare_multi_pack_index_one(r, alt->path);\n+\t\tprepare_multi_pack_index_one(r, alt->path, 0);\n \t\tprepare_packed_git_one(r, alt->path, 0);\n \t}\n \trearrange_packed_git(r);\ndiff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\nindex 8e19972e89..831b586d02 100644\n--- a/t/helper/test-read-midx.c\n+++ b/t/helper/test-read-midx.c\n@@ -7,7 +7,7 @@\n static int read_midx_file(const char *object_dir)\n {\n \tuint32_t i;\n-\tstruct multi_pack_index *m = load_multi_pack_index(object_dir);\n+\tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n \n \tif (!m)\n \t\treturn 1;\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356103","messageId":"20180820165124.152146-9-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 8/9] midx: test a few commands that use get_all_packs","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:52:06Z","receivedAt":"2018-08-20T16:52:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The new get_all_packs() method exposed the packfiles coverede by\na multi-pack-index. Before, the 'git cat-file --batch' and\n'git count-objects' commands would skip objects in an environment\nwith a multi-pack-index.\n\nFurther, a reachability bitmap would be ignored if its pack-file\nwas covered by a multi-pack-index.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n t/t5319-multi-pack-index.sh | 21 ++++++++++++++++++---\n 1 file changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 4b6e2825a6..424d0c640f 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -86,8 +86,14 @@ test_expect_success 'write midx with one v1 pack' '\n '\n \n midx_git_two_modes () {\n-\tgit -c core.multiPackIndex=false $1 >expect &&\n-\tgit -c core.multiPackIndex=true $1 >actual &&\n+\tif [ \"$2\" = \"sorted\" ]\n+\tthen\n+\t\tgit -c core.multiPackIndex=false $1 | sort >expect &&\n+\t\tgit -c core.multiPackIndex=true $1 | sort >actual\n+\telse\n+\t\tgit -c core.multiPackIndex=false $1 >expect &&\n+\t\tgit -c core.multiPackIndex=true $1 >actual\n+\tfi &&\n \ttest_cmp expect actual\n }\n \n@@ -95,7 +101,10 @@ compare_results_with_midx () {\n \tMSG=$1\n \ttest_expect_success \"check normal git operations: $MSG\" '\n \t\tmidx_git_two_modes \"rev-list --objects --all\" &&\n-\t\tmidx_git_two_modes \"log --raw\"\n+\t\tmidx_git_two_modes \"log --raw\" &&\n+\t\tmidx_git_two_modes \"count-objects --verbose\" &&\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check\" &&\n+\t\tmidx_git_two_modes \"cat-file --batch-all-objects --buffer --batch-check --unsorted\" sorted\n \t'\n }\n \n@@ -149,6 +158,12 @@ test_expect_success 'repack removes multi-pack-index' '\n \n compare_results_with_midx \"after repack\"\n \n+test_expect_success 'multi-pack-index and pack-bitmap' '\n+\tgit -c repack.writeBitmaps=true repack -ad &&\n+\tgit multi-pack-index write &&\n+\tgit rev-list --test-bitmap HEAD\n+'\n+\n test_expect_success 'multi-pack-index and alternates' '\n \tgit init --bare alt.git &&\n \techo $(pwd)/alt.git/objects >.git/objects/info/alternates &&\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356104","messageId":"20180820165124.152146-10-dstolee@microsoft.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"[PATCH 9/9] pack-objects: consider packs in multi-pack-index","fromName":"Derrick Stolee","fromEmail":"dstolee@microsoft.com","sentAt":"2018-08-20T16:52:08Z","receivedAt":"2018-08-20T16:52:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"When running 'git pack-objects --local', we want to avoid packing\nobjects that are in an alternate. Currently, we check for these\nobjects using the packed_git_mru list, which excludes the pack-files\ncovered by a multi-pack-index.\n\nAdd a new iteration over the multi-pack-indexes to find these\ncopies and mark them as unwanted.\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\n---\n builtin/pack-objects.c      | 28 ++++++++++++++++++++++++++++\n t/t5319-multi-pack-index.sh |  8 +++++++-\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1a896d7810..4a9a42d29a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -31,6 +31,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"dir.h\"\n+#include \"midx.h\"\n \n #define IN_PACK(obj) oe_in_pack(&to_pack, obj)\n #define SIZE(obj) oe_size(&to_pack, obj)\n@@ -1034,6 +1035,7 @@ static int want_object_in_pack(const struct object_id *oid,\n {\n \tint want;\n \tstruct list_head *pos;\n+\tstruct multi_pack_index *m;\n \n \tif (!exclude && local && has_loose_object_nonlocal(oid))\n \t\treturn 0;\n@@ -1048,6 +1050,32 @@ static int want_object_in_pack(const struct object_id *oid,\n \t\tif (want != -1)\n \t\t\treturn want;\n \t}\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\t\tstruct packed_git *p = e.p;\n+\t\t\toff_t offset;\n+\t\t\t\n+\t\t\tif (p == *found_pack)\n+\t\t\t\toffset = *found_offset;\n+\t\t\telse\n+\t\t\t\toffset = find_pack_entry_one(oid->hash, p);\n+\n+\t\t\tif (offset) {\n+\t\t\t\tif (!*found_pack) {\n+\t\t\t\t\tif (!is_pack_valid(p))\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t*found_offset = offset;\n+\t\t\t\t\t*found_pack = p;\n+\t\t\t\t}\n+\t\t\t\twant = want_found_object(exclude, p);\n+\t\t\t\tif (want != -1)\n+\t\t\t\t\treturn want;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \tlist_for_each(pos, get_packed_git_mru(the_repository)) {\n \t\tstruct packed_git *p = list_entry(pos, struct packed_git, mru);\n \t\toff_t offset;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 424d0c640f..6f56b38674 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -176,7 +176,13 @@ test_expect_success 'multi-pack-index and alternates' '\n compare_results_with_midx \"with alternate (local midx)\"\n \n test_expect_success 'multi-pack-index in an alternate' '\n-\tmv .git/objects/pack/* alt.git/objects/pack\n+\tmv .git/objects/pack/* alt.git/objects/pack &&\n+\ttest_commit add_local_objects &&\n+\tgit repack --local &&\n+\tgit multi-pack-index write &&\n+\tmidx_read_expect 1 3 4 $objdir &&\n+\tgit reset --hard HEAD~1 &&\n+\trm -f .git/objects/pack/*\n '\n \n compare_results_with_midx \"with alternate (remote midx)\"\n-- \n2.18.0.118.gd4f65b8d14\n\n"},{"id":"356137","messageId":"CAGZ79kZJJk5DSu+kNBZgxMvy36Fq9tYcBG-BLmpJ6LOKPRtGVg@mail.gmail.com","threadId":"49171","inReplyTo":"20180820165124.152146-3-dstolee@microsoft.com","subject":"Re: [PATCH 2/9] multi-pack-index: store local property","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T21:14:27Z","receivedAt":"2018-08-20T21:14:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:\n>\n> A pack-file is 'local' if it is stored within the usual object\n> directory. If it is stored in an alternate, it is non-local.\n>\n> Pack-files are stored using a 'pack_local' member in the packed_git\n> struct. Add a similar 'local' member to the multi_pack_index struct\n> and 'local' parameters to the methods that load and prepare multi-\n> pack-indexes.\n\nGoing by that example, maybe we'd want to have it be the first bit\nof a bitfield, c.f.\n\n    unsigned pack_local:1,\n        pack_keep:1,\n        pack_keep_in_core:1,\n        freshened:1,\n        do_not_close:1,\n        pack_promisor:1;\n\nin the definition of packed_git.\n\nIs there value in documenting both packfiles as well as midx variables?\n(When going with the bitfield example, it may be a bit more worthwhile,\nas the commit message is harder to find, as it will need repeated\nblaming in the future, if there is a commit on top that adds another\nbit to the field; I am unsure, but I would lean towards documentation\nas it becomes a bit unclear: Is the local flag for the midx file that\nis read, or rather for the underlying packs? What exactly is local? )\n\nAFAICT this patch alone doesn't have any effect, yet, as it only pipes\nthe flag thru, is that worth mentioning or is that obvious?\n\nThanks,\nStefan\n"},{"id":"356138","messageId":"CAGZ79kY8i3y6_r=bQAx7V5=ckSmb_RgyByjJwtS+7Qr2y8x8LQ@mail.gmail.com","threadId":"49171","inReplyTo":"20180820165124.152146-4-dstolee@microsoft.com","subject":"Re: [PATCH 3/9] midx: mark bad packed objects","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T21:23:06Z","receivedAt":"2018-08-20T21:23:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:\n>\n> When an object fails to decompress from a pack-file, we mark the object\n> as 'bad' so we can retry with a different copy of the object (if such a\n> copy exists).\n>\n> Before now, the multi-pack-index did not update the bad objects list for\n> the pack-files it contains, and we did not check the bad objects list\n> when reading an object. Now, do both.\n\nThis sounds like a bug fix unlike patches 1 & 2 that sound like\nfeature work(2) or making code more readable(1).\n(After studying the code, this doesn't sound like a bug fix any more,\nbut a safety thing)\n\nIs it worth having this on a separate track coming in\nfaster than the rest of this series?\n\n>\n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> ---\n>  midx.c | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n>\n> diff --git a/midx.c b/midx.c\n> index 6824acf5f8..7fa75a37a3 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -280,6 +280,16 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n>         if (!is_pack_valid(p))\n>                 return 0;\n>\n> +       if (p->num_bad_objects) {\n> +               uint32_t i;\n\nIs there a reason that i needs to be if 32 bits?\nWould size_t (for iterating) or 'int' (as a default\nlike in many for loops) be ok, too?\n"},{"id":"356142","messageId":"CAGZ79kZFXof091d4qXUVyLJ9wGPL0sfs9XruZ-3Hjoq74-tjCg@mail.gmail.com","threadId":"49171","inReplyTo":"20180820165124.152146-8-dstolee@microsoft.com","subject":"Re: [PATCH 7/9] treewide: use get_all_packs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T22:01:59Z","receivedAt":"2018-08-20T22:02:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:\n>\n> There are many places in the codebase that want to iterate over\n> all packfiles known to Git. The purposes are wide-ranging, and\n> those that can take advantage of the multi-pack-index already\n> do. So, use get_all_packs() instead of get_packed_git() to be\n> sure we are iterating over all packfiles.\n\nSo get_packed_git shouold not be used any further?\nDo we want to annotate it as deprecated, to be deleted\nin the future? Or even remove it as part of this series\n(that might be an issue with other series in flight).\n"},{"id":"356143","messageId":"CAGZ79kYVsmw5LDr-saorv7P3CVxbgxZhF4kAxwCYv=OAeuc5Cg@mail.gmail.com","threadId":"49171","inReplyTo":"20180820165124.152146-9-dstolee@microsoft.com","subject":"Re: [PATCH 8/9] midx: test a few commands that use get_all_packs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-20T22:03:04Z","receivedAt":"2018-08-20T22:03:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:\n>\n> The new get_all_packs() method exposed the packfiles coverede by\n\ns/coverede/covered/\n"},{"id":"356168","messageId":"da0185cf-7d48-862b-eb25-5d5bd8f4e833@gmail.com","threadId":"49171","inReplyTo":"CAGZ79kY8i3y6_r=bQAx7V5=ckSmb_RgyByjJwtS+7Qr2y8x8LQ@mail.gmail.com","subject":"Re: [PATCH 3/9] midx: mark bad packed objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T13:53:44Z","receivedAt":"2018-08-21T13:53:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2018 5:23 PM, Stefan Beller wrote:\n> On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:\n>> When an object fails to decompress from a pack-file, we mark the object\n>> as 'bad' so we can retry with a different copy of the object (if such a\n>> copy exists).\n>>\n>> Before now, the multi-pack-index did not update the bad objects list for\n>> the pack-files it contains, and we did not check the bad objects list\n>> when reading an object. Now, do both.\n> This sounds like a bug fix unlike patches 1 & 2 that sound like\n> feature work(2) or making code more readable(1).\n> (After studying the code, this doesn't sound like a bug fix any more,\n> but a safety thing)\n\nIt is a safety thing. One codepath that needs this includes this comment:\n\n             /*\n              * We're probably in deep shit, but let's try to fetch\n              * the required base anyway from another pack or loose.\n              * This is costly but should happen only in the presence\n              * of a corrupted pack, and is better than failing outright.\n              */\n\nThis goes back to 8eca0b47: implement some resilience against pack \ncorruptions. The logic is tested in t5303-pack-corruption-resilience.sh, \nwith the test title '... and a redundant pack allows for full recovery too'.\n\n> Is it worth having this on a separate track coming in\n> faster than the rest of this series?\n\nds/multi-pack-index is cooking in 'next' until after 2.19.0, so I'm not \nsure it's worth splitting things up at this point.\n\n>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n>> ---\n>>   midx.c | 10 ++++++++++\n>>   1 file changed, 10 insertions(+)\n>>\n>> diff --git a/midx.c b/midx.c\n>> index 6824acf5f8..7fa75a37a3 100644\n>> --- a/midx.c\n>> +++ b/midx.c\n>> @@ -280,6 +280,16 @@ static int nth_midxed_pack_entry(struct multi_pack_index *m, struct pack_entry *\n>>          if (!is_pack_valid(p))\n>>                  return 0;\n>>\n>> +       if (p->num_bad_objects) {\n>> +               uint32_t i;\n> Is there a reason that i needs to be if 32 bits?\n> Would size_t (for iterating) or 'int' (as a default\n> like in many for loops) be ok, too?\n\ni is bounded by p->num_bad_objects, which is also uint32_t.\n\nThanks,\n\n-Stolee\n\n"},{"id":"356169","messageId":"05f42490-bf11-8673-61db-e0afd04af43a@gmail.com","threadId":"49171","inReplyTo":"CAGZ79kZFXof091d4qXUVyLJ9wGPL0sfs9XruZ-3Hjoq74-tjCg@mail.gmail.com","subject":"Re: [PATCH 7/9] treewide: use get_all_packs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T13:56:42Z","receivedAt":"2018-08-21T13:56:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2018 6:01 PM, Stefan Beller wrote:\n> On Mon, Aug 20, 2018 at 9:52 AM Derrick Stolee <dstolee@microsoft.com> wrote:\n>> There are many places in the codebase that want to iterate over\n>> all packfiles known to Git. The purposes are wide-ranging, and\n>> those that can take advantage of the multi-pack-index already\n>> do. So, use get_all_packs() instead of get_packed_git() to be\n>> sure we are iterating over all packfiles.\n> So get_packed_git shouold not be used any further?\n> Do we want to annotate it as deprecated, to be deleted\n> in the future? Or even remove it as part of this series\n> (that might be an issue with other series in flight).\n\nPerhaps the right thing to do would be to put a big warning over the \ndefinition to say \"only use this if you really don't want \nget_all_packs()\" or something. The reason I didn't remove it from \npackfile.h is that it is used in sha1-name.c alongside \nget_multi_pack_index() (so making it 'static' would be incorrect).\n\n"},{"id":"356170","messageId":"CACsJy8CKnXVJYkKM_W=N=Vq-TVXf+YCqZP_uP7B-dN_6xddB=g@mail.gmail.com","threadId":"49171","inReplyTo":"20180820165124.152146-1-dstolee@microsoft.com","subject":"Re: [PATCH 0/9] multi-pack-index cleanups","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-21T14:34:14Z","receivedAt":"2018-08-21T14:34:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 20, 2018 at 6:53 PM Derrick Stolee <dstolee@microsoft.com> wrote:\n> To better understand the implications of the multi-pack-index with\n> other scenarios, I ran the test suite after adding a step to 'git repack'\n> to write a multi-pack-index, and to default core.multiPackIndex to 'true'.\n> This commit is available as [1].\n>\n> ...\n>\n> [1] https://github.com/derrickstolee/git/commit/098dd1d515b592fb165a276241d7d68d1cde0036\n>     DO-NOT-MERGE: compute multi-pack-index on repack.\n\nIt should be able to _merge_ this patch, but only activate it when\nsome test environment variable is set. On Travis x86 we run the test\nsuite twice, once normal, and once with special features activated\n(see \"if test \"$jobname\" = \"linux-gcc\"\" in ci/run-build-and-tests.sh).\nI don't think midx is incompatible with any of those features, so we\ncould activate and run the whole test suite with it too.\n-- \nDuy\n"},{"id":"356172","messageId":"03fd9f73-417c-5afc-a276-d2194eb17668@gmail.com","threadId":"49171","inReplyTo":"CACsJy8CKnXVJYkKM_W=N=Vq-TVXf+YCqZP_uP7B-dN_6xddB=g@mail.gmail.com","subject":"Re: [PATCH 0/9] multi-pack-index cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-08-21T14:44:03Z","receivedAt":"2018-08-21T14:44:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/21/2018 10:34 AM, Duy Nguyen wrote:\n> On Mon, Aug 20, 2018 at 6:53 PM Derrick Stolee <dstolee@microsoft.com> wrote:\n>> To better understand the implications of the multi-pack-index with\n>> other scenarios, I ran the test suite after adding a step to 'git repack'\n>> to write a multi-pack-index, and to default core.multiPackIndex to 'true'.\n>> This commit is available as [1].\n>>\n>> ...\n>>\n>> [1] https://github.com/derrickstolee/git/commit/098dd1d515b592fb165a276241d7d68d1cde0036\n>>      DO-NOT-MERGE: compute multi-pack-index on repack.\n> It should be able to _merge_ this patch, but only activate it when\n> some test environment variable is set. On Travis x86 we run the test\n> suite twice, once normal, and once with special features activated\n> (see \"if test \"$jobname\" = \"linux-gcc\"\" in ci/run-build-and-tests.sh).\n> I don't think midx is incompatible with any of those features, so we\n> could activate and run the whole test suite with it too.\n\nInteresting. I'll look into this. A similar approach may work for the \ncommit-graph.\n\nThanks,\n\n-Stolee\n\n"}]}