{"thread":{"id":"56455","subject":"[PATCH 0/4] pack-bitmap: permute existing namehash values","startedAt":"2021-09-07T21:17:55Z","lastAt":"2021-09-17T22:12:54Z","messageCount":55,"participants":["Taylor Blau","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"434950","messageId":"cover.1631049462.git.me@ttaylorr.com","threadId":"56455","inReplyTo":null,"subject":"[PATCH 0/4] pack-bitmap: permute existing namehash values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-07T21:17:50Z","receivedAt":"2021-09-07T21:17:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series depends on tb/multi-pack-bitmaps and is used to propagate\nthe hashcache between .bitmap files when generating multi-pack reachability\nbitmaps.\n\nThe gist is that single pack bitmaps get the namehash values via the traversal\nused to generate the pack, but there is no such traversal for MIDX bitmaps, and\nthus the hash-cache extension (if we were to write it) would contain all zeros.\n\nThis series teaches the pack-bitmap code how to permute the hashcache values\nfrom an existing bitmap into the new one. This doesn't keep the hashcache\nup-to-date, but it at least prevents existing data from falling out when\ngenerating a MIDX bitmap.\n\nTaylor Blau (4):\n t/helper/test-bitmap.c: add 'dump-hashes' mode\n pack-bitmap.c: propagate namehash values from existing bitmaps\n midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps\n t5326: test propagating hashcache values\n\n Documentation/config/pack.txt | 4 ++++\n builtin/multi-pack-index.c | 21 ++++++++++++++++++++\n midx.c | 6 +++++-\n midx.h | 1 +\n pack-bitmap.c | 36 +++++++++++++++++++++++++++++------\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 10 +++++++++-\n t/t5326-multi-pack-bitmaps.sh | 32 +++++++++++++++++++++++++++++++\n 8 files changed, 103 insertions(+), 8 deletions(-)\n\n-- \n2.33.0.96.g73915697e6\n"},{"id":"434951","messageId":"918f9b275a1a03b6383e32f28cf1db26a6d67633.1631049462.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631049462.git.me@ttaylorr.com","subject":"[PATCH 1/4] t/helper/test-bitmap.c: add 'dump-hashes' mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-07T21:17:54Z","receivedAt":"2021-09-07T21:18:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The pack-bitmap writer code is about to learn how to propagate values\nfrom an existing hash-cache. To prepare, teach the test-bitmap helper to\ndump the values from a bitmap's hash-cache extension in order to test\nthose changes.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 22 ++++++++++++++++++++++\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 10 +++++++++-\n 3 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fa69ed7a6d..e44af36933 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1744,6 +1744,28 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_hashes(struct repository *r)\n+{\n+\tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n+\tstruct object_id oid;\n+\tuint32_t i;\n+\n+\tif (!bitmap_git->hashes)\n+\t\tgoto cleanup;\n+\n+\tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n+\t\tnth_bitmap_object_oid(bitmap_git, &oid, i);\n+\n+\t\tprintf(\"%s %\"PRIu32\"\\n\",\n+\t\t oid_to_hex(&oid), get_be32(bitmap_git->hashes + i));\n+\t}\n+\n+cleanup:\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int rebuild_bitmap(const uint32_t *reposition,\n \t\t struct ewah_bitmap *source,\n \t\t struct bitmap *dest)\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 81664f933f..b29514418c 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -53,6 +53,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_hashes(struct repository *r);\n struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n \t\t\t\t\t struct list_objects_filter_options *filter,\n \t\t\t\t\t int filter_provided_objects);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 134a1e9d76..ff35f5999b 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -7,6 +7,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_dump_hashes(void)\n+{\n+\treturn test_bitmap_hashes(the_repository);\n+}\n+\n int cmd__bitmap(int argc, const char **argv)\n {\n \tsetup_git_directory();\n@@ -16,9 +21,12 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (!strcmp(argv[1], \"dump-hashes\"))\n+\t\treturn bitmap_dump_hashes();\n \n usage:\n-\tusage(\"\\ttest-tool bitmap list-commits\");\n+\tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap dump-hashes\");\n \n \treturn -1;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"434952","messageId":"fa9f5633f6a0955c045168c80d4d9d7987a8327d.1631049462.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631049462.git.me@ttaylorr.com","subject":"[PATCH 2/4] pack-bitmap.c: propagate namehash values from existing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-07T21:17:58Z","receivedAt":"2021-09-07T21:18:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When an old bitmap exists while writing a new one, we load it and build\na \"reposition\" table which maps bit positions of objects from the old\nbitmap to their respective positions in the new bitmap. This can help\nwhen we encounter a commit which was selected in both the old and new\nbitmap, since we only need to permute its bit (not recompute it from\nscratch).\n\nWe do not, however, repurpose existing namehash values in the case of\nthe hash-cache extension. There has been thus far no good reason to do\nso, since all of the namehash values for objects in the new bitmap would\nbe populated during the traversal that was just performed by\npack-objects when generating single-pack reachability bitmaps.\n\nBut this isn't the case for multi-pack bitmaps, which are written via\n`git multi-pack-index write --bitmap` and do not perform any traversal.\nIn this case all namehash values are set to zero, but we don't even\nbother to check the `pack.writeBitmapHashcache` option anyway, so it\nfails to matter.\n\nThere are two approaches we could take to fill in non-zero hash-cache\nvalues:\n\n - have either the multi-pack-index builtin run its own\n traversal to attempt to fill in some values, or let a hypothetical\n caller (like `pack-objects` when `repack` eventually drives the\n `multi-pack-index` builtin) fill in the values they found during\n their traversal\n\n - or copy any existing namehash values that were stored in an\n existing bitmap to their corresponding positions in the new bitmap\n\nIn a system where a repository is generally repacked with `git repack\n--geometric=` and occasionally repacked with `git repack -a`, the\nhash-cache coverage will tend towards all objects.\n\nSince populating the hash-cache is additive (i.e., doing so only helps\nour delta search), any intermediate lack of full coverage is just fine.\nSo let's start by just propagating any values from the existing\nhash-cache if we see one.\n\nThe next patch will respect the `pack.writeBitmapHashcache` option while\nwriting MIDX bitmaps, and then test this new behavior.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex e44af36933..cb876e7e9d 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1815,18 +1815,20 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < num_objects; ++i) {\n \t\tstruct object_id oid;\n \t\tstruct object_entry *oe;\n+\t\tuint32_t index_pos;\n \n \t\tif (bitmap_is_midx(bitmap_git))\n-\t\t\tnth_midxed_object_oid(&oid,\n-\t\t\t\t\t bitmap_git->midx,\n-\t\t\t\t\t pack_pos_to_midx(bitmap_git->midx, i));\n+\t\t\tindex_pos = pack_pos_to_midx(bitmap_git->midx, i);\n \t\telse\n-\t\t\tnth_packed_object_id(&oid, bitmap_git->pack,\n-\t\t\t\t\t pack_pos_to_index(bitmap_git->pack, i));\n+\t\t\tindex_pos = pack_pos_to_index(bitmap_git->pack, i);\n+\t\tnth_bitmap_object_oid(bitmap_git, &oid, index_pos);\n \t\toe = packlist_find(mapping, &oid);\n \n-\t\tif (oe)\n+\t\tif (oe) {\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\n+\t\t\tif (bitmap_git->hashes && !oe->hash)\n+\t\t\t\toe->hash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t}\n \t}\n \n \treturn reposition;\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"434953","messageId":"be8f47e13c612f2fbe4d5f4f49794529b9424664.1631049462.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631049462.git.me@ttaylorr.com","subject":"[PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-07T21:18:01Z","receivedAt":"2021-09-07T21:18:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, the bitmap writing code learned to propagate\nvalues from an existing hash-cache extension into the bitmap that it is\nwriting.\n\nNow that that functionality exists, let's expose it by teaching the 'git\nmulti-pack-index' builtin to respect the `pack.writeBitmapHashCache`\noption so that the hash-cache may be written at all.\n\nTwo minor points worth noting here:\n\n - The 'git multi-pack-index write' sub-command didn't previously read\n any configuration (instead this is handled in the base command). A\n separate handler is added here to respect this write-specific\n config option.\n\n - I briefly considered adding a 'bitmap_flags' field to the static\n options struct, but decided against it since it would require\n plumbing through a new parameter to the write_midx_file() function.\n\n Instead, a new MIDX-specific flag is added, which is translated to\n the corresponding bitmap one.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/pack.txt | 4 ++++\n builtin/multi-pack-index.c | 21 +++++++++++++++++++++\n midx.c | 6 +++++-\n midx.h | 1 +\n 4 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 763f7af7c4..ad7f73a1ea 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -159,6 +159,10 @@ pack.writeBitmapHashCache::\n \tbetween an older, bitmapped pack and objects that have been\n \tpushed since the last gc). The downside is that it consumes 4\n \tbytes per object of disk space. Defaults to true.\n++\n+When writing a multi-pack reachability bitmap, no new namehashes are\n+computed; instead, any namehashes stored in an existing bitmap are\n+permuted into their appropriate location when writing a new bitmap.\n \n pack.writeReverseIndex::\n \tWhen true, git will write a corresponding .rev file (see:\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 73c0113b48..dd1652531b 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -61,6 +61,23 @@ static struct option *add_common_options(struct option *prev)\n \treturn parse_options_concat(common_opts, prev);\n }\n \n+static int git_multi_pack_index_write_config(const char *var, const char *value,\n+\t\t\t\t\t void *cb)\n+{\n+\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n+\t\tif (git_config_bool(var, value))\n+\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t\telse\n+\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t}\n+\n+\t/*\n+\t * No need to fall-back to 'git_default_config', since this was already\n+\t * called in 'cmd_multi_pack_index()'.\n+\t */\n+\treturn 0;\n+}\n+\n static int cmd_multi_pack_index_write(int argc, const char **argv)\n {\n \tstruct option *options;\n@@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \t\tOPT_END(),\n \t};\n \n+\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\n+\tgit_config(git_multi_pack_index_write_config, NULL);\n+\n \toptions = add_common_options(builtin_multi_pack_index_write_options);\n \n \ttrace2_cmd_mode(argv[0]);\ndiff --git a/midx.c b/midx.c\nindex ccdc3e5702..6c35dcd557 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1008,9 +1008,13 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \tstruct pack_idx_entry **index;\n \tstruct commit **commits = NULL;\n \tuint32_t i, commits_nr;\n+\tuint16_t options = 0;\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n \tint ret;\n \n+\tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n+\t\toptions |= BITMAP_OPT_HASH_CACHE;\n+\n \tprepare_midx_packing_data(&pdata, ctx);\n \n \tcommits = find_commits_for_midx_bitmap(&commits_nr, ctx);\n@@ -1049,7 +1053,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \t\tgoto cleanup;\n \n \tbitmap_writer_set_checksum(midx_hash);\n-\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, 0);\n+\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, options);\n \n cleanup:\n \tfree(index);\ndiff --git a/midx.h b/midx.h\nindex aa3da557bb..541d9ac728 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -44,6 +44,7 @@ struct multi_pack_index {\n #define MIDX_PROGRESS (1 << 0)\n #define MIDX_WRITE_REV_INDEX (1 << 1)\n #define MIDX_WRITE_BITMAP (1 << 2)\n+#define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n \n const unsigned char *get_midx_checksum(struct multi_pack_index *m);\n char *get_midx_filename(const char *object_dir);\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"434954","messageId":"acf3ec60cb6f151a9f121d242f38fef6711cced4.1631049462.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631049462.git.me@ttaylorr.com","subject":"[PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-07T21:18:05Z","receivedAt":"2021-09-07T21:18:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that we both can propagate values from the hashcache, and respect\nthe configuration to enable the hashcache at all, test that both of\nthese function correctly by hardening their behavior with a test.\n\nSigned-off-by: Taylor Blau \n---\n t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 4ad7c2c969..24148ca35b 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '\n \t)\n '\n \n+test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n+\trm -fr repo &&\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config pack.writeBitmapHashCache true &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit base2 &&\n+\t\tgit repack -adb &&\n+\n+\t\ttest-tool bitmap dump-hashes >pack.raw &&\n+\t\ttest_file_not_empty pack.raw &&\n+\t\tsort pack.raw >pack.hashes &&\n+\n+\t\ttest_commit new &&\n+\t\tgit repack &&\n+\t\tgit multi-pack-index write --bitmap &&\n+\n+\t\ttest-tool bitmap dump-hashes >midx.raw &&\n+\t\tsort midx.raw >midx.hashes &&\n+\n+\t\t# ensure that every namehash in the pack bitmap can be found in\n+\t\t# the midx bitmap (i.e., that there are no oid-namehash pairs\n+\t\t# unique to the pack bitmap).\n+\t\tcomm -23 pack.hashes midx.hashes >dropped.hashes &&\n+\t\ttest_must_be_empty dropped.hashes\n+\t)\n+'\n+\n test_done\n-- \n2.33.0.96.g73915697e6\n"},{"id":"434989","messageId":"878s07kfsg.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"918f9b275a1a03b6383e32f28cf1db26a6d67633.1631049462.git.me@ttaylorr.com","subject":"Re: [PATCH 1/4] t/helper/test-bitmap.c: add 'dump-hashes' mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T01:37:05Z","receivedAt":"2021-09-08T01:38:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 07 2021, Taylor Blau wrote:\n\n> +int test_bitmap_hashes(struct repository *r)\n> +{\n> +\tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n> +\tstruct object_id oid;\n> +\tuint32_t i;\n> +\n> +\tif (!bitmap_git->hashes)\n> +\t\tgoto cleanup;\n> +\n> +\tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n> +\t\tnth_bitmap_object_oid(bitmap_git, &oid, i);\n> +\n> +\t\tprintf(\"%s %\"PRIu32\"\\n\",\n> +\t\t oid_to_hex(&oid), get_be32(bitmap_git->hashes + i));\n> +\t}\n> +\n> +cleanup:\n> +\tfree_bitmap_index(bitmap_git);\n> +\n> +\treturn 0;\n> +}\n\nSeems like it should return void, it doesn't ever return non-0, nor in\nthe rest of this series....\n\n> +static int bitmap_dump_hashes(void)\n> +{\n> +\treturn test_bitmap_hashes(the_repository);\n> +}\n> [...]\n> \t\treturn bitmap_list_commits();\n> +\tif (!strcmp(argv[1], \"dump-hashes\"))\n> +\t\treturn bitmap_dump_hashes();\n\nPerhaps the return code only for the brevity of this test-only code?\nSeems like having bitmap_dump_hashes() do the \"return 0\" would be better\nin that case.\n"},{"id":"435000","messageId":"874kavkfjg.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"be8f47e13c612f2fbe4d5f4f49794529b9424664.1631049462.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T01:40:19Z","receivedAt":"2021-09-08T01:43:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 07 2021, Taylor Blau wrote:\n\n> +static int git_multi_pack_index_write_config(const char *var, const char *value,\n> +\t\t\t\t\t void *cb)\n> +{\n> +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n> +\t\tif (git_config_bool(var, value))\n> +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> +\t\telse\n> +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n> +\t}\n> +\n> +\t/*\n> +\t * No need to fall-back to 'git_default_config', since this was already\n> +\t * called in 'cmd_multi_pack_index()'.\n> +\t */\n> +\treturn 0;\n> +}\n> +\n> static int cmd_multi_pack_index_write(int argc, const char **argv)\n> {\n> \tstruct option *options;\n> @@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n> \t\tOPT_END(),\n> \t};\n> \n> +\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> +\n> +\tgit_config(git_multi_pack_index_write_config, NULL);\n> +\n\nSince this is a write-only config option it would seem more logical to\njust call git_config() once, and have a git_multip_pack_index_config,\nwhich then would fall back on git_default_config, so we iterate it once,\nand no need for a comment about the oddity.\n"},{"id":"435005","messageId":"87zgsnj0q0.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"acf3ec60cb6f151a9f121d242f38fef6711cced4.1631049462.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T01:46:29Z","receivedAt":"2021-09-08T01:50:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 07 2021, Taylor Blau wrote:\n\n> Now that we both can propagate values from the hashcache, and respect\n> the configuration to enable the hashcache at all, test that both of\n> these function correctly by hardening their behavior with a test.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++\n> 1 file changed, 32 insertions(+)\n>\n> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> index 4ad7c2c969..24148ca35b 100755\n> --- a/t/t5326-multi-pack-bitmaps.sh\n> +++ b/t/t5326-multi-pack-bitmaps.sh\n> @@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '\n> \t)\n> '\n> \n> +test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n> +\trm -fr repo &&\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n\nIt seems the need for this \"rm -fr repo\" dance instead of just relying\non test_when_finished \"rm -rf repo\" is because of a 3x tests in a\nfunction in tb/multi-pack-bitmaps that should probably be combined into\none, i.e. they share the same logical \"repo\" setup.\n\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\tgit config pack.writeBitmapHashCache true &&\n\ns/git config/test_config/, surely.\n"},{"id":"435010","messageId":"YTge2NlvvRC+VoPY@nand.local","threadId":"56455","inReplyTo":"878s07kfsg.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/4] t/helper/test-bitmap.c: add 'dump-hashes' mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-08T02:24:24Z","receivedAt":"2021-09-08T02:24:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 08, 2021 at 03:37:05AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > +static int bitmap_dump_hashes(void)\n> > +{\n> > +\treturn test_bitmap_hashes(the_repository);\n> > +}\n> > [...]\n> > \t\treturn bitmap_list_commits();\n> > +\tif (!strcmp(argv[1], \"dump-hashes\"))\n> > +\t\treturn bitmap_dump_hashes();\n>\n> Perhaps the return code only for the brevity of this test-only code?\n> Seems like having bitmap_dump_hashes() do the \"return 0\" would be better\n> in that case.\n\nYeah, it is silly to just return a constant from bitmap_dump_hashes\n(and ditto for bitmap_list_commits), but it makes this easier to write.\n\nI don't care either way. If you feel strongly, then please say so (but I\ncan't imagine that you do).\n\nThanks,\nTaylor\n"},{"id":"435011","messageId":"YTgfuAKJv1rXXm7i@nand.local","threadId":"56455","inReplyTo":"874kavkfjg.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-08T02:28:08Z","receivedAt":"2021-09-08T02:28:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 08, 2021 at 03:40:19AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Tue, Sep 07 2021, Taylor Blau wrote:\n>\n> > +static int git_multi_pack_index_write_config(const char *var, const char *value,\n> > +\t\t\t\t\t void *cb)\n> > +{\n> > +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n> > +\t\tif (git_config_bool(var, value))\n> > +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> > +\t\telse\n> > +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * No need to fall-back to 'git_default_config', since this was already\n> > +\t * called in 'cmd_multi_pack_index()'.\n> > +\t */\n> > +\treturn 0;\n> > +}\n> > +\n> > static int cmd_multi_pack_index_write(int argc, const char **argv)\n> > {\n> > \tstruct option *options;\n> > @@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n> > \t\tOPT_END(),\n> > \t};\n> >\n> > +\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> > +\n> > +\tgit_config(git_multi_pack_index_write_config, NULL);\n> > +\n>\n> Since this is a write-only config option it would seem more logical to\n> just call git_config() once, and have a git_multip_pack_index_config,\n> which then would fall back on git_default_config, so we iterate it once,\n> and no need for a comment about the oddity.\n\nPerhaps, but I'm not crazy about each sub-command having to call\ngit_config() itself when 'write' is the only one that actually has any\nvalues to read.\n\nFWIW, the commit-graph builtin does the same thing as is written here\n(calling git_config() twice, once in cmd_commit_graph() with\ngit_default_config as the callback and again in cmd_commit_graph_write()\nwith git_commit_graph_write_config as the callback).\n\nSo I'm not opposed to cleaning it up, but I'd rather be consistent with\nthe existing behavior. To be honest, I'm not at all convinced that\nreading the config twice is a bottleneck here when compared to\ngenerating a MIDX.\n\nThanks,\nTaylor\n"},{"id":"435012","messageId":"YTggYXtyS/AW4JRF@nand.local","threadId":"56455","inReplyTo":"87zgsnj0q0.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-08T02:30:57Z","receivedAt":"2021-09-08T02:31:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 08, 2021 at 03:46:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Tue, Sep 07 2021, Taylor Blau wrote:\n>\n> > Now that we both can propagate values from the hashcache, and respect\n> > the configuration to enable the hashcache at all, test that both of\n> > these function correctly by hardening their behavior with a test.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++\n> > 1 file changed, 32 insertions(+)\n> >\n> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> > index 4ad7c2c969..24148ca35b 100755\n> > --- a/t/t5326-multi-pack-bitmaps.sh\n> > +++ b/t/t5326-multi-pack-bitmaps.sh\n> > @@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '\n> > \t)\n> > '\n> >\n> > +test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n> > +\trm -fr repo &&\n> > +\tgit init repo &&\n> > +\ttest_when_finished \"rm -fr repo\" &&\n>\n> It seems the need for this \"rm -fr repo\" dance instead of just relying\n> on test_when_finished \"rm -rf repo\" is because of a 3x tests in a\n> function in tb/multi-pack-bitmaps that should probably be combined into\n> one, i.e. they share the same logical \"repo\" setup.\n\nYeah. There's definitely room for clean-up there if we want to have each\nof the tests operate on the same repo. I have always found sharing a\nrepo between tests difficult to reason about, since we have to assume\nthat any --run parameters could be passed in.\n\nSo I have tended to err on the side of creating and throwing away a new\nrepo per test, but I understand that's somewhat atypical for Git's\ntests.\n\n> > +\t(\n> > +\t\tcd repo &&\n> > +\n> > +\t\tgit config pack.writeBitmapHashCache true &&\n>\n> s/git config/test_config/, surely.\n\nNo, since this is in a subshell (and we don't care about unsetting the\nvalue of a config in a repo that we're going to throw away, anyways).\n\n(Side-note: since this has a /bin/sh shebang, we can't detect that we're\nin a subshell and so test_config actually _would_ work here. But\nswitching this test to run with /bin/bash where we can detect whether or\nnot we're in a subshell would cause this test to fail with test_config).\n\nThanks,\nTaylor\n"},{"id":"435221","messageId":"87zgsmdu6d.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"YTgfuAKJv1rXXm7i@nand.local","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T08:18:10Z","receivedAt":"2021-09-09T08:34:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 07 2021, Taylor Blau wrote:\n\n> On Wed, Sep 08, 2021 at 03:40:19AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Tue, Sep 07 2021, Taylor Blau wrote:\n>>\n>> > +static int git_multi_pack_index_write_config(const char *var, const char *value,\n>> > +\t\t\t\t\t void *cb)\n>> > +{\n>> > +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n>> > +\t\tif (git_config_bool(var, value))\n>> > +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n>> > +\t\telse\n>> > +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n>> > +\t}\n>> > +\n>> > +\t/*\n>> > +\t * No need to fall-back to 'git_default_config', since this was already\n>> > +\t * called in 'cmd_multi_pack_index()'.\n>> > +\t */\n>> > +\treturn 0;\n>> > +}\n>> > +\n>> > static int cmd_multi_pack_index_write(int argc, const char **argv)\n>> > {\n>> > \tstruct option *options;\n>> > @@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>> > \t\tOPT_END(),\n>> > \t};\n>> >\n>> > +\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n>> > +\n>> > +\tgit_config(git_multi_pack_index_write_config, NULL);\n>> > +\n>>\n>> Since this is a write-only config option it would seem more logical to\n>> just call git_config() once, and have a git_multip_pack_index_config,\n>> which then would fall back on git_default_config, so we iterate it once,\n>> and no need for a comment about the oddity.\n>\n> Perhaps, but I'm not crazy about each sub-command having to call\n> git_config() itself when 'write' is the only one that actually has any\n> values to read.\n>\n> FWIW, the commit-graph builtin does the same thing as is written here\n> (calling git_config() twice, once in cmd_commit_graph() with\n> git_default_config as the callback and again in cmd_commit_graph_write()\n> with git_commit_graph_write_config as the callback).\n\nI didn't notice your earlier d356d5debe5 (commit-graph: introduce\n'commitGraph.maxNewFilters', 2020-09-17). As an aside the test added in\nthat commit seems to be broken or not testing that code change at all,\nif I comment out the git_config(git_commit_graph_write_config, &opts)\nit'll pass.\n\nAs a comment on this series I'd find 4/4 squashed into 3/4 easier to\nread, when I did a \"git blame\" and found d356d5debe5 I discovered the\ntest right away, if and when this gets merged someone might do the same,\nbut not find the test as easily (they'd probably then grep the config\nvariable name and find it eventually...).\n\nMore importantly, the same issue with the commit-graph test seems to be\nthe case here, if I comment out the added config reading code it'll\nstill pass, it seems to be testing something, but not that the config is\nbeing read.\n\n> So I'm not opposed to cleaning it up, but I'd rather be consistent with\n> the existing behavior. To be honest, I'm not at all convinced that\n> reading the config twice is a bottleneck here when compared to\n> generating a MIDX.\n\nIt's never going to matter at all for performance, I should have been\nclearer with my comments. I meant them purely as a \"this code is hard to\nfollow\" comment.\n\nI.e. since we read the config twice, and in both commit-graph.c and\nmulti-pack-index.c munge and write to the \"opts\" struct on\nparse_options(), you'll need to follow logic like:\n\n 1. Read config in cmd_X(), might set variable xyz\n 2. Do parse_options() in cmd_X(), might set variable xyz also\n 3. Now in cmd_X_subcmd(), read config, might set variable xyz\n 4. Do parse_options() in cmd_X(), migh set variable xyz also\n\nOf course in this case the relevant opts.flags only matters for the\n\"write\" subcommand, so on more careful reading we don't need to worry\nabout the value flip-flopping between config defaults and getopts\nsettings, but just in terms of establishing a pattern we'll be following\nin the subcommand built-ins I think this is setting us up for more\ncomplexity than is needed.\n\nAs far as being consistent with existing behavior, in git-worktree,\ngit-stash which are both similarly structured subcommands we follow the\npattern of calling git_config() once, it seems to me better to follow\nthat pattern than the one in d356d5debe5 if the config can be\nunambiguously parsed in one pass.\n"},{"id":"435224","messageId":"87v93adr8r.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"87zgsmdu6d.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T09:34:16Z","receivedAt":"2021-09-09T09:37:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 09 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Sep 07 2021, Taylor Blau wrote:\n>\n>> On Wed, Sep 08, 2021 at 03:40:19AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> On Tue, Sep 07 2021, Taylor Blau wrote:\n>>>\n>>> > +static int git_multi_pack_index_write_config(const char *var, const char *value,\n>>> > +\t\t\t\t\t void *cb)\n>>> > +{\n>>> > +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n>>> > +\t\tif (git_config_bool(var, value))\n>>> > +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n>>> > +\t\telse\n>>> > +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n>>> > +\t}\n>>> > +\n>>> > +\t/*\n>>> > +\t * No need to fall-back to 'git_default_config', since this was already\n>>> > +\t * called in 'cmd_multi_pack_index()'.\n>>> > +\t */\n>>> > +\treturn 0;\n>>> > +}\n>>> > +\n>>> > static int cmd_multi_pack_index_write(int argc, const char **argv)\n>>> > {\n>>> > \tstruct option *options;\n>>> > @@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n>>> > \t\tOPT_END(),\n>>> > \t};\n>>> >\n>>> > +\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n>>> > +\n>>> > +\tgit_config(git_multi_pack_index_write_config, NULL);\n>>> > +\n>>>\n>>> Since this is a write-only config option it would seem more logical to\n>>> just call git_config() once, and have a git_multip_pack_index_config,\n>>> which then would fall back on git_default_config, so we iterate it once,\n>>> and no need for a comment about the oddity.\n>>\n>> Perhaps, but I'm not crazy about each sub-command having to call\n>> git_config() itself when 'write' is the only one that actually has any\n>> values to read.\n>>\n>> FWIW, the commit-graph builtin does the same thing as is written here\n>> (calling git_config() twice, once in cmd_commit_graph() with\n>> git_default_config as the callback and again in cmd_commit_graph_write()\n>> with git_commit_graph_write_config as the callback).\n>\n> I didn't notice your earlier d356d5debe5 (commit-graph: introduce\n> 'commitGraph.maxNewFilters', 2020-09-17). As an aside the test added in\n> that commit seems to be broken or not testing that code change at all,\n> if I comment out the git_config(git_commit_graph_write_config, &opts)\n> it'll pass.\n>\n> As a comment on this series I'd find 4/4 squashed into 3/4 easier to\n> read, when I did a \"git blame\" and found d356d5debe5 I discovered the\n> test right away, if and when this gets merged someone might do the same,\n> but not find the test as easily (they'd probably then grep the config\n> variable name and find it eventually...).\n>\n> More importantly, the same issue with the commit-graph test seems to be\n> the case here, if I comment out the added config reading code it'll\n> still pass, it seems to be testing something, but not that the config is\n> being read.\n>\n>> So I'm not opposed to cleaning it up, but I'd rather be consistent with\n>> the existing behavior. To be honest, I'm not at all convinced that\n>> reading the config twice is a bottleneck here when compared to\n>> generating a MIDX.\n>\n> It's never going to matter at all for performance, I should have been\n> clearer with my comments. I meant them purely as a \"this code is hard to\n> follow\" comment.\n>\n> I.e. since we read the config twice, and in both commit-graph.c and\n> multi-pack-index.c munge and write to the \"opts\" struct on\n> parse_options(), you'll need to follow logic like:\n>\n> 1. Read config in cmd_X(), might set variable xyz\n> 2. Do parse_options() in cmd_X(), might set variable xyz also\n> 3. Now in cmd_X_subcmd(), read config, might set variable xyz\n> 4. Do parse_options() in cmd_X(), migh set variable xyz also\n>\n> Of course in this case the relevant opts.flags only matters for the\n> \"write\" subcommand, so on more careful reading we don't need to worry\n> about the value flip-flopping between config defaults and getopts\n> settings, but just in terms of establishing a pattern we'll be following\n> in the subcommand built-ins I think this is setting us up for more\n> complexity than is needed.\n>\n> As far as being consistent with existing behavior, in git-worktree,\n> git-stash which are both similarly structured subcommands we follow the\n> pattern of calling git_config() once, it seems to me better to follow\n> that pattern than the one in d356d5debe5 if the config can be\n> unambiguously parsed in one pass.\n\nIn similar spirit as my\nhttps://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/ I\nstarted seeing if not doing the flags via getopt but instead variables &\nsetting the flags later was better, and came up with this on top. Not\nfor this series, more to muse on how we can write these subcommands in a\nsimpler manner (or not).\n\nI may have discovered a subtle bug in the process, in\ncmd_multi_pack_index_repack() we end up calling write_midx_internal(),\nwhich cares about MIDX_WRITE_REV_INDEX, but only\ncmd_multi_pack_index_write() will set that flag, both before & after my\npatch. Are we using the wrong flags during repack as a result?\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex dd1652531bf..1b97b2ee4e1 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -45,14 +45,16 @@ static char const * const builtin_multi_pack_index_usage[] = {\n static struct opts_multi_pack_index {\n \tconst char *object_dir;\n \tconst char *preferred_pack;\n-\tunsigned long batch_size;\n-\tunsigned flags;\n-} opts;\n+\tint progress;\n+\tint write_bitmap_hash_cache;\n+} opts = {\n+\t.write_bitmap_hash_cache = -1,\n+};\n \n static struct option common_opts[] = {\n \tOPT_FILENAME(0, \"object-dir\", &opts.object_dir,\n \t N_(\"object directory containing set of packfile and pack-index pairs\")),\n-\tOPT_BIT(0, \"progress\", &opts.flags, N_(\"force progress reporting\"), MIDX_PROGRESS),\n+\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \tOPT_END(),\n };\n \n@@ -61,38 +63,29 @@ static struct option *add_common_options(struct option *prev)\n \treturn parse_options_concat(common_opts, prev);\n }\n \n-static int git_multi_pack_index_write_config(const char *var, const char *value,\n-\t\t\t\t\t void *cb)\n+static int git_multi_pack_index_config(const char *var, const char *value,\n+\t\t\t\t void *cb)\n {\n \tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n-\t\tif (git_config_bool(var, value))\n-\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n-\t\telse\n-\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t\topts.write_bitmap_hash_cache = git_config_bool(var, value);\n+\t\treturn 0;\n \t}\n \n-\t/*\n-\t * No need to fall-back to 'git_default_config', since this was already\n-\t * called in 'cmd_multi_pack_index()'.\n-\t */\n-\treturn 0;\n+\treturn git_default_config(var, value, NULL);\n }\n \n static int cmd_multi_pack_index_write(int argc, const char **argv)\n {\n \tstruct option *options;\n+\tstatic int write_bitmap = 0;\n \tstatic struct option builtin_multi_pack_index_write_options[] = {\n \t\tOPT_STRING(0, \"preferred-pack\", &opts.preferred_pack,\n \t\t\t N_(\"preferred-pack\"),\n \t\t\t N_(\"pack for reuse when computing a multi-pack bitmap\")),\n-\t\tOPT_BIT(0, \"bitmap\", &opts.flags, N_(\"write multi-pack bitmap\"),\n-\t\t\tMIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX),\n+\t\tOPT_BOOL(0, \"bitmap\", &write_bitmap, N_(\"write multi-pack bitmap\")),\n \t\tOPT_END(),\n \t};\n-\n-\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n-\n-\tgit_config(git_multi_pack_index_write_config, NULL);\n+\tunsigned flags = 0;\n \n \toptions = add_common_options(builtin_multi_pack_index_write_options);\n \n@@ -107,8 +100,15 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \n \tFREE_AND_NULL(options);\n \n-\treturn write_midx_file(opts.object_dir, opts.preferred_pack,\n-\t\t\t opts.flags);\n+\tif (opts.progress)\n+\t\tflags |= MIDX_PROGRESS;\n+\t/* Both -1 default and 1 via config */\n+\tif (!opts.write_bitmap_hash_cache)\n+\t\tflags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\tif (write_bitmap)\n+\t\tflags |= MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX;\n+\n+\treturn write_midx_file(opts.object_dir, opts.preferred_pack, flags);\n }\n \n static int cmd_multi_pack_index_verify(int argc, const char **argv)\n@@ -124,7 +124,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)\n \t\tusage_with_options(builtin_multi_pack_index_verify_usage,\n \t\t\t\t options);\n \n-\treturn verify_midx_file(the_repository, opts.object_dir, opts.flags);\n+\treturn verify_midx_file(the_repository, opts.object_dir, opts.progress);\n }\n \n static int cmd_multi_pack_index_expire(int argc, const char **argv)\n@@ -140,14 +140,15 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)\n \t\tusage_with_options(builtin_multi_pack_index_expire_usage,\n \t\t\t\t options);\n \n-\treturn expire_midx_packs(the_repository, opts.object_dir, opts.flags);\n+\treturn expire_midx_packs(the_repository, opts.object_dir, opts.progress);\n }\n \n static int cmd_multi_pack_index_repack(int argc, const char **argv)\n {\n+\tstatic unsigned long batch_size = 0;\n \tstruct option *options;\n \tstatic struct option builtin_multi_pack_index_repack_options[] = {\n-\t\tOPT_MAGNITUDE(0, \"batch-size\", &opts.batch_size,\n+\t\tOPT_MAGNITUDE(0, \"batch-size\", &batch_size,\n \t\t N_(\"during repack, collect pack-files of smaller size into a batch that is larger than this size\")),\n \t\tOPT_END(),\n \t};\n@@ -167,7 +168,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv)\n \tFREE_AND_NULL(options);\n \n \treturn midx_repack(the_repository, opts.object_dir,\n-\t\t\t (size_t)opts.batch_size, opts.flags);\n+\t\t\t (size_t)batch_size,\n+\t\t\t opts.progress ? MIDX_PROGRESS : 0);\n }\n \n int cmd_multi_pack_index(int argc, const char **argv,\n@@ -175,10 +177,10 @@ int cmd_multi_pack_index(int argc, const char **argv,\n {\n \tstruct option *builtin_multi_pack_index_options = common_opts;\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_multi_pack_index_config, NULL);\n \n \tif (isatty(2))\n-\t\topts.flags |= MIDX_PROGRESS;\n+\t\topts.progress = 1;\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_multi_pack_index_options,\n \t\t\t builtin_multi_pack_index_usage,\ndiff --git a/midx.c b/midx.c\nindex 6c35dcd557c..3e722888d69 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1482,7 +1482,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(struct repository *r, const char *object_dir, unsigned flags)\n+int verify_midx_file(struct repository *r, const char *object_dir, int opt_progress)\n {\n \tstruct pair_pos_vs_id *pairs = NULL;\n \tuint32_t i;\n@@ -1505,7 +1505,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \tif (!midx_checksum_valid(m))\n \t\tmidx_report(_(\"incorrect checksum\"));\n \n-\tif (flags & MIDX_PROGRESS)\n+\tif (opt_progress)\n \t\tprogress = start_delayed_progress(_(\"Looking for referenced packfiles\"),\n \t\t\t\t\t m->num_packs);\n \tfor (i = 0; i < m->num_packs; i++) {\n@@ -1534,7 +1534,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\treturn verify_midx_error;\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n+\tif (opt_progress)\n \t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n \t\t\t\t\t\t m->num_objects - 1);\n \tfor (i = 0; i < m->num_objects - 1; i++) {\n@@ -1563,14 +1563,14 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\tpairs[i].pack_int_id = nth_midxed_pack_int_id(m, i);\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n+\tif (opt_progress)\n \t\tprogress = start_sparse_progress(_(\"Sorting objects by packfile\"),\n \t\t\t\t\t\t m->num_objects);\n \tdisplay_progress(progress, 0); /* TODO: Measure QSORT() progress */\n \tQSORT(pairs, m->num_objects, compare_pair_pos_vs_id);\n \tstop_progress(&progress);\n \n-\tif (flags & MIDX_PROGRESS)\n+\tif (opt_progress)\n \t\tprogress = start_sparse_progress(_(\"Verifying object offsets\"), m->num_objects);\n \tfor (i = 0; i < m->num_objects; i++) {\n \t\tstruct object_id oid;\ndiff --git a/midx.h b/midx.h\nindex 541d9ac728d..0dfe6a54ef3 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -64,7 +64,7 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, i\n \n int write_midx_file(const char *object_dir, const char *preferred_pack_name, unsigned flags);\n void clear_midx_file(struct repository *r);\n-int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags);\n+int verify_midx_file(struct repository *r, const char *object_dir, int opt_progress);\n int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flags);\n int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags);\n \n"},{"id":"435271","messageId":"YToeZIIrFH42BRe7@nand.local","threadId":"56455","inReplyTo":"87zgsmdu6d.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-09T14:47:00Z","receivedAt":"2021-09-09T14:49:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 09, 2021 at 10:18:10AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Tue, Sep 07 2021, Taylor Blau wrote:\n>\n> > On Wed, Sep 08, 2021 at 03:40:19AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >>\n> >> On Tue, Sep 07 2021, Taylor Blau wrote:\n> >>\n> >> > +static int git_multi_pack_index_write_config(const char *var, const char *value,\n> >> > +\t\t\t\t\t void *cb)\n> >> > +{\n> >> > +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n> >> > +\t\tif (git_config_bool(var, value))\n> >> > +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> >> > +\t\telse\n> >> > +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n> >> > +\t}\n> >> > +\n> >> > +\t/*\n> >> > +\t * No need to fall-back to 'git_default_config', since this was already\n> >> > +\t * called in 'cmd_multi_pack_index()'.\n> >> > +\t */\n> >> > +\treturn 0;\n> >> > +}\n> >> > +\n> >> > static int cmd_multi_pack_index_write(int argc, const char **argv)\n> >> > {\n> >> > \tstruct option *options;\n> >> > @@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n> >> > \t\tOPT_END(),\n> >> > \t};\n> >> >\n> >> > +\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> >> > +\n> >> > +\tgit_config(git_multi_pack_index_write_config, NULL);\n> >> > +\n> >>\n> >> Since this is a write-only config option it would seem more logical to\n> >> just call git_config() once, and have a git_multip_pack_index_config,\n> >> which then would fall back on git_default_config, so we iterate it once,\n> >> and no need for a comment about the oddity.\n> >\n> > Perhaps, but I'm not crazy about each sub-command having to call\n> > git_config() itself when 'write' is the only one that actually has any\n> > values to read.\n> >\n> > FWIW, the commit-graph builtin does the same thing as is written here\n> > (calling git_config() twice, once in cmd_commit_graph() with\n> > git_default_config as the callback and again in cmd_commit_graph_write()\n> > with git_commit_graph_write_config as the callback).\n>\n> I didn't notice your earlier d356d5debe5 (commit-graph: introduce\n> 'commitGraph.maxNewFilters', 2020-09-17). As an aside the test added in\n> that commit seems to be broken or not testing that code change at all,\n> if I comment out the git_config(git_commit_graph_write_config, &opts)\n> it'll pass.\n\nThat makes sense; the test that d356d5debe5 added is ensuring that the\noption `--max-new-filters` overrides any configured value of\n`commitGraph.maxNewFilters` (so not reading the configuration would be\nfine there).\n\n> More importantly, the same issue with the commit-graph test seems to be\n> the case here, if I comment out the added config reading code it'll\n> still pass, it seems to be testing something, but not that the config is\n> being read.\n\nI think this also makes sense; since MIDX_WRITE_BITMAP_HASH_CACHE is the\ndefault and is set in cmd_multi_pack_index_write(). So it may be worth\nadding a test to say \"make sure the hash-cache _isn't_ written when I:\n\n git config pack.writeBitmapHashCache &&\n git multi-pack-index write --bitmap\n\nBut I don't feel strongly about it (hence I didn't write such a test in\nthe original version which I sent here). If you think it would be\nhelpful in a newer version, I'm happy to add it.\n\nThanks,\nTaylor\n"},{"id":"435272","messageId":"YTogUpdtjsFlbqYM@nand.local","threadId":"56455","inReplyTo":"87v93adr8r.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-09T14:55:14Z","receivedAt":"2021-09-09T14:55:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 09, 2021 at 11:34:16AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> In similar spirit as my\n> https://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/ I\n> started seeing if not doing the flags via getopt but instead variables &\n> setting the flags later was better, and came up with this on top. Not\n> for this series, more to muse on how we can write these subcommands in a\n> simpler manner (or not).\n\nSure, I think that everything you wrote here is correct. I don't have a\nstrong opinion, really, but one benefit of not manipulating a single\n'flags' int is that we don't have to do stuff like:\n\n if (git_config_bool(var, value))\n opts.flags |= FLAG_FOO;\n else\n opts.flags &= ~FLAG_FOO;\n\nand instead can write `opts.foo = git_config_bool(var, value)`.\n\nOf course, the trade-off is that you later have to turn `opts.foo` into\nflags at some point (or pass each option as an individual parameter). So\nnothing's free, and I see it as a toss-up between which is easier to\nread and write.\n\n> I may have discovered a subtle bug in the process, in\n> cmd_multi_pack_index_repack() we end up calling write_midx_internal(),\n> which cares about MIDX_WRITE_REV_INDEX, but only\n> cmd_multi_pack_index_write() will set that flag, both before & after my\n> patch. Are we using the wrong flags during repack as a result?\n\nOnly the `write` sub-command would ever want to set that flag, since we\ndon't support writing a bitmap after `expire`. So that part seems right,\nbut perhaps there is a another problem you're seeing?\n\nThanks,\nTaylor\n"},{"id":"435278","messageId":"8735qdeoa0.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"YTogUpdtjsFlbqYM@nand.local","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-09T15:50:47Z","receivedAt":"2021-09-09T15:56:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 09 2021, Taylor Blau wrote:\n\n> On Thu, Sep 09, 2021 at 11:34:16AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> In similar spirit as my\n>> https://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/ I\n>> started seeing if not doing the flags via getopt but instead variables &\n>> setting the flags later was better, and came up with this on top. Not\n>> for this series, more to muse on how we can write these subcommands in a\n>> simpler manner (or not).\n>\n> Sure, I think that everything you wrote here is correct. I don't have a\n> strong opinion, really, but one benefit of not manipulating a single\n> 'flags' int is that we don't have to do stuff like:\n>\n> if (git_config_bool(var, value))\n> opts.flags |= FLAG_FOO;\n> else\n> opts.flags &= ~FLAG_FOO;\n>\n> and instead can write `opts.foo = git_config_bool(var, value)`.\n\n*nod*\n\n> Of course, the trade-off is that you later have to turn `opts.foo` into\n> flags at some point (or pass each option as an individual parameter). So\n> nothing's free, and I see it as a toss-up between which is easier to\n> read and write.\n\nI think that trade-off is usually a benefit, also in the pack-write.c\netc. case, i.e. you enforce a clear boundary between the built-in and\nthe underlying API, and don't have to e.g. wonder if some write flag is\nhandled by verify() (which will just care about the progress flag), as\nyou pass some moral equivalent of a \"struct\nall_the_options_for_all_the_things\" around between all of them.\n\nI realize trying to solve that problem from a different angle may be how\nyou ended up going for per-subcommand config reading....\n\n>> I may have discovered a subtle bug in the process, in\n>> cmd_multi_pack_index_repack() we end up calling write_midx_internal(),\n>> which cares about MIDX_WRITE_REV_INDEX, but only\n>> cmd_multi_pack_index_write() will set that flag, both before & after my\n>> patch. Are we using the wrong flags during repack as a result?\n>\n> Only the `write` sub-command would ever want to set that flag, since we\n> don't support writing a bitmap after `expire`. So that part seems right,\n> but perhaps there is a another problem you're seeing?\n\nIn midx_repack() we'll call write_midx_internal(). That function gets\nthe \"flags\" we pass to midx_repack() and will check\nMIDX_WRITE_REV_INDEX. I haven't checked whether we actually reach that,\nbut that's what I was wondering, i.e. whether the repack routine would\n\"write\" when repacking, and we missed the flag option there.\n"},{"id":"435279","messageId":"YTo0+FzYjpYG7B5I@nand.local","threadId":"56455","inReplyTo":"8735qdeoa0.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-09T16:23:20Z","receivedAt":"2021-09-09T16:23:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 09, 2021 at 05:50:47PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> I may have discovered a subtle bug in the process, in\n> >> cmd_multi_pack_index_repack() we end up calling write_midx_internal(),\n> >> which cares about MIDX_WRITE_REV_INDEX, but only\n> >> cmd_multi_pack_index_write() will set that flag, both before & after my\n> >> patch. Are we using the wrong flags during repack as a result?\n> >\n> > Only the `write` sub-command would ever want to set that flag, since we\n> > don't support writing a bitmap after `expire`. So that part seems right,\n> > but perhaps there is a another problem you're seeing?\n>\n> In midx_repack() we'll call write_midx_internal(). That function gets\n> the \"flags\" we pass to midx_repack() and will check\n> MIDX_WRITE_REV_INDEX. I haven't checked whether we actually reach that,\n> but that's what I was wondering, i.e. whether the repack routine would\n> \"write\" when repacking, and we missed the flag option there.\n\nI don't think it's a problem in practice. We would never have\nMIDX_WRITE_REV_INDEX set when executing cmd_multi_pack_index_repack(),\n(and the same is true for all other subcommands besides `write`) because\nthe default value for flags is just MIDX_PROGRESS (if isatty(2)), and we\nonly add the WRITE_REV_INDEX bit from within the write handler.\n\nMore generally, that is to say \"we only support writing a bitmap from\nthe `write` sub-command\". There is no reason that we couldn't lift this\nlimitation and support writing a bitmap on the resulting MIDX after\n`expire` or `repack` we just haven't done so.\n\nBut I don't see any problems with not getting the right flags, etc.\n\nThanks,\nTaylor\n"},{"id":"435677","messageId":"xmqqee9tia2b.fsf@gitster.g","threadId":"56455","inReplyTo":"be8f47e13c612f2fbe4d5f4f49794529b9424664.1631049462.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T00:38:36Z","receivedAt":"2021-09-13T00:38:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> +static int git_multi_pack_index_write_config(const char *var, const char *value,\n> +\t\t\t\t\t void *cb)\n> +{\n> +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n> +\t\tif (git_config_bool(var, value))\n> +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> +\t\telse\n> +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n> +\t}\n> +\n> +\t/*\n> +\t * No need to fall-back to 'git_default_config', since this was already\n> +\t * called in 'cmd_multi_pack_index()'.\n> +\t */\n\nIt's probably not just \"No need to\", but calling default_config() or\nany \"more generic\" config this late is a wrong pattern, as the one\nthat was already called in the caller may have been overridden by\nthe command line options parser, and calling \"more generic\" config\ncallback after that would be a no-no. So I think this should read\n\"we should never make a fall-back call to ...\", instead.\n\nGranted, cmd_multi_pack_index() only uses default_config(), and what\nit sets will not be overridden by the command line argument parser\nof the multi-pack-index command, so it is not a problem yet, but if\nwe ever introduce configuration variables that are applicable to\nmultiple subcommands of multi-pack-index, this distinction may start\nto matter.\n\nUnless a Git subcommand cannot work with just a single call to\ngit_config() with a callback upfront, I actually think it may be\nbetter for them to issue a more pointed git_config_get_*() on the\nvariables they are interested in, never making a separate call to\ngit_config().\n\nBut that can be left for the future.\n"},{"id":"435679","messageId":"xmqqa6khi9ph.fsf@gitster.g","threadId":"56455","inReplyTo":"acf3ec60cb6f151a9f121d242f38fef6711cced4.1631049462.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T00:46:18Z","receivedAt":"2021-09-13T00:46:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Now that we both can propagate values from the hashcache, and respect\n> the configuration to enable the hashcache at all, test that both of\n> these function correctly by hardening their behavior with a test.\n\nWhen we serve \"git fetch\" requests from a resulting repository,\nbecause we have namehash available without traversing, we can serve\na packfile to the requestor, using the reachability bitmap to pick\nobjects that need to be sent, grouping the blobs with the same\nnamehash to be deltified against each other, resulting in a pack\nthat is better compressed than before?\n\nWhat I am wondering is if we can come up with a \"now, because we no\nlonger lose hashcache, we can do X so much better, here are the\nnumbers\".\n\n"},{"id":"435801","messageId":"YT/3BuDa7KfUN/38@nand.local","threadId":"56455","inReplyTo":"xmqqa6khi9ph.fsf@gitster.g","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T01:12:38Z","receivedAt":"2021-09-14T01:12:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 12, 2021 at 05:46:18PM -0700, Junio C Hamano wrote:\n> What I am wondering is if we can come up with a \"now, because we no\n> longer lose hashcache, we can do X so much better, here are the\n> numbers\".\n\nSure, here they are. Bear in mind that these numbers are (a) on git.git\nand (b) with `pack.threads` set to 1 so the overall runtime looks more\nlike CPU time.\n\n Test origin/tb/multi-pack-bitmaps HEAD\n -------------------------------------------------------------------------------------\n 5326.4: simulated clone 1.87(1.80+0.07) 1.46(1.42+0.03) -21.9%\n 5326.5: simulated fetch 2.66(2.61+0.04) 1.47(1.43+0.04) -44.7%\n 5326.6: pack to file (bitmap) 2.74(2.62+0.12) 1.89(1.82+0.07) -31.0%\n\nApologies for taking a little while to respond, I spent longer than I'm\nwilling to admit double checking these numbers with Peff because of\ninconsistencies in my testing setup.\n\nAlas, there they are. They are basically no different than having the\nname-hash for single pack bitmaps, it's just now we don't throw them\naway when generating a MIDX bitmap from a state where the repository\nalready has a single-pack bitmap.\n\nThanks,\nTaylor\n"},{"id":"435802","messageId":"YT/3mEv9OnC/CH67@nand.local","threadId":"56455","inReplyTo":"xmqqee9tia2b.fsf@gitster.g","subject":"Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T01:15:04Z","receivedAt":"2021-09-14T01:15:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Sep 12, 2021 at 05:38:36PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > +static int git_multi_pack_index_write_config(const char *var, const char *value,\n> > +\t\t\t\t\t void *cb)\n> > +{\n> > +\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n> > +\t\tif (git_config_bool(var, value))\n> > +\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n> > +\t\telse\n> > +\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * No need to fall-back to 'git_default_config', since this was already\n> > +\t * called in 'cmd_multi_pack_index()'.\n> > +\t */\n>\n> It's probably not just \"No need to\", but calling default_config() or\n> any \"more generic\" config this late is a wrong pattern [...]\n\nMakes sense to me, will apply. Thanks!\n\nThanks,\nTaylor\n"},{"id":"435805","messageId":"xmqqpmtbc3o3.fsf@gitster.g","threadId":"56455","inReplyTo":"YT/3BuDa7KfUN/38@nand.local","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-14T02:05:32Z","receivedAt":"2021-09-14T02:05:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Alas, there they are. They are basically no different than having the\n> name-hash for single pack bitmaps, it's just now we don't throw them\n> away when generating a MIDX bitmap from a state where the repository\n> already has a single-pack bitmap.\n\nI actually wasn't expecting any CPU/time difference.\n\nI hope that we are talking about the same name-hash, which is used\nto sort the blobs so that when pack-objects try to find a good delta\nbase, the blobs from the same path will sit close to each other and\nhopefully fit in the pack window.\n\nThe effect I was hoping to see by not discarding the information was\nthat we find better delta base hence smaller deltas in the resulting\npackfiles.\n\nThanks.\n"},{"id":"435821","messageId":"YUAvBso+UsBTYizb@nand.local","threadId":"56455","inReplyTo":"xmqqpmtbc3o3.fsf@gitster.g","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T05:11:34Z","receivedAt":"2021-09-14T05:11:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 13, 2021 at 07:05:32PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Alas, there they are. They are basically no different than having the\n> > name-hash for single pack bitmaps, it's just now we don't throw them\n> > away when generating a MIDX bitmap from a state where the repository\n> > already has a single-pack bitmap.\n>\n> I actually wasn't expecting any CPU/time difference.\n\nI think it is possible to see the CPU usage go down without affecting the\nresulting pack size. See below for a more detailed analysis.\n\n> I hope that we are talking about the same name-hash, which is used\n> to sort the blobs so that when pack-objects try to find a good delta\n> base, the blobs from the same path will sit close to each other and\n> hopefully fit in the pack window.\n\nYes, of course.\n\n> The effect I was hoping to see by not discarding the information was\n> that we find better delta base hence smaller deltas in the resulting\n> packfiles.\n\nI think it is possible to observe either a decrease in CPU or a decrease\nin the resulting pack size.\n\nIn my experience having the name-hash filled in results in finding good\ndelta pairs much more quickly than without, but that in many\nrepositories the resulting pack size is basically the same. In other\nwords, the resulting pack is pretty similar whether you use the\nname-hash or not, it just affects how quickly you get there.\n\nSome experiments to back that up: I instrumented the existing p5326 by\nreplacing anything like \"pack-objects ... --stdout >/dev/null\" with\n\"pack-objects ... --stdout >pack.tmp\" and then added test_size's to\nmeasure the size of each pack.\n\nOn the tip of this branch, the results are:\n\n\t\tTest origin/tb/multi-pack-bitmaps HEAD\n\t\t----------------------------------------------------------------------------\n\t\t5326.5: simulated clone size 3.3G 3.3G +0.0%\n\t\t5326.7: simulated fetch size 10.5M 10.5M -0.2%\n\t\t5326.21: clone (partial bitmap) 3.3G 3.3G +0.0%\n\nLooking at c171d3e677 (pack-bitmap: implement optional name_hash cache,\n2013-10-22), I modified[1] that script to replace timing pack-objects with\ncounting the number of bytes it wrote.\n\nDoing that shows that the name-hash doesn't make a substantial difference in\nthe resulting pack size (numbers on a recent-ish copy of the kernel):\n\n\t\tTest c171d3e677d777c50231d8dea32ae691936da819^ c171d3e677d777c50231d8dea32ae691936da819\n\t\t--------------------------------------------------------------------------------------------------------------\n\t\t9999.3: simulated clone 3.2G 3.2G +0.0%\n\t\t9999.4: simulated fetch 32 32 +0.0%\n\t\t9999.6: partial bitmap 3.1G 3.1G +0.0%\n\n(As a mostly-unrelated aside, I was curious why the pack size jumped from 3.2GB\nto 3.3GB, but I can reproduce that jump even in p5310--the single pack bitmap\ntest--on the tip of my branch. So it does appear to be a regression which I'll\nlook into, but it's unrelated to this branch or MIDX bitmaps).\n\nThanks,\nTaylor\n\n[1]: https://gist.github.com/ttaylorr/6cfa3eb9fd012f81b833873d50f96f71\n"},{"id":"435822","messageId":"YUAwZDLGTmRfswif@nand.local","threadId":"56455","inReplyTo":"YUAvBso+UsBTYizb@nand.local","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T05:17:24Z","receivedAt":"2021-09-14T05:17:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 01:11:34AM -0400, Taylor Blau wrote:\n> On the tip of this branch, the results are:\n>\n> [...]\n\nEek, those tabs are horrific. I must have left my editor in paste mode\nwhen inserting them, sorry about that.\n\nThanks,\nTaylor\n"},{"id":"435823","messageId":"YUAxxPcd7RHC2AH+@coredump.intra.peff.net","threadId":"56455","inReplyTo":"xmqqpmtbc3o3.fsf@gitster.g","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T05:23:16Z","receivedAt":"2021-09-14T05:23:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 13, 2021 at 07:05:32PM -0700, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> > Alas, there they are. They are basically no different than having the\n> > name-hash for single pack bitmaps, it's just now we don't throw them\n> > away when generating a MIDX bitmap from a state where the repository\n> > already has a single-pack bitmap.\n> \n> I actually wasn't expecting any CPU/time difference.\n\nI was, for the same reason we saw an improvement there in ae4f07fbcc\n(pack-bitmap: implement optional name_hash cache, 2013-12-21): without a\nname-hash, we try a bunch of fruitless deltas before we find a decent\none.\n\n> I hope that we are talking about the same name-hash, which is used\n> to sort the blobs so that when pack-objects try to find a good delta\n> base, the blobs from the same path will sit close to each other and\n> hopefully fit in the pack window.\n\nYes, exactly. We spend less time finding the good ones if the likely\ncandidates are close together. We may _also_ find better ones overall,\ndepending on the number of candidates and the window size.\n\nThe bitmap perf tests (neither p5310 nor its new midx cousin p5326)\ndon't check the output size.\n\n> The effect I was hoping to see by not discarding the information was\n> that we find better delta base hence smaller deltas in the resulting\n> packfiles.\n\nIf we add a size check like so[1]:\n\ndiff --git a/t/perf/lib-bitmap.sh b/t/perf/lib-bitmap.sh\nindex 63d3bc7cec..648cd5b13d 100644\n--- a/t/perf/lib-bitmap.sh\n+++ b/t/perf/lib-bitmap.sh\n@@ -10,7 +10,11 @@ test_full_bitmap () {\n \t\t{\n \t\t\techo HEAD &&\n \t\t\techo ^$have\n-\t\t} | git pack-objects --revs --stdout >/dev/null\n+\t\t} | git pack-objects --revs --stdout >tmp.pack\n+\t'\n+\n+\ttest_size 'fetch size' '\n+\t\twc -c Some experiments to back that up: I instrumented the existing p5326 by\n> replacing anything like \"pack-objects ... --stdout >/dev/null\" with\n> \"pack-objects ... --stdout >pack.tmp\" and then added test_size's to\n> measure the size of each pack.\n> \n> On the tip of this branch, the results are:\n> \n> \t\tTest origin/tb/multi-pack-bitmaps HEAD\n> \t\t----------------------------------------------------------------------------\n> \t\t5326.5: simulated clone size 3.3G 3.3G +0.0%\n> \t\t5326.7: simulated fetch size 10.5M 10.5M -0.2%\n> \t\t5326.21: clone (partial bitmap) 3.3G 3.3G +0.0%\n\nI wouldn't expect a change in the clone size. We're already including\nall the objects from the single pack, so we won't even look for new\ndeltas.\n\nIn my run, I did see a small improvement in the fetch size (though my\nsize both before and after was larger than yours). This is going to\ndepend on the exact set of deltas we have (which in turn depends on how\nyour repo happens to have been packed before the script even starts) and\nwhich ones the client actually wants (which may depend on the exact tip\nof your repo).\n\nPresumably you also saw a decrease in the user CPU time of 5326.6 here.\nIf not, you may have forgotten the extra patch to create the pack\nbitmap.\n\n-Peff\n"},{"id":"435825","messageId":"YUAzlYYeUG0Xa8Sj@nand.local","threadId":"56455","inReplyTo":"YUAyw+qVkKBEZtLx@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T05:31:01Z","receivedAt":"2021-09-14T05:31:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 14, 2021 at 01:27:31AM -0400, Jeff King wrote:\n> On Tue, Sep 14, 2021 at 01:11:34AM -0400, Taylor Blau wrote:\n>\n> > Some experiments to back that up: I instrumented the existing p5326 by\n> > replacing anything like \"pack-objects ... --stdout >/dev/null\" with\n> > \"pack-objects ... --stdout >pack.tmp\" and then added test_size's to\n> > measure the size of each pack.\n> >\n> > On the tip of this branch, the results are:\n> >\n> > \t\tTest origin/tb/multi-pack-bitmaps HEAD\n> > \t\t----------------------------------------------------------------------------\n> > \t\t5326.5: simulated clone size 3.3G 3.3G +0.0%\n> > \t\t5326.7: simulated fetch size 10.5M 10.5M -0.2%\n> > \t\t5326.21: clone (partial bitmap) 3.3G 3.3G +0.0%\n>\n> I wouldn't expect a change in the clone size. We're already including\n> all the objects from the single pack, so we won't even look for new\n> deltas.\n>\n> In my run, I did see a small improvement in the fetch size (though my\n> size both before and after was larger than yours). This is going to\n> depend on the exact set of deltas we have (which in turn depends on how\n> your repo happens to have been packed before the script even starts) and\n> which ones the client actually wants (which may depend on the exact tip\n> of your repo).\n\nYes, I agree with all of that. I am still interested in trying to figure\nout why the resulting clone size seems to go up (independent of the\nchanges here). I'm bisecting it, but it's slow, since every step\nrequires you to repack the kernel.\n\n> Presumably you also saw a decrease in the user CPU time of 5326.6 here.\n> If not, you may have forgotten the extra patch to create the pack\n> bitmap.\n\nI did, but didn't bother to include the timings in the quoted part,\nsince I already shared them in [1].\n\nI have a handful of new patches for an updated version of this series\nwhich explains the extra patch you are talking about, too.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YT%2F3BuDa7KfUN%2F38@nand.local/\n"},{"id":"435827","messageId":"xmqqilz34sgt.fsf@gitster.g","threadId":"56455","inReplyTo":"YUAxxPcd7RHC2AH+@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-14T05:49:22Z","receivedAt":"2021-09-14T05:49:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 13, 2021 at 07:05:32PM -0700, Junio C Hamano wrote:\n>\n>> Taylor Blau writes:\n>> \n>> > Alas, there they are. They are basically no different than having the\n>> > name-hash for single pack bitmaps, it's just now we don't throw them\n>> > away when generating a MIDX bitmap from a state where the repository\n>> > already has a single-pack bitmap.\n>> \n>> I actually wasn't expecting any CPU/time difference.\n>\n> I was, for the same reason we saw an improvement there in ae4f07fbcc\n> (pack-bitmap: implement optional name_hash cache, 2013-12-21): without a\n> name-hash, we try a bunch of fruitless deltas before we find a decent\n> one.\n\nNice. We learn new things every once in a while ;-)\n\n>> I hope that we are talking about the same name-hash, which is used\n>> to sort the blobs so that when pack-objects try to find a good delta\n>> base, the blobs from the same path will sit close to each other and\n>> hopefully fit in the pack window.\n>\n> Yes, exactly. We spend less time finding the good ones if the likely\n> candidates are close together. We may _also_ find better ones overall,\n> depending on the number of candidates and the window size.\n\nIt is a pleasant surprise that in a real history like linux.git we\ncan even find good delta base without the name hash (unless we are\nusing insanely wide window size, that is). The objects in such a\ncase will be sorted solely by size, larger to smaller, and we need\nto find a good delta base within that window. It may not be as\nhorrible as fast-import (which only tries to delta against the\nprevious single object), but with ~70k paths in a single revision\nwith history that is ~1m deep, I was pessimistic to see a size-only\nsort to drop even a pair of blobs from the same path within the\ndefault window size of 10. But it seems that such a pessimism is\nunwarranted, which is a good news ;-).\n"},{"id":"435925","messageId":"cover.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631049462.git.me@ttaylorr.com","subject":"[PATCH v2 0/7] pack-bitmap: permute existing namehash values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:05:59Z","receivedAt":"2021-09-14T22:06:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small-ish rerool of my series to carry forward values from an existing\nhash-cache when generating multi-pack reachability bitmaps.\n\nSince last time, the performance tests in p5326 were cleaned up in order to\nproduce timings for generating a pack when using a MIDX bitmap with the\nhash-cache extension.\n\nThe test-tool implementation in the second patch was also fixed to print the\ncorrect OID for each name-hash. In the previous version, the order we looked up\nread the hash-cache was according to pack order, when it should be in index\norder. This bug still allowed the tests to pass, because the two orderings are\npermutations of one another, so the expected and actual orderings were wrong in\nthe same way.\n\nIt is based on the `tb/multi-pack-bitmaps` topic, which graduated to master.\nThanks in advance for your review.\n\nTaylor Blau (7):\n t/helper/test-bitmap.c: add 'dump-hashes' mode\n pack-bitmap.c: propagate namehash values from existing bitmaps\n midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps\n p5326: create missing 'perf-tag' tag\n p5326: don't set core.multiPackIndex unnecessarily\n p5326: generate pack bitmaps before writing the MIDX bitmap\n t5326: test propagating hashcache values\n\n Documentation/config/pack.txt | 4 +++\n builtin/multi-pack-index.c | 21 +++++++++++++++\n midx.c | 6 ++++-\n midx.h | 1 +\n pack-bitmap.c | 41 +++++++++++++++++++++++++-----\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 10 +++++++-\n t/perf/p5326-multi-pack-bitmaps.sh | 8 +++---\n t/t5326-multi-pack-bitmaps.sh | 32 +++++++++++++++++++++++\n 9 files changed, 113 insertions(+), 11 deletions(-)\n\nRange-diff against v1:\n1: 918f9b275a ! 1: 4f2b8d9530 t/helper/test-bitmap.c: add 'dump-hashes' mode\n @@ pack-bitmap.c: int test_bitmap_commits(struct repository *r)\n +{\n +\tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n +\tstruct object_id oid;\n -+\tuint32_t i;\n ++\tuint32_t i, index_pos;\n +\n +\tif (!bitmap_git->hashes)\n +\t\tgoto cleanup;\n +\n +\tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n -+\t\tnth_bitmap_object_oid(bitmap_git, &oid, i);\n ++\t\tif (bitmap_is_midx(bitmap_git))\n ++\t\t\tindex_pos = pack_pos_to_midx(bitmap_git->midx, i);\n ++\t\telse\n ++\t\t\tindex_pos = pack_pos_to_index(bitmap_git->pack, i);\n ++\n ++\t\tnth_bitmap_object_oid(bitmap_git, &oid, index_pos);\n +\n +\t\tprintf(\"%s %\"PRIu32\"\\n\",\n -+\t\t oid_to_hex(&oid), get_be32(bitmap_git->hashes + i));\n ++\t\t oid_to_hex(&oid), get_be32(bitmap_git->hashes + index_pos));\n +\t}\n +\n +cleanup:\n2: fa9f5633f6 = 2: 2cd2f3aa5e pack-bitmap.c: propagate namehash values from existing bitmaps\n3: be8f47e13c ! 3: f0d8f106c2 midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps\n @@ builtin/multi-pack-index.c: static struct option *add_common_options(struct opti\n +\t}\n +\n +\t/*\n -+\t * No need to fall-back to 'git_default_config', since this was already\n -+\t * called in 'cmd_multi_pack_index()'.\n ++\t * We should never make a fall-back call to 'git_default_config', since\n ++\t * this was already called in 'cmd_multi_pack_index()'.\n +\t */\n +\treturn 0;\n +}\n-: ---------- > 4: a8c6e845ad p5326: create missing 'perf-tag' tag\n-: ---------- > 5: 191922c8f2 p5326: don't set core.multiPackIndex unnecessarily\n-: ---------- > 6: 040bb40548 p5326: generate pack bitmaps before writing the MIDX bitmap\n4: acf3ec60cb ! 7: fdf71432b3 t5326: test propagating hashcache values\n @@ Commit message\n the configuration to enable the hashcache at all, test that both of\n these function correctly by hardening their behavior with a test.\n \n + Like the hash-cache in classic single-pack bitmaps, this helps more\n + proportionally the more up-to-date your bitmap coverage is. When our\n + bitmap coverage is out-of-date with the ref tips, we spend more time\n + proportionally traversing, and all of that traversal gets the name-hash\n + filled in.\n +\n + But for the up-to-date bitmaps, this helps quite a bit. These numbers\n + are on git.git, with `pack.threads=1` to help see the difference\n + reflected in the overall runtime.\n +\n + Test origin/tb/multi-pack-bitmaps HEAD\n + -------------------------------------------------------------------------------------\n + 5326.4: simulated clone 1.87(1.80+0.07) 1.46(1.42+0.03) -21.9%\n + 5326.5: simulated fetch 2.66(2.61+0.04) 1.47(1.43+0.04) -44.7%\n + 5326.6: pack to file (bitmap) 2.74(2.62+0.12) 1.89(1.82+0.07) -31.0%\n +\n Signed-off-by: Taylor Blau \n \n ## t/t5326-multi-pack-bitmaps.sh ##\n-- \n2.33.0.96.g73915697e6\n"},{"id":"435926","messageId":"4f2b8d95303d54a50b13117d771da690c4700d3e.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 1/7] t/helper/test-bitmap.c: add 'dump-hashes' mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:02Z","receivedAt":"2021-09-14T22:06:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The pack-bitmap writer code is about to learn how to propagate values\nfrom an existing hash-cache. To prepare, teach the test-bitmap helper to\ndump the values from a bitmap's hash-cache extension in order to test\nthose changes.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 27 +++++++++++++++++++++++++++\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 10 +++++++++-\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 8504110a4d..04de387318 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1742,6 +1742,33 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_hashes(struct repository *r)\n+{\n+\tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n+\tstruct object_id oid;\n+\tuint32_t i, index_pos;\n+\n+\tif (!bitmap_git->hashes)\n+\t\tgoto cleanup;\n+\n+\tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n+\t\tif (bitmap_is_midx(bitmap_git))\n+\t\t\tindex_pos = pack_pos_to_midx(bitmap_git->midx, i);\n+\t\telse\n+\t\t\tindex_pos = pack_pos_to_index(bitmap_git->pack, i);\n+\n+\t\tnth_bitmap_object_oid(bitmap_git, &oid, index_pos);\n+\n+\t\tprintf(\"%s %\"PRIu32\"\\n\",\n+\t\t oid_to_hex(&oid), get_be32(bitmap_git->hashes + index_pos));\n+\t}\n+\n+cleanup:\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int rebuild_bitmap(const uint32_t *reposition,\n \t\t struct ewah_bitmap *source,\n \t\t struct bitmap *dest)\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 469090bad2..ed46d27077 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -52,6 +52,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_hashes(struct repository *r);\n struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n \t\t\t\t\t struct list_objects_filter_options *filter,\n \t\t\t\t\t int filter_provided_objects);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 134a1e9d76..ff35f5999b 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -7,6 +7,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_dump_hashes(void)\n+{\n+\treturn test_bitmap_hashes(the_repository);\n+}\n+\n int cmd__bitmap(int argc, const char **argv)\n {\n \tsetup_git_directory();\n@@ -16,9 +21,12 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (!strcmp(argv[1], \"dump-hashes\"))\n+\t\treturn bitmap_dump_hashes();\n \n usage:\n-\tusage(\"\\ttest-tool bitmap list-commits\");\n+\tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap dump-hashes\");\n \n \treturn -1;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"435927","messageId":"2cd2f3aa5e74a3aefd6924c528cbfb578a510d31.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 2/7] pack-bitmap.c: propagate namehash values from existing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:04Z","receivedAt":"2021-09-14T22:06:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When an old bitmap exists while writing a new one, we load it and build\na \"reposition\" table which maps bit positions of objects from the old\nbitmap to their respective positions in the new bitmap. This can help\nwhen we encounter a commit which was selected in both the old and new\nbitmap, since we only need to permute its bit (not recompute it from\nscratch).\n\nWe do not, however, repurpose existing namehash values in the case of\nthe hash-cache extension. There has been thus far no good reason to do\nso, since all of the namehash values for objects in the new bitmap would\nbe populated during the traversal that was just performed by\npack-objects when generating single-pack reachability bitmaps.\n\nBut this isn't the case for multi-pack bitmaps, which are written via\n`git multi-pack-index write --bitmap` and do not perform any traversal.\nIn this case all namehash values are set to zero, but we don't even\nbother to check the `pack.writeBitmapHashcache` option anyway, so it\nfails to matter.\n\nThere are two approaches we could take to fill in non-zero hash-cache\nvalues:\n\n - have either the multi-pack-index builtin run its own\n traversal to attempt to fill in some values, or let a hypothetical\n caller (like `pack-objects` when `repack` eventually drives the\n `multi-pack-index` builtin) fill in the values they found during\n their traversal\n\n - or copy any existing namehash values that were stored in an\n existing bitmap to their corresponding positions in the new bitmap\n\nIn a system where a repository is generally repacked with `git repack\n--geometric=` and occasionally repacked with `git repack -a`, the\nhash-cache coverage will tend towards all objects.\n\nSince populating the hash-cache is additive (i.e., doing so only helps\nour delta search), any intermediate lack of full coverage is just fine.\nSo let's start by just propagating any values from the existing\nhash-cache if we see one.\n\nThe next patch will respect the `pack.writeBitmapHashcache` option while\nwriting MIDX bitmaps, and then test this new behavior.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 04de387318..33a3732992 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1818,18 +1818,20 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < num_objects; ++i) {\n \t\tstruct object_id oid;\n \t\tstruct object_entry *oe;\n+\t\tuint32_t index_pos;\n \n \t\tif (bitmap_is_midx(bitmap_git))\n-\t\t\tnth_midxed_object_oid(&oid,\n-\t\t\t\t\t bitmap_git->midx,\n-\t\t\t\t\t pack_pos_to_midx(bitmap_git->midx, i));\n+\t\t\tindex_pos = pack_pos_to_midx(bitmap_git->midx, i);\n \t\telse\n-\t\t\tnth_packed_object_id(&oid, bitmap_git->pack,\n-\t\t\t\t\t pack_pos_to_index(bitmap_git->pack, i));\n+\t\t\tindex_pos = pack_pos_to_index(bitmap_git->pack, i);\n+\t\tnth_bitmap_object_oid(bitmap_git, &oid, index_pos);\n \t\toe = packlist_find(mapping, &oid);\n \n-\t\tif (oe)\n+\t\tif (oe) {\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\n+\t\t\tif (bitmap_git->hashes && !oe->hash)\n+\t\t\t\toe->hash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t}\n \t}\n \n \treturn reposition;\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"435928","messageId":"f0d8f106c22dae64eaf4b77d1ecf67ef79c4515c.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 3/7] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:06Z","receivedAt":"2021-09-14T22:06:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, the bitmap writing code learned to propagate\nvalues from an existing hash-cache extension into the bitmap that it is\nwriting.\n\nNow that that functionality exists, let's expose it by teaching the 'git\nmulti-pack-index' builtin to respect the `pack.writeBitmapHashCache`\noption so that the hash-cache may be written at all.\n\nTwo minor points worth noting here:\n\n - The 'git multi-pack-index write' sub-command didn't previously read\n any configuration (instead this is handled in the base command). A\n separate handler is added here to respect this write-specific\n config option.\n\n - I briefly considered adding a 'bitmap_flags' field to the static\n options struct, but decided against it since it would require\n plumbing through a new parameter to the write_midx_file() function.\n\n Instead, a new MIDX-specific flag is added, which is translated to\n the corresponding bitmap one.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/pack.txt | 4 ++++\n builtin/multi-pack-index.c | 21 +++++++++++++++++++++\n midx.c | 6 +++++-\n midx.h | 1 +\n 4 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 763f7af7c4..ad7f73a1ea 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -159,6 +159,10 @@ pack.writeBitmapHashCache::\n \tbetween an older, bitmapped pack and objects that have been\n \tpushed since the last gc). The downside is that it consumes 4\n \tbytes per object of disk space. Defaults to true.\n++\n+When writing a multi-pack reachability bitmap, no new namehashes are\n+computed; instead, any namehashes stored in an existing bitmap are\n+permuted into their appropriate location when writing a new bitmap.\n \n pack.writeReverseIndex::\n \tWhen true, git will write a corresponding .rev file (see:\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 73c0113b48..578ffea6e8 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -61,6 +61,23 @@ static struct option *add_common_options(struct option *prev)\n \treturn parse_options_concat(common_opts, prev);\n }\n \n+static int git_multi_pack_index_write_config(const char *var, const char *value,\n+\t\t\t\t\t void *cb)\n+{\n+\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n+\t\tif (git_config_bool(var, value))\n+\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t\telse\n+\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t}\n+\n+\t/*\n+\t * We should never make a fall-back call to 'git_default_config', since\n+\t * this was already called in 'cmd_multi_pack_index()'.\n+\t */\n+\treturn 0;\n+}\n+\n static int cmd_multi_pack_index_write(int argc, const char **argv)\n {\n \tstruct option *options;\n@@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \t\tOPT_END(),\n \t};\n \n+\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\n+\tgit_config(git_multi_pack_index_write_config, NULL);\n+\n \toptions = add_common_options(builtin_multi_pack_index_write_options);\n \n \ttrace2_cmd_mode(argv[0]);\ndiff --git a/midx.c b/midx.c\nindex 864034a6ad..38c8600458 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1008,9 +1008,13 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \tstruct pack_idx_entry **index;\n \tstruct commit **commits = NULL;\n \tuint32_t i, commits_nr;\n+\tuint16_t options = 0;\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n \tint ret;\n \n+\tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n+\t\toptions |= BITMAP_OPT_HASH_CACHE;\n+\n \tprepare_midx_packing_data(&pdata, ctx);\n \n \tcommits = find_commits_for_midx_bitmap(&commits_nr, ctx);\n@@ -1049,7 +1053,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \t\tgoto cleanup;\n \n \tbitmap_writer_set_checksum(midx_hash);\n-\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, 0);\n+\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, options);\n \n cleanup:\n \tfree(index);\ndiff --git a/midx.h b/midx.h\nindex aa3da557bb..541d9ac728 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -44,6 +44,7 @@ struct multi_pack_index {\n #define MIDX_PROGRESS (1 << 0)\n #define MIDX_WRITE_REV_INDEX (1 << 1)\n #define MIDX_WRITE_BITMAP (1 << 2)\n+#define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n \n const unsigned char *get_midx_checksum(struct multi_pack_index *m);\n char *get_midx_filename(const char *object_dir);\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"435929","messageId":"a8c6e845adf559a96e37c973fea16da8a42e7cba.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 4/7] p5326: create missing 'perf-tag' tag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:09Z","receivedAt":"2021-09-14T22:06:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Some of the tests in test_full_bitmap rely on having a tag named\nperf-tag in place. We could create it in test_full_bitmap(), but we want\nto have it in place before the repack starts.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5326-multi-pack-bitmaps.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\nindex 5845109ac7..51b5636259 100755\n--- a/t/perf/p5326-multi-pack-bitmaps.sh\n+++ b/t/perf/p5326-multi-pack-bitmaps.sh\n@@ -10,6 +10,12 @@ test_expect_success 'enable multi-pack index' '\n \tgit config core.multiPackIndex true\n '\n \n+# we need to create the tag up front such that it is covered by the repack and\n+# thus by generated bitmaps.\n+test_expect_success 'create tags' '\n+\tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n+'\n+\n test_perf 'setup multi-pack index' '\n \tgit repack -ad &&\n \tgit multi-pack-index write --bitmap\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"435930","messageId":"191922c8f28c338ab3aa4faa57d75770a9eccd02.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 5/7] p5326: don't set core.multiPackIndex unnecessarily","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:11Z","receivedAt":"2021-09-14T22:06:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When this performance test was originally written, `core.multiPackIndex`\nwas not the default and thus had to be enabled. But now that we have\n18e449f86b (midx: enable core.multiPackIndex by default, 2020-09-25), we\nno longer need this.\n\nDrop the unnecessary setup (even though it's not hurting anything, it is\nunnecessary at best and confusing at worst).\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5326-multi-pack-bitmaps.sh | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\nindex 51b5636259..a9c5499537 100755\n--- a/t/perf/p5326-multi-pack-bitmaps.sh\n+++ b/t/perf/p5326-multi-pack-bitmaps.sh\n@@ -6,10 +6,6 @@ test_description='Tests performance using midx bitmaps'\n \n test_perf_large_repo\n \n-test_expect_success 'enable multi-pack index' '\n-\tgit config core.multiPackIndex true\n-'\n-\n # we need to create the tag up front such that it is covered by the repack and\n # thus by generated bitmaps.\n test_expect_success 'create tags' '\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"435931","messageId":"040bb40548017bae807c1d349fa078c21ac46725.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 6/7] p5326: generate pack bitmaps before writing the MIDX bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:14Z","receivedAt":"2021-09-14T22:06:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To help test the performance of permuting the contents of the hash-cache\nwhen generating a MIDX bitmap, we need a bitmap which has its hash-cache\npopulated.\n\nAnd since multi-pack bitmaps don't add *new* values to the hash-cache,\nwe have to rely on a single-pack bitmap to generate those values for us.\n\nTherefore, generate a pack bitmap before the MIDX one in order to ensure\nthat the MIDX bitmap has entries in its hash-cache.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5326-multi-pack-bitmaps.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\nindex a9c5499537..38557859b7 100755\n--- a/t/perf/p5326-multi-pack-bitmaps.sh\n+++ b/t/perf/p5326-multi-pack-bitmaps.sh\n@@ -13,7 +13,7 @@ test_expect_success 'create tags' '\n '\n \n test_perf 'setup multi-pack index' '\n-\tgit repack -ad &&\n+\tgit repack -adb &&\n \tgit multi-pack-index write --bitmap\n '\n \n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"435932","messageId":"fdf71432b32ef47b23b79ddfc3db8de8734ef7a5.1631657157.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"[PATCH v2 7/7] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-14T22:06:16Z","receivedAt":"2021-09-14T22:06:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that we both can propagate values from the hashcache, and respect\nthe configuration to enable the hashcache at all, test that both of\nthese function correctly by hardening their behavior with a test.\n\nLike the hash-cache in classic single-pack bitmaps, this helps more\nproportionally the more up-to-date your bitmap coverage is. When our\nbitmap coverage is out-of-date with the ref tips, we spend more time\nproportionally traversing, and all of that traversal gets the name-hash\nfilled in.\n\nBut for the up-to-date bitmaps, this helps quite a bit. These numbers\nare on git.git, with `pack.threads=1` to help see the difference\nreflected in the overall runtime.\n\n Test origin/tb/multi-pack-bitmaps HEAD\n -------------------------------------------------------------------------------------\n 5326.4: simulated clone 1.87(1.80+0.07) 1.46(1.42+0.03) -21.9%\n 5326.5: simulated fetch 2.66(2.61+0.04) 1.47(1.43+0.04) -44.7%\n 5326.6: pack to file (bitmap) 2.74(2.62+0.12) 1.89(1.82+0.07) -31.0%\n\nSigned-off-by: Taylor Blau \n---\n t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 4ad7c2c969..24148ca35b 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '\n \t)\n '\n \n+test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n+\trm -fr repo &&\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\tgit config pack.writeBitmapHashCache true &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit base2 &&\n+\t\tgit repack -adb &&\n+\n+\t\ttest-tool bitmap dump-hashes >pack.raw &&\n+\t\ttest_file_not_empty pack.raw &&\n+\t\tsort pack.raw >pack.hashes &&\n+\n+\t\ttest_commit new &&\n+\t\tgit repack &&\n+\t\tgit multi-pack-index write --bitmap &&\n+\n+\t\ttest-tool bitmap dump-hashes >midx.raw &&\n+\t\tsort midx.raw >midx.hashes &&\n+\n+\t\t# ensure that every namehash in the pack bitmap can be found in\n+\t\t# the midx bitmap (i.e., that there are no oid-namehash pairs\n+\t\t# unique to the pack bitmap).\n+\t\tcomm -23 pack.hashes midx.hashes >dropped.hashes &&\n+\t\ttest_must_be_empty dropped.hashes\n+\t)\n+'\n+\n test_done\n-- \n2.33.0.96.g73915697e6\n"},{"id":"436180","messageId":"YUPHBv3d5p67NAnJ@coredump.intra.peff.net","threadId":"56455","inReplyTo":"a8c6e845adf559a96e37c973fea16da8a42e7cba.1631657157.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/7] p5326: create missing 'perf-tag' tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T22:36:54Z","receivedAt":"2021-09-16T22:36:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 06:06:09PM -0400, Taylor Blau wrote:\n\n> Some of the tests in test_full_bitmap rely on having a tag named\n> perf-tag in place. We could create it in test_full_bitmap(), but we want\n> to have it in place before the repack starts.\n\nI wondered how p5326 ever could have worked without this.\n\nBut I think the answer is that it was added in a parallel-ish branch via\n540cdc11ad (pack-bitmap: avoid traversal of objects referenced by\nuninteresting tag, 2021-03-22).\n\nIf that had then been merged with the midx-bitmap topic, we would have\nseen a conflict as yours tried to delete the surrounding tests that got\nmoved into lib-bitmap.sh. But we didn't see such a merge. Your\n9387fbd646 (p5310: extract full and partial bitmap tests, moves the\nperf-tag test, so p5326 was broken then (well, in the follow-on commit\nthat introduced it).\n\nKnowing the history of the midx-bitmap series, I'm almost certain what\nhappened is that it got rebased, and you probably _did_ see a textual\nconflict, which you resolved correctly, moving the perf-tag test into\nlib-bitmap.sh But you missed the semantic conflict that p5326 also\nneeded to add in the setup step.\n\nAll of which is to say this looks fine to me. ;) I was just talking out\nloud to try to understand what had happened.\n\n-Peff\n"},{"id":"436181","messageId":"YUPHWRDXNBM3W3z/@coredump.intra.peff.net","threadId":"56455","inReplyTo":"191922c8f28c338ab3aa4faa57d75770a9eccd02.1631657157.git.me@ttaylorr.com","subject":"Re: [PATCH v2 5/7] p5326: don't set core.multiPackIndex unnecessarily","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T22:38:17Z","receivedAt":"2021-09-16T22:38:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 06:06:11PM -0400, Taylor Blau wrote:\n\n> When this performance test was originally written, `core.multiPackIndex`\n> was not the default and thus had to be enabled. But now that we have\n> 18e449f86b (midx: enable core.multiPackIndex by default, 2020-09-25), we\n> no longer need this.\n> \n> Drop the unnecessary setup (even though it's not hurting anything, it is\n> unnecessary at best and confusing at worst).\n\nMakes sense. Sometimes it is worthwhile to carry extra code in the perf\ntests for older versions, so that we can test performance across a wider\nspan. But there were no midx bitmaps at all when 18e449f86b happened, so\nthere is little point in worrying about going back that far for this\ntest.\n\n-Peff\n"},{"id":"436184","messageId":"YUPJDf7u8Nn5als3@coredump.intra.peff.net","threadId":"56455","inReplyTo":"040bb40548017bae807c1d349fa078c21ac46725.1631657157.git.me@ttaylorr.com","subject":"Re: [PATCH v2 6/7] p5326: generate pack bitmaps before writing the MIDX bitmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T22:45:33Z","receivedAt":"2021-09-16T22:45:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 06:06:14PM -0400, Taylor Blau wrote:\n\n> To help test the performance of permuting the contents of the hash-cache\n> when generating a MIDX bitmap, we need a bitmap which has its hash-cache\n> populated.\n> \n> And since multi-pack bitmaps don't add *new* values to the hash-cache,\n> we have to rely on a single-pack bitmap to generate those values for us.\n> \n> Therefore, generate a pack bitmap before the MIDX one in order to ensure\n> that the MIDX bitmap has entries in its hash-cache.\n\nMakes sense. This is a little more contrived of a setup than the\noriginal, but an utterly realistic one. If you are using midx bitmaps,\nyou are probably interspersing them with occasional full pack bitmaps.\n\nIt might be interesting to also do:\n\n rm -f .git/objects/pack/pack-*.bitmap\n\nafter generating the midx bitmap. That would confirm the further timing\ntests are using the midx bitmap, and not ever \"cheating\" by looking at\nthe pack one (having poked in this direction before, I know that this\nall works, so it would be a future-proofing thing).\n\n> diff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\n> index a9c5499537..38557859b7 100755\n> --- a/t/perf/p5326-multi-pack-bitmaps.sh\n> +++ b/t/perf/p5326-multi-pack-bitmaps.sh\n> @@ -13,7 +13,7 @@ test_expect_success 'create tags' '\n> '\n> \n> test_perf 'setup multi-pack index' '\n> -\tgit repack -ad &&\n> +\tgit repack -adb &&\n> \tgit multi-pack-index write --bitmap\n> '\n\nThis sort-of existed before your series, but I think is a bit \"worse\"\nnow: we are timing both \"repack\" and \"multi-pack-index\" write together.\nSo:\n\n - the timing for the midx write that we are interested in timing will\n be diluted by the much-bigger full-repack\n\n - we'll actually do _three_ full repacks (the default\n GIT_PERF_REPEAT_COUNT for the \"run\" script), since it's inside a\n test_perf()\n\nSo:\n\n test_expect_success 'start with bitmapped pack' '\n\tgit repack -adb\n '\n\n test_perf 'setup multi-pack index' '\n\tgit multi-pack-index write --bitmap\n '\n\nwould run faster and give us more interesting timings. Possibly you'd\nwant to drop the midx and its bitmaps as part of that test_perf, too.\nThe first run will be using the pack bitmap, and the others will use the\nmidx. I doubt it makes much difference either way, though.\n\nAnd of course if you want to take my earlier suggestion, then it's easy\nto add:\n\n test_expect_success 'drop pack bitmap' '\n\trm -f .git/objects/pack/pack-*.bitmap\n '\n\nafterwards; you wouldn't want to do it inside the test_perf() call\nbecause of the repeat-count.\n\n-Peff\n"},{"id":"436188","messageId":"YUPKDhi+rgwptZdY@coredump.intra.peff.net","threadId":"56455","inReplyTo":"fdf71432b32ef47b23b79ddfc3db8de8734ef7a5.1631657157.git.me@ttaylorr.com","subject":"Re: [PATCH v2 7/7] t5326: test propagating hashcache values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T22:49:50Z","receivedAt":"2021-09-16T22:49:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 06:06:16PM -0400, Taylor Blau wrote:\n\n> Now that we both can propagate values from the hashcache, and respect\n> the configuration to enable the hashcache at all, test that both of\n> these function correctly by hardening their behavior with a test.\n> \n> Like the hash-cache in classic single-pack bitmaps, this helps more\n> proportionally the more up-to-date your bitmap coverage is. When our\n> bitmap coverage is out-of-date with the ref tips, we spend more time\n> proportionally traversing, and all of that traversal gets the name-hash\n> filled in.\n> \n> But for the up-to-date bitmaps, this helps quite a bit. These numbers\n> are on git.git, with `pack.threads=1` to help see the difference\n> reflected in the overall runtime.\n> \n> Test origin/tb/multi-pack-bitmaps HEAD\n> -------------------------------------------------------------------------------------\n> 5326.4: simulated clone 1.87(1.80+0.07) 1.46(1.42+0.03) -21.9%\n> 5326.5: simulated fetch 2.66(2.61+0.04) 1.47(1.43+0.04) -44.7%\n> 5326.6: pack to file (bitmap) 2.74(2.62+0.12) 1.89(1.82+0.07) -31.0%\n\nThe percentages here match timings I did. Doing it with linux.git gives\nbigger absolute numbers, but I think this is sufficient (and a lot less\npainful when people are trying to replicate).\n\n> +test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n> +\trm -fr repo &&\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\tgit config pack.writeBitmapHashCache true &&\n\nThis is the default as of your earlier commits, so we could probably\ndrop this. I don't mind keeping it as explicit documentation of what we\nexpect, though.\n\n-Peff\n"},{"id":"436191","messageId":"YUPKnd3jekNR80i4@coredump.intra.peff.net","threadId":"56455","inReplyTo":"cover.1631657157.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/7] pack-bitmap: permute existing namehash values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-16T22:52:13Z","receivedAt":"2021-09-16T22:52:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 06:05:59PM -0400, Taylor Blau wrote:\n\n> Here is a small-ish rerool of my series to carry forward values from an existing\n> hash-cache when generating multi-pack reachability bitmaps.\n> \n> Since last time, the performance tests in p5326 were cleaned up in order to\n> produce timings for generating a pack when using a MIDX bitmap with the\n> hash-cache extension.\n\nI had seen most of these in prototype form before, but I hadn't yet\ncarefully looked at your cleanups for upstream. I gave them all a\ncareful read, and it all looks good to me.\n\nI did have a few comments on patch 6 that I think are worth following up\non. We can also do so on top if you prefer.\n\n-Peff\n"},{"id":"436203","messageId":"YUQWMWiVDyDz9qRV@nand.local","threadId":"56455","inReplyTo":"YUPHBv3d5p67NAnJ@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/7] p5326: create missing 'perf-tag' tag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T04:14:41Z","receivedAt":"2021-09-17T04:14:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 16, 2021 at 06:36:54PM -0400, Jeff King wrote:\n> On Tue, Sep 14, 2021 at 06:06:09PM -0400, Taylor Blau wrote:\n>\n> > Some of the tests in test_full_bitmap rely on having a tag named\n> > perf-tag in place. We could create it in test_full_bitmap(), but we want\n> > to have it in place before the repack starts.\n>\n> I wondered how p5326 ever could have worked without this.\n>\n> [...]\n>\n> Knowing the history of the midx-bitmap series, I'm almost certain what\n> happened is that it got rebased, and you probably _did_ see a textual\n> conflict, which you resolved correctly, moving the perf-tag test into\n> lib-bitmap.sh But you missed the semantic conflict that p5326 also\n> needed to add in the setup step.\n\nSounds about right :-).\n\nThanks,\nTaylor\n"},{"id":"436204","messageId":"YUQXhsJYhxGdjSfL@nand.local","threadId":"56455","inReplyTo":"YUPJDf7u8Nn5als3@coredump.intra.peff.net","subject":"Re: [PATCH v2 6/7] p5326: generate pack bitmaps before writing the MIDX bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T04:20:22Z","receivedAt":"2021-09-17T04:20:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 16, 2021 at 06:45:33PM -0400, Jeff King wrote:\n> It might be interesting to also do:\n>\n> rm -f .git/objects/pack/pack-*.bitmap\n>\n> after generating the midx bitmap. That would confirm the further timing\n> tests are using the midx bitmap, and not ever \"cheating\" by looking at\n> the pack one (having poked in this direction before, I know that this\n> all works, so it would be a future-proofing thing).\n\nThis and the suggestion to avoid timing the single pack bitmap\ngeneration are both good ones.\n\nI think to be totally accurate we would want to drop the pack bitmap\nbefore every MIDX bitmap generation, but I also think that in practice\nit does not matter much.\n\nThe reuse code currently does not try too hard to recognize the\nsituation of \"oh, I selected the same exact commits and don't have to do\nany work\". It kind of does eventually, but only after doing a lot of\npreparation.\n\nSo I'm dubious as to whether we're really timing anything *that* useful,\nbut it's probably worth keeping around anyway.\n\nThanks,\nTaylor\n"},{"id":"436218","messageId":"87y27veeyj.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"YTggYXtyS/AW4JRF@nand.local","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-17T08:56:09Z","receivedAt":"2021-09-17T09:26:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 07 2021, Taylor Blau wrote:\n\n> On Wed, Sep 08, 2021 at 03:46:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Tue, Sep 07 2021, Taylor Blau wrote:\n>>\n>> > Now that we both can propagate values from the hashcache, and respect\n>> > the configuration to enable the hashcache at all, test that both of\n>> > these function correctly by hardening their behavior with a test.\n>> >\n>> > Signed-off-by: Taylor Blau \n>> > ---\n>> > t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++\n>> > 1 file changed, 32 insertions(+)\n>> >\n>> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n>> > index 4ad7c2c969..24148ca35b 100755\n>> > --- a/t/t5326-multi-pack-bitmaps.sh\n>> > +++ b/t/t5326-multi-pack-bitmaps.sh\n>> > @@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '\n>> > \t)\n>> > '\n>> >\n>> > +test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n>> > +\trm -fr repo &&\n>> > +\tgit init repo &&\n>> > +\ttest_when_finished \"rm -fr repo\" &&\n>>\n>> It seems the need for this \"rm -fr repo\" dance instead of just relying\n>> on test_when_finished \"rm -rf repo\" is because of a 3x tests in a\n>> function in tb/multi-pack-bitmaps that should probably be combined into\n>> one, i.e. they share the same logical \"repo\" setup.\n>\n> Yeah. There's definitely room for clean-up there if we want to have each\n> of the tests operate on the same repo. I have always found sharing a\n> repo between tests difficult to reason about, since we have to assume\n> that any --run parameters could be passed in.\n>\n> So I have tended to err on the side of creating and throwing away a new\n> repo per test, but I understand that's somewhat atypical for Git's\n> tests.\n\nJust in an effort to make myself clear here, because between your note\nin https://lore.kernel.org/git/YUOds4kcHRgMk5nC@nand.local/ and\nre-reading my mail here I can barely understand what I meant here :)\n\nWhat I mean is that the whole \"rm -rf\" dance at the start of tests is\nfallout from an earlier call you made in c51f5a6437c (t5326: test\nmulti-pack bitmap behavior, 2021-08-31) to split up three tests that\nreally are the same logical test in terms of setup/teardown.\n\nIf they were to be re-combined as shown in the diff-on-top at the end\nhere none of the tests need a \"rm -rf repo\" at the beginning, because\nthey can all rely on a starting pattern of:\n\n test_when_finished \"rm -rf repo\" &&\n git init repo &&\n ( cd repo ... )\n\nOr, you can also just not re-use the \"repo\" name, which is what I did in\nthe fsck tests at\nhttps://lore.kernel.org/git/patch-v6-01.22-ebe89f65354-20210907T104559Z-avarab@gmail.com/;\nthen you don't even need test_when_finished \"rm -rf[ ...]\".\n\nNone of this requires fixing here or a re-roll, unless you happen to\nthink this diff is the best thing since sliced bread (assume my\nSigned-off-by).\n\nBut just as a general musing on patterns to use in tests, I see why you\nused that 1x test as 3x, because you want \"test_expect_success\" to give\nyou the label on for each \"step\".\n\nI think that would be worth fixing in test-lib.sh, there's no inherent\nreason we couldn't support \"checkpoints\" or \"subtests\" within\n\"test_expect_success\" (the latter being a part of the TAP protocol).\n\nBut until then IMNSHO this sort of thing is an anti-pattern,\ni.e. needing to write things like:\n\n test_expect_success '[...]' 'A'\n test_expect_success '[...]' 'B'\n\nWhere a part of what B needs to do is to clean up the mess left behind\nA, or relies on its setup.\n\nThat's just added verbosity and context when reading the code. Ideally\nyou'd just need to read the \"setup\" at the start, and then individual\ntests, with this pattern you need to read the preceding test(s) to see\nwhere the crap they're cleaning came from, and if it's even needed etc.\n\n(For the \"setup\" part I've suggested a test_expect_setup, see\nhttps://lore.kernel.org/git/8735vrvg39.fsf@evledraar.gmail.com/).\n\nThe \"needs the setup\" part of this has the negative side-effect of\nbreaking the semantics of the --run option. As noted in the E-Mail\nlinked in the last paragraph it doesn't work well in general because of\nthings like this, but let's steer in the right direction.\n\nI.e. with this change you can run:\n\n ./t5326-multi-pack-bitmaps.sh --run=111-113\n\nWhich is great, usually you need at least 1, ,r\n1-10, to get the setup. But because you split them this\nbreaks:\n\n ./t5326-multi-pack-bitmaps.sh --run=112-113\n\nI.e. we'll run only the 2nd and 3rd test in a sequence that needs the\n1st for setup.\n\nFor context: My preference for this isn't just an asthetic or\ntheoretical thing. It's really nice to be hacking some code, find that\nI've broken the bitmap code somehow in your test #112, and be able to\njust run (since running the whole thing takes 13s on my box, and I'd\nlike to test in a tight loop):\n\n ./t5326-multi-pack-bitmaps.sh --run=112\n\nBut that breaks as noted above, so I need read earlier tests (I was\nprobably looking at test #112 already at this point) and see how their\nsetup works etc.\n\nAnyway, as with so many things in git.git being able to just assume\n--run works like this isn't something you can rely on in the general\ncase, but just as a matter of where we should be headed...\n\n>> > +\t(\n>> > +\t\tcd repo &&\n>> > +\n>> > +\t\tgit config pack.writeBitmapHashCache true &&\n>>\n>> s/git config/test_config/, surely.\n>\n> No, since this is in a subshell (and we don't care about unsetting the\n> value of a config in a repo that we're going to throw away, anyways).\n>\n> (Side-note: since this has a /bin/sh shebang, we can't detect that we're\n> in a subshell and so test_config actually _would_ work here. But\n> switching this test to run with /bin/bash where we can detect whether or\n> not we're in a subshell would cause this test to fail with test_config).\n\nYes, you're 100% right here. This was purely a misreading on my part, I\nmanaged to somehow not see/take into account the subshell. Using\ntest_config makes no sense here.\n\nThe diff-on-top for discussion mentioned above:\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 24148ca35b3..a6bb0abb387 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -133,8 +133,8 @@ bitmap_reuse_tests() {\n \tfrom=$1\n \tto=$2\n \n-\ttest_expect_success \"setup pack reuse tests ($from -> $to)\" '\n-\t\trm -fr repo &&\n+\ttest_expect_success \"setup/build/verify ($from -> $to) pack reuse bitmaps\" '\n+\t\ttest_when_finished \"rm -rf repo\" &&\n \t\tgit init repo &&\n \t\t(\n \t\t\tcd repo &&\n@@ -148,13 +148,9 @@ bitmap_reuse_tests() {\n \t\t\t\tgit multi-pack-index write --bitmap\n \t\t\telse\n \t\t\t\tgit repack -Adb\n-\t\t\tfi\n-\t\t)\n-\t'\n+\t\t\tfi &&\n \n-\ttest_expect_success \"build bitmap from existing ($from -> $to)\" '\n-\t\t(\n-\t\t\tcd repo &&\n+\t\t\t# Build\n \t\t\ttest_commit_bulk --id=further 16 &&\n \t\t\tgit tag new-tip &&\n \n@@ -164,13 +160,9 @@ bitmap_reuse_tests() {\n \t\t\t\tgit multi-pack-index write --bitmap\n \t\t\telse\n \t\t\t\tgit repack -Adb\n-\t\t\tfi\n-\t\t)\n-\t'\n+\t\t\tfi &&\n \n-\ttest_expect_success \"verify resulting bitmaps ($from -> $to)\" '\n-\t\t(\n-\t\t\tcd repo &&\n+\t\t\t# Verify\n \t\t\tgit for-each-ref &&\n \t\t\tgit rev-list --test-bitmap refs/tags/old-tip &&\n \t\t\tgit rev-list --test-bitmap refs/tags/new-tip\n@@ -183,9 +175,8 @@ bitmap_reuse_tests 'MIDX' 'pack'\n bitmap_reuse_tests 'MIDX' 'MIDX'\n \n test_expect_success 'missing object closure fails gracefully' '\n-\trm -fr repo &&\n-\tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n \t(\n \t\tcd repo &&\n \n@@ -217,9 +208,8 @@ test_expect_success 'setup partial bitmaps' '\n basic_bitmap_tests HEAD~\n \n test_expect_success 'removing a MIDX clears stale bitmaps' '\n-\trm -fr repo &&\n-\tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n \t(\n \t\tcd repo &&\n \t\ttest_commit base &&\n@@ -245,8 +235,8 @@ test_expect_success 'removing a MIDX clears stale bitmaps' '\n '\n \n test_expect_success 'pack.preferBitmapTips' '\n-\tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n \t(\n \t\tcd repo &&\n \n@@ -284,9 +274,8 @@ test_expect_success 'pack.preferBitmapTips' '\n '\n \n test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n-\trm -fr repo &&\n-\tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n+\tgit init repo &&\n \t(\n \t\tcd repo &&\n \n"},{"id":"436240","messageId":"YUTRHaHDjCaxmQyY@nand.local","threadId":"56455","inReplyTo":"87y27veeyj.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T17:32:13Z","receivedAt":"2021-09-17T17:32:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 17, 2021 at 10:56:09AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >> It seems the need for this \"rm -fr repo\" dance instead of just relying\n> >> on test_when_finished \"rm -rf repo\" is because of a 3x tests in a\n> >> function in tb/multi-pack-bitmaps that should probably be combined into\n> >> one, i.e. they share the same logical \"repo\" setup.\n> >\n> > Yeah. There's definitely room for clean-up there if we want to have each\n> > of the tests operate on the same repo. I have always found sharing a\n> > repo between tests difficult to reason about, since we have to assume\n> > that any --run parameters could be passed in.\n> >\n> > So I have tended to err on the side of creating and throwing away a new\n> > repo per test, but I understand that's somewhat atypical for Git's\n> > tests.\n>\n> Just in an effort to make myself clear here, because between your note\n> in https://lore.kernel.org/git/YUOds4kcHRgMk5nC@nand.local/ and\n> re-reading my mail here I can barely understand what I meant here :)\n\nThanks for clarifying; if I could summarize I would say that this\ndiscussion got started since a new test I added starts with:\n\n rm -fr repo &&\n git init repo &&\n test_when_finished \"rm -fr repo\"\n\nwhere the first rm is designed to clean any state left behind from the\nsplit-up tests added in c51f5a6437c.\n\nI understand your suggestion, and I even think that it may be worth\ndoing, but I'm not sure that I buy that any such cleanup is related to\nthis series for any reason other than \"you happened to add a new test in\nthis file which extended the pattern\".\n\nSo let's pursue the cleanup, but outside of this series (and maybe once\nthe dust has settled more generally on the MIDX bitmaps topics to avoid\nhaving the maintainer deal with any conflicts).\n\n> Or, you can also just not re-use the \"repo\" name, which is what I did in\n> the fsck tests at\n> https://lore.kernel.org/git/patch-v6-01.22-ebe89f65354-20210907T104559Z-avarab@gmail.com/;\n> then you don't even need test_when_finished \"rm -rf[ ...]\".\n\nTBH, I think that this is the most appealing thing to do. It's easy and\ndoesn't require us to think too hard about test_expect_setup or\nsub-tests or anything like that where I'm not sure the complexity is\nwarranted.\n\nI would probably do something like this:\n\n--- >8 ---\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex ec4aa89f63..11845f67ae 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -132,12 +132,12 @@ test_expect_success 'clone with bitmaps enabled' '\n bitmap_reuse_tests() {\n \tfrom=$1\n \tto=$2\n+\trepo=\"bitmap-reuse-$from-$to\"\n\n \ttest_expect_success \"setup pack reuse tests ($from -> $to)\" '\n-\t\trm -fr repo &&\n-\t\tgit init repo &&\n+\t\tgit init $repo &&\n \t\t(\n-\t\t\tcd repo &&\n+\t\t\tcd $repo &&\n \t\t\ttest_commit_bulk 16 &&\n \t\t\tgit tag old-tip &&\n\n@@ -154,7 +154,7 @@ bitmap_reuse_tests() {\n\n \ttest_expect_success \"build bitmap from existing ($from -> $to)\" '\n \t\t(\n-\t\t\tcd repo &&\n+\t\t\tcd $repo &&\n \t\t\ttest_commit_bulk --id=further 16 &&\n \t\t\tgit tag new-tip &&\n\n@@ -170,7 +170,7 @@ bitmap_reuse_tests() {\n\n \ttest_expect_success \"verify resulting bitmaps ($from -> $to)\" '\n \t\t(\n-\t\t\tcd repo &&\n+\t\t\tcd $repo &&\n \t\t\tgit for-each-ref &&\n \t\t\tgit rev-list --test-bitmap refs/tags/old-tip &&\n \t\t\tgit rev-list --test-bitmap refs/tags/new-tip\n@@ -183,7 +183,6 @@ bitmap_reuse_tests 'MIDX' 'pack'\n bitmap_reuse_tests 'MIDX' 'MIDX'\n\n test_expect_success 'missing object closure fails gracefully' '\n-\trm -fr repo &&\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n \t(\n@@ -217,7 +216,6 @@ test_expect_success 'setup partial bitmaps' '\n basic_bitmap_tests HEAD~\n\n test_expect_success 'removing a MIDX clears stale bitmaps' '\n-\trm -fr repo &&\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n \t(\n@@ -284,7 +282,6 @@ test_expect_success 'pack.preferBitmapTips' '\n '\n\n test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n-\trm -fr repo &&\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n \t(\n"},{"id":"436249","messageId":"878rzvdmwq.fsf@evledraar.gmail.com","threadId":"56455","inReplyTo":"YUTRHaHDjCaxmQyY@nand.local","subject":"Re: [PATCH 4/4] t5326: test propagating hashcache values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-17T19:22:51Z","receivedAt":"2021-09-17T19:26:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 17 2021, Taylor Blau wrote:\n\n> On Fri, Sep 17, 2021 at 10:56:09AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> >> It seems the need for this \"rm -fr repo\" dance instead of just relying\n>> >> on test_when_finished \"rm -rf repo\" is because of a 3x tests in a\n>> >> function in tb/multi-pack-bitmaps that should probably be combined into\n>> >> one, i.e. they share the same logical \"repo\" setup.\n>> >\n>> > Yeah. There's definitely room for clean-up there if we want to have each\n>> > of the tests operate on the same repo. I have always found sharing a\n>> > repo between tests difficult to reason about, since we have to assume\n>> > that any --run parameters could be passed in.\n>> >\n>> > So I have tended to err on the side of creating and throwing away a new\n>> > repo per test, but I understand that's somewhat atypical for Git's\n>> > tests.\n>>\n>> Just in an effort to make myself clear here, because between your note\n>> in https://lore.kernel.org/git/YUOds4kcHRgMk5nC@nand.local/ and\n>> re-reading my mail here I can barely understand what I meant here :)\n>\n> Thanks for clarifying; if I could summarize I would say that this\n> discussion got started since a new test I added starts with:\n>\n> rm -fr repo &&\n> git init repo &&\n> test_when_finished \"rm -fr repo\"\n>\n> where the first rm is designed to clean any state left behind from the\n> split-up tests added in c51f5a6437c.\n>\n> I understand your suggestion, and I even think that it may be worth\n> doing, but I'm not sure that I buy that any such cleanup is related to\n> this series for any reason other than \"you happened to add a new test in\n> this file which extended the pattern\".\n>\n> So let's pursue the cleanup, but outside of this series (and maybe once\n> the dust has settled more generally on the MIDX bitmaps topics to avoid\n> having the maintainer deal with any conflicts).\n\nYes I agree, FWIW I didn't look carefully at what was in-flight where at\nthe time, or the series this depends on was itself in \"seen\", I can't\nremember which..\n\n>> Or, you can also just not re-use the \"repo\" name, which is what I did in\n>> the fsck tests at\n>> https://lore.kernel.org/git/patch-v6-01.22-ebe89f65354-20210907T104559Z-avarab@gmail.com/;\n>> then you don't even need test_when_finished \"rm -rf[ ...]\".\n>\n> TBH, I think that this is the most appealing thing to do. It's easy and\n> doesn't require us to think too hard about test_expect_setup or\n> sub-tests or anything like that where I'm not sure the complexity is\n> warranted.\n>\n> I would probably do something like this:\n\nYeah, that bypasses the cleanup bit, but leaves --run broken as I\ndescribed. For new code I think it's good practice to have --run work\ntoo.\n\nAgain, I think that's a non-issue here considering the rest of the\ndumpster fire in the test suite when it comes to that, so I'm not\nsuggesting a re-roll or whatever.\n\nI just used the upthread as a jumping off point since you had a question\nabout these patterns in the other topic, and perhaps you/some bystander\nwould be convinced and follow that pattern in the future.\n\n> --- >8 ---\n>\n> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> index ec4aa89f63..11845f67ae 100755\n> --- a/t/t5326-multi-pack-bitmaps.sh\n> +++ b/t/t5326-multi-pack-bitmaps.sh\n> @@ -132,12 +132,12 @@ test_expect_success 'clone with bitmaps enabled' '\n> bitmap_reuse_tests() {\n> \tfrom=$1\n> \tto=$2\n> +\trepo=\"bitmap-reuse-$from-$to\"\n>\n> \ttest_expect_success \"setup pack reuse tests ($from -> $to)\" '\n> -\t\trm -fr repo &&\n> -\t\tgit init repo &&\n> +\t\tgit init $repo &&\n> \t\t(\n> -\t\t\tcd repo &&\n> +\t\t\tcd $repo &&\n> \t\t\ttest_commit_bulk 16 &&\n> \t\t\tgit tag old-tip &&\n>\n> @@ -154,7 +154,7 @@ bitmap_reuse_tests() {\n>\n> \ttest_expect_success \"build bitmap from existing ($from -> $to)\" '\n> \t\t(\n> -\t\t\tcd repo &&\n> +\t\t\tcd $repo &&\n> \t\t\ttest_commit_bulk --id=further 16 &&\n> \t\t\tgit tag new-tip &&\n>\n> @@ -170,7 +170,7 @@ bitmap_reuse_tests() {\n>\n> \ttest_expect_success \"verify resulting bitmaps ($from -> $to)\" '\n> \t\t(\n> -\t\t\tcd repo &&\n> +\t\t\tcd $repo &&\n> \t\t\tgit for-each-ref &&\n> \t\t\tgit rev-list --test-bitmap refs/tags/old-tip &&\n> \t\t\tgit rev-list --test-bitmap refs/tags/new-tip\n> @@ -183,7 +183,6 @@ bitmap_reuse_tests 'MIDX' 'pack'\n> bitmap_reuse_tests 'MIDX' 'MIDX'\n>\n> test_expect_success 'missing object closure fails gracefully' '\n> -\trm -fr repo &&\n> \tgit init repo &&\n> \ttest_when_finished \"rm -fr repo\" &&\n> \t(\n> @@ -217,7 +216,6 @@ test_expect_success 'setup partial bitmaps' '\n> basic_bitmap_tests HEAD~\n>\n> test_expect_success 'removing a MIDX clears stale bitmaps' '\n> -\trm -fr repo &&\n> \tgit init repo &&\n> \ttest_when_finished \"rm -fr repo\" &&\n> \t(\n> @@ -284,7 +282,6 @@ test_expect_success 'pack.preferBitmapTips' '\n> '\n>\n> test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n> -\trm -fr repo &&\n> \tgit init repo &&\n> \ttest_when_finished \"rm -fr repo\" &&\n> \t(\n\n"},{"id":"436261","messageId":"cover.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631049462.git.me@ttaylorr.com","subject":"[PATCH v3 0/7] pack-bitmap: permute existing namehash values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:11Z","receivedAt":"2021-09-17T21:21:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a very small reroll of my series to permute values from an existing\nhash-cache when generating multi-pack reachability bitmaps.\n\nThe only changes since last time are a slightly modified version of the\nperformance tests which don't time generating the pack-bitmap. And the new test\nin t5326 no longer has a redundant `git config pack.writeBitmapHashCache` since\nit is true by default.\n\nIt is based on the `tb/multi-pack-bitmaps` topic, which graduated to master.\nThanks in advance for your review on this hopefully-final iteration :-).\n\nTaylor Blau (7):\n t/helper/test-bitmap.c: add 'dump-hashes' mode\n pack-bitmap.c: propagate namehash values from existing bitmaps\n midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps\n p5326: create missing 'perf-tag' tag\n p5326: don't set core.multiPackIndex unnecessarily\n p5326: generate pack bitmaps before writing the MIDX bitmap\n t5326: test propagating hashcache values\n\n Documentation/config/pack.txt | 4 +++\n builtin/multi-pack-index.c | 21 +++++++++++++++\n midx.c | 6 ++++-\n midx.h | 1 +\n pack-bitmap.c | 41 +++++++++++++++++++++++++-----\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 10 +++++++-\n t/perf/p5326-multi-pack-bitmaps.sh | 15 ++++++++---\n t/t5326-multi-pack-bitmaps.sh | 30 ++++++++++++++++++++++\n 9 files changed, 118 insertions(+), 11 deletions(-)\n\nRange-diff against v2:\n1: 4f2b8d9530 = 1: 4f2b8d9530 t/helper/test-bitmap.c: add 'dump-hashes' mode\n2: 2cd2f3aa5e = 2: 2cd2f3aa5e pack-bitmap.c: propagate namehash values from existing bitmaps\n3: f0d8f106c2 = 3: f0d8f106c2 midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps\n4: a8c6e845ad = 4: a8c6e845ad p5326: create missing 'perf-tag' tag\n5: 191922c8f2 = 5: 191922c8f2 p5326: don't set core.multiPackIndex unnecessarily\n6: 040bb40548 ! 6: 59b6914ef8 p5326: generate pack bitmaps before writing the MIDX bitmap\n @@ Commit message\n we have to rely on a single-pack bitmap to generate those values for us.\n\n Therefore, generate a pack bitmap before the MIDX one in order to ensure\n - that the MIDX bitmap has entries in its hash-cache.\n + that the MIDX bitmap has entries in its hash-cache. Since we don't want\n + to time generating the pack bitmap, move that to a non-perf test run\n + before we try to generate the MIDX bitmap.\n +\n + Likewise, get rid of the pack bitmap afterwords, to make certain that we\n + are not accidentally using it in the performance tests run later on.\n\n Signed-off-by: Taylor Blau \n\n ## t/perf/p5326-multi-pack-bitmaps.sh ##\n @@ t/perf/p5326-multi-pack-bitmaps.sh: test_expect_success 'create tags' '\n + \tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n '\n\n ++test_expect_success 'start with bitmapped pack' '\n ++\tgit repack -adb\n ++'\n ++\n test_perf 'setup multi-pack index' '\n -\tgit repack -ad &&\n -+\tgit repack -adb &&\n \tgit multi-pack-index write --bitmap\n '\n\n ++test_expect_success 'drop pack bitmap' '\n ++\trm -f .git/objects/pack/pack-*.bitmap\n ++'\n ++\n + test_full_bitmap\n +\n + test_expect_success 'create partial bitmap state' '\n7: fdf71432b3 ! 7: bb16125915 t5326: test propagating hashcache values\n @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'pack.preferBitmapTips' '\n +\t(\n +\t\tcd repo &&\n +\n -+\t\tgit config pack.writeBitmapHashCache true &&\n -+\n +\t\ttest_commit base &&\n +\t\ttest_commit base2 &&\n +\t\tgit repack -adb &&\n--\n2.33.0.96.g73915697e6\n"},{"id":"436262","messageId":"4f2b8d95303d54a50b13117d771da690c4700d3e.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 1/7] t/helper/test-bitmap.c: add 'dump-hashes' mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:14Z","receivedAt":"2021-09-17T21:21:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The pack-bitmap writer code is about to learn how to propagate values\nfrom an existing hash-cache. To prepare, teach the test-bitmap helper to\ndump the values from a bitmap's hash-cache extension in order to test\nthose changes.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 27 +++++++++++++++++++++++++++\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 10 +++++++++-\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 8504110a4d..04de387318 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1742,6 +1742,33 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_hashes(struct repository *r)\n+{\n+\tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n+\tstruct object_id oid;\n+\tuint32_t i, index_pos;\n+\n+\tif (!bitmap_git->hashes)\n+\t\tgoto cleanup;\n+\n+\tfor (i = 0; i < bitmap_num_objects(bitmap_git); i++) {\n+\t\tif (bitmap_is_midx(bitmap_git))\n+\t\t\tindex_pos = pack_pos_to_midx(bitmap_git->midx, i);\n+\t\telse\n+\t\t\tindex_pos = pack_pos_to_index(bitmap_git->pack, i);\n+\n+\t\tnth_bitmap_object_oid(bitmap_git, &oid, index_pos);\n+\n+\t\tprintf(\"%s %\"PRIu32\"\\n\",\n+\t\t oid_to_hex(&oid), get_be32(bitmap_git->hashes + index_pos));\n+\t}\n+\n+cleanup:\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int rebuild_bitmap(const uint32_t *reposition,\n \t\t struct ewah_bitmap *source,\n \t\t struct bitmap *dest)\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 469090bad2..ed46d27077 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -52,6 +52,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_hashes(struct repository *r);\n struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n \t\t\t\t\t struct list_objects_filter_options *filter,\n \t\t\t\t\t int filter_provided_objects);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 134a1e9d76..ff35f5999b 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -7,6 +7,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_dump_hashes(void)\n+{\n+\treturn test_bitmap_hashes(the_repository);\n+}\n+\n int cmd__bitmap(int argc, const char **argv)\n {\n \tsetup_git_directory();\n@@ -16,9 +21,12 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (!strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (!strcmp(argv[1], \"dump-hashes\"))\n+\t\treturn bitmap_dump_hashes();\n \n usage:\n-\tusage(\"\\ttest-tool bitmap list-commits\");\n+\tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap dump-hashes\");\n \n \treturn -1;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"436263","messageId":"2cd2f3aa5e74a3aefd6924c528cbfb578a510d31.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 2/7] pack-bitmap.c: propagate namehash values from existing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:17Z","receivedAt":"2021-09-17T21:21:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When an old bitmap exists while writing a new one, we load it and build\na \"reposition\" table which maps bit positions of objects from the old\nbitmap to their respective positions in the new bitmap. This can help\nwhen we encounter a commit which was selected in both the old and new\nbitmap, since we only need to permute its bit (not recompute it from\nscratch).\n\nWe do not, however, repurpose existing namehash values in the case of\nthe hash-cache extension. There has been thus far no good reason to do\nso, since all of the namehash values for objects in the new bitmap would\nbe populated during the traversal that was just performed by\npack-objects when generating single-pack reachability bitmaps.\n\nBut this isn't the case for multi-pack bitmaps, which are written via\n`git multi-pack-index write --bitmap` and do not perform any traversal.\nIn this case all namehash values are set to zero, but we don't even\nbother to check the `pack.writeBitmapHashcache` option anyway, so it\nfails to matter.\n\nThere are two approaches we could take to fill in non-zero hash-cache\nvalues:\n\n - have either the multi-pack-index builtin run its own\n traversal to attempt to fill in some values, or let a hypothetical\n caller (like `pack-objects` when `repack` eventually drives the\n `multi-pack-index` builtin) fill in the values they found during\n their traversal\n\n - or copy any existing namehash values that were stored in an\n existing bitmap to their corresponding positions in the new bitmap\n\nIn a system where a repository is generally repacked with `git repack\n--geometric=` and occasionally repacked with `git repack -a`, the\nhash-cache coverage will tend towards all objects.\n\nSince populating the hash-cache is additive (i.e., doing so only helps\nour delta search), any intermediate lack of full coverage is just fine.\nSo let's start by just propagating any values from the existing\nhash-cache if we see one.\n\nThe next patch will respect the `pack.writeBitmapHashcache` option while\nwriting MIDX bitmaps, and then test this new behavior.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 04de387318..33a3732992 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1818,18 +1818,20 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < num_objects; ++i) {\n \t\tstruct object_id oid;\n \t\tstruct object_entry *oe;\n+\t\tuint32_t index_pos;\n \n \t\tif (bitmap_is_midx(bitmap_git))\n-\t\t\tnth_midxed_object_oid(&oid,\n-\t\t\t\t\t bitmap_git->midx,\n-\t\t\t\t\t pack_pos_to_midx(bitmap_git->midx, i));\n+\t\t\tindex_pos = pack_pos_to_midx(bitmap_git->midx, i);\n \t\telse\n-\t\t\tnth_packed_object_id(&oid, bitmap_git->pack,\n-\t\t\t\t\t pack_pos_to_index(bitmap_git->pack, i));\n+\t\t\tindex_pos = pack_pos_to_index(bitmap_git->pack, i);\n+\t\tnth_bitmap_object_oid(bitmap_git, &oid, index_pos);\n \t\toe = packlist_find(mapping, &oid);\n \n-\t\tif (oe)\n+\t\tif (oe) {\n \t\t\treposition[i] = oe_in_pack_pos(mapping, oe) + 1;\n+\t\t\tif (bitmap_git->hashes && !oe->hash)\n+\t\t\t\toe->hash = get_be32(bitmap_git->hashes + index_pos);\n+\t\t}\n \t}\n \n \treturn reposition;\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"436264","messageId":"f0d8f106c22dae64eaf4b77d1ecf67ef79c4515c.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 3/7] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:19Z","receivedAt":"2021-09-17T21:21:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the previous commit, the bitmap writing code learned to propagate\nvalues from an existing hash-cache extension into the bitmap that it is\nwriting.\n\nNow that that functionality exists, let's expose it by teaching the 'git\nmulti-pack-index' builtin to respect the `pack.writeBitmapHashCache`\noption so that the hash-cache may be written at all.\n\nTwo minor points worth noting here:\n\n - The 'git multi-pack-index write' sub-command didn't previously read\n any configuration (instead this is handled in the base command). A\n separate handler is added here to respect this write-specific\n config option.\n\n - I briefly considered adding a 'bitmap_flags' field to the static\n options struct, but decided against it since it would require\n plumbing through a new parameter to the write_midx_file() function.\n\n Instead, a new MIDX-specific flag is added, which is translated to\n the corresponding bitmap one.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/config/pack.txt | 4 ++++\n builtin/multi-pack-index.c | 21 +++++++++++++++++++++\n midx.c | 6 +++++-\n midx.h | 1 +\n 4 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 763f7af7c4..ad7f73a1ea 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -159,6 +159,10 @@ pack.writeBitmapHashCache::\n \tbetween an older, bitmapped pack and objects that have been\n \tpushed since the last gc). The downside is that it consumes 4\n \tbytes per object of disk space. Defaults to true.\n++\n+When writing a multi-pack reachability bitmap, no new namehashes are\n+computed; instead, any namehashes stored in an existing bitmap are\n+permuted into their appropriate location when writing a new bitmap.\n \n pack.writeReverseIndex::\n \tWhen true, git will write a corresponding .rev file (see:\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 73c0113b48..578ffea6e8 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -61,6 +61,23 @@ static struct option *add_common_options(struct option *prev)\n \treturn parse_options_concat(common_opts, prev);\n }\n \n+static int git_multi_pack_index_write_config(const char *var, const char *value,\n+\t\t\t\t\t void *cb)\n+{\n+\tif (!strcmp(var, \"pack.writebitmaphashcache\")) {\n+\t\tif (git_config_bool(var, value))\n+\t\t\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t\telse\n+\t\t\topts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;\n+\t}\n+\n+\t/*\n+\t * We should never make a fall-back call to 'git_default_config', since\n+\t * this was already called in 'cmd_multi_pack_index()'.\n+\t */\n+\treturn 0;\n+}\n+\n static int cmd_multi_pack_index_write(int argc, const char **argv)\n {\n \tstruct option *options;\n@@ -73,6 +90,10 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)\n \t\tOPT_END(),\n \t};\n \n+\topts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;\n+\n+\tgit_config(git_multi_pack_index_write_config, NULL);\n+\n \toptions = add_common_options(builtin_multi_pack_index_write_options);\n \n \ttrace2_cmd_mode(argv[0]);\ndiff --git a/midx.c b/midx.c\nindex 864034a6ad..38c8600458 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1008,9 +1008,13 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \tstruct pack_idx_entry **index;\n \tstruct commit **commits = NULL;\n \tuint32_t i, commits_nr;\n+\tuint16_t options = 0;\n \tchar *bitmap_name = xstrfmt(\"%s-%s.bitmap\", midx_name, hash_to_hex(midx_hash));\n \tint ret;\n \n+\tif (flags & MIDX_WRITE_BITMAP_HASH_CACHE)\n+\t\toptions |= BITMAP_OPT_HASH_CACHE;\n+\n \tprepare_midx_packing_data(&pdata, ctx);\n \n \tcommits = find_commits_for_midx_bitmap(&commits_nr, ctx);\n@@ -1049,7 +1053,7 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \t\tgoto cleanup;\n \n \tbitmap_writer_set_checksum(midx_hash);\n-\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, 0);\n+\tbitmap_writer_finish(index, pdata.nr_objects, bitmap_name, options);\n \n cleanup:\n \tfree(index);\ndiff --git a/midx.h b/midx.h\nindex aa3da557bb..541d9ac728 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -44,6 +44,7 @@ struct multi_pack_index {\n #define MIDX_PROGRESS (1 << 0)\n #define MIDX_WRITE_REV_INDEX (1 << 1)\n #define MIDX_WRITE_BITMAP (1 << 2)\n+#define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)\n \n const unsigned char *get_midx_checksum(struct multi_pack_index *m);\n char *get_midx_filename(const char *object_dir);\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"436265","messageId":"a8c6e845adf559a96e37c973fea16da8a42e7cba.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 4/7] p5326: create missing 'perf-tag' tag","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:22Z","receivedAt":"2021-09-17T21:21:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Some of the tests in test_full_bitmap rely on having a tag named\nperf-tag in place. We could create it in test_full_bitmap(), but we want\nto have it in place before the repack starts.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5326-multi-pack-bitmaps.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\nindex 5845109ac7..51b5636259 100755\n--- a/t/perf/p5326-multi-pack-bitmaps.sh\n+++ b/t/perf/p5326-multi-pack-bitmaps.sh\n@@ -10,6 +10,12 @@ test_expect_success 'enable multi-pack index' '\n \tgit config core.multiPackIndex true\n '\n \n+# we need to create the tag up front such that it is covered by the repack and\n+# thus by generated bitmaps.\n+test_expect_success 'create tags' '\n+\tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n+'\n+\n test_perf 'setup multi-pack index' '\n \tgit repack -ad &&\n \tgit multi-pack-index write --bitmap\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"436266","messageId":"191922c8f28c338ab3aa4faa57d75770a9eccd02.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 5/7] p5326: don't set core.multiPackIndex unnecessarily","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:24Z","receivedAt":"2021-09-17T21:21:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When this performance test was originally written, `core.multiPackIndex`\nwas not the default and thus had to be enabled. But now that we have\n18e449f86b (midx: enable core.multiPackIndex by default, 2020-09-25), we\nno longer need this.\n\nDrop the unnecessary setup (even though it's not hurting anything, it is\nunnecessary at best and confusing at worst).\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5326-multi-pack-bitmaps.sh | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\nindex 51b5636259..a9c5499537 100755\n--- a/t/perf/p5326-multi-pack-bitmaps.sh\n+++ b/t/perf/p5326-multi-pack-bitmaps.sh\n@@ -6,10 +6,6 @@ test_description='Tests performance using midx bitmaps'\n \n test_perf_large_repo\n \n-test_expect_success 'enable multi-pack index' '\n-\tgit config core.multiPackIndex true\n-'\n-\n # we need to create the tag up front such that it is covered by the repack and\n # thus by generated bitmaps.\n test_expect_success 'create tags' '\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"436267","messageId":"59b6914ef864f31b53ed7e89f73db5c35b298449.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 6/7] p5326: generate pack bitmaps before writing the MIDX bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:27Z","receivedAt":"2021-09-17T21:21:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To help test the performance of permuting the contents of the hash-cache\nwhen generating a MIDX bitmap, we need a bitmap which has its hash-cache\npopulated.\n\nAnd since multi-pack bitmaps don't add *new* values to the hash-cache,\nwe have to rely on a single-pack bitmap to generate those values for us.\n\nTherefore, generate a pack bitmap before the MIDX one in order to ensure\nthat the MIDX bitmap has entries in its hash-cache. Since we don't want\nto time generating the pack bitmap, move that to a non-perf test run\nbefore we try to generate the MIDX bitmap.\n\nLikewise, get rid of the pack bitmap afterwords, to make certain that we\nare not accidentally using it in the performance tests run later on.\n\nSigned-off-by: Taylor Blau \n---\n t/perf/p5326-multi-pack-bitmaps.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/t/perf/p5326-multi-pack-bitmaps.sh b/t/perf/p5326-multi-pack-bitmaps.sh\nindex a9c5499537..f2fa228f16 100755\n--- a/t/perf/p5326-multi-pack-bitmaps.sh\n+++ b/t/perf/p5326-multi-pack-bitmaps.sh\n@@ -12,11 +12,18 @@ test_expect_success 'create tags' '\n \tgit tag --message=\"tag pointing to HEAD\" perf-tag HEAD\n '\n \n+test_expect_success 'start with bitmapped pack' '\n+\tgit repack -adb\n+'\n+\n test_perf 'setup multi-pack index' '\n-\tgit repack -ad &&\n \tgit multi-pack-index write --bitmap\n '\n \n+test_expect_success 'drop pack bitmap' '\n+\trm -f .git/objects/pack/pack-*.bitmap\n+'\n+\n test_full_bitmap\n \n test_expect_success 'create partial bitmap state' '\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"436268","messageId":"bb1612591516bf7e88886d405fc06dd1d2e9a8bb.1631913631.git.me@ttaylorr.com","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"[PATCH v3 7/7] t5326: test propagating hashcache values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-17T21:21:29Z","receivedAt":"2021-09-17T21:21:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that we both can propagate values from the hashcache, and respect\nthe configuration to enable the hashcache at all, test that both of\nthese function correctly by hardening their behavior with a test.\n\nLike the hash-cache in classic single-pack bitmaps, this helps more\nproportionally the more up-to-date your bitmap coverage is. When our\nbitmap coverage is out-of-date with the ref tips, we spend more time\nproportionally traversing, and all of that traversal gets the name-hash\nfilled in.\n\nBut for the up-to-date bitmaps, this helps quite a bit. These numbers\nare on git.git, with `pack.threads=1` to help see the difference\nreflected in the overall runtime.\n\n Test origin/tb/multi-pack-bitmaps HEAD\n -------------------------------------------------------------------------------------\n 5326.4: simulated clone 1.87(1.80+0.07) 1.46(1.42+0.03) -21.9%\n 5326.5: simulated fetch 2.66(2.61+0.04) 1.47(1.43+0.04) -44.7%\n 5326.6: pack to file (bitmap) 2.74(2.62+0.12) 1.89(1.82+0.07) -31.0%\n\nSigned-off-by: Taylor Blau \n---\n t/t5326-multi-pack-bitmaps.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 4ad7c2c969..ec4aa89f63 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -283,4 +283,34 @@ test_expect_success 'pack.preferBitmapTips' '\n \t)\n '\n \n+test_expect_success 'hash-cache values are propagated from pack bitmaps' '\n+\trm -fr repo &&\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit base &&\n+\t\ttest_commit base2 &&\n+\t\tgit repack -adb &&\n+\n+\t\ttest-tool bitmap dump-hashes >pack.raw &&\n+\t\ttest_file_not_empty pack.raw &&\n+\t\tsort pack.raw >pack.hashes &&\n+\n+\t\ttest_commit new &&\n+\t\tgit repack &&\n+\t\tgit multi-pack-index write --bitmap &&\n+\n+\t\ttest-tool bitmap dump-hashes >midx.raw &&\n+\t\tsort midx.raw >midx.hashes &&\n+\n+\t\t# ensure that every namehash in the pack bitmap can be found in\n+\t\t# the midx bitmap (i.e., that there are no oid-namehash pairs\n+\t\t# unique to the pack bitmap).\n+\t\tcomm -23 pack.hashes midx.hashes >dropped.hashes &&\n+\t\ttest_must_be_empty dropped.hashes\n+\t)\n+'\n+\n test_done\n-- \n2.33.0.96.g73915697e6\n"},{"id":"436274","messageId":"YUUS4Mrh85KPucmi@coredump.intra.peff.net","threadId":"56455","inReplyTo":"cover.1631913631.git.me@ttaylorr.com","subject":"Re: [PATCH v3 0/7] pack-bitmap: permute existing namehash values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-17T22:12:48Z","receivedAt":"2021-09-17T22:12:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 17, 2021 at 05:21:11PM -0400, Taylor Blau wrote:\n\n> Here is a very small reroll of my series to permute values from an existing\n> hash-cache when generating multi-pack reachability bitmaps.\n> \n> The only changes since last time are a slightly modified version of the\n> performance tests which don't time generating the pack-bitmap. And the new test\n> in t5326 no longer has a redundant `git config pack.writeBitmapHashCache` since\n> it is true by default.\n\nThanks for picking up my suggestions. This all looks good to me!\n\n-Peff\n"}]}