{"thread":{"id":"59573","subject":"[PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default","startedAt":"2023-04-10T22:53:28Z","lastAt":"2023-04-13T16:14:18Z","messageCount":29,"participants":["Taylor Blau","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"475088","messageId":"cover.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":null,"subject":"[PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:18Z","receivedAt":"2023-04-10T22:53:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series enables pack reverse-indexes to be written to disk by\ndefault instead of computed on-the-fly in memory.\n\nFor repositories with large packs, this can have a significant benefit\nfor both end-users and Git forges. Extensive performance tests appear in\nthe fifth and sixth commit, but here are some highlights:\n\n - The time it takes to generate a pack containing objects for the 10\n most-recent commits in linux.git (starting from 68047c48b228) drops\n from ~540ms to ~240ms, yielding a ~2.22x improvement.\n\n - The time it takes to compute the on-disk object size of a single\n object drops from ~300ms to 4ms, yielding a ~76.96x improvement.\n\nReverse indexes are a trade-off between the time they take to be\ncomputed versus the time it takes to access a single record. In-memory\nreverse indexes take time to generate proportional to the number of\npacked objects in the repository, but have instantaneous lookup time.\n\nOn-disk reverse indexes can be \"generated\" instantly (the time it takes\nto generate them is a one-time cost, loading them is instantaneous).\nBut individual record lookup time is slower, since it involves multiple\ndisk I/O operations.\n\nIn the vast majority of cases, this trade-off favors the on-disk \".rev\"\nfiles. But in certain cases, the in-memory variant performs more\nfavorably. Since these cases are narrow, and performance is machine- and\nrepository-dependent, this series also introduces a new configuration\noption to disable reading \".rev\" files in the third commit.\n\nThe series is structured as follows:\n\n - A couple of cleanup patches to plug a leak in stage_tmp_packfiles().\n - Three patches to enable `pack.readReverseIndex`.\n - Another patch to change the default of `pack.writeReverseIndex` from\n \"false\" to \"true\".\n - A final patch to enable the test suite to be run in a mode that does\n not use on-disk \".rev\" files.\n\nThanks in advance for your review. I'm really excited to get this in the\nhands of users after a couple of years of running this at GitHub (and\nbeing opt-in otherwise).\n\nTaylor Blau (7):\n pack-write.c: plug a leak in stage_tmp_packfiles()\n t5325: mark as leak-free\n pack-revindex: make `load_pack_revindex` take a repository\n pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK\n pack-revindex: introduce `pack.readReverseIndex`\n config: enable `pack.writeReverseIndex` by default\n t: invert `GIT_TEST_WRITE_REV_INDEX`\n\n Documentation/config/pack.txt | 8 +++++++-\n builtin/index-pack.c | 5 +++--\n builtin/pack-objects.c | 5 +++--\n ci/run-build-and-tests.sh | 1 -\n pack-bitmap.c | 5 +++--\n pack-revindex.c | 12 +++++++++---\n pack-revindex.h | 6 ++++--\n pack-write.c | 2 ++\n packfile.c | 2 +-\n repo-settings.c | 1 +\n repository.h | 1 +\n t/README | 2 +-\n t/perf/p5312-pack-bitmaps-revs.sh | 3 +--\n t/t5325-reverse-index.sh | 16 +++++++++++++++-\n 14 files changed, 51 insertions(+), 18 deletions(-)\n\n-- \n2.40.0.323.g9c80379958\n"},{"id":"475089","messageId":"18be29c3988295cd58521f8cc4a729897df074c8.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH 1/7] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:25Z","receivedAt":"2023-04-10T22:53:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `stage_tmp_packfiles()` generates a filename to use for\nstaging the contents of what will become the pack's \".rev\" file.\n\nThe name is generated in `write_rev_file_order()` (via its caller\n`write_rev_file()`) in a string buffer, and the result is returned back\nto `stage_tmp_packfiles()` which uses it to rename the temporary file\ninto place via `rename_tmp_packfiles()`.\n\nThat name is not visible outside of `stage_tmp_packfiles()`, so it can\n(and should) be `free()`'d at the end of that function. We can't free it\nin `rename_tmp_packfile()` since not all of its `source` arguments are\nunreachable after calling it.\n\nInstead, simply free() `rev_tmp_name` at the end of\n`stage_tmp_packfiles()`.\n\n(Note that the same leak exists for `mtimes_tmp_name`, but we do not\naddress it in this commit).\n\nSigned-off-by: Taylor Blau \n---\n pack-write.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex f171405495..f27c1f7f28 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n \tif (mtimes_tmp_name)\n \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n+\n+\tfree((char *)rev_tmp_name);\n }\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n-- \n2.40.0.323.g9c80379958\n\n"},{"id":"475090","messageId":"affb5e2574bc1072d8f4ab8e3dccde7a5a39e725.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH 2/7] t5325: mark as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:28Z","receivedAt":"2023-04-10T22:53:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This test is leak-free as of the previous commit, so let's mark it as\nsuch to ensure we don't regress and introduce a leak in the future.\n\nSigned-off-by: Taylor Blau \n---\n t/t5325-reverse-index.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex d042d26f2b..48c14d8576 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='on-disk reverse index'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # The below tests want control over the 'pack.writeReverseIndex' setting\n-- \n2.40.0.323.g9c80379958\n\n"},{"id":"475091","messageId":"be4faf11011efcfab479e5785e6c2bbac95309bd.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH 3/7] pack-revindex: make `load_pack_revindex` take a repository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:31Z","receivedAt":"2023-04-10T22:53:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a future commit, we will introduce a `pack.readReverseIndex`\nconfiguration, which forces Git to generate the reverse index from\nscratch instead of loading it from disk.\n\nIn order to avoid reading this configuration value more than once, we'll\nuse the `repo_settings` struct to lazily load this value.\n\nIn order to access the `struct repo_settings`, add a repository argument\nto `load_pack_revindex`, and update all callers to pass the correct\ninstance (in all cases, `the_repository`).\n\nSigned-off-by: Taylor Blauy \n---\n pack-bitmap.c | 5 +++--\n pack-revindex.c | 4 ++--\n pack-revindex.h | 3 ++-\n packfile.c | 2 +-\n 4 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b2e7d06d60..8e3bb00931 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -477,13 +477,14 @@ static int load_reverse_index(struct bitmap_index *bitmap_git)\n \t\t * since we will need to make use of them in pack-objects.\n \t\t */\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n-\t\t\tret = load_pack_revindex(bitmap_git->midx->packs[i]);\n+\t\t\tret = load_pack_revindex(the_repository,\n+\t\t\t\t\t\t bitmap_git->midx->packs[i]);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n \t\treturn 0;\n \t}\n-\treturn load_pack_revindex(bitmap_git->pack);\n+\treturn load_pack_revindex(the_repository, bitmap_git->pack);\n }\n \n static int load_bitmap(struct bitmap_index *bitmap_git)\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 03c7e81f9d..e3d69cc0f7 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -283,7 +283,7 @@ static int load_pack_revindex_from_disk(struct packed_git *p)\n \treturn ret;\n }\n \n-int load_pack_revindex(struct packed_git *p)\n+int load_pack_revindex(struct repository *r, struct packed_git *p)\n {\n \tif (p->revindex || p->revindex_data)\n \t\treturn 0;\n@@ -356,7 +356,7 @@ int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)\n {\n \tunsigned lo, hi;\n \n-\tif (load_pack_revindex(p) < 0)\n+\tif (load_pack_revindex(the_repository, p) < 0)\n \t\treturn -1;\n \n \tlo = 0;\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 4974e75eb4..3d1969ce8b 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -39,6 +39,7 @@\n \n struct packed_git;\n struct multi_pack_index;\n+struct repository;\n \n /*\n * load_pack_revindex populates the revindex's internal data-structures for the\n@@ -47,7 +48,7 @@ struct multi_pack_index;\n * If a '.rev' file is present it is mmap'd, and pointers are assigned into it\n * (instead of using the in-memory variant).\n */\n-int load_pack_revindex(struct packed_git *p);\n+int load_pack_revindex(struct repository *r, struct packed_git *p);\n \n /*\n * load_midx_revindex loads the '.rev' file corresponding to the given\ndiff --git a/packfile.c b/packfile.c\nindex b120405ccc..717fd685c9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2151,7 +2151,7 @@ int for_each_object_in_pack(struct packed_git *p,\n \tint r = 0;\n \n \tif (flags & FOR_EACH_OBJECT_PACK_ORDER) {\n-\t\tif (load_pack_revindex(p))\n+\t\tif (load_pack_revindex(the_repository, p))\n \t\t\treturn -1;\n \t}\n \n-- \n2.40.0.323.g9c80379958\n\n"},{"id":"475092","messageId":"0f368e2347e10325d0c5a34939191994aa8899b9.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH 4/7] pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:34Z","receivedAt":"2023-04-10T22:53:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In ec8e7760ac (pack-revindex: ensure that on-disk reverse indexes are\ngiven precedence, 2021-01-25), we introduced\nGIT_TEST_REV_INDEX_DIE_IN_MEMORY to abort the process when Git generated\na reverse index from scratch.\n\nec8e7760ac was about ensuring that Git prefers a .rev file when\navailable over generating the same information in memory from scratch.\n\nIn a subsequent patch, we'll introduce `pack.readReverseIndex`, which\nmay be used to disable reading \".rev\" files when available. In order to\nensure that those files are indeed being ignored, introduce an analogous\noption to abort the process when Git reads a \".rev\" file from disk.\n\nSigned-off-by: Taylor Blau \n---\n pack-revindex.c | 3 +++\n pack-revindex.h | 1 +\n 2 files changed, 4 insertions(+)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex e3d69cc0f7..44e1b3fed9 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -205,6 +205,9 @@ static int load_revindex_from_disk(char *revindex_name,\n \tsize_t revindex_size;\n \tstruct revindex_header *hdr;\n \n+\tif (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))\n+\t\tdie(\"dying as requested by '%s'\", GIT_TEST_REV_INDEX_DIE_ON_DISK);\n+\n \tfd = git_open(revindex_name);\n \n \tif (fd < 0) {\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 3d1969ce8b..ef8afee88b 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -36,6 +36,7 @@\n \n #define GIT_TEST_WRITE_REV_INDEX \"GIT_TEST_WRITE_REV_INDEX\"\n #define GIT_TEST_REV_INDEX_DIE_IN_MEMORY \"GIT_TEST_REV_INDEX_DIE_IN_MEMORY\"\n+#define GIT_TEST_REV_INDEX_DIE_ON_DISK \"GIT_TEST_REV_INDEX_DIE_ON_DISK\"\n \n struct packed_git;\n struct multi_pack_index;\n-- \n2.40.0.323.g9c80379958\n\n"},{"id":"475093","messageId":"6f692d470cb691402020e2e423e2ec660aba26ce.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH 5/7] pack-revindex: introduce `pack.readReverseIndex`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:37Z","receivedAt":"2023-04-10T22:53:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since 1615c567b8 (Documentation/config/pack.txt: advertise\n'pack.writeReverseIndex', 2021-01-25), we have had the\n`pack.writeReverseIndex` configuration option, which tells Git whether\nor not it is allowed to write a \".rev\" file when indexing a pack.\n\nIntroduce a complementary configuration knob, `pack.readReverseIndex` to\ncontrol whether or not Git will read any \".rev\" file(s) that may be\navailable on disk.\n\nThis option is useful for debugging, as well as disabling the effect of\n\".rev\" files in certain instances.\n\nThis is useful because of the trade-off[^1] between the time it takes to\ngenerate a reverse index (slow from scratch, fast when reading an\nexisting \".rev\" file), and the time it takes to access a record (the\nopposite).\n\nFor example, even though it is faster to use the on-disk reverse index\nwhen computing the on-disk size of a packed object, it is slower to\nenumerate the same value for all objects.\n\nHere are a couple of examples from linux.git. When computing the above\nfor a single object, using the on-disk reverse index is significantly\nfaster:\n\n $ git rev-parse HEAD >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} cat-file --batch-check=\"%(objectsize:disk)\" \n---\n Documentation/config/pack.txt | 6 ++++++\n pack-revindex.c | 5 ++++-\n repo-settings.c | 1 +\n repository.h | 1 +\n t/t5325-reverse-index.sh | 11 +++++++++++\n 5 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 53093d9996..7db7fed466 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -171,6 +171,12 @@ pack.writeBitmapLookupTable::\n \tbeneficial in repositories that have relatively large bitmap\n \tindexes. Defaults to false.\n \n+pack.readReverseIndex::\n+\tWhen true, git will read any .rev file(s) that may be available\n+\t(see: linkgit:gitformat-pack[5]). When false, the reverse index\n+\twill be generated from scratch and stored in memory. Defaults to\n+\ttrue.\n+\n pack.writeReverseIndex::\n \tWhen true, git will write a corresponding .rev file (see:\n \tlinkgit:gitformat-pack[5])\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 44e1b3fed9..29f5358b25 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -291,7 +291,10 @@ int load_pack_revindex(struct repository *r, struct packed_git *p)\n \tif (p->revindex || p->revindex_data)\n \t\treturn 0;\n \n-\tif (!load_pack_revindex_from_disk(p))\n+\tprepare_repo_settings(r);\n+\n+\tif (r->settings.pack_read_reverse_index &&\n+\t !load_pack_revindex_from_disk(p))\n \t\treturn 0;\n \telse if (!create_pack_revindex_in_memory(p))\n \t\treturn 0;\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0a6c0b381f..bdd7640ab0 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -63,6 +63,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n \trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n+\trepo_cfg_bool(r, \"pack.readreverseindex\", &r->settings.pack_read_reverse_index, 1);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 15a8afc5fb..ed73e799b6 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -37,6 +37,7 @@ struct repo_settings {\n \tint fetch_write_commit_graph;\n \tint command_requires_full_index;\n \tint sparse_index;\n+\tint pack_read_reverse_index;\n \n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 48c14d8576..66171c1d67 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -96,6 +96,17 @@ test_expect_success 'reverse index is not generated when available on disk' '\n \t\t--batch-check=\"%(objectsize:disk)\" tip &&\n+\tGIT_TEST_REV_INDEX_DIE_ON_DISK=1 git cat-file \\\n+\t\t--batch-check=\"%(objectsize:disk)\" in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} pack-objects --delta-base-offset --revs --stdout /dev/null'\n Benchmark 1: git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 543.0 ms ± 20.3 ms [User: 616.2 ms, System: 58.8 ms]\n Range (min … max): 521.0 ms … 577.9 ms 10 runs\n\n Benchmark 2: git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 245.0 ms ± 11.4 ms [User: 335.6 ms, System: 31.3 ms]\n Range (min … max): 226.0 ms … 259.6 ms 13 runs\n\n Summary\n 'git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null' ran\n\t2.22 ± 0.13 times faster than 'git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null'\n\nThe same is true of writing a pack containing the objects for the 30\nmost-recent commits:\n\n $ { git rev-parse HEAD && printf '^' && git rev-parse HEAD~30 } >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} pack-objects --delta-base-offset --revs --stdout /dev/null'\n Benchmark 1: git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 866.5 ms ± 16.2 ms [User: 1414.5 ms, System: 97.0 ms]\n Range (min … max): 839.3 ms … 886.9 ms 10 runs\n\n Benchmark 2: git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 581.6 ms ± 10.2 ms [User: 1181.7 ms, System: 62.6 ms]\n Range (min … max): 567.5 ms … 599.3 ms 10 runs\n\n Summary\n 'git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null' ran\n\t1.49 ± 0.04 times faster than 'git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null'\n\n...and savings on trivial operations like computing the on-disk size of\na single (packed) object are even more dramatic:\n\n $ git rev-parse HEAD >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} cat-file --batch-check=\"%(objectsize:disk)\" \n---\n Documentation/config/pack.txt | 2 +-\n builtin/index-pack.c | 1 +\n builtin/pack-objects.c | 1 +\n t/perf/p5312-pack-bitmaps-revs.sh | 3 +--\n t/t5325-reverse-index.sh | 1 +\n 5 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 7db7fed466..d4c7c9d4e4 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -182,4 +182,4 @@ pack.writeReverseIndex::\n \tlinkgit:gitformat-pack[5])\n \tfor each new packfile that it writes in all places except for\n \tlinkgit:git-fast-import[1] and in the bulk checkin mechanism.\n-\tDefaults to false.\n+\tDefaults to true.\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b17e79cd40..323c063f9d 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1753,6 +1753,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tfsck_options.walk = mark_link;\n \n \treset_pack_idx_option(&opts);\n+\topts.flags |= WRITE_REV;\n \tgit_config(git_index_pack_config, &opts);\n \tif (prefix && chdir(prefix))\n \t\tdie(_(\"Cannot come back to cwd\"));\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 77d88f85b0..dbaa04482f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4293,6 +4293,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t}\n \n \treset_pack_idx_option(&pack_idx_opts);\n+\tpack_idx_opts.flags |= WRITE_REV;\n \tgit_config(git_pack_config, NULL);\n \tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n \t\tpack_idx_opts.flags |= WRITE_REV;\ndiff --git a/t/perf/p5312-pack-bitmaps-revs.sh b/t/perf/p5312-pack-bitmaps-revs.sh\nindex 0684b690af..ceec60656b 100755\n--- a/t/perf/p5312-pack-bitmaps-revs.sh\n+++ b/t/perf/p5312-pack-bitmaps-revs.sh\n@@ -12,8 +12,7 @@ test_lookup_pack_bitmap () {\n \ttest_perf_large_repo\n \n \ttest_expect_success 'setup bitmap config' '\n-\t\tgit config pack.writebitmaps true &&\n-\t\tgit config pack.writeReverseIndex true\n+\t\tgit config pack.writebitmaps true\n \t'\n \n \t# we need to create the tag up front such that it is covered by the repack and\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 66171c1d67..149dcf5193 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -14,6 +14,7 @@ packdir=.git/objects/pack\n test_expect_success 'setup' '\n \ttest_commit base &&\n \n+\ttest_config pack.writeReverseIndex false &&\n \tpack=$(git pack-objects --all $packdir/pack) &&\n \trev=$packdir/pack-$pack.rev &&\n \n-- \n2.40.0.323.g9c80379958\n\n"},{"id":"475095","messageId":"9c80379958824ac8fb7834f4f98000d11c3dc4e0.1681166596.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH 7/7] t: invert `GIT_TEST_WRITE_REV_INDEX`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-10T22:53:43Z","receivedAt":"2023-04-10T22:54:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Back in e8c58f894b (t: support GIT_TEST_WRITE_REV_INDEX, 2021-01-25), we\nadded a test knob to conditionally enable writing a \".rev\" file when\nindexing a pack. At the time, this was used to ensure that the test\nsuite worked even when \".rev\" files were written, which served as a\nstress-test for the on-disk reverse index implementation.\n\nNow that reading from on-disk \".rev\" files is enabled by default, the\ntest knob `GIT_TEST_WRITE_REV_INDEX` no longer has any meaning.\n\nWe could get rid of the option entirely, but there would be no\nconvenient way to test Git when \".rev\" files *aren't* in place.\n\nInstead of getting rid of the option, invert its meaning to instead\ndisable writing \".rev\" files, thereby running the test suite in a mode\nwhere the reverse index is generated from scratch.\n\nThis ensures that we are still running and exercising Git's behavior\nwhen forced to generate reverse indexes from scratch.\n\nSigned-off-by: Taylor Blau \n---\n builtin/index-pack.c | 4 ++--\n builtin/pack-objects.c | 4 ++--\n ci/run-build-and-tests.sh | 1 -\n pack-revindex.h | 2 +-\n t/README | 2 +-\n t/t5325-reverse-index.sh | 2 +-\n 6 files changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 323c063f9d..9e36c985cf 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1758,8 +1758,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (prefix && chdir(prefix))\n \t\tdie(_(\"Cannot come back to cwd\"));\n \n-\tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n-\t\trev_index = 1;\n+\tif (git_env_bool(GIT_TEST_NO_WRITE_REV_INDEX, 0))\n+\t\trev_index = 0;\n \telse\n \t\trev_index = !!(opts.flags & (WRITE_REV_VERIFY | WRITE_REV));\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex dbaa04482f..1797871ce9 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4295,8 +4295,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \treset_pack_idx_option(&pack_idx_opts);\n \tpack_idx_opts.flags |= WRITE_REV;\n \tgit_config(git_pack_config, NULL);\n-\tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n-\t\tpack_idx_opts.flags |= WRITE_REV;\n+\tif (git_env_bool(GIT_TEST_NO_WRITE_REV_INDEX, 0))\n+\t\tpack_idx_opts.flags &= ~WRITE_REV;\n \n \tprogress = isatty(2);\n \targc = parse_options(argc, argv, prefix, pack_objects_options,\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex b098e10f52..e9fbfb6345 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -27,7 +27,6 @@ linux-TEST-vars)\n \texport GIT_TEST_MULTI_PACK_INDEX=1\n \texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n-\texport GIT_TEST_WRITE_REV_INDEX=1\n \texport GIT_TEST_CHECKOUT_WORKERS=2\n \t;;\n linux-clang)\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex ef8afee88b..46e834064e 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -34,7 +34,7 @@\n #define RIDX_SIGNATURE 0x52494458 /* \"RIDX\" */\n #define RIDX_VERSION 1\n \n-#define GIT_TEST_WRITE_REV_INDEX \"GIT_TEST_WRITE_REV_INDEX\"\n+#define GIT_TEST_NO_WRITE_REV_INDEX \"GIT_TEST_NO_WRITE_REV_INDEX\"\n #define GIT_TEST_REV_INDEX_DIE_IN_MEMORY \"GIT_TEST_REV_INDEX_DIE_IN_MEMORY\"\n #define GIT_TEST_REV_INDEX_DIE_ON_DISK \"GIT_TEST_REV_INDEX_DIE_ON_DISK\"\n \ndiff --git a/t/README b/t/README\nindex 29576c3748..bdfac4cceb 100644\n--- a/t/README\n+++ b/t/README\n@@ -475,7 +475,7 @@ GIT_TEST_DEFAULT_HASH= specifies which hash algorithm to\n use in the test scripts. Recognized values for are \"sha1\"\n and \"sha256\".\n \n-GIT_TEST_WRITE_REV_INDEX=, when true enables the\n+GIT_TEST_NO_WRITE_REV_INDEX=, when true disables the\n 'pack.writeReverseIndex' setting.\n \n GIT_TEST_SPARSE_INDEX=, when true enables index writes to use the\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 149dcf5193..0548fce1aa 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -7,7 +7,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n # The below tests want control over the 'pack.writeReverseIndex' setting\n # themselves to assert various combinations of it with other options.\n-sane_unset GIT_TEST_WRITE_REV_INDEX\n+sane_unset GIT_TEST_NO_WRITE_REV_INDEX\n \n packdir=.git/objects/pack\n \n-- \n2.40.0.323.g9c80379958\n"},{"id":"475143","messageId":"423b5c5c-ef86-198a-16f7-c836ed78928c@github.com","threadId":"59573","inReplyTo":"18be29c3988295cd58521f8cc4a729897df074c8.1681166596.git.me@ttaylorr.com","subject":"Re: [PATCH 1/7] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-11T13:23:31Z","receivedAt":"2023-04-11T13:23:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/10/2023 6:53 PM, Taylor Blau wrote:\n\n> Instead, simply free() `rev_tmp_name` at the end of\n> `stage_tmp_packfiles()`.\n\n> @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n> \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n> \tif (mtimes_tmp_name)\n> \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n> +\n> +\tfree((char *)rev_tmp_name);\n\nJust cut off from the context is a \"if (rev_tmp_name)\", so it might be\ngood to group this into that block, since we have the condition, anyway.\n\n\nBut I was also thinking about how we like to use \"const\" as an indicator\nas \"I am not responsible for free()ing this\". And this comes from the\npublic write_rev_file() method. Based on the API prototype, we could\nthink that this string is held by a static strbuf (making the method\nnot reentrant, but that happens sometimes in our methods). But generally,\nI wanted to inspect what it would take to make the API reflect the fact\nthat it can return a \"new\" string.\n\nBut there are two issues:\n\n 1. The actual logic is inside write_rev_file_order(), so that API\n needs to change, too.\n\n 2. The \"new\" string is created only if the rev_name parameter is\n NULL, which is somewhat understandable but still requires\n inside knowledge about the implementation to make that choice.\n\n 3. If we inspect the callers to these methods, only one caller\n passes a non-null name: builtin/index-pack.c. The rest pass NULL,\n including write_midx_reverse_index() (which then leaks the name).\n\nThe below diff includes my attempt to change the API to return a\nnon-const string that must be freed by the callers.\n\nThanks,\n-Stolee\n\n--- >8 ---\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b17e79cd40f..6d2fa52f9c4 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1725,7 +1725,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index;\n \tconst char *curr_index;\n-\tconst char *curr_rev_index = NULL;\n+\tchar *curr_rev_index = NULL;\n \tconst char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL;\n \tconst char *keep_msg = NULL;\n \tconst char *promisor_msg = NULL;\n@@ -1956,8 +1956,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tfree((void *) curr_pack);\n \tif (!index_name)\n \t\tfree((void *) curr_index);\n-\tif (!rev_index_name)\n-\t\tfree((void *) curr_rev_index);\n+\tfree(curr_rev_index);\n \n \t/*\n \t * Let the caller know this pack is not self contained\ndiff --git a/midx.c b/midx.c\nindex 9af3e5de889..85154bedd73 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -945,7 +945,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \t\t\t\t struct write_midx_context *ctx)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst char *tmp_file;\n+\tchar *tmp_file;\n \n \ttrace2_region_enter(\"midx\", \"write_midx_reverse_index\", the_repository);\n \n@@ -958,6 +958,7 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n \t\tdie(_(\"cannot store reverse index file\"));\n \n \tstrbuf_release(&buf);\n+\tfree(tmp_file);\n \n \ttrace2_region_leave(\"midx\", \"write_midx_reverse_index\", the_repository);\n }\ndiff --git a/pack-write.c b/pack-write.c\nindex f1714054951..73850c061d9 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -207,15 +207,15 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)\n \thashwrite(f, hash, the_hash_algo->rawsz);\n }\n \n-const char *write_rev_file(const char *rev_name,\n-\t\t\t struct pack_idx_entry **objects,\n-\t\t\t uint32_t nr_objects,\n-\t\t\t const unsigned char *hash,\n-\t\t\t unsigned flags)\n+char *write_rev_file(const char *rev_name,\n+\t\t struct pack_idx_entry **objects,\n+\t\t uint32_t nr_objects,\n+\t\t const unsigned char *hash,\n+\t\t unsigned flags)\n {\n \tuint32_t *pack_order;\n \tuint32_t i;\n-\tconst char *ret;\n+\tchar *ret;\n \n \tif (!(flags & WRITE_REV) && !(flags & WRITE_REV_VERIFY))\n \t\treturn NULL;\n@@ -233,12 +233,13 @@ const char *write_rev_file(const char *rev_name,\n \treturn ret;\n }\n \n-const char *write_rev_file_order(const char *rev_name,\n-\t\t\t\t uint32_t *pack_order,\n-\t\t\t\t uint32_t nr_objects,\n-\t\t\t\t const unsigned char *hash,\n-\t\t\t\t unsigned flags)\n+char *write_rev_file_order(const char *rev_name,\n+\t\t\t uint32_t *pack_order,\n+\t\t\t uint32_t nr_objects,\n+\t\t\t const unsigned char *hash,\n+\t\t\t unsigned flags)\n {\n+\tchar *ret_name;\n \tstruct hashfile *f;\n \tint fd;\n \n@@ -249,10 +250,11 @@ const char *write_rev_file_order(const char *rev_name,\n \t\tif (!rev_name) {\n \t\t\tstruct strbuf tmp_file = STRBUF_INIT;\n \t\t\tfd = odb_mkstemp(&tmp_file, \"pack/tmp_rev_XXXXXX\");\n-\t\t\trev_name = strbuf_detach(&tmp_file, NULL);\n+\t\t\trev_name = ret_name = strbuf_detach(&tmp_file, NULL);\n \t\t} else {\n \t\t\tunlink(rev_name);\n \t\t\tfd = xopen(rev_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\t\tret_name = xstrdup(rev_name);\n \t\t}\n \t\tf = hashfd(fd, rev_name);\n \t} else if (flags & WRITE_REV_VERIFY) {\n@@ -264,6 +266,7 @@ const char *write_rev_file_order(const char *rev_name,\n \t\t\t} else\n \t\t\t\tdie_errno(_(\"could not stat: %s\"), rev_name);\n \t\t}\n+\t\tret_name = xstrdup(rev_name);\n \t\tf = hashfd_check(rev_name);\n \t} else\n \t\treturn NULL;\n@@ -280,7 +283,7 @@ const char *write_rev_file_order(const char *rev_name,\n \t\t\t CSUM_HASH_IN_STREAM | CSUM_CLOSE |\n \t\t\t ((flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));\n \n-\treturn rev_name;\n+\treturn ret_name;\n }\n \n static void write_mtimes_header(struct hashfile *f)\n@@ -543,7 +546,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\t\t unsigned char hash[],\n \t\t\t char **idx_tmp_name)\n {\n-\tconst char *rev_tmp_name = NULL;\n+\tchar *rev_tmp_name = NULL;\n \tconst char *mtimes_tmp_name = NULL;\n \n \tif (adjust_shared_perm(pack_tmp_name))\n@@ -564,8 +567,10 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t}\n \n \trename_tmp_packfile(name_buffer, pack_tmp_name, \"pack\");\n-\tif (rev_tmp_name)\n+\tif (rev_tmp_name) {\n \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n+\t\tfree(rev_tmp_name);\n+\t}\n \tif (mtimes_tmp_name)\n \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n }\ndiff --git a/pack.h b/pack.h\nindex 3ab9e3f60c0..02bbdfb19cc 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -96,8 +96,8 @@ struct ref;\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);\n \n-const char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n-const char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n+char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n+char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);\n \n /*\n * The \"hdr\" output buffer should be at least this big, which will handle sizes\n"},{"id":"475144","messageId":"d81c0fe8-580f-dbab-9904-e0ea8459576c@github.com","threadId":"59573","inReplyTo":"be4faf11011efcfab479e5785e6c2bbac95309bd.1681166596.git.me@ttaylorr.com","subject":"Re: [PATCH 3/7] pack-revindex: make `load_pack_revindex` take a repository","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-11T13:45:21Z","receivedAt":"2023-04-11T13:45:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/10/2023 6:53 PM, Taylor Blau wrote:\n> In a future commit, we will introduce a `pack.readReverseIndex`\n> configuration, which forces Git to generate the reverse index from\n> scratch instead of loading it from disk.\n> \n> In order to avoid reading this configuration value more than once, we'll\n> use the `repo_settings` struct to lazily load this value.\n> \n> In order to access the `struct repo_settings`, add a repository argument\n> to `load_pack_revindex`, and update all callers to pass the correct\n> instance (in all cases, `the_repository`).\n\nIf all callers use the_repository, then we could presumably use\nthe_repository within the method directly. However, there are some\ncases where the call chain is less obvious that we have already\nentered something that is \"repository scoped\".\n\nThe patch below applies on top of this one and is the result of\nexploring the two callers within pack-bitmap.c. Since they are\nstatic, I was able to only modify things within that file, but\nfound two callers to _those_ methods that were repository scoped,\nso without making this connection we are losing that scope.\n\nThere are other non-static methods in pack-bitmap.c that might\nbenefit from wiring a repository pointer through (or adding a\nrepository pointer to struct bitmap_index to get it for free),\nbut I used the trick of defining a local repository pointer at\nthe top of the method to make it easier to change in the future.\n\nThanks,\n-Stolee\n\n\n--- >8 ---\n\nFrom 9816f7026199981b86d9f3e2188036e1b20bc2f9 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Tue, 11 Apr 2023 09:34:42 -0400\nSubject: [PATCH] pack-bitmap: use struct repository more often\n\nThe previous change introduced a 'struct repository *' parameter to\nload_pack_revindex(). To satisfy the callers within pack-bitmap.c, these\nparameters were filled with 'the_repository'.\n\nHowever, these callers are sometimes included in methods that are\nalready scoped to a 'struct repository *' parameter. By dropping the\nlink from that repository and using the_repository, we are giving a\nfalse impression that this portion of the rev-index API is properly\nscoped to a single repository.\n\nExpand the static methods in pack-bitmap.c that call\nload_pack_revindex() to include a 'struct repository *' parameter.\nModify the callers of those methods to pass a repository as appropriate.\nFor the methods without an appropriate repository, create a local\nvariable equal to the_repository so it is easier to convert them to\nusing a parameter in the future.\n\nIn the case of prepare_bitmap_git(), we already have a repository\npointer parameter that can be used. In prepare_bitmap_walk(), the\nrev_info struct contains a repository pointer.\n\nSigned-off-by: Derrick Stolee \n---\n pack-bitmap.c | 24 +++++++++++++-----------\n 1 file changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 8e3bb00931..38b35c4823 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -463,7 +463,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \treturn 0;\n }\n \n-static int load_reverse_index(struct bitmap_index *bitmap_git)\n+static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_git)\n {\n \tif (bitmap_is_midx(bitmap_git)) {\n \t\tuint32_t i;\n@@ -477,24 +477,23 @@ static int load_reverse_index(struct bitmap_index *bitmap_git)\n \t\t * since we will need to make use of them in pack-objects.\n \t\t */\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n-\t\t\tret = load_pack_revindex(the_repository,\n-\t\t\t\t\t\t bitmap_git->midx->packs[i]);\n+\t\t\tret = load_pack_revindex(r, bitmap_git->midx->packs[i]);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n \t\treturn 0;\n \t}\n-\treturn load_pack_revindex(the_repository, bitmap_git->pack);\n+\treturn load_pack_revindex(r, bitmap_git->pack);\n }\n \n-static int load_bitmap(struct bitmap_index *bitmap_git)\n+static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git)\n {\n \tassert(bitmap_git->map);\n \n \tbitmap_git->bitmaps = kh_init_oid_map();\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n-\tif (load_reverse_index(bitmap_git))\n+\tif (load_reverse_index(r, bitmap_git))\n \t\tgoto failed;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n@@ -581,7 +580,7 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n {\n \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n \n-\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))\n+\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))\n \t\treturn bitmap_git;\n \n \tfree_bitmap_index(bitmap_git);\n@@ -590,9 +589,10 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n \n struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)\n {\n+\tstruct repository *r = the_repository;\n \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n \n-\tif (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(bitmap_git))\n+\tif (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(r, bitmap_git))\n \t\treturn bitmap_git;\n \n \tfree_bitmap_index(bitmap_git);\n@@ -1593,7 +1593,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n \t * from disk. this is the point of no return; after this the rev_list\n \t * becomes invalidated and we must perform the revwalk through bitmaps\n \t */\n-\tif (load_bitmap(bitmap_git) < 0)\n+\tif (load_bitmap(revs->repo, bitmap_git) < 0)\n \t\tgoto cleanup;\n \n \tobject_array_clear(&revs->pending);\n@@ -1743,6 +1743,7 @@ int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\t\t uint32_t *entries,\n \t\t\t\t struct bitmap **reuse_out)\n {\n+\tstruct repository *r = the_repository;\n \tstruct packed_git *pack;\n \tstruct bitmap *result = bitmap_git->result;\n \tstruct bitmap *reuse;\n@@ -1753,7 +1754,7 @@ int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \n \tassert(result);\n \n-\tload_reverse_index(bitmap_git);\n+\tload_reverse_index(r, bitmap_git);\n \n \tif (bitmap_is_midx(bitmap_git))\n \t\tpack = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];\n@@ -2133,11 +2134,12 @@ int rebuild_bitmap(const uint32_t *reposition,\n uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \t\t\t\tstruct packing_data *mapping)\n {\n+\tstruct repository *r = the_repository;\n \tuint32_t i, num_objects;\n \tuint32_t *reposition;\n \n \tif (!bitmap_is_midx(bitmap_git))\n-\t\tload_reverse_index(bitmap_git);\n+\t\tload_reverse_index(r, bitmap_git);\n \telse if (load_midx_revindex(bitmap_git->midx) < 0)\n \t\tBUG(\"rebuild_existing_bitmaps: missing required rev-cache \"\n \t\t \"extension\");\n-- \n2.40.0.vfs.0.0.3.g5872ac9aaa4\n\n"},{"id":"475145","messageId":"18a54c6e-7357-42ca-779f-2ba3e6653880@github.com","threadId":"59573","inReplyTo":"9c80379958824ac8fb7834f4f98000d11c3dc4e0.1681166596.git.me@ttaylorr.com","subject":"Re: [PATCH 7/7] t: invert `GIT_TEST_WRITE_REV_INDEX`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-11T13:51:06Z","receivedAt":"2023-04-11T13:51:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/10/2023 6:53 PM, Taylor Blau wrote:\n> Instead of getting rid of the option, invert its meaning to instead\n> disable writing \".rev\" files, thereby running the test suite in a mode\n> where the reverse index is generated from scratch.\n> \n> This ensures that we are still running and exercising Git's behavior\n> when forced to generate reverse indexes from scratch.\n\nI don't think this is true because you remove the environment\nvariable from the following test. Replacing the line with\nGIT_TEST_NO_WRITE_REV_INDEX=1 would keep us testing the from-scratch\ncase as a side-effect in other tests.\n\n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index b098e10f52..e9fbfb6345 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -27,7 +27,6 @@ linux-TEST-vars)\n> \texport GIT_TEST_MULTI_PACK_INDEX=1\n> \texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n> \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n> -\texport GIT_TEST_WRITE_REV_INDEX=1\n> \texport GIT_TEST_CHECKOUT_WORKERS=2\n> \t;;\n\nThanks,\n-Stolee\n"},{"id":"475146","messageId":"bfa4c23d-8b38-7505-fb57-a02e0a028292@github.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"Re: [PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-11T13:54:08Z","receivedAt":"2023-04-11T13:54:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/10/2023 6:53 PM, Taylor Blau wrote:\n> In the vast majority of cases, this trade-off favors the on-disk \".rev\"\n> files. But in certain cases, the in-memory variant performs more\n> favorably. Since these cases are narrow, and performance is machine- and\n> repository-dependent, this series also introduces a new configuration\n> option to disable reading \".rev\" files in the third commit.\n\nI agree that the performance trade-off indicates that having the .rev\nfiles is preferred. It makes operations that _can_ be very fast as fast\nas possible (inspecting a small number of objects is much faster because\nwe don't generate the in-memory index in O(N) time) and you create a knob\nfor disabling it in the case that we are already doing something that\ninspects almost-all objects.\n \n> The series is structured as follows:\n> \n> - A couple of cleanup patches to plug a leak in stage_tmp_packfiles().\n> - Three patches to enable `pack.readReverseIndex`.\n> - Another patch to change the default of `pack.writeReverseIndex` from\n> \"false\" to \"true\".\n> - A final patch to enable the test suite to be run in a mode that does\n> not use on-disk \".rev\" files.\n\nThis was an easy series to read. I applied the patches locally and poked\naround in the resulting code as I went along. This led to a couple places\nwhere I recommend a few changes, including a new patch that wires\nrepository pointers through a few more method layers.\n\nThanks,\n-Stolee\n"},{"id":"475174","messageId":"ZDXQOIRqHdV7ONcN@nand.local","threadId":"59573","inReplyTo":"423b5c5c-ef86-198a-16f7-c836ed78928c@github.com","subject":"Re: [PATCH 1/7] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:25:12Z","receivedAt":"2023-04-11T21:25:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 09:23:31AM -0400, Derrick Stolee wrote:\n> On 4/10/2023 6:53 PM, Taylor Blau wrote:\n>\n> > Instead, simply free() `rev_tmp_name` at the end of\n> > `stage_tmp_packfiles()`.\n>\n> > @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n> > \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n> > \tif (mtimes_tmp_name)\n> > \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n> > +\n> > +\tfree((char *)rev_tmp_name);\n>\n> Just cut off from the context is a \"if (rev_tmp_name)\", so it might be\n> good to group this into that block, since we have the condition, anyway.\n\nDefinitely possible, though FWIW it's fine to have this free()\npositioned at the end of the function, since we initialize rev_tmp_name\nto NULL (making this a noop when not writing an on-disk reverse index).\n\n> But I was also thinking about how we like to use \"const\" as an indicator\n> as \"I am not responsible for free()ing this\". And this comes from the\n> public write_rev_file() method. Based on the API prototype, we could\n> think that this string is held by a static strbuf (making the method\n> not reentrant, but that happens sometimes in our methods). But generally,\n> I wanted to inspect what it would take to make the API reflect the fact\n> that it can return a \"new\" string.\n>\n> But there are two issues:\n>\n> 1. The actual logic is inside write_rev_file_order(), so that API\n> needs to change, too.\n>\n> 2. The \"new\" string is created only if the rev_name parameter is\n> NULL, which is somewhat understandable but still requires\n> inside knowledge about the implementation to make that choice.\n>\n> 3. If we inspect the callers to these methods, only one caller\n> passes a non-null name: builtin/index-pack.c. The rest pass NULL,\n> including write_midx_reverse_index() (which then leaks the name).\n>\n> The below diff includes my attempt to change the API to return a\n> non-const string that must be freed by the callers.\n\nI like this direction. I think that all things being equal (and unless\nyou feel strongly about it in the meantime), I'd just as soon pursue\nthis as a \"fast follow\" to avoid intermixing this API change with the\nprimary intent of this series.\n\nIn the meantime, dropping the const via a cast down to \"char *\" works\nfine to plug the leak here.\n\nThanks,\nTaylor\n"},{"id":"475175","messageId":"ZDXRajRky5XtFenU@nand.local","threadId":"59573","inReplyTo":"d81c0fe8-580f-dbab-9904-e0ea8459576c@github.com","subject":"Re: [PATCH 3/7] pack-revindex: make `load_pack_revindex` take a repository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:30:18Z","receivedAt":"2023-04-11T21:30:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 09:45:21AM -0400, Derrick Stolee wrote:\n> On 4/10/2023 6:53 PM, Taylor Blau wrote:\n> > In a future commit, we will introduce a `pack.readReverseIndex`\n> > configuration, which forces Git to generate the reverse index from\n> > scratch instead of loading it from disk.\n> >\n> > In order to avoid reading this configuration value more than once, we'll\n> > use the `repo_settings` struct to lazily load this value.\n> >\n> > In order to access the `struct repo_settings`, add a repository argument\n> > to `load_pack_revindex`, and update all callers to pass the correct\n> > instance (in all cases, `the_repository`).\n>\n> If all callers use the_repository, then we could presumably use\n> the_repository within the method directly. However, there are some\n> cases where the call chain is less obvious that we have already\n> entered something that is \"repository scoped\".\n>\n> The patch below applies on top of this one and is the result of\n> exploring the two callers within pack-bitmap.c. Since they are\n> static, I was able to only modify things within that file, but\n> found two callers to _those_ methods that were repository scoped,\n> so without making this connection we are losing that scope.\n>\n> There are other non-static methods in pack-bitmap.c that might\n> benefit from wiring a repository pointer through (or adding a\n> repository pointer to struct bitmap_index to get it for free),\n> but I used the trick of defining a local repository pointer at\n> the top of the method to make it easier to change in the future.\n>\n> Thanks,\n> -Stolee\n\n> @@ -581,7 +580,7 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n> {\n> \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n>\n> -\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))\n> +\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))\n> \t\treturn bitmap_git;\n>\n> \tfree_bitmap_index(bitmap_git);\n\nOops; we are indeed dropping the repository pointer that was given to\nprepare_bitmap_git() here. It's unfortunate that we have to work through\nso many layers to propagate it back down, but I agree that it's the\nright thing to do.\n\n> @@ -590,9 +589,10 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n>\n> struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)\n> {\n> +\tstruct repository *r = the_repository;\n\nOK; and here we're using the trick you mentioned in the patch message to\navoid having to propagate this even further out. The rest of the patch\nlooks sensible to me.\n\nIn terms of working this one in, it feels odd to include it as a\nseparate commit since we know the one immediately prior to it is kind of\nbroken.\n\nDo you want to squash these together? Something else? Anything is fine\nwith me here.\n\nThanks,\nTaylor\n"},{"id":"475176","messageId":"ZDXSHal5IBynUTMX@nand.local","threadId":"59573","inReplyTo":"18a54c6e-7357-42ca-779f-2ba3e6653880@github.com","subject":"Re: [PATCH 7/7] t: invert `GIT_TEST_WRITE_REV_INDEX`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:33:17Z","receivedAt":"2023-04-11T21:33:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 09:51:06AM -0400, Derrick Stolee wrote:\n> On 4/10/2023 6:53 PM, Taylor Blau wrote:\n> > Instead of getting rid of the option, invert its meaning to instead\n> > disable writing \".rev\" files, thereby running the test suite in a mode\n> > where the reverse index is generated from scratch.\n> >\n> > This ensures that we are still running and exercising Git's behavior\n> > when forced to generate reverse indexes from scratch.\n>\n> I don't think this is true because you remove the environment\n> variable from the following test.\n\nThis is intentional; I wrote that last sentence (\"This ensures...\") with\nan implied \"[When set], this ensures...\" at the beginning of it. IOW, if\nyou wanted to run the test suite with primarily in-memory reverse\nindexes, you still could.\n\n> Replacing the line with GIT_TEST_NO_WRITE_REV_INDEX=1 would keep us\n> testing the from-scratch case as a side-effect in other tests.\n>\n> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> > index b098e10f52..e9fbfb6345 100755\n> > --- a/ci/run-build-and-tests.sh\n> > +++ b/ci/run-build-and-tests.sh\n> > @@ -27,7 +27,6 @@ linux-TEST-vars)\n> > \texport GIT_TEST_MULTI_PACK_INDEX=1\n> > \texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n> > \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n> > -\texport GIT_TEST_WRITE_REV_INDEX=1\n> > \texport GIT_TEST_CHECKOUT_WORKERS=2\n> > \t;;\n\nI wasn't quite sure what to do here. On one hand, sticking\nGIT_TEST_NO_WRITE_REV_INDEX=1 here would ensure that the linux-TEST-vars\ntest is still exercising the old code.\n\nIs that desirable? I dunno. On one hand it increases our coverage, but\non the other hand I've always treated this suite as for experimental\nfeatures, not older ones.\n\nBut I think all things being equal, I'd rather have more CI coverage\nthan not, so I'll add it back in. Thanks for a sanity check on this one.\n\nThanks,\nTaylor\n"},{"id":"475177","messageId":"ZDXTwZcRx7rGa5vW@nand.local","threadId":"59573","inReplyTo":"bfa4c23d-8b38-7505-fb57-a02e0a028292@github.com","subject":"Re: [PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:40:17Z","receivedAt":"2023-04-11T21:40:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 09:54:08AM -0400, Derrick Stolee wrote:\n> On 4/10/2023 6:53 PM, Taylor Blau wrote:\n> > In the vast majority of cases, this trade-off favors the on-disk \".rev\"\n> > files. But in certain cases, the in-memory variant performs more\n> > favorably. Since these cases are narrow, and performance is machine- and\n> > repository-dependent, this series also introduces a new configuration\n> > option to disable reading \".rev\" files in the third commit.\n>\n> I agree that the performance trade-off indicates that having the .rev\n> files is preferred. It makes operations that _can_ be very fast as fast\n> as possible (inspecting a small number of objects is much faster because\n> we don't generate the in-memory index in O(N) time) and you create a knob\n> for disabling it in the case that we are already doing something that\n> inspects almost-all objects.\n\nSweet; I'm glad that you agree.\n\nFWIW for non-GitHub folks, observing a slow-down here has never been an\nissue for us. So much so that I wrote the pack.readReverseIndex knob\nyesterday for the purpose of sending this series.\n\nThat said, I think that including it here is still worthwhile, since the\ncases where performance really suffers (e.g., `git cat-file\n--batch-all-objects --batch-check='%(objectsize:disk)'`) isn't something\nthat GitHub runs regularly if at all.\n\nTo accommodate different workflows, I think having the option to opt-out\nof reading the on-disk \".rev\" files is worthwhile.\n\n> This was an easy series to read. I applied the patches locally and poked\n> around in the resulting code as I went along. This led to a couple places\n> where I recommend a few changes, including a new patch that wires\n> repository pointers through a few more method layers.\n\nThanks for taking a look. Based on your review, there are only a couple\nof things on my mind:\n\n - I amended the last patch to more clearly state when we would want to\n run the suite GIT_TEST_NO_WRITE_REV_INDEXES=1 set, and kept it in\n the linux-TEST-vars configuration.\n\n - How do you want to handle that extra patch? As I noted in [1], I\n think squashing the two together in one way or another makes sense.\n So really we have to figure out (a) if you think that is the right\n way to go, and (b) if so, how to handle attribution / the commit\n message.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/ZDXRajRky5XtFenU@nand.local/\n"},{"id":"475219","messageId":"ba936ba7-b6f6-de57-ddca-accf6cebec72@github.com","threadId":"59573","inReplyTo":"ZDXRajRky5XtFenU@nand.local","subject":"Re: [PATCH 3/7] pack-revindex: make `load_pack_revindex` take a repository","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-12T17:33:39Z","receivedAt":"2023-04-12T17:33:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/11/2023 5:30 PM, Taylor Blau wrote:\n> On Tue, Apr 11, 2023 at 09:45:21AM -0400, Derrick Stolee wrote:\n\n>> @@ -581,7 +580,7 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n>> {\n>> \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n>>\n>> -\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))\n>> +\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))\n>> \t\treturn bitmap_git;\n>>\n>> \tfree_bitmap_index(bitmap_git);\n> \n> Oops; we are indeed dropping the repository pointer that was given to\n> prepare_bitmap_git() here. It's unfortunate that we have to work through\n> so many layers to propagate it back down, but I agree that it's the\n> right thing to do.\n> \n>> @@ -590,9 +589,10 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n>>\n>> struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)\n>> {\n>> +\tstruct repository *r = the_repository;\n> \n> OK; and here we're using the trick you mentioned in the patch message to\n> avoid having to propagate this even further out. The rest of the patch\n> looks sensible to me.\n> \n> In terms of working this one in, it feels odd to include it as a\n> separate commit since we know the one immediately prior to it is kind of\n> broken.\n> \n> Do you want to squash these together? Something else? Anything is fine\n> with me here.\n\nFeel free to squash it in, to avoid having a commit where the chain is\nbroken.\n\nThanks,\n-Stolee\n"},{"id":"475220","messageId":"c04a0ea4-c107-f8d2-4a53-f6aa9ee934ef@github.com","threadId":"59573","inReplyTo":"ZDXSHal5IBynUTMX@nand.local","subject":"Re: [PATCH 7/7] t: invert `GIT_TEST_WRITE_REV_INDEX`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-12T17:37:03Z","receivedAt":"2023-04-12T17:37:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/11/2023 5:33 PM, Taylor Blau wrote:\n> On Tue, Apr 11, 2023 at 09:51:06AM -0400, Derrick Stolee wrote:\n>> On 4/10/2023 6:53 PM, Taylor Blau wrote:\n>>> Instead of getting rid of the option, invert its meaning to instead\n>>> disable writing \".rev\" files, thereby running the test suite in a mode\n>>> where the reverse index is generated from scratch.\n>>>\n>>> This ensures that we are still running and exercising Git's behavior\n>>> when forced to generate reverse indexes from scratch.\n>>\n>> I don't think this is true because you remove the environment\n>> variable from the following test.\n> \n> This is intentional; I wrote that last sentence (\"This ensures...\") with\n> an implied \"[When set], this ensures...\" at the beginning of it. IOW, if\n> you wanted to run the test suite with primarily in-memory reverse\n> indexes, you still could.\n\nI guess I think of \"we are still running and exercising\" as only\nbeing true if it happens during regular CI, not just when someone\nthinks to try it.\n\n> I wasn't quite sure what to do here. On one hand, sticking\n> GIT_TEST_NO_WRITE_REV_INDEX=1 here would ensure that the linux-TEST-vars\n> test is still exercising the old code.\n> \n> Is that desirable? I dunno. On one hand it increases our coverage, but\n> on the other hand I've always treated this suite as for experimental\n> features, not older ones.\n> \n> But I think all things being equal, I'd rather have more CI coverage\n> than not, so I'll add it back in. Thanks for a sanity check on this one.\n\nIf we think there is value in continuing to test the case without .rev\nfiles by default, then adding it in would be good. I think it has some\nvalue at least for some time after the transition. We're unlikely to\nremember to remove it, but having the extra mode in this build is not\ntoo much overhead.\n\nWe should consider auditing this build just to be sure we want to\ncontinue testing these options or if some could be removed in favor\nof new defaults.\n\nThanks,\n-Stolee\n"},{"id":"475221","messageId":"a4154bd2-3f71-42ba-e21d-097811169a99@github.com","threadId":"59573","inReplyTo":"ZDXTwZcRx7rGa5vW@nand.local","subject":"Re: [PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-12T17:39:19Z","receivedAt":"2023-04-12T17:39:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/11/2023 5:40 PM, Taylor Blau wrote:\n> On Tue, Apr 11, 2023 at 09:54:08AM -0400, Derrick Stolee wrote:\n>> On 4/10/2023 6:53 PM, Taylor Blau wrote:\n>>> In the vast majority of cases, this trade-off favors the on-disk \".rev\"\n>>> files. But in certain cases, the in-memory variant performs more\n>>> favorably. Since these cases are narrow, and performance is machine- and\n>>> repository-dependent, this series also introduces a new configuration\n>>> option to disable reading \".rev\" files in the third commit.\n>>\n>> I agree that the performance trade-off indicates that having the .rev\n>> files is preferred. It makes operations that _can_ be very fast as fast\n>> as possible (inspecting a small number of objects is much faster because\n>> we don't generate the in-memory index in O(N) time) and you create a knob\n>> for disabling it in the case that we are already doing something that\n>> inspects almost-all objects.\n> \n> Sweet; I'm glad that you agree.\n> \n> FWIW for non-GitHub folks, observing a slow-down here has never been an\n> issue for us. So much so that I wrote the pack.readReverseIndex knob\n> yesterday for the purpose of sending this series.\n> \n> That said, I think that including it here is still worthwhile, since the\n> cases where performance really suffers (e.g., `git cat-file\n> --batch-all-objects --batch-check='%(objectsize:disk)'`) isn't something\n> that GitHub runs regularly if at all.\n> \n> To accommodate different workflows, I think having the option to opt-out\n> of reading the on-disk \".rev\" files is worthwhile.\n\nThe only thing I can think of that would actually use this kind of\nbehavior is git-sizer, but even that doesn't actually report the on-disk\nsize (yet) and instead inflates all deltas when reporting size counts. The\ndifference in performance here is likely minimal for that tool.\n \n>> This was an easy series to read. I applied the patches locally and poked\n>> around in the resulting code as I went along. This led to a couple places\n>> where I recommend a few changes, including a new patch that wires\n>> repository pointers through a few more method layers.\n> \n> Thanks for taking a look. Based on your review, there are only a couple\n> of things on my mind:\n> \n> - I amended the last patch to more clearly state when we would want to\n> run the suite GIT_TEST_NO_WRITE_REV_INDEXES=1 set, and kept it in\n> the linux-TEST-vars configuration.\n\nI think this is the right thing to do. Thanks.\n\n> - How do you want to handle that extra patch? As I noted in [1], I\n> think squashing the two together in one way or another makes sense.\n> So really we have to figure out (a) if you think that is the right\n> way to go, and (b) if so, how to handle attribution / the commit\n> message.\n\nSquashing makes sense. You could make me a co-author, or not. It's the\nnatural thing to do once the problem to solve is identified.\n\nThanks,\n-Stolee\n"},{"id":"475250","messageId":"cover.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681166596.git.me@ttaylorr.com","subject":"[PATCH v2 0/7] pack-revindex: enable on-disk reverse indexes by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:14Z","receivedAt":"2023-04-12T22:20:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a (tiny) reroll of my series to enable pack reverse-indexes to\nbe written to disk by default instead of computed on-the-fly in memory.\n\nThe original cover letter[1] and commits herein have all of the gore-y\ndetails. Not much has changed since last time, except:\n\n - squashing in a patch from Stolee to propagate a `struct repository\n *` a little further out from `load_pack_revindex()`\n\n - a tweak to the linux-TEST-vars CI job to continue running it with\n GIT_TEST_NO_WRITE_REV_INDEX\n\n - and an additional benchmark to demonstrate the performance of `git\n cat-file --batch-check='%(objectsize:disk)' --batch--all-objects\n --unordered` with and without on-disk reverse indexes\n\nAs always, a range-diff is included below for convenience. Thanks in\nadvance for taking (another) look!\n\n[1]: https://lore.kernel.org/git/cover.1681166596.git.me@ttaylorr.com/\n\nTaylor Blau (7):\n pack-write.c: plug a leak in stage_tmp_packfiles()\n t5325: mark as leak-free\n pack-revindex: make `load_pack_revindex` take a repository\n pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK\n pack-revindex: introduce `pack.readReverseIndex`\n config: enable `pack.writeReverseIndex` by default\n t: invert `GIT_TEST_WRITE_REV_INDEX`\n\n Documentation/config/pack.txt | 8 +++++++-\n builtin/index-pack.c | 5 +++--\n builtin/pack-objects.c | 5 +++--\n ci/run-build-and-tests.sh | 2 +-\n pack-bitmap.c | 23 +++++++++++++----------\n pack-revindex.c | 12 +++++++++---\n pack-revindex.h | 6 ++++--\n pack-write.c | 2 ++\n packfile.c | 2 +-\n repo-settings.c | 1 +\n repository.h | 1 +\n t/README | 2 +-\n t/perf/p5312-pack-bitmaps-revs.sh | 3 +--\n t/t5325-reverse-index.sh | 16 +++++++++++++++-\n 14 files changed, 62 insertions(+), 26 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: 18be29c3988 pack-write.c: plug a leak in stage_tmp_packfiles()\n-: ----------- > 2: affb5e2574b t5325: mark as leak-free\n1: be4faf11011 ! 3: 687a9a58924 pack-revindex: make `load_pack_revindex` take a repository\n @@ Commit message\n to `load_pack_revindex`, and update all callers to pass the correct\n instance (in all cases, `the_repository`).\n \n - Signed-off-by: Taylor Blauy \n + In certain instances, a new function-local variable is introduced to\n + take the place of a `struct repository *` argument to the function\n + itself to avoid propagating the new parameter even further throughout\n + the tree.\n +\n + Co-authored-by: Derrick Stolee \n + Signed-off-by: Derrick Stolee \n + Signed-off-by: Taylor Blau \n \n ## pack-bitmap.c ##\n +@@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n + \treturn 0;\n + }\n + \n +-static int load_reverse_index(struct bitmap_index *bitmap_git)\n ++static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_git)\n + {\n + \tif (bitmap_is_midx(bitmap_git)) {\n + \t\tuint32_t i;\n @@ pack-bitmap.c: static int load_reverse_index(struct bitmap_index *bitmap_git)\n \t\t * since we will need to make use of them in pack-objects.\n \t\t */\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n -\t\t\tret = load_pack_revindex(bitmap_git->midx->packs[i]);\n -+\t\t\tret = load_pack_revindex(the_repository,\n -+\t\t\t\t\t\t bitmap_git->midx->packs[i]);\n ++\t\t\tret = load_pack_revindex(r, bitmap_git->midx->packs[i]);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n \t\treturn 0;\n \t}\n -\treturn load_pack_revindex(bitmap_git->pack);\n -+\treturn load_pack_revindex(the_repository, bitmap_git->pack);\n ++\treturn load_pack_revindex(r, bitmap_git->pack);\n }\n \n - static int load_bitmap(struct bitmap_index *bitmap_git)\n +-static int load_bitmap(struct bitmap_index *bitmap_git)\n ++static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git)\n + {\n + \tassert(bitmap_git->map);\n + \n + \tbitmap_git->bitmaps = kh_init_oid_map();\n + \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n + \n +-\tif (load_reverse_index(bitmap_git))\n ++\tif (load_reverse_index(r, bitmap_git))\n + \t\tgoto failed;\n + \n + \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n +@@ pack-bitmap.c: struct bitmap_index *prepare_bitmap_git(struct repository *r)\n + {\n + \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n + \n +-\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))\n ++\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))\n + \t\treturn bitmap_git;\n + \n + \tfree_bitmap_index(bitmap_git);\n +@@ pack-bitmap.c: struct bitmap_index *prepare_bitmap_git(struct repository *r)\n + \n + struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)\n + {\n ++\tstruct repository *r = the_repository;\n + \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n + \n +-\tif (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(bitmap_git))\n ++\tif (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(r, bitmap_git))\n + \t\treturn bitmap_git;\n + \n + \tfree_bitmap_index(bitmap_git);\n +@@ pack-bitmap.c: struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n + \t * from disk. this is the point of no return; after this the rev_list\n + \t * becomes invalidated and we must perform the revwalk through bitmaps\n + \t */\n +-\tif (load_bitmap(bitmap_git) < 0)\n ++\tif (load_bitmap(revs->repo, bitmap_git) < 0)\n + \t\tgoto cleanup;\n + \n + \tobject_array_clear(&revs->pending);\n +@@ pack-bitmap.c: int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n + \t\t\t\t uint32_t *entries,\n + \t\t\t\t struct bitmap **reuse_out)\n + {\n ++\tstruct repository *r = the_repository;\n + \tstruct packed_git *pack;\n + \tstruct bitmap *result = bitmap_git->result;\n + \tstruct bitmap *reuse;\n +@@ pack-bitmap.c: int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n + \n + \tassert(result);\n + \n +-\tload_reverse_index(bitmap_git);\n ++\tload_reverse_index(r, bitmap_git);\n + \n + \tif (bitmap_is_midx(bitmap_git))\n + \t\tpack = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];\n +@@ pack-bitmap.c: int rebuild_bitmap(const uint32_t *reposition,\n + uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n + \t\t\t\tstruct packing_data *mapping)\n + {\n ++\tstruct repository *r = the_repository;\n + \tuint32_t i, num_objects;\n + \tuint32_t *reposition;\n + \n + \tif (!bitmap_is_midx(bitmap_git))\n +-\t\tload_reverse_index(bitmap_git);\n ++\t\tload_reverse_index(r, bitmap_git);\n + \telse if (load_midx_revindex(bitmap_git->midx) < 0)\n + \t\tBUG(\"rebuild_existing_bitmaps: missing required rev-cache \"\n + \t\t \"extension\");\n \n ## pack-revindex.c ##\n @@ pack-revindex.c: static int load_pack_revindex_from_disk(struct packed_git *p)\n2: 0f368e2347e = 4: 8eec5bacd3a pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK\n3: 6f692d470cb ! 5: a62fc3e4ec1 pack-revindex: introduce `pack.readReverseIndex`\n @@ Commit message\n 'git.compile -c pack.readReverseIndex=false cat-file --batch-check=\"%(objectsize:disk)\" --batch-all-objects' ran\n 2.06 ± 0.02 times faster than 'git.compile -c pack.readReverseIndex=true cat-file --batch-check=\"%(objectsize:disk)\" --batch-all-objects'\n \n + Luckily, the results when running `git cat-file` with `--unordered` are\n + closer together:\n +\n + $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} cat-file --unordered --batch-check=\"%(objectsize:disk)\" --batch-all-objects'\n + Benchmark 1: git.compile -c pack.readReverseIndex=false cat-file --unordered --batch-check=\"%(objectsize:disk)\" --batch-all-objects\n + Time (mean ± σ): 5.066 s ± 0.105 s [User: 4.792 s, System: 0.274 s]\n + Range (min … max): 4.943 s … 5.220 s 10 runs\n +\n + Benchmark 2: git.compile -c pack.readReverseIndex=true cat-file --unordered --batch-check=\"%(objectsize:disk)\" --batch-all-objects\n + Time (mean ± σ): 6.193 s ± 0.069 s [User: 5.937 s, System: 0.255 s]\n + Range (min … max): 6.145 s … 6.356 s 10 runs\n +\n + Summary\n + 'git.compile -c pack.readReverseIndex=false cat-file --unordered --batch-check=\"%(objectsize:disk)\" --batch-all-objects' ran\n + 1.22 ± 0.03 times faster than 'git.compile -c pack.readReverseIndex=true cat-file --unordered --batch-check=\"%(objectsize:disk)\" --batch-all-objects'\n +\n Because the equilibrium point between these two is highly machine- and\n repository-dependent, allow users to configure whether or not they will\n read any \".rev\" file(s) with this configuration knob.\n4: 56a0fc0098e = 6: f8298fb0bac config: enable `pack.writeReverseIndex` by default\n5: 9c803799588 ! 7: edff6a80c63 t: invert `GIT_TEST_WRITE_REV_INDEX`\n @@ Commit message\n disable writing \".rev\" files, thereby running the test suite in a mode\n where the reverse index is generated from scratch.\n \n - This ensures that we are still running and exercising Git's behavior\n - when forced to generate reverse indexes from scratch.\n + This ensures that, when GIT_TEST_NO_WRITE_REV_INDEX is set to some\n + spelling of \"true\", we are still running and exercising Git's behavior\n + when forced to generate reverse indexes from scratch. Do so by setting\n + it in the linux-TEST-vars CI run to ensure that we are maintaining good\n + coverage of this now-legacy code.\n \n Signed-off-by: Taylor Blau \n \n @@ ci/run-build-and-tests.sh: linux-TEST-vars)\n \texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n -\texport GIT_TEST_WRITE_REV_INDEX=1\n ++\texport GIT_TEST_NO_WRITE_REV_INDEX=1\n \texport GIT_TEST_CHECKOUT_WORKERS=2\n \t;;\n linux-clang)\n-- \n2.40.0.323.gedff6a80c63\n"},{"id":"475251","messageId":"18be29c3988295cd58521f8cc4a729897df074c8.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"[PATCH v2 1/7] pack-write.c: plug a leak in stage_tmp_packfiles()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:17Z","receivedAt":"2023-04-12T22:20:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `stage_tmp_packfiles()` generates a filename to use for\nstaging the contents of what will become the pack's \".rev\" file.\n\nThe name is generated in `write_rev_file_order()` (via its caller\n`write_rev_file()`) in a string buffer, and the result is returned back\nto `stage_tmp_packfiles()` which uses it to rename the temporary file\ninto place via `rename_tmp_packfiles()`.\n\nThat name is not visible outside of `stage_tmp_packfiles()`, so it can\n(and should) be `free()`'d at the end of that function. We can't free it\nin `rename_tmp_packfile()` since not all of its `source` arguments are\nunreachable after calling it.\n\nInstead, simply free() `rev_tmp_name` at the end of\n`stage_tmp_packfiles()`.\n\n(Note that the same leak exists for `mtimes_tmp_name`, but we do not\naddress it in this commit).\n\nSigned-off-by: Taylor Blau \n---\n pack-write.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex f1714054951..f27c1f7f281 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\trename_tmp_packfile(name_buffer, rev_tmp_name, \"rev\");\n \tif (mtimes_tmp_name)\n \t\trename_tmp_packfile(name_buffer, mtimes_tmp_name, \"mtimes\");\n+\n+\tfree((char *)rev_tmp_name);\n }\n \n void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought)\n-- \n2.40.0.323.gedff6a80c63\n\n"},{"id":"475252","messageId":"affb5e2574bc1072d8f4ab8e3dccde7a5a39e725.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"[PATCH v2 2/7] t5325: mark as leak-free","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:21Z","receivedAt":"2023-04-12T22:20:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This test is leak-free as of the previous commit, so let's mark it as\nsuch to ensure we don't regress and introduce a leak in the future.\n\nSigned-off-by: Taylor Blau \n---\n t/t5325-reverse-index.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex d042d26f2b3..48c14d85764 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='on-disk reverse index'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # The below tests want control over the 'pack.writeReverseIndex' setting\n-- \n2.40.0.323.gedff6a80c63\n\n"},{"id":"475253","messageId":"687a9a589249a327251b90f558917aa0f4ae79a4.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"[PATCH v2 3/7] pack-revindex: make `load_pack_revindex` take a repository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:24Z","receivedAt":"2023-04-12T22:20:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a future commit, we will introduce a `pack.readReverseIndex`\nconfiguration, which forces Git to generate the reverse index from\nscratch instead of loading it from disk.\n\nIn order to avoid reading this configuration value more than once, we'll\nuse the `repo_settings` struct to lazily load this value.\n\nIn order to access the `struct repo_settings`, add a repository argument\nto `load_pack_revindex`, and update all callers to pass the correct\ninstance (in all cases, `the_repository`).\n\nIn certain instances, a new function-local variable is introduced to\ntake the place of a `struct repository *` argument to the function\nitself to avoid propagating the new parameter even further throughout\nthe tree.\n\nCo-authored-by: Derrick Stolee \nSigned-off-by: Derrick Stolee \nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 23 +++++++++++++----------\n pack-revindex.c | 4 ++--\n pack-revindex.h | 3 ++-\n packfile.c | 2 +-\n 4 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b2e7d06d604..38b35c48237 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -463,7 +463,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \treturn 0;\n }\n \n-static int load_reverse_index(struct bitmap_index *bitmap_git)\n+static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_git)\n {\n \tif (bitmap_is_midx(bitmap_git)) {\n \t\tuint32_t i;\n@@ -477,23 +477,23 @@ static int load_reverse_index(struct bitmap_index *bitmap_git)\n \t\t * since we will need to make use of them in pack-objects.\n \t\t */\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n-\t\t\tret = load_pack_revindex(bitmap_git->midx->packs[i]);\n+\t\t\tret = load_pack_revindex(r, bitmap_git->midx->packs[i]);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n \t\treturn 0;\n \t}\n-\treturn load_pack_revindex(bitmap_git->pack);\n+\treturn load_pack_revindex(r, bitmap_git->pack);\n }\n \n-static int load_bitmap(struct bitmap_index *bitmap_git)\n+static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git)\n {\n \tassert(bitmap_git->map);\n \n \tbitmap_git->bitmaps = kh_init_oid_map();\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n-\tif (load_reverse_index(bitmap_git))\n+\tif (load_reverse_index(r, bitmap_git))\n \t\tgoto failed;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n@@ -580,7 +580,7 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n {\n \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n \n-\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(bitmap_git))\n+\tif (!open_bitmap(r, bitmap_git) && !load_bitmap(r, bitmap_git))\n \t\treturn bitmap_git;\n \n \tfree_bitmap_index(bitmap_git);\n@@ -589,9 +589,10 @@ struct bitmap_index *prepare_bitmap_git(struct repository *r)\n \n struct bitmap_index *prepare_midx_bitmap_git(struct multi_pack_index *midx)\n {\n+\tstruct repository *r = the_repository;\n \tstruct bitmap_index *bitmap_git = xcalloc(1, sizeof(*bitmap_git));\n \n-\tif (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(bitmap_git))\n+\tif (!open_midx_bitmap_1(bitmap_git, midx) && !load_bitmap(r, bitmap_git))\n \t\treturn bitmap_git;\n \n \tfree_bitmap_index(bitmap_git);\n@@ -1592,7 +1593,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n \t * from disk. this is the point of no return; after this the rev_list\n \t * becomes invalidated and we must perform the revwalk through bitmaps\n \t */\n-\tif (load_bitmap(bitmap_git) < 0)\n+\tif (load_bitmap(revs->repo, bitmap_git) < 0)\n \t\tgoto cleanup;\n \n \tobject_array_clear(&revs->pending);\n@@ -1742,6 +1743,7 @@ int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\t\t uint32_t *entries,\n \t\t\t\t struct bitmap **reuse_out)\n {\n+\tstruct repository *r = the_repository;\n \tstruct packed_git *pack;\n \tstruct bitmap *result = bitmap_git->result;\n \tstruct bitmap *reuse;\n@@ -1752,7 +1754,7 @@ int reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \n \tassert(result);\n \n-\tload_reverse_index(bitmap_git);\n+\tload_reverse_index(r, bitmap_git);\n \n \tif (bitmap_is_midx(bitmap_git))\n \t\tpack = bitmap_git->midx->packs[midx_preferred_pack(bitmap_git)];\n@@ -2132,11 +2134,12 @@ int rebuild_bitmap(const uint32_t *reposition,\n uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \t\t\t\tstruct packing_data *mapping)\n {\n+\tstruct repository *r = the_repository;\n \tuint32_t i, num_objects;\n \tuint32_t *reposition;\n \n \tif (!bitmap_is_midx(bitmap_git))\n-\t\tload_reverse_index(bitmap_git);\n+\t\tload_reverse_index(r, bitmap_git);\n \telse if (load_midx_revindex(bitmap_git->midx) < 0)\n \t\tBUG(\"rebuild_existing_bitmaps: missing required rev-cache \"\n \t\t \"extension\");\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 03c7e81f9da..e3d69cc0f7a 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -283,7 +283,7 @@ static int load_pack_revindex_from_disk(struct packed_git *p)\n \treturn ret;\n }\n \n-int load_pack_revindex(struct packed_git *p)\n+int load_pack_revindex(struct repository *r, struct packed_git *p)\n {\n \tif (p->revindex || p->revindex_data)\n \t\treturn 0;\n@@ -356,7 +356,7 @@ int offset_to_pack_pos(struct packed_git *p, off_t ofs, uint32_t *pos)\n {\n \tunsigned lo, hi;\n \n-\tif (load_pack_revindex(p) < 0)\n+\tif (load_pack_revindex(the_repository, p) < 0)\n \t\treturn -1;\n \n \tlo = 0;\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 4974e75eb4d..3d1969ce8b9 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -39,6 +39,7 @@\n \n struct packed_git;\n struct multi_pack_index;\n+struct repository;\n \n /*\n * load_pack_revindex populates the revindex's internal data-structures for the\n@@ -47,7 +48,7 @@ struct multi_pack_index;\n * If a '.rev' file is present it is mmap'd, and pointers are assigned into it\n * (instead of using the in-memory variant).\n */\n-int load_pack_revindex(struct packed_git *p);\n+int load_pack_revindex(struct repository *r, struct packed_git *p);\n \n /*\n * load_midx_revindex loads the '.rev' file corresponding to the given\ndiff --git a/packfile.c b/packfile.c\nindex b120405ccc8..717fd685c97 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2151,7 +2151,7 @@ int for_each_object_in_pack(struct packed_git *p,\n \tint r = 0;\n \n \tif (flags & FOR_EACH_OBJECT_PACK_ORDER) {\n-\t\tif (load_pack_revindex(p))\n+\t\tif (load_pack_revindex(the_repository, p))\n \t\t\treturn -1;\n \t}\n \n-- \n2.40.0.323.gedff6a80c63\n\n"},{"id":"475254","messageId":"8eec5bacd3a39d4d1586459f4a23422aff903139.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"[PATCH v2 4/7] pack-revindex: introduce GIT_TEST_REV_INDEX_DIE_ON_DISK","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:27Z","receivedAt":"2023-04-12T22:20:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In ec8e7760ac (pack-revindex: ensure that on-disk reverse indexes are\ngiven precedence, 2021-01-25), we introduced\nGIT_TEST_REV_INDEX_DIE_IN_MEMORY to abort the process when Git generated\na reverse index from scratch.\n\nec8e7760ac was about ensuring that Git prefers a .rev file when\navailable over generating the same information in memory from scratch.\n\nIn a subsequent patch, we'll introduce `pack.readReverseIndex`, which\nmay be used to disable reading \".rev\" files when available. In order to\nensure that those files are indeed being ignored, introduce an analogous\noption to abort the process when Git reads a \".rev\" file from disk.\n\nSigned-off-by: Taylor Blau \n---\n pack-revindex.c | 3 +++\n pack-revindex.h | 1 +\n 2 files changed, 4 insertions(+)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex e3d69cc0f7a..44e1b3fed95 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -205,6 +205,9 @@ static int load_revindex_from_disk(char *revindex_name,\n \tsize_t revindex_size;\n \tstruct revindex_header *hdr;\n \n+\tif (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))\n+\t\tdie(\"dying as requested by '%s'\", GIT_TEST_REV_INDEX_DIE_ON_DISK);\n+\n \tfd = git_open(revindex_name);\n \n \tif (fd < 0) {\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 3d1969ce8b9..ef8afee88b0 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -36,6 +36,7 @@\n \n #define GIT_TEST_WRITE_REV_INDEX \"GIT_TEST_WRITE_REV_INDEX\"\n #define GIT_TEST_REV_INDEX_DIE_IN_MEMORY \"GIT_TEST_REV_INDEX_DIE_IN_MEMORY\"\n+#define GIT_TEST_REV_INDEX_DIE_ON_DISK \"GIT_TEST_REV_INDEX_DIE_ON_DISK\"\n \n struct packed_git;\n struct multi_pack_index;\n-- \n2.40.0.323.gedff6a80c63\n\n"},{"id":"475255","messageId":"a62fc3e4ec1aca773b6c3e47828db235df53c747.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"[PATCH v2 5/7] pack-revindex: introduce `pack.readReverseIndex`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:30Z","receivedAt":"2023-04-12T22:20:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Since 1615c567b8 (Documentation/config/pack.txt: advertise\n'pack.writeReverseIndex', 2021-01-25), we have had the\n`pack.writeReverseIndex` configuration option, which tells Git whether\nor not it is allowed to write a \".rev\" file when indexing a pack.\n\nIntroduce a complementary configuration knob, `pack.readReverseIndex` to\ncontrol whether or not Git will read any \".rev\" file(s) that may be\navailable on disk.\n\nThis option is useful for debugging, as well as disabling the effect of\n\".rev\" files in certain instances.\n\nThis is useful because of the trade-off[^1] between the time it takes to\ngenerate a reverse index (slow from scratch, fast when reading an\nexisting \".rev\" file), and the time it takes to access a record (the\nopposite).\n\nFor example, even though it is faster to use the on-disk reverse index\nwhen computing the on-disk size of a packed object, it is slower to\nenumerate the same value for all objects.\n\nHere are a couple of examples from linux.git. When computing the above\nfor a single object, using the on-disk reverse index is significantly\nfaster:\n\n $ git rev-parse HEAD >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} cat-file --batch-check=\"%(objectsize:disk)\" \n---\n Documentation/config/pack.txt | 6 ++++++\n pack-revindex.c | 5 ++++-\n repo-settings.c | 1 +\n repository.h | 1 +\n t/t5325-reverse-index.sh | 11 +++++++++++\n 5 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 53093d99969..7db7fed4667 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -171,6 +171,12 @@ pack.writeBitmapLookupTable::\n \tbeneficial in repositories that have relatively large bitmap\n \tindexes. Defaults to false.\n \n+pack.readReverseIndex::\n+\tWhen true, git will read any .rev file(s) that may be available\n+\t(see: linkgit:gitformat-pack[5]). When false, the reverse index\n+\twill be generated from scratch and stored in memory. Defaults to\n+\ttrue.\n+\n pack.writeReverseIndex::\n \tWhen true, git will write a corresponding .rev file (see:\n \tlinkgit:gitformat-pack[5])\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 44e1b3fed95..29f5358b256 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -291,7 +291,10 @@ int load_pack_revindex(struct repository *r, struct packed_git *p)\n \tif (p->revindex || p->revindex_data)\n \t\treturn 0;\n \n-\tif (!load_pack_revindex_from_disk(p))\n+\tprepare_repo_settings(r);\n+\n+\tif (r->settings.pack_read_reverse_index &&\n+\t !load_pack_revindex_from_disk(p))\n \t\treturn 0;\n \telse if (!create_pack_revindex_in_memory(p))\n \t\treturn 0;\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0a6c0b381fe..bdd7640ab0b 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -63,6 +63,7 @@ void prepare_repo_settings(struct repository *r)\n \trepo_cfg_bool(r, \"core.multipackindex\", &r->settings.core_multi_pack_index, 1);\n \trepo_cfg_bool(r, \"index.sparse\", &r->settings.sparse_index, 0);\n \trepo_cfg_bool(r, \"index.skiphash\", &r->settings.index_skip_hash, r->settings.index_skip_hash);\n+\trepo_cfg_bool(r, \"pack.readreverseindex\", &r->settings.pack_read_reverse_index, 1);\n \n \t/*\n \t * The GIT_TEST_MULTI_PACK_INDEX variable is special in that\ndiff --git a/repository.h b/repository.h\nindex 15a8afc5fb5..ed73e799b61 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -37,6 +37,7 @@ struct repo_settings {\n \tint fetch_write_commit_graph;\n \tint command_requires_full_index;\n \tint sparse_index;\n+\tint pack_read_reverse_index;\n \n \tstruct fsmonitor_settings *fsmonitor; /* lazily loaded */\n \ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 48c14d85764..66171c1d67b 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -96,6 +96,17 @@ test_expect_success 'reverse index is not generated when available on disk' '\n \t\t--batch-check=\"%(objectsize:disk)\" tip &&\n+\tGIT_TEST_REV_INDEX_DIE_ON_DISK=1 git cat-file \\\n+\t\t--batch-check=\"%(objectsize:disk)\" \n---\n builtin/index-pack.c | 4 ++--\n builtin/pack-objects.c | 4 ++--\n ci/run-build-and-tests.sh | 2 +-\n pack-revindex.h | 2 +-\n t/README | 2 +-\n t/t5325-reverse-index.sh | 2 +-\n 6 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 323c063f9db..9e36c985cf1 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1758,8 +1758,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (prefix && chdir(prefix))\n \t\tdie(_(\"Cannot come back to cwd\"));\n \n-\tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n-\t\trev_index = 1;\n+\tif (git_env_bool(GIT_TEST_NO_WRITE_REV_INDEX, 0))\n+\t\trev_index = 0;\n \telse\n \t\trev_index = !!(opts.flags & (WRITE_REV_VERIFY | WRITE_REV));\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex dbaa04482fd..1797871ce90 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4295,8 +4295,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \treset_pack_idx_option(&pack_idx_opts);\n \tpack_idx_opts.flags |= WRITE_REV;\n \tgit_config(git_pack_config, NULL);\n-\tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n-\t\tpack_idx_opts.flags |= WRITE_REV;\n+\tif (git_env_bool(GIT_TEST_NO_WRITE_REV_INDEX, 0))\n+\t\tpack_idx_opts.flags &= ~WRITE_REV;\n \n \tprogress = isatty(2);\n \targc = parse_options(argc, argv, prefix, pack_objects_options,\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex b098e10f52a..a18b13a41dd 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -27,7 +27,7 @@ linux-TEST-vars)\n \texport GIT_TEST_MULTI_PACK_INDEX=1\n \texport GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=1\n \texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\n-\texport GIT_TEST_WRITE_REV_INDEX=1\n+\texport GIT_TEST_NO_WRITE_REV_INDEX=1\n \texport GIT_TEST_CHECKOUT_WORKERS=2\n \t;;\n linux-clang)\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex ef8afee88b0..46e834064e1 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -34,7 +34,7 @@\n #define RIDX_SIGNATURE 0x52494458 /* \"RIDX\" */\n #define RIDX_VERSION 1\n \n-#define GIT_TEST_WRITE_REV_INDEX \"GIT_TEST_WRITE_REV_INDEX\"\n+#define GIT_TEST_NO_WRITE_REV_INDEX \"GIT_TEST_NO_WRITE_REV_INDEX\"\n #define GIT_TEST_REV_INDEX_DIE_IN_MEMORY \"GIT_TEST_REV_INDEX_DIE_IN_MEMORY\"\n #define GIT_TEST_REV_INDEX_DIE_ON_DISK \"GIT_TEST_REV_INDEX_DIE_ON_DISK\"\n \ndiff --git a/t/README b/t/README\nindex 29576c37488..bdfac4cceb2 100644\n--- a/t/README\n+++ b/t/README\n@@ -475,7 +475,7 @@ GIT_TEST_DEFAULT_HASH= specifies which hash algorithm to\n use in the test scripts. Recognized values for are \"sha1\"\n and \"sha256\".\n \n-GIT_TEST_WRITE_REV_INDEX=, when true enables the\n+GIT_TEST_NO_WRITE_REV_INDEX=, when true disables the\n 'pack.writeReverseIndex' setting.\n \n GIT_TEST_SPARSE_INDEX=, when true enables index writes to use the\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 149dcf5193b..0548fce1aa6 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -7,7 +7,7 @@ TEST_PASSES_SANITIZE_LEAK=true\n \n # The below tests want control over the 'pack.writeReverseIndex' setting\n # themselves to assert various combinations of it with other options.\n-sane_unset GIT_TEST_WRITE_REV_INDEX\n+sane_unset GIT_TEST_NO_WRITE_REV_INDEX\n \n packdir=.git/objects/pack\n \n-- \n2.40.0.323.gedff6a80c63\n"},{"id":"475257","messageId":"f8298fb0bac04ee80f9981f849dda76047ba09f0.1681338013.git.me@ttaylorr.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"[PATCH v2 6/7] config: enable `pack.writeReverseIndex` by default","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-12T22:20:33Z","receivedAt":"2023-04-12T22:21:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Back in e37d0b8730 (builtin/index-pack.c: write reverse indexes,\n2021-01-25), Git learned how to read and write a pack's reverse index\nfrom a file instead of in-memory.\n\nA pack's reverse index is a mapping from pack position (that is, the\norder that objects appear together in a \".pack\") to their position in\nlexical order (that is, the order that objects are listed in an \".idx\"\nfile).\n\nReverse indexes are consulted often during pack-objects, as well as\nduring auxiliary operations that require mapping between pack offsets,\npack order, and index index.\n\nThey are useful in GitHub's infrastructure, where we have seen a\ndramatic increase in performance when writing \".rev\" files[1]. In\nparticular:\n\n - an ~80% reduction in the time it takes to serve fetches on a popular\n repository, Homebrew/homebrew-core.\n\n - a ~60% reduction in the peak memory usage to serve fetches on that\n same repository.\n\n - a collective savings of ~35% in CPU time across all pack-objects\n invocations serving fetches across all repositories in a single\n datacenter.\n\nReverse indexes are also beneficial to end-users as well as forges. For\nexample, the time it takes to generate a pack containing the objects for\nthe 10 most recent commits in linux.git (representing a typical push) is\nsignificantly faster when on-disk reverse indexes are available:\n\n $ { git rev-parse HEAD && printf '^' && git rev-parse HEAD~10 } >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} pack-objects --delta-base-offset --revs --stdout /dev/null'\n Benchmark 1: git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 543.0 ms ± 20.3 ms [User: 616.2 ms, System: 58.8 ms]\n Range (min … max): 521.0 ms … 577.9 ms 10 runs\n\n Benchmark 2: git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 245.0 ms ± 11.4 ms [User: 335.6 ms, System: 31.3 ms]\n Range (min … max): 226.0 ms … 259.6 ms 13 runs\n\n Summary\n 'git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null' ran\n\t2.22 ± 0.13 times faster than 'git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null'\n\nThe same is true of writing a pack containing the objects for the 30\nmost-recent commits:\n\n $ { git rev-parse HEAD && printf '^' && git rev-parse HEAD~30 } >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} pack-objects --delta-base-offset --revs --stdout /dev/null'\n Benchmark 1: git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 866.5 ms ± 16.2 ms [User: 1414.5 ms, System: 97.0 ms]\n Range (min … max): 839.3 ms … 886.9 ms 10 runs\n\n Benchmark 2: git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null\n Time (mean ± σ): 581.6 ms ± 10.2 ms [User: 1181.7 ms, System: 62.6 ms]\n Range (min … max): 567.5 ms … 599.3 ms 10 runs\n\n Summary\n 'git.compile -c pack.readReverseIndex=true pack-objects --delta-base-offset --revs --stdout /dev/null' ran\n\t1.49 ± 0.04 times faster than 'git.compile -c pack.readReverseIndex=false pack-objects --delta-base-offset --revs --stdout /dev/null'\n\n...and savings on trivial operations like computing the on-disk size of\na single (packed) object are even more dramatic:\n\n $ git rev-parse HEAD >in\n $ hyperfine -L v false,true 'git.compile -c pack.readReverseIndex={v} cat-file --batch-check=\"%(objectsize:disk)\" \n---\n Documentation/config/pack.txt | 2 +-\n builtin/index-pack.c | 1 +\n builtin/pack-objects.c | 1 +\n t/perf/p5312-pack-bitmaps-revs.sh | 3 +--\n t/t5325-reverse-index.sh | 1 +\n 5 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 7db7fed4667..d4c7c9d4e4e 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -182,4 +182,4 @@ pack.writeReverseIndex::\n \tlinkgit:gitformat-pack[5])\n \tfor each new packfile that it writes in all places except for\n \tlinkgit:git-fast-import[1] and in the bulk checkin mechanism.\n-\tDefaults to false.\n+\tDefaults to true.\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex b17e79cd40f..323c063f9db 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1753,6 +1753,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tfsck_options.walk = mark_link;\n \n \treset_pack_idx_option(&opts);\n+\topts.flags |= WRITE_REV;\n \tgit_config(git_index_pack_config, &opts);\n \tif (prefix && chdir(prefix))\n \t\tdie(_(\"Cannot come back to cwd\"));\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 77d88f85b04..dbaa04482fd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4293,6 +4293,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t}\n \n \treset_pack_idx_option(&pack_idx_opts);\n+\tpack_idx_opts.flags |= WRITE_REV;\n \tgit_config(git_pack_config, NULL);\n \tif (git_env_bool(GIT_TEST_WRITE_REV_INDEX, 0))\n \t\tpack_idx_opts.flags |= WRITE_REV;\ndiff --git a/t/perf/p5312-pack-bitmaps-revs.sh b/t/perf/p5312-pack-bitmaps-revs.sh\nindex 0684b690af0..ceec60656b5 100755\n--- a/t/perf/p5312-pack-bitmaps-revs.sh\n+++ b/t/perf/p5312-pack-bitmaps-revs.sh\n@@ -12,8 +12,7 @@ test_lookup_pack_bitmap () {\n \ttest_perf_large_repo\n \n \ttest_expect_success 'setup bitmap config' '\n-\t\tgit config pack.writebitmaps true &&\n-\t\tgit config pack.writeReverseIndex true\n+\t\tgit config pack.writebitmaps true\n \t'\n \n \t# we need to create the tag up front such that it is covered by the repack and\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 66171c1d67b..149dcf5193b 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -14,6 +14,7 @@ packdir=.git/objects/pack\n test_expect_success 'setup' '\n \ttest_commit base &&\n \n+\ttest_config pack.writeReverseIndex false &&\n \tpack=$(git pack-objects --all $packdir/pack) &&\n \trev=$packdir/pack-$pack.rev &&\n \n-- \n2.40.0.323.gedff6a80c63\n\n"},{"id":"475298","messageId":"2b651052-5eba-6ec6-1d15-17ff60be9b4b@github.com","threadId":"59573","inReplyTo":"cover.1681338013.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/7] pack-revindex: enable on-disk reverse indexes by default","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-13T13:40:01Z","receivedAt":"2023-04-13T13:42:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/12/2023 6:20 PM, Taylor Blau wrote:\n> Here is a (tiny) reroll of my series to enable pack reverse-indexes to\n> be written to disk by default instead of computed on-the-fly in memory.\n> \n> The original cover letter[1] and commits herein have all of the gore-y\n> details. Not much has changed since last time, except:\n> \n> - squashing in a patch from Stolee to propagate a `struct repository\n> *` a little further out from `load_pack_revindex()`\n> \n> - a tweak to the linux-TEST-vars CI job to continue running it with\n> GIT_TEST_NO_WRITE_REV_INDEX\n> \n> - and an additional benchmark to demonstrate the performance of `git\n> cat-file --batch-check='%(objectsize:disk)' --batch--all-objects\n> --unordered` with and without on-disk reverse indexes\n\nThanks for addressing my concerns from v1. This version LGTM.\n\n-Stolee\n\n"},{"id":"475310","messageId":"xmqqttxjyd2r.fsf@gitster.g","threadId":"59573","inReplyTo":"56a0fc0098e0b0551e01414c8e67c17fb1ba3054.1681166596.git.me@ttaylorr.com","subject":"Re: [PATCH 6/7] config: enable `pack.writeReverseIndex` by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-13T16:14:04Z","receivedAt":"2023-04-13T16:14:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> They are useful in GitHub's infrastructure, where we have seen a\n> dramatic increase in performance when writing \".rev\" files[1]. In\n> particular:\n>\n> - an ~80% reduction in the time it takes to serve fetches on a popular\n> repository, Homebrew/homebrew-core.\n>\n> - a ~60% reduction in the peak memory usage to serve fetches on that\n> same repository.\n>\n> - a collective savings of ~35% in CPU time across all pack-objects\n> invocations serving fetches across all repositories in a single\n> datacenter.\n>\n> Reverse indexes are also beneficial to end-users as well as forges. For\n> example, the time it takes to generate a pack containing the objects for\n> the 10 most recent commits in linux.git (representing a typical push) is\n> significantly faster when on-disk reverse indexes are available:\n> ...\n> In the more than two years since e37d0b8730 was merged, Git's\n> implementation of on-disk reverse indexes has been thoroughly tested,\n> both from users enabling `pack.writeReverseIndexes`, and from GitHub's\n> deployment of the feature. The latter has been running without incident\n> for more than two years.\n\nWhat is missing is what extra overhead this adds to \"git gc\". Does\nit add 5%? 15%? How big an overhead do we find reasonable?\n\nOther than that, it looks like that the series is very well crafted.\nIt was somewhat surprising that we still need to add a few new\nend-user facing or meant-for-testing knobs for a feature that we\nclaim is well battle tested. It is understandable that we benefit\nby having knobs to selectively _disable_ a part of the feature now\nit will be on by default.\n\nWill queue. Thanks.\n\n"}]}