{"thread":{"id":"54020","subject":"[PATCH] multi-pack-index: repack batches below --batch-size","startedAt":"2020-08-11T15:30:24Z","lastAt":"2020-08-11T21:06:12Z","messageCount":3,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403358","messageId":"pull.698.git.1597159818457.gitgitgadget@gmail.com","threadId":"54020","inReplyTo":null,"subject":"[PATCH] multi-pack-index: repack batches below --batch-size","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-11T15:30:18Z","receivedAt":"2020-08-11T15:30:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe --batch-size= option of 'git multi-pack-index repack' is\nintended to limit the amount of work done by the repack. In the case of\na large repository, this command should repack a number of small\npack-files but leave the large pack-files alone. Most often, the\nrepository has one large pack-file from a 'git clone' operation and\nnumber of smaller pack-files from incremental 'git fetch' operations.\n\nThe issue with '--batch-size' is that it also _prevents_ the repack from\nhappening if the expected size of the resulting pack-file is too small.\nThis was intended as a way to avoid frequent churn of small pack-files,\nbut it has mostly caused confusion when a repository is of \"medium\"\nsize. That is, not enormous like the Windows OS repository, but also not\nso small that this incremental repack isn't valuable.\n\nThe solution presented here is to collect pack-files for repack if their\nexpected size is smaller than the batch-size parameter until either the\ntotal expected size exceeds the batch-size or all pack-files are\nconsidered. If there are at least two pack-files, then these are\ncombined to a new pack-file whose size should not be too much larger\nthan the batch-size.\n\nThis new strategy should succeed in keeping the number of pack-files\nsmall in these \"medium\" size repositories. The concern about churn is\nlikely not interesting, as the real control over that is the frequency\nin which the repack command is run.\n\nSigned-off-by: Derrick Stolee \n---\n multi-pack-index: repack batches below --batch-size\n \n As reported [1], the 'git multi-pack-index repack' command has some\n unexpected behavior due to the nature of \"expected size\" for un-thinned\n fetch packs and the fact that the batch size requires the total size to\n be at least as large as that batch-size. By removing this minimum size\n restriction, we will repack more frequently and prevent this \"many\n pack-file\" problems.\n \n [1] \n https://lore.kernel.org/git/6FA8F54A-C92D-497B-895F-AC6E8287AACD@gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-698%2Fderrickstolee%2Fmidx-repack-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-698/derrickstolee/midx-repack-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/698\n\n Documentation/git-multi-pack-index.txt | 11 ++++++-----\n midx.c | 2 +-\n t/t5319-multi-pack-index.sh | 18 ++++++++++++++++++\n 3 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\nindex 0c6619493c..eb0caa0439 100644\n--- a/Documentation/git-multi-pack-index.txt\n+++ b/Documentation/git-multi-pack-index.txt\n@@ -51,11 +51,12 @@ repack::\n \tmulti-pack-index, then divide by the total number of objects in\n \tthe pack and multiply by the pack size. We select packs with\n \texpected size below the batch size until the set of packs have\n-\ttotal expected size at least the batch size. If the total size\n-\tdoes not reach the batch size, then do nothing. If a new pack-\n-\tfile is created, rewrite the multi-pack-index to reference the\n-\tnew pack-file. A later run of 'git multi-pack-index expire' will\n-\tdelete the pack-files that were part of this batch.\n+\ttotal expected size at least the batch size, or all pack-files\n+\tare considered. If only one pack-file is selected, then do\n+\tnothing. If a new pack-file is created, rewrite the\n+\tmulti-pack-index to reference the new pack-file. A later run of\n+\t'git multi-pack-index expire' will delete the pack-files that\n+\twere part of this batch.\n +\n If `repack.packKeptObjects` is `false`, then any pack-files with an\n associated `.keep` file will not be selected for the batch to repack.\ndiff --git a/midx.c b/midx.c\nindex 6d1584ca51..38690b46c9 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1371,7 +1371,7 @@ static int fill_included_packs_batch(struct repository *r,\n \n \tfree(pack_info);\n \n-\tif (total_size < batch_size || packs_to_repack < 2)\n+\tif (packs_to_repack < 2)\n \t\treturn 1;\n \n \treturn 0;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 7214cab36c..b05190f500 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -643,6 +643,7 @@ test_expect_success 'expire respects .keep files' '\n '\n \n test_expect_success 'repack --batch-size=0 repacks everything' '\n+\tcp -r dup dup2 &&\n \t(\n \t\tcd dup &&\n \t\trm .git/objects/pack/*.keep &&\n@@ -662,4 +663,21 @@ test_expect_success 'repack --batch-size=0 repacks everything' '\n \t)\n '\n \n+test_expect_success 'repack --batch-size= repacks everything' '\n+\t(\n+\t\tcd dup2 &&\n+\t\trm .git/objects/pack/*.keep &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 2 idx-list &&\n+\t\tgit multi-pack-index repack --batch-size=2000000 &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 3 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 3 midx-list &&\n+\t\tgit multi-pack-index expire &&\n+\t\tls -al .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 1 idx-list\n+\t)\n+'\n+\n test_done\n\nbase-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc\n-- \ngitgitgadget\n"},{"id":"403362","messageId":"20200811155013.GF19871@syl.lan","threadId":"54020","inReplyTo":"pull.698.git.1597159818457.gitgitgadget@gmail.com","subject":"Re: [PATCH] multi-pack-index: repack batches below --batch-size","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T15:50:13Z","receivedAt":"2020-08-11T15:50:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 03:30:18PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> The --batch-size= option of 'git multi-pack-index repack' is\n> intended to limit the amount of work done by the repack. In the case of\n> a large repository, this command should repack a number of small\n> pack-files but leave the large pack-files alone. Most often, the\n> repository has one large pack-file from a 'git clone' operation and\n> number of smaller pack-files from incremental 'git fetch' operations.\n>\n> The issue with '--batch-size' is that it also _prevents_ the repack from\n> happening if the expected size of the resulting pack-file is too small.\n> This was intended as a way to avoid frequent churn of small pack-files,\n> but it has mostly caused confusion when a repository is of \"medium\"\n> size. That is, not enormous like the Windows OS repository, but also not\n> so small that this incremental repack isn't valuable.\n>\n> The solution presented here is to collect pack-files for repack if their\n> expected size is smaller than the batch-size parameter until either the\n> total expected size exceeds the batch-size or all pack-files are\n> considered. If there are at least two pack-files, then these are\n> combined to a new pack-file whose size should not be too much larger\n> than the batch-size.\n>\n> This new strategy should succeed in keeping the number of pack-files\n> small in these \"medium\" size repositories. The concern about churn is\n> likely not interesting, as the real control over that is the frequency\n> in which the repack command is run.\n\nOK. This '--batch-size' parameter is a little magical to me, but the\nbehavior you are describing seems sane. I think that your assessment of\nthe down-side is reasonable, too.\n\nLooks fine to me.\n\n Reviewed-by: Taylor Blau \n\n> Signed-off-by: Derrick Stolee \n> ---\n> multi-pack-index: repack batches below --batch-size\n>\n> As reported [1], the 'git multi-pack-index repack' command has some\n> unexpected behavior due to the nature of \"expected size\" for un-thinned\n> fetch packs and the fact that the batch size requires the total size to\n> be at least as large as that batch-size. By removing this minimum size\n> restriction, we will repack more frequently and prevent this \"many\n> pack-file\" problems.\n>\n> [1]\n> https://lore.kernel.org/git/6FA8F54A-C92D-497B-895F-AC6E8287AACD@gmail.com/\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-698%2Fderrickstolee%2Fmidx-repack-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-698/derrickstolee/midx-repack-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/698\n>\n> Documentation/git-multi-pack-index.txt | 11 ++++++-----\n> midx.c | 2 +-\n> t/t5319-multi-pack-index.sh | 18 ++++++++++++++++++\n> 3 files changed, 25 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\n> index 0c6619493c..eb0caa0439 100644\n> --- a/Documentation/git-multi-pack-index.txt\n> +++ b/Documentation/git-multi-pack-index.txt\n> @@ -51,11 +51,12 @@ repack::\n> \tmulti-pack-index, then divide by the total number of objects in\n> \tthe pack and multiply by the pack size. We select packs with\n> \texpected size below the batch size until the set of packs have\n> -\ttotal expected size at least the batch size. If the total size\n> -\tdoes not reach the batch size, then do nothing. If a new pack-\n> -\tfile is created, rewrite the multi-pack-index to reference the\n> -\tnew pack-file. A later run of 'git multi-pack-index expire' will\n> -\tdelete the pack-files that were part of this batch.\n> +\ttotal expected size at least the batch size, or all pack-files\n> +\tare considered. If only one pack-file is selected, then do\n> +\tnothing. If a new pack-file is created, rewrite the\n> +\tmulti-pack-index to reference the new pack-file. A later run of\n> +\t'git multi-pack-index expire' will delete the pack-files that\n> +\twere part of this batch.\n> +\n> If `repack.packKeptObjects` is `false`, then any pack-files with an\n> associated `.keep` file will not be selected for the batch to repack.\n> diff --git a/midx.c b/midx.c\n> index 6d1584ca51..38690b46c9 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1371,7 +1371,7 @@ static int fill_included_packs_batch(struct repository *r,\n>\n> \tfree(pack_info);\n>\n> -\tif (total_size < batch_size || packs_to_repack < 2)\n> +\tif (packs_to_repack < 2)\n> \t\treturn 1;\n>\n> \treturn 0;\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 7214cab36c..b05190f500 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -643,6 +643,7 @@ test_expect_success 'expire respects .keep files' '\n> '\n>\n> test_expect_success 'repack --batch-size=0 repacks everything' '\n> +\tcp -r dup dup2 &&\n> \t(\n> \t\tcd dup &&\n> \t\trm .git/objects/pack/*.keep &&\n> @@ -662,4 +663,21 @@ test_expect_success 'repack --batch-size=0 repacks everything' '\n> \t)\n> '\n>\n> +test_expect_success 'repack --batch-size= repacks everything' '\n> +\t(\n> +\t\tcd dup2 &&\n> +\t\trm .git/objects/pack/*.keep &&\n> +\t\tls .git/objects/pack/*idx >idx-list &&\n> +\t\ttest_line_count = 2 idx-list &&\n> +\t\tgit multi-pack-index repack --batch-size=2000000 &&\n> +\t\tls .git/objects/pack/*idx >idx-list &&\n> +\t\ttest_line_count = 3 idx-list &&\n> +\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n> +\t\ttest_line_count = 3 midx-list &&\n> +\t\tgit multi-pack-index expire &&\n> +\t\tls -al .git/objects/pack/*idx >idx-list &&\n> +\t\ttest_line_count = 1 idx-list\n> +\t)\n> +'\n> +\n> test_done\n>\n> base-commit: 47ae905ffb98cc4d4fd90083da6bc8dab55d9ecc\n> --\n> gitgitgadget\nThanks,\nTaylor\n"},{"id":"403407","messageId":"xmqqlfik99kw.fsf@gitster.c.googlers.com","threadId":"54020","inReplyTo":"20200811155013.GF19871@syl.lan","subject":"Re: [PATCH] multi-pack-index: repack batches below --batch-size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-11T21:06:07Z","receivedAt":"2020-08-11T21:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> OK. This '--batch-size' parameter is a little magical to me, but the\n> behavior you are describing seems sane. I think that your assessment of\n> the down-side is reasonable, too.\n>\n> Looks fine to me.\n>\n> Reviewed-by: Taylor Blau \n>\n>> Signed-off-by: Derrick Stolee \n\nThanks, both.\n"}]}