{"thread":{"id":"60752","subject":"[PATCH 0/2] pack-objects: enable multi-pack reuse via feature.experimental","startedAt":"2024-01-16T19:03:43Z","lastAt":"2024-02-06T07:25:39Z","messageCount":11,"participants":["Taylor Blau","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"486856","messageId":"cover.1705431816.git.me@ttaylorr.com","threadId":"60752","inReplyTo":null,"subject":"[PATCH 0/2] pack-objects: enable multi-pack reuse via feature.experimental","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-16T19:03:41Z","receivedAt":"2024-01-16T19:03:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series teaches pack-objects to perform multi-pack reuse by\nway of the feature.experimental configuration. The hope is that this\nwill expose this new feature to more users who might not otherwise be\naware of lesser-known configuration options for pack-objects's\ninternals.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n  t5332-multi-pack-reuse.sh: extract pack-objects helper functions\n  pack-objects: enable multi-pack reuse via `feature.experimental`\n\n Documentation/config/feature.txt |  3 ++\n builtin/pack-objects.c           |  2 +\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t5332-multi-pack-reuse.sh      | 84 ++++++++++++++++----------------\n 5 files changed, 50 insertions(+), 41 deletions(-)\n\n\nbase-commit: d4dbce1db5cd227a57074bcfc7ec9f0655961bba\n-- \n2.43.0.334.gd4dbce1db5.dirty\n"},{"id":"486857","messageId":"94dd41e1afdd6d926a106ab387295cf5fce624cf.1705431816.git.me@ttaylorr.com","threadId":"60752","inReplyTo":"cover.1705431816.git.me@ttaylorr.com","subject":"[PATCH 1/2] t5332-multi-pack-reuse.sh: extract pack-objects helper functions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-16T19:03:44Z","receivedAt":"2024-01-16T19:03:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Most of the tests in t5332 perform some setup before repeating a common\nrefrain that looks like:\n\n    : >trace2.txt &&\n    GIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n      git pack-objects --stdout --revs --all >/dev/null &&\n\n    test_pack_reused $objects_nr <trace2.txt &&\n    test_packs_reused $packs_nr <trace2.txt\n\nThe next commit will add more tests which repeat the above refrain.\nAvoid duplicating this invocation even further and prepare for the\nfollowing commit by wrapping the above in a helper function called\n`test_pack_objects_reused_all()`.\n\nIntroduce another similar function `test_pack_objects_reused`, which\nexpects to read a list of revisions over stdin for tests which need more\nfine-grained control of the contents of the pack they generate.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5332-multi-pack-reuse.sh | 70 +++++++++++++++----------------------\n 1 file changed, 28 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 2ba788b042..b53e821bc0 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -23,6 +23,26 @@ pack_position () {\n \tgrep \"$1\" objects | cut -d\" \" -f1\n }\n \n+# test_pack_objects_reused_all <pack-reused> <packs-reused>\n+test_pack_objects_reused_all () {\n+\t: >trace2.txt &&\n+\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n+\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n+\n+\ttest_pack_reused \"$1\" <trace2.txt &&\n+\ttest_packs_reused \"$2\" <trace2.txt\n+}\n+\n+# test_pack_objects_reused <pack-reused> <packs-reused>\n+test_pack_objects_reused () {\n+\t: >trace2.txt &&\n+\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n+\t\tgit pack-objects --stdout --revs >/dev/null &&\n+\n+\ttest_pack_reused \"$1\" <trace2.txt &&\n+\ttest_packs_reused \"$2\" <trace2.txt\n+}\n+\n test_expect_success 'preferred pack is reused for single-pack reuse' '\n \ttest_config pack.allowPackReuse single &&\n \n@@ -34,14 +54,10 @@ test_expect_success 'preferred pack is reused for single-pack reuse' '\n \n \tgit multi-pack-index write --bitmap &&\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n-\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused_all 3 1\n '\n \n+\n test_expect_success 'enable multi-pack reuse' '\n \tgit config pack.allowPackReuse multi\n '\n@@ -57,21 +73,11 @@ test_expect_success 'reuse all objects from subset of bitmapped packs' '\n \t^$(git rev-parse A)\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs <in >/dev/null &&\n-\n-\ttest_pack_reused 6 <trace2.txt &&\n-\ttest_packs_reused 2 <trace2.txt\n+\ttest_pack_objects_reused 6 2 <in\n '\n \n test_expect_success 'reuse all objects from all packs' '\n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n-\n-\ttest_pack_reused 9 <trace2.txt &&\n-\ttest_packs_reused 3 <trace2.txt\n+\ttest_pack_objects_reused_all 9 3\n '\n \n test_expect_success 'reuse objects from first pack with middle gap' '\n@@ -104,12 +110,7 @@ test_expect_success 'reuse objects from first pack with middle gap' '\n \t^$(git rev-parse D)\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n-\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused 3 1 <in\n '\n \n test_expect_success 'reuse objects from middle pack with middle gap' '\n@@ -125,12 +126,7 @@ test_expect_success 'reuse objects from middle pack with middle gap' '\n \t^$(git rev-parse D)\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n-\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused 3 1 <in\n '\n \n test_expect_success 'omit delta with uninteresting base (same pack)' '\n@@ -160,10 +156,6 @@ test_expect_success 'omit delta with uninteresting base (same pack)' '\n \t^$base\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n-\n \t# We can only reuse the 3 objects corresponding to \"other\" from\n \t# the latest pack.\n \t#\n@@ -175,8 +167,7 @@ test_expect_success 'omit delta with uninteresting base (same pack)' '\n \t# The remaining objects from the other pack are similarly not\n \t# reused because their objects are on the uninteresting side of\n \t# the query.\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused 3 1 <in\n '\n \n test_expect_success 'omit delta from uninteresting base (cross pack)' '\n@@ -189,15 +180,10 @@ test_expect_success 'omit delta from uninteresting base (cross pack)' '\n \n \tgit multi-pack-index write --bitmap --preferred-pack=\"pack-$P.idx\" &&\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --all >/dev/null &&\n-\n \tpacks_nr=\"$(find $packdir -type f -name \"pack-*.pack\" | wc -l)\" &&\n \tobjects_nr=\"$(git rev-list --count --all --objects)\" &&\n \n-\ttest_pack_reused $(($objects_nr - 1)) <trace2.txt &&\n-\ttest_packs_reused $packs_nr <trace2.txt\n+\ttest_pack_objects_reused_all $(($objects_nr - 1)) $packs_nr\n '\n \n test_done\n-- \n2.43.0.334.gd4dbce1db5.dirty\n\n"},{"id":"486858","messageId":"a2d0af562a5d0a4052c94f87c1d71639ff0b87f2.1705431816.git.me@ttaylorr.com","threadId":"60752","inReplyTo":"cover.1705431816.git.me@ttaylorr.com","subject":"[PATCH 2/2] pack-objects: enable multi-pack reuse via `feature.experimental`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-16T19:03:47Z","receivedAt":"2024-01-16T19:03:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that multi-pack reuse is supported, enable it via the\nfeature.experimental configuration in addition to the classic\n`pack.allowPackReuse`.\n\nThis will allow more users to experiment with the new behavior who might\nnot otherwise be aware of the existing `pack.allowPackReuse`\nconfiguration option.\n\nThe enum with values NO_PACK_REUSE, SINGLE_PACK_REUSE, and\nMULTI_PACK_REUSE is defined statically in builtin/pack-objects.c's\ncompilation unit. We could hoist that enum into a scope visible from the\nrepository_settings struct, and then use that enum value in\npack-objects. Instead, define a single int that indicates what\npack-objects's default value should be to avoid additional unnecessary\ncode movement.\n\nThough `feature.experimental` implies `pack.allowPackReuse=multi`, this\ncan still be overridden by explicitly setting the latter configuration\nto either \"single\" or \"false\". Tests covering all of these cases are\nshowin t5332.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/feature.txt |  3 +++\n builtin/pack-objects.c           |  2 ++\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t5332-multi-pack-reuse.sh      | 16 ++++++++++++++++\n 5 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex bf9546fca4..f061b64b74 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -17,6 +17,9 @@ skipping more commits at a time, reducing the number of round trips.\n +\n * `pack.useBitmapBoundaryTraversal=true` may improve bitmap traversal times by\n walking fewer objects.\n++\n+* `pack.allowPackReuse=multi` may improve the time it takes to create a pack by\n+reusing objects from multiple packs instead of just one.\n \n feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d8c2128a97..329aeac804 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4396,6 +4396,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tprepare_repo_settings(the_repository);\n \t\tif (sparse < 0)\n \t\t\tsparse = the_repository->settings.pack_use_sparse;\n+\t\tif (the_repository->settings.pack_use_multi_pack_reuse)\n+\t\t\tallow_pack_reuse = MULTI_PACK_REUSE;\n \t}\n \n \treset_pack_idx_option(&pack_idx_opts);\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 30cd478762..a0b590bc6c 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (experimental) {\n \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\tr->settings.pack_use_bitmap_boundary_traversal = 1;\n+\t\tr->settings.pack_use_multi_pack_reuse = 1;\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\ndiff --git a/repository.h b/repository.h\nindex 5f18486f64..b92881b0a3 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -36,6 +36,7 @@ struct repo_settings {\n \tint sparse_index;\n \tint pack_read_reverse_index;\n \tint pack_use_bitmap_boundary_traversal;\n+\tint pack_use_multi_pack_reuse;\n \n \t/*\n \t * Does this repository have core.useReplaceRefs=true (on by\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex b53e821bc0..ccc8735db6 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -57,6 +57,22 @@ test_expect_success 'preferred pack is reused for single-pack reuse' '\n \ttest_pack_objects_reused_all 3 1\n '\n \n+test_expect_success 'multi-pack reuse is disabled by default' '\n+\ttest_pack_objects_reused_all 3 1\n+'\n+\n+test_expect_success 'feature.experimental implies multi-pack reuse' '\n+\ttest_config feature.experimental true &&\n+\n+\ttest_pack_objects_reused_all 6 2\n+'\n+\n+test_expect_success 'multi-pack reuse can be disabled with feature.experimental' '\n+\ttest_config feature.experimental true &&\n+\ttest_config pack.allowPackReuse single &&\n+\n+\ttest_pack_objects_reused_all 3 1\n+'\n \n test_expect_success 'enable multi-pack reuse' '\n \tgit config pack.allowPackReuse multi\n-- \n2.43.0.334.gd4dbce1db5.dirty\n"},{"id":"486908","messageId":"ZaeCeY-9AIR-zt7u@tanuki","threadId":"60752","inReplyTo":"94dd41e1afdd6d926a106ab387295cf5fce624cf.1705431816.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] t5332-multi-pack-reuse.sh: extract pack-objects helper functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-17T07:32:09Z","receivedAt":"2024-01-17T07:33:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 16, 2024 at 02:03:44PM -0500, Taylor Blau wrote:\n> Most of the tests in t5332 perform some setup before repeating a common\n> refrain that looks like:\n> \n>     : >trace2.txt &&\n>     GIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n>       git pack-objects --stdout --revs --all >/dev/null &&\n> \n>     test_pack_reused $objects_nr <trace2.txt &&\n>     test_packs_reused $packs_nr <trace2.txt\n> \n> The next commit will add more tests which repeat the above refrain.\n> Avoid duplicating this invocation even further and prepare for the\n> following commit by wrapping the above in a helper function called\n> `test_pack_objects_reused_all()`.\n> \n> Introduce another similar function `test_pack_objects_reused`, which\n> expects to read a list of revisions over stdin for tests which need more\n> fine-grained control of the contents of the pack they generate.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  t/t5332-multi-pack-reuse.sh | 70 +++++++++++++++----------------------\n>  1 file changed, 28 insertions(+), 42 deletions(-)\n> \n> diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\n> index 2ba788b042..b53e821bc0 100755\n> --- a/t/t5332-multi-pack-reuse.sh\n> +++ b/t/t5332-multi-pack-reuse.sh\n> @@ -23,6 +23,26 @@ pack_position () {\n>  \tgrep \"$1\" objects | cut -d\" \" -f1\n>  }\n>  \n> +# test_pack_objects_reused_all <pack-reused> <packs-reused>\n> +test_pack_objects_reused_all () {\n> +\t: >trace2.txt &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> +\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n> +\n> +\ttest_pack_reused \"$1\" <trace2.txt &&\n> +\ttest_packs_reused \"$2\" <trace2.txt\n> +}\n> +\n> +# test_pack_objects_reused <pack-reused> <packs-reused>\n> +test_pack_objects_reused () {\n> +\t: >trace2.txt &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> +\t\tgit pack-objects --stdout --revs >/dev/null &&\n> +\n> +\ttest_pack_reused \"$1\" <trace2.txt &&\n> +\ttest_packs_reused \"$2\" <trace2.txt\n> +}\n> +\n>  test_expect_success 'preferred pack is reused for single-pack reuse' '\n>  \ttest_config pack.allowPackReuse single &&\n>  \n[snip]\n> @@ -104,12 +110,7 @@ test_expect_success 'reuse objects from first pack with middle gap' '\n>  \t^$(git rev-parse D)\n>  \tEOF\n>  \n> -\t: >trace2.txt &&\n> -\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> -\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n> -\n> -\ttest_pack_reused 3 <trace2.txt &&\n> -\ttest_packs_reused 1 <trace2.txt\n> +\ttest_pack_objects_reused 3 1 <in\n\nThis conversion causes us to drop the `--delta-base-offset` flag. It\nwould be great to have an explanation in the commit message why it is\nfine to drop it.\n\nOther than that this looks like a nice simplification to me.\n\nPatrick\n"},{"id":"486909","messageId":"ZaeCf1KCwAUCeBPy@tanuki","threadId":"60752","inReplyTo":"a2d0af562a5d0a4052c94f87c1d71639ff0b87f2.1705431816.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] pack-objects: enable multi-pack reuse via `feature.experimental`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-17T07:32:15Z","receivedAt":"2024-01-17T07:33:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 16, 2024 at 02:03:47PM -0500, Taylor Blau wrote:\n> Now that multi-pack reuse is supported, enable it via the\n> feature.experimental configuration in addition to the classic\n> `pack.allowPackReuse`.\n> \n> This will allow more users to experiment with the new behavior who might\n> not otherwise be aware of the existing `pack.allowPackReuse`\n> configuration option.\n> \n> The enum with values NO_PACK_REUSE, SINGLE_PACK_REUSE, and\n> MULTI_PACK_REUSE is defined statically in builtin/pack-objects.c's\n> compilation unit. We could hoist that enum into a scope visible from the\n> repository_settings struct, and then use that enum value in\n> pack-objects. Instead, define a single int that indicates what\n> pack-objects's default value should be to avoid additional unnecessary\n> code movement.\n> \n> Though `feature.experimental` implies `pack.allowPackReuse=multi`, this\n> can still be overridden by explicitly setting the latter configuration\n> to either \"single\" or \"false\". Tests covering all of these cases are\n> showin t5332.\n\nI do not mind adding more configs to `feature.experimental` because it\nis the best mechanism we have for a staged rollout of features. It is\nnot ideal by any means as we have no way to tell how many people enable\nthis, or whether they hit any bugs. But we do not really have any\nalternatives.\n\nBut one thing I would like to see is to have a clear plan for how\nexperimental features become stable. It's not a huge problem (yet)\nbecause there are only two experimental features. One of them\n(\"pack.useBitmapBoundaryTraversal=true\") was recently added by you via\nb0afdce5da (pack-bitmap.c: use commit boundary during bitmap traversal,\n2023-05-08), which is perfectly fine. But the other one\n(\"fetch.negotiationAlgorithm=skipping\") has been added has been added\nvia b5651a2092 (experimental: default to fetch.writeCommitGraph=false,\n2020-07-06), so it's been experimental for ~3.5 years by now.\n\nI would like to avoid cases like this by laying out a plan for when\nexperimental features become the new default. It could be as simple as\n\"Let's wait N releases and then mark it stable\". But having something\nand documenting such a plan in our code makes it a lot more actionable\nto promote those features to become stable eventually.\n\nI know that this is not in any way specific to your patch, but I thought\nthis was a good opportunity to start this discussion. If we can agree on\nmy opinion then it would be great to add a comment to the experimental\nfeature to explain such an exit criterion.\n\nOther than that this patch looks good to me, thanks!\n\nPatrick\n"},{"id":"487961","messageId":"ZcFk4yvj6TM2WzXn@nand.local","threadId":"60752","inReplyTo":"ZaeCeY-9AIR-zt7u@tanuki","subject":"Re: [PATCH 1/2] t5332-multi-pack-reuse.sh: extract pack-objects helper functions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-05T22:44:51Z","receivedAt":"2024-02-05T22:44:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 17, 2024 at 08:32:09AM +0100, Patrick Steinhardt wrote:\n> > @@ -104,12 +110,7 @@ test_expect_success 'reuse objects from first pack with middle gap' '\n> >  \t^$(git rev-parse D)\n> >  \tEOF\n> >\n> > -\t: >trace2.txt &&\n> > -\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> > -\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n> > -\n> > -\ttest_pack_reused 3 <trace2.txt &&\n> > -\ttest_packs_reused 1 <trace2.txt\n> > +\ttest_pack_objects_reused 3 1 <in\n>\n> This conversion causes us to drop the `--delta-base-offset` flag. It\n> would be great to have an explanation in the commit message why it is\n> fine to drop it.\n\nOops, that was unintentional. I'll resend a new round that fixes this\nissue shortly.\n\n> Other than that this looks like a nice simplification to me.\n\nThanks for the review!\n\nThanks,\nTaylor\n"},{"id":"487962","messageId":"ZcFlshJhJJIOsXqJ@nand.local","threadId":"60752","inReplyTo":"ZaeCf1KCwAUCeBPy@tanuki","subject":"Re: [PATCH 2/2] pack-objects: enable multi-pack reuse via `feature.experimental`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-05T22:48:18Z","receivedAt":"2024-02-05T22:48:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 17, 2024 at 08:32:15AM +0100, Patrick Steinhardt wrote:\n> I would like to avoid cases like this by laying out a plan for when\n> experimental features become the new default. It could be as simple as\n> \"Let's wait N releases and then mark it stable\". But having something\n> and documenting such a plan in our code makes it a lot more actionable\n> to promote those features to become stable eventually.\n\nFair point. I think that these have been mostly ad-hoc. Unfortunately,\nwhen folks leave the project or change their attention to working on a\ndifferent feature, things that were left in feature.experimental may be\nforgotten about.\n\nWhen this inevitably happens, it would be nice to have a written record\n(either in the repository, or here on the mailing list) so that other\nfolks can take up the mantle and graduate those feature(s) as\nappropriate.\n\n> I know that this is not in any way specific to your patch, but I thought\n> this was a good opportunity to start this discussion. If we can agree on\n> my opinion then it would be great to add a comment to the experimental\n> feature to explain such an exit criterion.\n\nI don't have a ton to add here for a graduation plan other than it would\nbe nice to enable it eventually, likely after a few releases without any\nshow-stopping bug reports.\n\n> Other than that this patch looks good to me, thanks!\n\nThanks again for the review!\n\nThanks,\nTaylor\n"},{"id":"487963","messageId":"cover.1707173415.git.me@ttaylorr.com","threadId":"60752","inReplyTo":"cover.1705431816.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] pack-objects: enable multi-pack reuse via feature.experimental","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-05T22:50:16Z","receivedAt":"2024-02-05T22:50:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small reroll of my series to teache pack-objects to perform\nmulti-pack reuse by way of the feature.experimental configuration.\n\nSimilar to last time, the hope is that this will expose this new feature\nto more users who might not otherwise be aware of lesser-known\nconfiguration options for pack-objects's internals.\n\nThe only changes since last time are as follows:\n\n  - Rebased forward onto 2a540e432f (The thirteenth batch, 2024-02-02).\n\n  - Re-introduced a missing `--delta-base-offset` argument into the new\n    helper function added and used by t5332.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n  t5332-multi-pack-reuse.sh: extract pack-objects helper functions\n  pack-objects: enable multi-pack reuse via `feature.experimental`\n\n Documentation/config/feature.txt |  3 ++\n builtin/pack-objects.c           |  2 +\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t5332-multi-pack-reuse.sh      | 85 +++++++++++++++++---------------\n 5 files changed, 51 insertions(+), 41 deletions(-)\n\nRange-diff against v1:\n1:  94dd41e1af ! 1:  db3d67bfa3 t5332-multi-pack-reuse.sh: extract pack-objects helper functions\n    @@ t/t5332-multi-pack-reuse.sh: pack_position () {\n     +test_pack_objects_reused_all () {\n     +\t: >trace2.txt &&\n     +\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n    -+\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n    ++\t\tgit pack-objects --stdout --revs --all --delta-base-offset \\\n    ++\t\t>/dev/null &&\n     +\n     +\ttest_pack_reused \"$1\" <trace2.txt &&\n     +\ttest_packs_reused \"$2\" <trace2.txt\n2:  a2d0af562a = 2:  683ffd154e pack-objects: enable multi-pack reuse via `feature.experimental`\n\nbase-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9\n-- \n2.43.0.524.g683ffd154e\n"},{"id":"487964","messageId":"db3d67bfa38bf3141214619e766d39dc7f709812.1707173415.git.me@ttaylorr.com","threadId":"60752","inReplyTo":"cover.1707173415.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] t5332-multi-pack-reuse.sh: extract pack-objects helper functions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-05T22:50:19Z","receivedAt":"2024-02-05T22:50:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Most of the tests in t5332 perform some setup before repeating a common\nrefrain that looks like:\n\n    : >trace2.txt &&\n    GIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n      git pack-objects --stdout --revs --all >/dev/null &&\n\n    test_pack_reused $objects_nr <trace2.txt &&\n    test_packs_reused $packs_nr <trace2.txt\n\nThe next commit will add more tests which repeat the above refrain.\nAvoid duplicating this invocation even further and prepare for the\nfollowing commit by wrapping the above in a helper function called\n`test_pack_objects_reused_all()`.\n\nIntroduce another similar function `test_pack_objects_reused`, which\nexpects to read a list of revisions over stdin for tests which need more\nfine-grained control of the contents of the pack they generate.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5332-multi-pack-reuse.sh | 71 +++++++++++++++----------------------\n 1 file changed, 29 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 2ba788b042..d516062f84 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -23,6 +23,27 @@ pack_position () {\n \tgrep \"$1\" objects | cut -d\" \" -f1\n }\n \n+# test_pack_objects_reused_all <pack-reused> <packs-reused>\n+test_pack_objects_reused_all () {\n+\t: >trace2.txt &&\n+\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n+\t\tgit pack-objects --stdout --revs --all --delta-base-offset \\\n+\t\t>/dev/null &&\n+\n+\ttest_pack_reused \"$1\" <trace2.txt &&\n+\ttest_packs_reused \"$2\" <trace2.txt\n+}\n+\n+# test_pack_objects_reused <pack-reused> <packs-reused>\n+test_pack_objects_reused () {\n+\t: >trace2.txt &&\n+\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n+\t\tgit pack-objects --stdout --revs >/dev/null &&\n+\n+\ttest_pack_reused \"$1\" <trace2.txt &&\n+\ttest_packs_reused \"$2\" <trace2.txt\n+}\n+\n test_expect_success 'preferred pack is reused for single-pack reuse' '\n \ttest_config pack.allowPackReuse single &&\n \n@@ -34,14 +55,10 @@ test_expect_success 'preferred pack is reused for single-pack reuse' '\n \n \tgit multi-pack-index write --bitmap &&\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n-\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused_all 3 1\n '\n \n+\n test_expect_success 'enable multi-pack reuse' '\n \tgit config pack.allowPackReuse multi\n '\n@@ -57,21 +74,11 @@ test_expect_success 'reuse all objects from subset of bitmapped packs' '\n \t^$(git rev-parse A)\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs <in >/dev/null &&\n-\n-\ttest_pack_reused 6 <trace2.txt &&\n-\ttest_packs_reused 2 <trace2.txt\n+\ttest_pack_objects_reused 6 2 <in\n '\n \n test_expect_success 'reuse all objects from all packs' '\n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n-\n-\ttest_pack_reused 9 <trace2.txt &&\n-\ttest_packs_reused 3 <trace2.txt\n+\ttest_pack_objects_reused_all 9 3\n '\n \n test_expect_success 'reuse objects from first pack with middle gap' '\n@@ -104,12 +111,7 @@ test_expect_success 'reuse objects from first pack with middle gap' '\n \t^$(git rev-parse D)\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n-\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused 3 1 <in\n '\n \n test_expect_success 'reuse objects from middle pack with middle gap' '\n@@ -125,12 +127,7 @@ test_expect_success 'reuse objects from middle pack with middle gap' '\n \t^$(git rev-parse D)\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n-\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused 3 1 <in\n '\n \n test_expect_success 'omit delta with uninteresting base (same pack)' '\n@@ -160,10 +157,6 @@ test_expect_success 'omit delta with uninteresting base (same pack)' '\n \t^$base\n \tEOF\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --revs <in >/dev/null &&\n-\n \t# We can only reuse the 3 objects corresponding to \"other\" from\n \t# the latest pack.\n \t#\n@@ -175,8 +168,7 @@ test_expect_success 'omit delta with uninteresting base (same pack)' '\n \t# The remaining objects from the other pack are similarly not\n \t# reused because their objects are on the uninteresting side of\n \t# the query.\n-\ttest_pack_reused 3 <trace2.txt &&\n-\ttest_packs_reused 1 <trace2.txt\n+\ttest_pack_objects_reused 3 1 <in\n '\n \n test_expect_success 'omit delta from uninteresting base (cross pack)' '\n@@ -189,15 +181,10 @@ test_expect_success 'omit delta from uninteresting base (cross pack)' '\n \n \tgit multi-pack-index write --bitmap --preferred-pack=\"pack-$P.idx\" &&\n \n-\t: >trace2.txt &&\n-\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --delta-base-offset --all >/dev/null &&\n-\n \tpacks_nr=\"$(find $packdir -type f -name \"pack-*.pack\" | wc -l)\" &&\n \tobjects_nr=\"$(git rev-list --count --all --objects)\" &&\n \n-\ttest_pack_reused $(($objects_nr - 1)) <trace2.txt &&\n-\ttest_packs_reused $packs_nr <trace2.txt\n+\ttest_pack_objects_reused_all $(($objects_nr - 1)) $packs_nr\n '\n \n test_done\n-- \n2.43.0.524.g683ffd154e\n\n"},{"id":"487965","messageId":"683ffd154e02b98a7a17c6200613e35183963823.1707173415.git.me@ttaylorr.com","threadId":"60752","inReplyTo":"cover.1707173415.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] pack-objects: enable multi-pack reuse via `feature.experimental`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-02-05T22:50:23Z","receivedAt":"2024-02-05T22:50:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Now that multi-pack reuse is supported, enable it via the\nfeature.experimental configuration in addition to the classic\n`pack.allowPackReuse`.\n\nThis will allow more users to experiment with the new behavior who might\nnot otherwise be aware of the existing `pack.allowPackReuse`\nconfiguration option.\n\nThe enum with values NO_PACK_REUSE, SINGLE_PACK_REUSE, and\nMULTI_PACK_REUSE is defined statically in builtin/pack-objects.c's\ncompilation unit. We could hoist that enum into a scope visible from the\nrepository_settings struct, and then use that enum value in\npack-objects. Instead, define a single int that indicates what\npack-objects's default value should be to avoid additional unnecessary\ncode movement.\n\nThough `feature.experimental` implies `pack.allowPackReuse=multi`, this\ncan still be overridden by explicitly setting the latter configuration\nto either \"single\" or \"false\". Tests covering all of these cases are\nshowin t5332.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/feature.txt |  3 +++\n builtin/pack-objects.c           |  2 ++\n repo-settings.c                  |  1 +\n repository.h                     |  1 +\n t/t5332-multi-pack-reuse.sh      | 16 ++++++++++++++++\n 5 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex bf9546fca4..f061b64b74 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -17,6 +17,9 @@ skipping more commits at a time, reducing the number of round trips.\n +\n * `pack.useBitmapBoundaryTraversal=true` may improve bitmap traversal times by\n walking fewer objects.\n++\n+* `pack.allowPackReuse=multi` may improve the time it takes to create a pack by\n+reusing objects from multiple packs instead of just one.\n \n feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d8c2128a97..329aeac804 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4396,6 +4396,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tprepare_repo_settings(the_repository);\n \t\tif (sparse < 0)\n \t\t\tsparse = the_repository->settings.pack_use_sparse;\n+\t\tif (the_repository->settings.pack_use_multi_pack_reuse)\n+\t\t\tallow_pack_reuse = MULTI_PACK_REUSE;\n \t}\n \n \treset_pack_idx_option(&pack_idx_opts);\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 30cd478762..a0b590bc6c 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -43,6 +43,7 @@ void prepare_repo_settings(struct repository *r)\n \tif (experimental) {\n \t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n \t\tr->settings.pack_use_bitmap_boundary_traversal = 1;\n+\t\tr->settings.pack_use_multi_pack_reuse = 1;\n \t}\n \tif (manyfiles) {\n \t\tr->settings.index_version = 4;\ndiff --git a/repository.h b/repository.h\nindex 7a250a6605..21949c5a17 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -39,6 +39,7 @@ struct repo_settings {\n \tint sparse_index;\n \tint pack_read_reverse_index;\n \tint pack_use_bitmap_boundary_traversal;\n+\tint pack_use_multi_pack_reuse;\n \n \t/*\n \t * Does this repository have core.useReplaceRefs=true (on by\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex d516062f84..b925a81d37 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -58,6 +58,22 @@ test_expect_success 'preferred pack is reused for single-pack reuse' '\n \ttest_pack_objects_reused_all 3 1\n '\n \n+test_expect_success 'multi-pack reuse is disabled by default' '\n+\ttest_pack_objects_reused_all 3 1\n+'\n+\n+test_expect_success 'feature.experimental implies multi-pack reuse' '\n+\ttest_config feature.experimental true &&\n+\n+\ttest_pack_objects_reused_all 6 2\n+'\n+\n+test_expect_success 'multi-pack reuse can be disabled with feature.experimental' '\n+\ttest_config feature.experimental true &&\n+\ttest_config pack.allowPackReuse single &&\n+\n+\ttest_pack_objects_reused_all 3 1\n+'\n \n test_expect_success 'enable multi-pack reuse' '\n \tgit config pack.allowPackReuse multi\n-- \n2.43.0.524.g683ffd154e\n"},{"id":"488044","messageId":"ZcHe67-6VLssXYDL@tanuki","threadId":"60752","inReplyTo":"db3d67bfa38bf3141214619e766d39dc7f709812.1707173415.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/2] t5332-multi-pack-reuse.sh: extract pack-objects helper functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-06T07:25:31Z","receivedAt":"2024-02-06T07:25:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 05, 2024 at 05:50:19PM -0500, Taylor Blau wrote:\n> Most of the tests in t5332 perform some setup before repeating a common\n> refrain that looks like:\n> \n>     : >trace2.txt &&\n>     GIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n>       git pack-objects --stdout --revs --all >/dev/null &&\n> \n>     test_pack_reused $objects_nr <trace2.txt &&\n>     test_packs_reused $packs_nr <trace2.txt\n> \n> The next commit will add more tests which repeat the above refrain.\n> Avoid duplicating this invocation even further and prepare for the\n> following commit by wrapping the above in a helper function called\n> `test_pack_objects_reused_all()`.\n> \n> Introduce another similar function `test_pack_objects_reused`, which\n> expects to read a list of revisions over stdin for tests which need more\n> fine-grained control of the contents of the pack they generate.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  t/t5332-multi-pack-reuse.sh | 71 +++++++++++++++----------------------\n>  1 file changed, 29 insertions(+), 42 deletions(-)\n> \n> diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\n> index 2ba788b042..d516062f84 100755\n> --- a/t/t5332-multi-pack-reuse.sh\n> +++ b/t/t5332-multi-pack-reuse.sh\n> @@ -23,6 +23,27 @@ pack_position () {\n>  \tgrep \"$1\" objects | cut -d\" \" -f1\n>  }\n>  \n> +# test_pack_objects_reused_all <pack-reused> <packs-reused>\n> +test_pack_objects_reused_all () {\n> +\t: >trace2.txt &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> +\t\tgit pack-objects --stdout --revs --all --delta-base-offset \\\n> +\t\t>/dev/null &&\n> +\n> +\ttest_pack_reused \"$1\" <trace2.txt &&\n> +\ttest_packs_reused \"$2\" <trace2.txt\n> +}\n> +\n> +# test_pack_objects_reused <pack-reused> <packs-reused>\n> +test_pack_objects_reused () {\n> +\t: >trace2.txt &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> +\t\tgit pack-objects --stdout --revs >/dev/null &&\n> +\n> +\ttest_pack_reused \"$1\" <trace2.txt &&\n> +\ttest_packs_reused \"$2\" <trace2.txt\n> +}\n> +\n>  test_expect_success 'preferred pack is reused for single-pack reuse' '\n>  \ttest_config pack.allowPackReuse single &&\n>  \n> @@ -34,14 +55,10 @@ test_expect_success 'preferred pack is reused for single-pack reuse' '\n>  \n>  \tgit multi-pack-index write --bitmap &&\n>  \n> -\t: >trace2.txt &&\n> -\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n> -\t\tgit pack-objects --stdout --revs --all >/dev/null &&\n> -\n> -\ttest_pack_reused 3 <trace2.txt &&\n> -\ttest_packs_reused 1 <trace2.txt\n> +\ttest_pack_objects_reused_all 3 1\n>  '\n\nSorry for being nitpicky, but now we have the reverse problem where this\nfunction adds `--delta-base-offset`. How about we adapt the helper\nfunction so that it accepts trailing arguments like this:\n\n```\ntest_pack_objects_reused () {\n\tlocal pack_reused=\"$1\"\n\tlocal packs_reused=\"$2\"\n\tshift 2\n\n\t: >trace2.txt &&\n\tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n\t\tgit pack-objects --stdout --revs \"$@\" >/dev/null &&\n\n\ttest_pack_reused \"$pack_reused\" <trace2.txt &&\n\ttest_packs_reused \"$packs_reused\" <trace2.txt\n}\n```\n\nThis merges the two helpers into a single one where callers can decide\nby themselves which arguments to pass to git-pack-objects(1).\n\nPatrick\n"}]}