{"thread":{"id":"53402","subject":"[PATCH] midx: apply gitconfig to midx repack","startedAt":"2020-05-05T13:06:48Z","lastAt":"2020-05-10T16:07:42Z","messageCount":26,"participants":["Son Luong Ngoc via GitGitGadget","Derrick Stolee","Son Luong Ngoc","Derrick Stolee via GitGitGadget","Eric Sunshine","Junio C Hamano","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397089","messageId":"pull.626.git.1588684003766.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":null,"subject":"[PATCH] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-05T13:06:43Z","receivedAt":"2020-05-05T13:06:48Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"From: Son Luong Ngoc \n\nMulti-Pack-Index repack is an incremental, repack solutions\nthat allows user to consolidate multiple packfiles in a non-disruptive\nway. However the new packfile could be created without some of the\ncapabilities of a packfile that is created by calling `git repack`.\n\nThis is because with `git repack`, there are configuration that would\nenable different flags to be passed down to `git pack-objects` plumbing.\n\nIn this patch, I applies those flags into `git multi-pack-index repack`\nso that it respect the `repack.*` config series.\n\nNote: I left out `repack.packKeptObjects` intentionally as I dont think\nits relevant to midx repack use case.\n\nSigned-off-by: Son Luong Ngoc \n---\n midx: apply gitconfig to midx repack\n \n Midx repack has largely been used in Microsoft Scalar on the client side\n to optimize the repository multiple packs state. However when I tried to\n apply this onto the server-side, I realized that there are certain\n features that were lacking compare to git repack. Most of these features\n are highly desirable on the server-side to create the most optimized\n pack possible.\n \n One of the example is delta_base_offset, comparing an midx repack\n with/without delta_base_offset, we can observe significant size\n differences.\n \n > du objects/pack/*pack\n 14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack\n 9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack\n \n Latest 2.26.2 (without delta_base_offset)\n > git multi-pack-index write\n > git multi-pack-index repack\n > git multi-pack-index expire\n > du objects/pack/*pack\n 9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack\n \n With delta_base_offset\n > git version\n git version 2.26.2.672.g232c24e857.dirty\n > git multi-pack-index write\n > git multi-pack-index repack\n > git multi-pack-index expire\n > du objects/pack/*pack\n 9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack\n \n In this patch, I intentionally leaving out repack.packKeptObjects as I\n don't think its very relevant to midx repack use case:\n \n * One could always exclude biggest packs with --batch-size option\n \n \n * For non-biggest-packs exclusion use case, its rather rare (unless you\n want to have a special pack with only commits and trees being\n excluded from repack to serve partial clone better?)\n \n \n \n Please let me know if anyone think that we should include that option\n for the sake of completions.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/626\n\n midx.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 9a61d3b37d9..88f16594268 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1361,6 +1361,10 @@ static int fill_included_packs_batch(struct repository *r,\n \treturn 0;\n }\n \n+static int delta_base_offset = 1;\n+static int write_bitmaps = -1;\n+static int use_delta_islands;\n+\n int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)\n {\n \tint result = 0;\n@@ -1381,12 +1385,25 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t} else if (fill_included_packs_all(m, include_pack))\n \t\tgoto cleanup;\n \n+ git_config_get_bool(\"repack.usedeltabaseoffset\", &delta_base_offset);\n+ git_config_get_bool(\"repack.writebitmaps\", &write_bitmaps);\n+ git_config_get_bool(\"repack.usedeltaislands\", &use_delta_islands);\n+\n \targv_array_push(&cmd.args, \"pack-objects\");\n \n \tstrbuf_addstr(&base_name, object_dir);\n \tstrbuf_addstr(&base_name, \"/pack/pack\");\n \targv_array_push(&cmd.args, base_name.buf);\n \n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n+\tif (use_delta_islands)\n+\t\targv_array_push(&cmd.args, \"--delta-islands\");\n+\tif (write_bitmaps > 0)\n+\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n+\telse if (write_bitmaps < 0)\n+\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n+\n \tif (flags & MIDX_PROGRESS)\n \t\targv_array_push(&cmd.args, \"--progress\");\n \telse\n\nbase-commit: b34789c0b0d3b137f0bb516b417bd8d75e0cb306\n-- \ngitgitgadget\n"},{"id":"397090","messageId":"8bd91a14-75dc-76e2-31b4-54eff5bea8dd@gmail.com","threadId":"53402","inReplyTo":"pull.626.git.1588684003766.gitgitgadget@gmail.com","subject":"Re: [PATCH] midx: apply gitconfig to midx repack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-05T13:50:37Z","receivedAt":"2020-05-05T13:50:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/5/2020 9:06 AM, Son Luong Ngoc via GitGitGadget wrote:\n> From: Son Luong Ngoc \n> \n> Multi-Pack-Index repack is an incremental, repack solutions\n> that allows user to consolidate multiple packfiles in a non-disruptive\n> way. However the new packfile could be created without some of the\n> capabilities of a packfile that is created by calling `git repack`.\n> \n> This is because with `git repack`, there are configuration that would\n> enable different flags to be passed down to `git pack-objects` plumbing.\n> \n> In this patch, I applies those flags into `git multi-pack-index repack`\n> so that it respect the `repack.*` config series.\n\nThis is a good idea! The fact that these are specified by 'git repack'\nand not 'git pack-objects' makes intervention here necessary.\n\nHowever, I don't think that all of these will apply properly.\n\n> Note: I left out `repack.packKeptObjects` intentionally as I dont think\n> its relevant to midx repack use case.\n\nI think it would be good to add this, but in a different way.\n\n> Signed-off-by: Son Luong Ngoc \n> ---\n> midx: apply gitconfig to midx repack\n> \n> Midx repack has largely been used in Microsoft Scalar on the client side\n> to optimize the repository multiple packs state. However when I tried to\n> apply this onto the server-side, I realized that there are certain\n> features that were lacking compare to git repack. Most of these features\n> are highly desirable on the server-side to create the most optimized\n> pack possible.\n> \n> One of the example is delta_base_offset, comparing an midx repack\n> with/without delta_base_offset, we can observe significant size\n> differences.\n> \n> > du objects/pack/*pack\n> 14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack\n> 9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack\n> \n> Latest 2.26.2 (without delta_base_offset)\n> > git multi-pack-index write\n> > git multi-pack-index repack\n> > git multi-pack-index expire\n> > du objects/pack/*pack\n> 9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack\n> \n> With delta_base_offset\n> > git version\n> git version 2.26.2.672.g232c24e857.dirty\n> > git multi-pack-index write\n> > git multi-pack-index repack\n> > git multi-pack-index expire\n> > du objects/pack/*pack\n> 9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack\n> \n> In this patch, I intentionally leaving out repack.packKeptObjects as I\n> don't think its very relevant to midx repack use case:\n> \n> * One could always exclude biggest packs with --batch-size option\n> \n> \n> * For non-biggest-packs exclusion use case, its rather rare (unless you\n> want to have a special pack with only commits and trees being\n> excluded from repack to serve partial clone better?)\n> \n> \n> \n> Please let me know if anyone think that we should include that option\n> for the sake of completions.\n\nIn the scenario where there is a .keep pack _and_ it is small enough to get\npicked up by the batch size, the 'git multi-pack-index repack' command will\ncreate a new pack containing its objects (and objects from other packs) but\nthe 'git multi-pack-index expire' command will not delete the pack with .keep.\n\nThe good news is that after the first repack, the objects in the pack are\nin a newer pack, so the multi-pack-index will not repack those objects from\nthat pack multiple times. However, this may be unintended behavior for the\nuser that specified the .keep pack.\n\nI think the right thing to do to respect \"repack.packKeptObjects = false\" is\nto ignore the packs when selecting the batch of objects. Instead of asking\nyou to do this, I added a patch below. Please take it into your v2, if you\ndon't mind.\n \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/626\n> \n> midx.c | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n> \n> diff --git a/midx.c b/midx.c\n> index 9a61d3b37d9..88f16594268 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1361,6 +1361,10 @@ static int fill_included_packs_batch(struct repository *r,\n> \treturn 0;\n> }\n> \n> +static int delta_base_offset = 1;\n> +static int write_bitmaps = -1;\n> +static int use_delta_islands;\n> +\n\nWhy not make these local to the midx_repack method?\n\n> int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)\n> {\n> \tint result = 0;\n> @@ -1381,12 +1385,25 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n> \t} else if (fill_included_packs_all(m, include_pack))\n> \t\tgoto cleanup;\n> \n> + git_config_get_bool(\"repack.usedeltabaseoffset\", &delta_base_offset);\n> + git_config_get_bool(\"repack.writebitmaps\", &write_bitmaps);\n> + git_config_get_bool(\"repack.usedeltaislands\", &use_delta_islands);\n> +\n\nIt looks like you have some spacing issues here. Perhaps use tabs?\n\n> \targv_array_push(&cmd.args, \"pack-objects\");\n> \n> \tstrbuf_addstr(&base_name, object_dir);\n> \tstrbuf_addstr(&base_name, \"/pack/pack\");\n> \targv_array_push(&cmd.args, base_name.buf);\n> \n> +\tif (delta_base_offset)\n> +\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n> +\tif (use_delta_islands)\n> +\t\targv_array_push(&cmd.args, \"--delta-islands\");\n\nThese two probably make sense.\n\n> +\tif (write_bitmaps > 0)\n> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n> +\telse if (write_bitmaps < 0)\n> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n\nThese make less sense. Unless --batch-size=0 and there are no .keep\npacks (with the patch below) I'm not sure we _can_ write bitmap indexes\nhere. The pack-file is not necessarily closed under reachability. Or,\nwill supplying these arguments to 'git pack-objects' actually do that\nclosure?\n\nI would be happy to special-case these options to the \"--batch-size=0\"\nsituation and otherwise ignore them. This then gets into enough\ncomplication that we should update the documentation as in the patch\nbelow.\n\nAt minimum, it would be good to have some tests that exercise these\ncode paths so we know they are behaving correctly.\n\nThanks,\n-Stolee\n\n\n-- >8 --\nFrom 8a115191cbf21c553675a235c8c678affbca609b Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Tue, 5 May 2020 09:37:50 -0400\nSubject: [PATCH] multi-pack-index: respect repack.packKeptObjects=false\n\nWhen selecting a batch of pack-files to repack in the \"git\nmulti-pack-index repack\" command, Git should respect the\nrepack.packKeptObjects config option. When false, this option says that\nthe pack-files with an associated \".keep\" file should not be repacked.\nThis config value is \"false\" by default.\n\nThere are two cases for selecting a batch of objects. The first is the\ncase where the input batch-size is zero, which specifies \"repack\neverything\". The second is with a non-zero batch size, which selects\npack-files using a greedy selection criteria. Both of these cases are\nupdated and tested.\n\nReported-by: Son Luong Ngoc \nSigned-off-by: Derrick Stolee \n---\n Documentation/git-multi-pack-index.txt | 3 +++\n midx.c | 26 +++++++++++++++++++++-----\n t/t5319-multi-pack-index.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 50 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\nindex 642d9ac5b7..0c6619493c 100644\n--- a/Documentation/git-multi-pack-index.txt\n+++ b/Documentation/git-multi-pack-index.txt\n@@ -56,6 +56,9 @@ repack::\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++\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 \n \n EXAMPLES\ndiff --git a/midx.c b/midx.c\nindex 1527e464a7..d055bf3cd3 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1280,15 +1280,26 @@ static int compare_by_mtime(const void *a_, const void *b_)\n \treturn 0;\n }\n \n-static int fill_included_packs_all(struct multi_pack_index *m,\n+static int fill_included_packs_all(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n \t\t\t\t unsigned char *include_pack)\n {\n-\tuint32_t i;\n+\tuint32_t i, count = 0;\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n+\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tif (prepare_midx_pack(r, m, i))\n+\t\t\tcontinue;\n+\t\tif (!pack_kept_objects && m->packs[i]->pack_keep)\n+\t\t\tcontinue;\n \n-\tfor (i = 0; i < m->num_packs; i++)\n \t\tinclude_pack[i] = 1;\n+\t\tcount++;\n+\t}\n \n-\treturn m->num_packs < 2;\n+\treturn count < 2;\n }\n \n static int fill_included_packs_batch(struct repository *r,\n@@ -1299,6 +1310,9 @@ static int fill_included_packs_batch(struct repository *r,\n \tuint32_t i, packs_to_repack;\n \tsize_t total_size;\n \tstruct repack_info *pack_info = xcalloc(m->num_packs, sizeof(struct repack_info));\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tpack_info[i].pack_int_id = i;\n@@ -1325,6 +1339,8 @@ static int fill_included_packs_batch(struct repository *r,\n \n \t\tif (!p)\n \t\t\tcontinue;\n+\t\tif (!pack_kept_objects && p->pack_keep)\n+\t\t\tcontinue;\n \t\tif (open_pack_index(p) || !p->num_objects)\n \t\t\tcontinue;\n \n@@ -1365,7 +1381,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tif (batch_size) {\n \t\tif (fill_included_packs_batch(r, m, include_pack, batch_size))\n \t\t\tgoto cleanup;\n-\t} else if (fill_included_packs_all(m, include_pack))\n+\t} else if (fill_included_packs_all(r, m, include_pack))\n \t\tgoto cleanup;\n \n \targv_array_push(&cmd.args, \"pack-objects\");\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43a7a66c9d..b2fece5d3d 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -533,6 +533,32 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n \t)\n '\n \n+test_expect_success 'repack respects repack.packKeptObjects=false' '\n+\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n+\t(\n+\t\tcd dup &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n+\t\tfor keep in $(cat keep-list)\n+\t\tdo\n+\t\t\ttouch $keep || return 1\n+\t\tdone &&\n+\t\tgit multi-pack-index repack --batch-size=0 &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list &&\n+\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n+\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n+\t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list\n+\t)\n+'\n+\n test_expect_success 'repack creates a new pack' '\n \t(\n \t\tcd dup &&\n-- \n2.26.2.vfs.1.2\n\n\n"},{"id":"397104","messageId":"74A7FE73-6B5F-4DCF-9A57-AD11306CFAF8@gmail.com","threadId":"53402","inReplyTo":"8bd91a14-75dc-76e2-31b4-54eff5bea8dd@gmail.com","subject":"Re: [PATCH] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-05-05T16:03:33Z","receivedAt":"2020-05-05T16:03:37Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi Derrick,\n\nThanks for a swift and comprehensive review.\n\n> On May 5, 2020, at 15:50, Derrick Stolee wrote:\n> \n> In the scenario where there is a .keep pack _and_ it is small enough to get\n> picked up by the batch size, the 'git multi-pack-index repack' command will\n> create a new pack containing its objects (and objects from other packs) but\n> the 'git multi-pack-index expire' command will not delete the pack with .keep.\n> \n> The good news is that after the first repack, the objects in the pack are\n> in a newer pack, so the multi-pack-index will not repack those objects from\n> that pack multiple times. However, this may be unintended behavior for the\n> user that specified the .keep pack.\n\nYup I experienced exactly this when trying to test midx repack/expire\nwith biggest pack file marked with `.keep`.\nLuckily the storage size bump for duplicated objects was not noticeable in my case.\nYou worded the situation precisely.\n\n> I think the right thing to do to respect \"repack.packKeptObjects = false\" is\n> to ignore the packs when selecting the batch of objects. Instead of asking\n> you to do this, I added a patch below. Please take it into your v2, if you\n> don't mind.\n\nGladly.\nThis should help me a lot for re-rolling V2.\n\n>> +static int delta_base_offset = 1;\n>> +static int write_bitmaps = -1;\n>> +static int use_delta_islands;\n>> +\n> \n> Why not make these local to the midx_repack method?\n\nNo practical reason except me shamelessly lifted those from builtin/repack.c.\nI was a bit confused how `git repack` houses these logic in the builtin file,\nwhile midx was having these logic in the midx.c instead of builtin/multi-pack-index.c.\n\nI make them local in V2.\n\n>> int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)\n>> {\n>> \tint result = 0;\n>> @@ -1381,12 +1385,25 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n>> \t} else if (fill_included_packs_all(m, include_pack))\n>> \t\tgoto cleanup;\n>> \n>> + git_config_get_bool(\"repack.usedeltabaseoffset\", &delta_base_offset);\n>> + git_config_get_bool(\"repack.writebitmaps\", &write_bitmaps);\n>> + git_config_get_bool(\"repack.usedeltaislands\", &use_delta_islands);\n>> +\n> \n> It looks like you have some spacing issues here. Perhaps use tabs?\n\nRookie mistake on my part. Will fix it in V2\n\n>> +\tif (write_bitmaps > 0)\n>> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n>> +\telse if (write_bitmaps < 0)\n>> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n> \n> These make less sense. Unless --batch-size=0 and there are no .keep\n> packs (with the patch below) I'm not sure we _can_ write bitmap indexes\n> here. The pack-file is not necessarily closed under reachability. Or,\n> will supplying these arguments to 'git pack-objects' actually do that\n> closure?\n> \n> I would be happy to special-case these options to the \"--batch-size=0\"\n> situation and otherwise ignore them. This then gets into enough\n> complication that we should update the documentation as in the patch\n> below.\n\nYou make a great point here. \nI completely missed this as I have been largely testing with repacking only 2 packs,\neffectively with --batch-size=0.\n\nI think having the bitmaps index is highly desirable in `--batch-size=0` case.\nI will try to include that in V2 (with Documentation).\n\n> At minimum, it would be good to have some tests that exercise these\n> code paths so we know they are behaving correctly.\n\nI will do some readings with the current tests for repack and midx.\nHopefully I will have something for V2. (^_^ !)\n\n> Thanks,\n> -Stolee\n\nCheers,\nSon Luong\n\n"},{"id":"397175","messageId":"E80AD11E-4151-4B6D-988C-B91D8A93F6B6@gmail.com","threadId":"53402","inReplyTo":"74A7FE73-6B5F-4DCF-9A57-AD11306CFAF8@gmail.com","subject":"Re: [PATCH] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-05-06T08:56:43Z","receivedAt":"2020-05-06T08:56:49Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi,\n\n> On May 5, 2020, at 18:03, Son Luong Ngoc wrote:\n>> On May 5, 2020, at 15:50, Derrick Stolee wrote:\n>>> +\tif (write_bitmaps > 0)\n>>> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n>>> +\telse if (write_bitmaps < 0)\n>>> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n>> \n>> These make less sense. Unless --batch-size=0 and there are no .keep\n>> packs (with the patch below) I'm not sure we _can_ write bitmap indexes\n>> here. The pack-file is not necessarily closed under reachability. Or,\n>> will supplying these arguments to 'git pack-objects' actually do that\n>> closure?\n>> \n>> I would be happy to special-case these options to the \"--batch-size=0\"\n>> situation and otherwise ignore them. This then gets into enough\n>> complication that we should update the documentation as in the patch\n>> below.\n> \n> You make a great point here. \n> I completely missed this as I have been largely testing with repacking only 2 packs,\n> effectively with --batch-size=0.\n> \n> I think having the bitmaps index is highly desirable in `--batch-size=0` case.\n> I will try to include that in V2 (with Documentation).\n\nHmm, I just realized that there is a check for `--all` is being passed on pack-objects side.\n\n\tif (batch_size == 0) {\n\t\targv_array_push(&cmd.args, \"--all\");\n\t\tif (write_bitmaps > 0)\n\t\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n\t\telse if (write_bitmaps < 0)\n\t\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n\t}\n\nIf I do something like this, the midx repack will become tremendously slow as I think pack-objects\nneeds to scan for all revs (fed from midx) and all refs.\nPerhaps special exception needed to be made on pack-objects side to trust that midx is feeding it\neverything there is?\n\nI think adding `write_bitmaps` support would be a bit out of my hand for now, so I will settle with\nthe delta configs and Derrick's patch for V2. (sending it later today)\n\n>> Thanks,\n>> -Stolee\n> \n> Cheers,\n> Son Luong\n> \n\n"},{"id":"397177","messageId":"pull.626.v2.git.1588758194.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.git.1588684003766.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-06T09:43:12Z","receivedAt":"2020-05-06T09:43:18Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Midx repack has largely been used in Microsoft Scalar on the client side to\noptimize the repository multiple packs state. However when I tried to apply\nthis onto the server-side, I realized that there are certain features that\nwere lacking compare to git repack. Most of these features are highly\ndesirable on the server-side to create the most optimized pack possible.\n\nOne of the example is delta_base_offset, comparing an midx repack\nwith/without delta_base_offset, we can observe significant size differences.\n\n> du objects/pack/*pack\n14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack\n9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack\n\nLatest 2.26.2 (without delta_base_offset)\n> git multi-pack-index write\n> git multi-pack-index repack\n> git multi-pack-index expire\n> du objects/pack/*pack\n9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack\n\nWith delta_base_offset\n> git version\ngit version 2.26.2.672.g232c24e857.dirty\n> git multi-pack-index write\n> git multi-pack-index repack\n> git multi-pack-index expire\n> du objects/pack/*pack\n9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack\n\nIn this patch, I intentionally leaving out repack.writeBitmaps as I see that\nit might need some update on pack-objects to improve the performance\n\nDerrick Stolee following patch with address repack. packKeptObjects support.\n\nDerrick Stolee (1):\n multi-pack-index: respect repack.packKeptObjects=false\n\nSon Luong Ngoc (1):\n midx: apply gitconfig to midx repack\n\n Documentation/git-multi-pack-index.txt | 3 +++\n midx.c | 36 ++++++++++++++++++++++----\n t/t5319-multi-pack-index.sh | 26 +++++++++++++++++++\n 3 files changed, 60 insertions(+), 5 deletions(-)\n\n\nbase-commit: b34789c0b0d3b137f0bb516b417bd8d75e0cb306\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/626\n\nRange-diff vs v1:\n\n 1: 215c882a503 ! 1: 21c648cc486 midx: apply gitconfig to midx repack\n @@ Commit message\n In this patch, I applies those flags into `git multi-pack-index repack`\n so that it respect the `repack.*` config series.\n \n - Note: I left out `repack.packKeptObjects` intentionally as I dont think\n - its relevant to midx repack use case.\n + Note:\n + - `repack.packKeptObjects` will be addressed by Derrick Stolee in\n + the following patch\n + - `repack.writeBitmaps` when `--batch-size=0` was NOT adopted here as it\n + requires `--all` to be passed onto `git pack-objects`, which is very\n + slow. I think it would be nice to have this in a future patch.\n \n Signed-off-by: Son Luong Ngoc \n \n ## midx.c ##\n -@@ midx.c: static int fill_included_packs_batch(struct repository *r,\n - \treturn 0;\n - }\n +@@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n + \tstruct child_process cmd = CHILD_PROCESS_INIT;\n + \tstruct strbuf base_name = STRBUF_INIT;\n + \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n ++\tint delta_base_offset = 1;\n ++\tint use_delta_islands;\n \n -+static int delta_base_offset = 1;\n -+static int write_bitmaps = -1;\n -+static int use_delta_islands;\n -+\n - int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)\n - {\n - \tint result = 0;\n + \tif (!m)\n + \t\treturn 0;\n @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t} else if (fill_included_packs_all(m, include_pack))\n \t\tgoto cleanup;\n \n -+ git_config_get_bool(\"repack.usedeltabaseoffset\", &delta_base_offset);\n -+ git_config_get_bool(\"repack.writebitmaps\", &write_bitmaps);\n -+ git_config_get_bool(\"repack.usedeltaislands\", &use_delta_islands);\n ++\trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n ++\trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n +\n \targv_array_push(&cmd.args, \"pack-objects\");\n \n @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat\n +\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n +\tif (use_delta_islands)\n +\t\targv_array_push(&cmd.args, \"--delta-islands\");\n -+\tif (write_bitmaps > 0)\n -+\t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n -+\telse if (write_bitmaps < 0)\n -+\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n +\n \tif (flags & MIDX_PROGRESS)\n \t\targv_array_push(&cmd.args, \"--progress\");\n -: ----------- > 2: 3d7b334f5c6 multi-pack-index: respect repack.packKeptObjects=false\n\n-- \ngitgitgadget\n"},{"id":"397176","messageId":"21c648cc486cf1abee51076d21e55649b1464516.1588758194.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v2.git.1588758194.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-06T09:43:13Z","receivedAt":"2020-05-06T09:43:19Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"From: Son Luong Ngoc \n\nMulti-Pack-Index repack is an incremental, repack solutions\nthat allows user to consolidate multiple packfiles in a non-disruptive\nway. However the new packfile could be created without some of the\ncapabilities of a packfile that is created by calling `git repack`.\n\nThis is because with `git repack`, there are configuration that would\nenable different flags to be passed down to `git pack-objects` plumbing.\n\nIn this patch, I applies those flags into `git multi-pack-index repack`\nso that it respect the `repack.*` config series.\n\nNote:\n- `repack.packKeptObjects` will be addressed by Derrick Stolee in\nthe following patch\n- `repack.writeBitmaps` when `--batch-size=0` was NOT adopted here as it\nrequires `--all` to be passed onto `git pack-objects`, which is very\nslow. I think it would be nice to have this in a future patch.\n\nSigned-off-by: Son Luong Ngoc \n---\n midx.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 9a61d3b37d9..3348f8e569b 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1369,6 +1369,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n+\tint delta_base_offset = 1;\n+\tint use_delta_islands;\n \n \tif (!m)\n \t\treturn 0;\n@@ -1381,12 +1383,20 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t} else if (fill_included_packs_all(m, include_pack))\n \t\tgoto cleanup;\n \n+\trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n+\trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n+\n \targv_array_push(&cmd.args, \"pack-objects\");\n \n \tstrbuf_addstr(&base_name, object_dir);\n \tstrbuf_addstr(&base_name, \"/pack/pack\");\n \targv_array_push(&cmd.args, base_name.buf);\n \n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n+\tif (use_delta_islands)\n+\t\targv_array_push(&cmd.args, \"--delta-islands\");\n+\n \tif (flags & MIDX_PROGRESS)\n \t\targv_array_push(&cmd.args, \"--progress\");\n \telse\n-- \ngitgitgadget\n\n"},{"id":"397178","messageId":"3d7b334f5c6a89f438bba34cf91259cb67aebcd0.1588758194.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v2.git.1588758194.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-06T09:43:14Z","receivedAt":"2020-05-06T09:43:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen selecting a batch of pack-files to repack in the \"git\nmulti-pack-index repack\" command, Git should respect the\nrepack.packKeptObjects config option. When false, this option says that\nthe pack-files with an associated \".keep\" file should not be repacked.\nThis config value is \"false\" by default.\n\nThere are two cases for selecting a batch of objects. The first is the\ncase where the input batch-size is zero, which specifies \"repack\neverything\". The second is with a non-zero batch size, which selects\npack-files using a greedy selection criteria. Both of these cases are\nupdated and tested.\n\nReported-by: Son Luong Ngoc \nSigned-off-by: Derrick Stolee \n---\n Documentation/git-multi-pack-index.txt | 3 +++\n midx.c | 26 +++++++++++++++++++++-----\n t/t5319-multi-pack-index.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 50 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\nindex 642d9ac5b72..0c6619493c1 100644\n--- a/Documentation/git-multi-pack-index.txt\n+++ b/Documentation/git-multi-pack-index.txt\n@@ -56,6 +56,9 @@ repack::\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++\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 \n \n EXAMPLES\ndiff --git a/midx.c b/midx.c\nindex 3348f8e569b..b8a52740832 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1293,15 +1293,26 @@ static int compare_by_mtime(const void *a_, const void *b_)\n \treturn 0;\n }\n \n-static int fill_included_packs_all(struct multi_pack_index *m,\n+static int fill_included_packs_all(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n \t\t\t\t unsigned char *include_pack)\n {\n-\tuint32_t i;\n+\tuint32_t i, count = 0;\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n+\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tif (prepare_midx_pack(r, m, i))\n+\t\t\tcontinue;\n+\t\tif (!pack_kept_objects && m->packs[i]->pack_keep)\n+\t\t\tcontinue;\n \n-\tfor (i = 0; i < m->num_packs; i++)\n \t\tinclude_pack[i] = 1;\n+\t\tcount++;\n+\t}\n \n-\treturn m->num_packs < 2;\n+\treturn count < 2;\n }\n \n static int fill_included_packs_batch(struct repository *r,\n@@ -1312,6 +1323,9 @@ static int fill_included_packs_batch(struct repository *r,\n \tuint32_t i, packs_to_repack;\n \tsize_t total_size;\n \tstruct repack_info *pack_info = xcalloc(m->num_packs, sizeof(struct repack_info));\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tpack_info[i].pack_int_id = i;\n@@ -1338,6 +1352,8 @@ static int fill_included_packs_batch(struct repository *r,\n \n \t\tif (!p)\n \t\t\tcontinue;\n+\t\tif (!pack_kept_objects && p->pack_keep)\n+\t\t\tcontinue;\n \t\tif (open_pack_index(p) || !p->num_objects)\n \t\t\tcontinue;\n \n@@ -1380,7 +1396,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tif (batch_size) {\n \t\tif (fill_included_packs_batch(r, m, include_pack, batch_size))\n \t\t\tgoto cleanup;\n-\t} else if (fill_included_packs_all(m, include_pack))\n+\t} else if (fill_included_packs_all(r, m, include_pack))\n \t\tgoto cleanup;\n \n \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 030a7222b2a..67afe1bb8d9 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -538,6 +538,32 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n \t)\n '\n \n+test_expect_success 'repack respects repack.packKeptObjects=false' '\n+\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n+\t(\n+\t\tcd dup &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n+\t\tfor keep in $(cat keep-list)\n+\t\tdo\n+\t\t\ttouch $keep || return 1\n+\t\tdone &&\n+\t\tgit multi-pack-index repack --batch-size=0 &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list &&\n+\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n+\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n+\t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list\n+\t)\n+'\n+\n test_expect_success 'repack creates a new pack' '\n \t(\n \t\tcd dup &&\n-- \ngitgitgadget\n"},{"id":"397180","messageId":"e991e5af-83bc-d868-473e-54ece3489a7e@gmail.com","threadId":"53402","inReplyTo":"21c648cc486cf1abee51076d21e55649b1464516.1588758194.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] midx: apply gitconfig to midx repack","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-06T12:03:34Z","receivedAt":"2020-05-06T12:03:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/6/2020 5:43 AM, Son Luong Ngoc via GitGitGadget wrote:\n> From: Son Luong Ngoc \n...\n> - `repack.writeBitmaps` when `--batch-size=0` was NOT adopted here as it\n> requires `--all` to be passed onto `git pack-objects`, which is very\n> slow. I think it would be nice to have this in a future patch.\n\nJust my two cents here: the reachability bitmaps are really tied to the\nidea of a single pack right now. To create bitmaps, I would currently\nsuggest using the 'git repack' builtin with the proper options. That\ncommand deletes the multi-pack-index, unfortunately, but it also produces\na single pack and deletes the others (when creating bitmaps).\n\nYou are right that the `--all` option required to pack-objects is not\nappropriate to add inside `git multi-pack-index repack` as that changes\nthe pattern. It requires loading all reachable objects, even if they are\nnot already in packs covered by the multi-pack-index. This at minimum\nviolates expectations with the --batch-size argument.\n\nIntegrating reachability bitmaps more closely with the multi-pack-index\nis certainly on our radar, but is a large endeavor.\n\nThis new patch looks good to me.\n\nThanks,\n-Stolee\n"},{"id":"397194","messageId":"CAPig+cSBBVjBs6ypcpk=s+j2Vu4OXbhUnrJPq8tyoCDr+hX4rw@mail.gmail.com","threadId":"53402","inReplyTo":"3d7b334f5c6a89f438bba34cf91259cb67aebcd0.1588758194.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-06T16:18:29Z","receivedAt":"2020-05-06T16:18:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 6, 2020 at 5:44 AM Derrick Stolee via GitGitGadget\n wrote:\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> @@ -538,6 +538,32 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n> +test_expect_success 'repack respects repack.packKeptObjects=false' '\n> + test_when_finished rm -f dup/.git/objects/pack/*keep &&\n> + (\n> + [...]\n> + THIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n> + BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n\nTaking jk/arith-expansion-coding-guidelines[1] into consideration,\nperhaps write this as:\n\n BATCH_SIZE=$((THIRD_SMALLEST_SIZE + 1)) &&\n\n[1]: https://lore.kernel.org/git/20200504160709.GB12842@coredump.intra.peff.net/\n"},{"id":"397204","messageId":"a9ceac1c-8609-74b3-f40a-6d9e68574cd8@gmail.com","threadId":"53402","inReplyTo":"CAPig+cSBBVjBs6ypcpk=s+j2Vu4OXbhUnrJPq8tyoCDr+hX4rw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-06T16:36:48Z","receivedAt":"2020-05-06T16:36:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/6/2020 12:18 PM, Eric Sunshine wrote:\n> On Wed, May 6, 2020 at 5:44 AM Derrick Stolee via GitGitGadget\n> wrote:\n>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n>> @@ -538,6 +538,32 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n>> +test_expect_success 'repack respects repack.packKeptObjects=false' '\n>> + test_when_finished rm -f dup/.git/objects/pack/*keep &&\n>> + (\n>> + [...]\n>> + THIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n>> + BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n> \n> Taking jk/arith-expansion-coding-guidelines[1] into consideration,\n> perhaps write this as:\n> \n> BATCH_SIZE=$((THIRD_SMALLEST_SIZE + 1)) &&\n\nThanks for pointing this out. This line is repeated in the test\nafter this one. That should be fixed, too.\n\nThanks,\n-Stolee\n\n"},{"id":"397205","messageId":"xmqqy2q56lo7.fsf@gitster.c.googlers.com","threadId":"53402","inReplyTo":"21c648cc486cf1abee51076d21e55649b1464516.1588758194.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] midx: apply gitconfig to midx repack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-06T17:03:04Z","receivedAt":"2020-05-06T17:03:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Son Luong Ngoc via GitGitGadget\" writes:\n\n> Multi-Pack-Index repack is an incremental, repack solutions\n> that allows user to consolidate multiple packfiles in a non-disruptive\n> way. However the new packfile could be created without some of the\n> capabilities of a packfile that is created by calling `git repack`.\n\nIt may be clear to you who wrote the patch, but it is quite unclear\nto readers how `repack` gets into the picture. The first sentence\ntalks about what \"git multi-pack-index repack\" subcommand. Unless\nyou mention that that \"git multi-pack-index repack\" subcommand calls\n\"git repack\" under the hood in order to create a new packfile, the\nsecond paragraph can be read as if you are pointing out a problem if\nthe user did\n\n\t$ git multi-pack-index repack\n\t$ git repack\n\nand the explicit \"repack\" initiated by the user may create a\npackfile that is somehow incompatible with what the previous repack\nwanted to do, or something like that.\n\n> This is because with `git repack`, there are configuration that would\n> enable different flags to be passed down to `git pack-objects` plumbing.\n\nAnd this does not help to clear the possible confusion, either.\n\nI think all of the above is clearer if you rewrite the above\n(including the title) like so:\n\n midx: teach \"git multi-pack-index repack\" honor \"git repack\" configuration\n\n When the \"repack\" subcommand of \"git multi-pack-index\" command\n creates new packfile(s), it does not call the \"git repack\"\n command but instead directly calls the \"git pack-objects\"\n command, and the configuration variables meant for the \"git\n repack\" command, like \"repack.usedaeltabaseoffset\", are ignored.\n\nNow the problem description is behind us, let's see the description\nof proposed solution. We write this part in imperative mood, as if\nwe are giving an order to the codebase to \"become like so\". We do\nnot say \"I do X, I do Y\".\n\n> In this patch, I applies those flags into `git multi-pack-index repack`\n> so that it respect the `repack.*` config series.\n\n Check the configuration variables used by \"git repack\" ourselves\n and pass the corresponding options to underlying \"git pack-objects\"\n in this codepath.\n\n\n> Note:\n> - `repack.packKeptObjects` will be addressed by Derrick Stolee in\n> the following patch\n\nThis definitely does not belong to the commit log message. It would\nmake a helpful note meant for the reviewers if written below the\nthree-dash line, though.\n\n> - `repack.writeBitmaps` when `--batch-size=0` was NOT adopted here as it\n> requires `--all` to be passed onto `git pack-objects`, which is very\n> slow. I think it would be nice to have this in a future patch.\n\nThe phrasing makes it hard to grok. Do you want to say that the\nrepack.writeBitmaps configuration variable is ignored?\n\nI think Derrick gave you the reason why bitmaps is not compatible\nwith midx in general, and that would be a better rationale to record\nwhy the configuration is ignored. Perhaps like\n\n Note that `repack.writeBitmaps` configuration is ignored, as the\n pack bitmap faciility is useful only with a single packfile.\n\nor something like that?\n\nDo we need to worry about the configuration variables understood by\nthe \"git pack-objects\" command to get in the way, by the way?\n\"pack.packsizelimit\" may cause \"git repack\" to produce more than one\npackfile, and if this codepath wants to avoid it (I do not know if\nthat is the case), it may have to override it from the command line,\nfor example.\n\n> Signed-off-by: Son Luong Ngoc \n> ---\n> midx.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/midx.c b/midx.c\n> index 9a61d3b37d9..3348f8e569b 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1369,6 +1369,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \tstruct strbuf base_name = STRBUF_INIT;\n> \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n> +\tint delta_base_offset = 1;\n\nBy default we use delta-base-offset, so if repo_config_get_bool()\ndid not see the repack.usedeltabaseoffset configuration defined in\nany configuration file, we still want to see 1 after it returns.\n\n> +\tint use_delta_islands;\n\nWhat is the reason why it is safe to leave this uninitialized? Did\nyou mean \n\n\tint use_delta_islands = 0;\n\nhere?\n\n> @@ -1381,12 +1383,20 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n> \t} else if (fill_included_packs_all(m, include_pack))\n> \t\tgoto cleanup;\n> \n> +\trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n> +\trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n> +\n> \targv_array_push(&cmd.args, \"pack-objects\");\n> \n> \tstrbuf_addstr(&base_name, object_dir);\n> \tstrbuf_addstr(&base_name, \"/pack/pack\");\n> \targv_array_push(&cmd.args, base_name.buf);\n> \n> +\tif (delta_base_offset)\n> +\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n> +\tif (use_delta_islands)\n> +\t\targv_array_push(&cmd.args, \"--delta-islands\");\n> +\n\nThese look like good changes.\n\n> \tif (flags & MIDX_PROGRESS)\n> \t\targv_array_push(&cmd.args, \"--progress\");\n> \telse\n\nThanks.\n"},{"id":"397263","messageId":"696D63FD-5AE2-41DB-8CF8-D81AB834EA45@gmail.com","threadId":"53402","inReplyTo":"xmqqy2q56lo7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-05-07T07:29:24Z","receivedAt":"2020-05-07T07:29:31Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi Junio,\n\nThanks for the feedbacks\n\n> On May 6, 2020, at 19:03, Junio C Hamano wrote:\n\n...\n> We write this part in imperative mood, as if\n> we are giving an order to the codebase to \"become like so\". We do\n> not say \"I do X, I do Y\".\n\nThis is a great feedback.\nI will try to include all of your suggestions and edit the message\nbefore submitting V3.\n\n>> Note:\n>> - `repack.packKeptObjects` will be addressed by Derrick Stolee in\n>> the following patch\n> \n> This definitely does not belong to the commit log message. It would\n> make a helpful note meant for the reviewers if written below the\n> three-dash line, though.\n\nDuly noted.\n\n> Do we need to worry about the configuration variables understood by\n> the \"git pack-objects\" command to get in the way, by the way?\n> \"pack.packsizelimit\" may cause \"git repack\" to produce more than one\n> packfile, and if this codepath wants to avoid it (I do not know if\n> that is the case), it may have to override it from the command line,\n> for example.\n\nI dont think we want to avoid the packsizelimit here.\nThe point of repacking with midx is to help\nend users consolidate multiple packfile in a non-disruptive way.\n\nIf you wish to put a constraint (i.e. packsizelimit, packKeptObjects) on this process,\nyou should be able to.\n\n>> Signed-off-by: Son Luong Ngoc \n>> ---\n>> midx.c | 10 ++++++++++\n>> 1 file changed, 10 insertions(+)\n>> \n>> diff --git a/midx.c b/midx.c\n>> index 9a61d3b37d9..3348f8e569b 100644\n>> --- a/midx.c\n>> +++ b/midx.c\n>> @@ -1369,6 +1369,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n>> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n>> \tstruct strbuf base_name = STRBUF_INIT;\n>> \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n>> +\tint delta_base_offset = 1;\n> \n> By default we use delta-base-offset, so if repo_config_get_bool()\n> did not see the repack.usedeltabaseoffset configuration defined in\n> any configuration file, we still want to see 1 after it returns.\n> \n>> +\tint use_delta_islands;\n> \n> What is the reason why it is safe to leave this uninitialized? Did\n> you mean \n> \n> \tint use_delta_islands = 0;\n> \n> here?\n\nI think I totally misread how repo_config_get_bool() supposed to work\nYour comment here made me re-read it and things got a lot clearer.\n\nWill set the default value to 0 in next version.\n\n> Thanks.\n\nMuch appreciate,\nSon Luong."},{"id":"397451","messageId":"pull.626.v3.git.1589034270.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v2.git.1588758194.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-09T14:24:27Z","receivedAt":"2020-05-09T14:24:35Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Midx repack has largely been used in Microsoft Scalar on the client side to\noptimize the repository multiple packs state. However when I tried to apply\nthis onto the server-side, I realized that there are certain features that\nwere lacking compare to git repack. Most of these features are highly\ndesirable on the server-side to create the most optimized pack possible.\n\nOne of the example is delta_base_offset, comparing an midx repack\nwith/without delta_base_offset, we can observe significant size differences.\n\n> du objects/pack/*pack\n14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack\n9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack\n\nLatest 2.26.2 (without delta_base_offset)\n> git multi-pack-index write\n> git multi-pack-index repack\n> git multi-pack-index expire\n> du objects/pack/*pack\n9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack\n\nWith delta_base_offset\n> git version\ngit version 2.26.2.672.g232c24e857.dirty\n> git multi-pack-index write\n> git multi-pack-index repack\n> git multi-pack-index expire\n> du objects/pack/*pack\n9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack\n\nNote that repack.writeBitmaps configuration is ignored, as the pack bitmap\nfacility is useful only with a single packfile.\n\nDerrick Stolee's following patch will address repack.packKeptObjects \nsupport.\n\nDerrick Stolee (1):\n multi-pack-index: respect repack.packKeptObjects=false\n\nSon Luong Ngoc (2):\n midx: teach \"git multi-pack-index repack\" honor \"git repack\"\n configurations\n Ensured t5319 follows arith expansion guideline\n\n Documentation/git-multi-pack-index.txt | 3 ++\n midx.c | 36 ++++++++++++---\n t/t5319-multi-pack-index.sh | 62 ++++++++++++++++++--------\n 3 files changed, 78 insertions(+), 23 deletions(-)\n\n\nbase-commit: b994622632154fc3b17fb40a38819ad954a5fb88\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/626\n\nRange-diff vs v2:\n\n 1: 21c648cc486 ! 1: a925307d4c5 midx: apply gitconfig to midx repack\n @@ Metadata\n Author: Son Luong Ngoc \n \n ## Commit message ##\n - midx: apply gitconfig to midx repack\n + midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations\n \n - Multi-Pack-Index repack is an incremental, repack solutions\n - that allows user to consolidate multiple packfiles in a non-disruptive\n - way. However the new packfile could be created without some of the\n - capabilities of a packfile that is created by calling `git repack`.\n + Previously, when the \"repack\" subcommand of \"git multi-pack-index\" command\n + creates new packfile(s), it does not call the \"git repack\" command but\n + instead directly calls the \"git pack-objects\" command, and the\n + configuration variables meant for the \"git repack\" command, like\n + \"repack.usedaeltabaseoffset\", are ignored.\n \n - This is because with `git repack`, there are configuration that would\n - enable different flags to be passed down to `git pack-objects` plumbing.\n + This patch ensured \"git multi-pack-index\" checks the configuration\n + variables used by \"git repack\" and passes the corresponding options to\n + the underlying \"git pack-objects\" command.\n \n - In this patch, I applies those flags into `git multi-pack-index repack`\n - so that it respect the `repack.*` config series.\n -\n - Note:\n - - `repack.packKeptObjects` will be addressed by Derrick Stolee in\n - the following patch\n - - `repack.writeBitmaps` when `--batch-size=0` was NOT adopted here as it\n - requires `--all` to be passed onto `git pack-objects`, which is very\n - slow. I think it would be nice to have this in a future patch.\n + Note that `repack.writeBitmaps` configuration is ignored, as the\n + pack bitmap facility is useful only with a single packfile.\n \n Signed-off-by: Son Luong Ngoc \n \n @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n +\tint delta_base_offset = 1;\n -+\tint use_delta_islands;\n ++\tint use_delta_islands = 0;\n \n \tif (!m)\n \t\treturn 0;\n 2: 3d7b334f5c6 = 2: 988697dd512 multi-pack-index: respect repack.packKeptObjects=false\n -: ----------- > 3: efeb3d7d132 Ensured t5319 follows arith expansion guideline\n\n-- \ngitgitgadget\n"},{"id":"397452","messageId":"a925307d4c57506f5236e60dc1390998e186cf26.1589034270.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v3.git.1589034270.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-09T14:24:28Z","receivedAt":"2020-05-09T14:24:35Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"From: Son Luong Ngoc \n\nPreviously, when the \"repack\" subcommand of \"git multi-pack-index\" command\ncreates new packfile(s), it does not call the \"git repack\" command but\ninstead directly calls the \"git pack-objects\" command, and the\nconfiguration variables meant for the \"git repack\" command, like\n\"repack.usedaeltabaseoffset\", are ignored.\n\nThis patch ensured \"git multi-pack-index\" checks the configuration\nvariables used by \"git repack\" and passes the corresponding options to\nthe underlying \"git pack-objects\" command.\n\nNote that `repack.writeBitmaps` configuration is ignored, as the\npack bitmap facility is useful only with a single packfile.\n\nSigned-off-by: Son Luong Ngoc \n---\n midx.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 9a61d3b37d9..1e76be56826 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1369,6 +1369,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n+\tint delta_base_offset = 1;\n+\tint use_delta_islands = 0;\n \n \tif (!m)\n \t\treturn 0;\n@@ -1381,12 +1383,20 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t} else if (fill_included_packs_all(m, include_pack))\n \t\tgoto cleanup;\n \n+\trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n+\trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n+\n \targv_array_push(&cmd.args, \"pack-objects\");\n \n \tstrbuf_addstr(&base_name, object_dir);\n \tstrbuf_addstr(&base_name, \"/pack/pack\");\n \targv_array_push(&cmd.args, base_name.buf);\n \n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n+\tif (use_delta_islands)\n+\t\targv_array_push(&cmd.args, \"--delta-islands\");\n+\n \tif (flags & MIDX_PROGRESS)\n \t\targv_array_push(&cmd.args, \"--progress\");\n \telse\n-- \ngitgitgadget\n\n"},{"id":"397453","messageId":"efeb3d7d1321e53e05079f296a5db5ab87f5fab2.1589034270.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v3.git.1589034270.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] Ensured t5319 follows arith expansion guideline","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-09T14:24:30Z","receivedAt":"2020-05-09T14:24:37Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"From: Son Luong Ngoc \n\nAs the old versions of dash is deprecated, dollar-sign inside\nartihmetic expansion is no longer needed.\nThis ensures t5319 follows the coding guideline updated\nin 'jk/arith-expansion-coding-guidelines' 6d4bf5813cd2c1a3b93fd4f0b231733f82133cce.\n\nReported-by: Eric Sunshine \nSigned-off-by: Son Luong Ngoc \n---\n t/t5319-multi-pack-index.sh | 38 ++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 67afe1bb8d9..065f48747f3 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -62,8 +62,8 @@ generate_objects () {\n \t} >wide_delta_$iii &&\n \t{\n \t\ttest-tool genrandom \"foo\"$i 100 &&\n-\t\ttest-tool genrandom \"foo\"$(( $i + 1 )) 100 &&\n-\t\ttest-tool genrandom \"foo\"$(( $i + 2 )) 100\n+\t\ttest-tool genrandom \"foo\"$(( i + 1 )) 100 &&\n+\t\ttest-tool genrandom \"foo\"$(( i + 2 )) 100\n \t} >deep_delta_$iii &&\n \t{\n \t\techo $iii &&\n@@ -251,21 +251,21 @@ MIDX_BYTE_OID_VERSION=5\n MIDX_BYTE_CHUNK_COUNT=6\n MIDX_HEADER_SIZE=12\n MIDX_BYTE_CHUNK_ID=$MIDX_HEADER_SIZE\n-MIDX_BYTE_CHUNK_OFFSET=$(($MIDX_HEADER_SIZE + 4))\n+MIDX_BYTE_CHUNK_OFFSET=$((MIDX_HEADER_SIZE + 4))\n MIDX_NUM_CHUNKS=5\n MIDX_CHUNK_LOOKUP_WIDTH=12\n-MIDX_OFFSET_PACKNAMES=$(($MIDX_HEADER_SIZE + \\\n-\t\t\t $MIDX_NUM_CHUNKS * $MIDX_CHUNK_LOOKUP_WIDTH))\n-MIDX_BYTE_PACKNAME_ORDER=$(($MIDX_OFFSET_PACKNAMES + 2))\n-MIDX_OFFSET_OID_FANOUT=$(($MIDX_OFFSET_PACKNAMES + $(test_oid packnameoff)))\n+MIDX_OFFSET_PACKNAMES=$((MIDX_HEADER_SIZE + \\\n+\t\t\t MIDX_NUM_CHUNKS * MIDX_CHUNK_LOOKUP_WIDTH))\n+MIDX_BYTE_PACKNAME_ORDER=$((MIDX_OFFSET_PACKNAMES + 2))\n+MIDX_OFFSET_OID_FANOUT=$((MIDX_OFFSET_PACKNAMES + $(test_oid packnameoff)))\n MIDX_OID_FANOUT_WIDTH=4\n-MIDX_BYTE_OID_FANOUT_ORDER=$((MIDX_OFFSET_OID_FANOUT + 250 * $MIDX_OID_FANOUT_WIDTH + $(test_oid fanoutoff)))\n-MIDX_OFFSET_OID_LOOKUP=$(($MIDX_OFFSET_OID_FANOUT + 256 * $MIDX_OID_FANOUT_WIDTH))\n-MIDX_BYTE_OID_LOOKUP=$(($MIDX_OFFSET_OID_LOOKUP + 16 * $HASH_LEN))\n-MIDX_OFFSET_OBJECT_OFFSETS=$(($MIDX_OFFSET_OID_LOOKUP + $NUM_OBJECTS * $HASH_LEN))\n+MIDX_BYTE_OID_FANOUT_ORDER=$((MIDX_OFFSET_OID_FANOUT + 250 * MIDX_OID_FANOUT_WIDTH + $(test_oid fanoutoff)))\n+MIDX_OFFSET_OID_LOOKUP=$((MIDX_OFFSET_OID_FANOUT + 256 * MIDX_OID_FANOUT_WIDTH))\n+MIDX_BYTE_OID_LOOKUP=$((MIDX_OFFSET_OID_LOOKUP + 16 * HASH_LEN))\n+MIDX_OFFSET_OBJECT_OFFSETS=$((MIDX_OFFSET_OID_LOOKUP + NUM_OBJECTS * HASH_LEN))\n MIDX_OFFSET_WIDTH=8\n-MIDX_BYTE_PACK_INT_ID=$(($MIDX_OFFSET_OBJECT_OFFSETS + 16 * $MIDX_OFFSET_WIDTH + 2))\n-MIDX_BYTE_OFFSET=$(($MIDX_OFFSET_OBJECT_OFFSETS + 16 * $MIDX_OFFSET_WIDTH + 6))\n+MIDX_BYTE_PACK_INT_ID=$((MIDX_OFFSET_OBJECT_OFFSETS + 16 * MIDX_OFFSET_WIDTH + 2))\n+MIDX_BYTE_OFFSET=$((MIDX_OFFSET_OBJECT_OFFSETS + 16 * MIDX_OFFSET_WIDTH + 6))\n \n test_expect_success 'verify bad version' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_VERSION \"\\00\" $objdir \\\n@@ -417,10 +417,10 @@ test_expect_success 'verify multi-pack-index with 64-bit offsets' '\n \n NUM_OBJECTS=63\n MIDX_OFFSET_OID_FANOUT=$((MIDX_OFFSET_PACKNAMES + 54))\n-MIDX_OFFSET_OID_LOOKUP=$((MIDX_OFFSET_OID_FANOUT + 256 * $MIDX_OID_FANOUT_WIDTH))\n-MIDX_OFFSET_OBJECT_OFFSETS=$(($MIDX_OFFSET_OID_LOOKUP + $NUM_OBJECTS * $HASH_LEN))\n-MIDX_OFFSET_LARGE_OFFSETS=$(($MIDX_OFFSET_OBJECT_OFFSETS + $NUM_OBJECTS * $MIDX_OFFSET_WIDTH))\n-MIDX_BYTE_LARGE_OFFSET=$(($MIDX_OFFSET_LARGE_OFFSETS + 3))\n+MIDX_OFFSET_OID_LOOKUP=$((MIDX_OFFSET_OID_FANOUT + 256 * MIDX_OID_FANOUT_WIDTH))\n+MIDX_OFFSET_OBJECT_OFFSETS=$((MIDX_OFFSET_OID_LOOKUP + NUM_OBJECTS * HASH_LEN))\n+MIDX_OFFSET_LARGE_OFFSETS=$((MIDX_OFFSET_OBJECT_OFFSETS + NUM_OBJECTS * MIDX_OFFSET_WIDTH))\n+MIDX_BYTE_LARGE_OFFSET=$((MIDX_OFFSET_LARGE_OFFSETS + 3))\n \n test_expect_success 'verify incorrect 64-bit offset' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_LARGE_OFFSET \"\\07\" objects64 \\\n@@ -555,7 +555,7 @@ test_expect_success 'repack respects repack.packKeptObjects=false' '\n \t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n \t\ttest_line_count = 5 midx-list &&\n \t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n-\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n+\t\tBATCH_SIZE=$((THIRD_SMALLEST_SIZE + 1)) &&\n \t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n \t\tls .git/objects/pack/*idx >idx-list &&\n \t\ttest_line_count = 5 idx-list &&\n@@ -570,7 +570,7 @@ test_expect_success 'repack creates a new pack' '\n \t\tls .git/objects/pack/*idx >idx-list &&\n \t\ttest_line_count = 5 idx-list &&\n \t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n-\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n+\t\tBATCH_SIZE=$((THIRD_SMALLEST_SIZE + 1)) &&\n \t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n \t\tls .git/objects/pack/*idx >idx-list &&\n \t\ttest_line_count = 6 idx-list &&\n-- \ngitgitgadget\n"},{"id":"397454","messageId":"988697dd5121430cd3ddfa60b1ebcf26027566ef.1589034270.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v3.git.1589034270.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-09T14:24:29Z","receivedAt":"2020-05-09T14:24:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen selecting a batch of pack-files to repack in the \"git\nmulti-pack-index repack\" command, Git should respect the\nrepack.packKeptObjects config option. When false, this option says that\nthe pack-files with an associated \".keep\" file should not be repacked.\nThis config value is \"false\" by default.\n\nThere are two cases for selecting a batch of objects. The first is the\ncase where the input batch-size is zero, which specifies \"repack\neverything\". The second is with a non-zero batch size, which selects\npack-files using a greedy selection criteria. Both of these cases are\nupdated and tested.\n\nReported-by: Son Luong Ngoc \nSigned-off-by: Derrick Stolee \n---\n Documentation/git-multi-pack-index.txt | 3 +++\n midx.c | 26 +++++++++++++++++++++-----\n t/t5319-multi-pack-index.sh | 26 ++++++++++++++++++++++++++\n 3 files changed, 50 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\nindex 642d9ac5b72..0c6619493c1 100644\n--- a/Documentation/git-multi-pack-index.txt\n+++ b/Documentation/git-multi-pack-index.txt\n@@ -56,6 +56,9 @@ repack::\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++\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 \n \n EXAMPLES\ndiff --git a/midx.c b/midx.c\nindex 1e76be56826..9b14d915db1 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1293,15 +1293,26 @@ static int compare_by_mtime(const void *a_, const void *b_)\n \treturn 0;\n }\n \n-static int fill_included_packs_all(struct multi_pack_index *m,\n+static int fill_included_packs_all(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n \t\t\t\t unsigned char *include_pack)\n {\n-\tuint32_t i;\n+\tuint32_t i, count = 0;\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n+\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tif (prepare_midx_pack(r, m, i))\n+\t\t\tcontinue;\n+\t\tif (!pack_kept_objects && m->packs[i]->pack_keep)\n+\t\t\tcontinue;\n \n-\tfor (i = 0; i < m->num_packs; i++)\n \t\tinclude_pack[i] = 1;\n+\t\tcount++;\n+\t}\n \n-\treturn m->num_packs < 2;\n+\treturn count < 2;\n }\n \n static int fill_included_packs_batch(struct repository *r,\n@@ -1312,6 +1323,9 @@ static int fill_included_packs_batch(struct repository *r,\n \tuint32_t i, packs_to_repack;\n \tsize_t total_size;\n \tstruct repack_info *pack_info = xcalloc(m->num_packs, sizeof(struct repack_info));\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tpack_info[i].pack_int_id = i;\n@@ -1338,6 +1352,8 @@ static int fill_included_packs_batch(struct repository *r,\n \n \t\tif (!p)\n \t\t\tcontinue;\n+\t\tif (!pack_kept_objects && p->pack_keep)\n+\t\t\tcontinue;\n \t\tif (open_pack_index(p) || !p->num_objects)\n \t\t\tcontinue;\n \n@@ -1380,7 +1396,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tif (batch_size) {\n \t\tif (fill_included_packs_batch(r, m, include_pack, batch_size))\n \t\t\tgoto cleanup;\n-\t} else if (fill_included_packs_all(m, include_pack))\n+\t} else if (fill_included_packs_all(r, m, include_pack))\n \t\tgoto cleanup;\n \n \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 030a7222b2a..67afe1bb8d9 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -538,6 +538,32 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n \t)\n '\n \n+test_expect_success 'repack respects repack.packKeptObjects=false' '\n+\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n+\t(\n+\t\tcd dup &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n+\t\tfor keep in $(cat keep-list)\n+\t\tdo\n+\t\t\ttouch $keep || return 1\n+\t\tdone &&\n+\t\tgit multi-pack-index repack --batch-size=0 &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list &&\n+\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n+\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n+\t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list\n+\t)\n+'\n+\n test_expect_success 'repack creates a new pack' '\n \t(\n \t\tcd dup &&\n-- \ngitgitgadget\n\n"},{"id":"397455","messageId":"20200509161159.GA15146@danh.dev","threadId":"53402","inReplyTo":"988697dd5121430cd3ddfa60b1ebcf26027566ef.1589034270.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-09T16:11:59Z","receivedAt":"2020-05-09T16:12:04Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-09 14:24:29+0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> +test_expect_success 'repack respects repack.packKeptObjects=false' '\n> +\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n> +\t(\n> +\t\tcd dup &&\n> +\t\tls .git/objects/pack/*idx >idx-list &&\n\nI think ls(1) is an overkill.\nI think:\n\n\techo .git/objects/pack/*idx\n\nis more efficient.\n\n> +\t\ttest_line_count = 5 idx-list &&\n> +\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n\nLikewise.\n\n> +\t\tfor keep in $(cat keep-list)\n> +\t\tdo\n> +\t\t\ttouch $keep || return 1\n\nIs this intended?\nSince touch(1) accepts multiple files as argument.\n\n> +\t\tdone &&\n> +\t\tgit multi-pack-index repack --batch-size=0 &&\n> +\t\tls .git/objects/pack/*idx >idx-list &&\n> +\t\ttest_line_count = 5 idx-list &&\n> +\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n> +\t\ttest_line_count = 5 midx-list &&\n> +\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n\nThis line is overly long.\nShould we write test-tool's output to temp file and process it?\n\nAnd I think either\n\n\tsed -n '3{p;q}'\n\nor:\n\n\tsed -n 3p\n\nis cleaner than\n\n\thead -n 3 | tail -n 1\n\n> +\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n\nI think we're better to make this correct in this patch instead of\nspend a dollar here, than take it back in the next patch.\n\n> +\t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n> +\t\tls .git/objects/pack/*idx >idx-list &&\n\n-- \nDanh\n"},{"id":"397457","messageId":"xmqq7dxlvypv.fsf@gitster.c.googlers.com","threadId":"53402","inReplyTo":"a925307d4c57506f5236e60dc1390998e186cf26.1589034270.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-09T16:51:08Z","receivedAt":"2020-05-09T16:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Son Luong Ngoc via GitGitGadget\" writes:\n\n> From: Son Luong Ngoc \n>\n> Previously, when the \"repack\" subcommand of \"git multi-pack-index\" command\n> creates new packfile(s), it does not call the \"git repack\" command but\n> instead directly calls the \"git pack-objects\" command, and the\n> configuration variables meant for the \"git repack\" command, like\n> \"repack.usedaeltabaseoffset\", are ignored.\n\nWhen we talk about the current state of the code (i.e. before\napplying this patch), we do not say \"previously\". It's not like you\nare complaining about a recent breakage, e.g. \"previously X worked\nlike this but since change Y, it instead works like that, which\nbreaks Z\".\n\n> This patch ensured \"git multi-pack-index\" checks the configuration\n> variables used by \"git repack\" and passes the corresponding options to\n> the underlying \"git pack-objects\" command.\n\nWe write this part in imperative mood, as if we are giving an order\nto the codebase to \"become like so\". We do not give an observation\nabout the patch or the author (\"This patch does X, this patch also\ndoes Y\", \"I do X, I do Y\").\n\nTaking these two together, perhaps like:\n\n When the \"repack\" subcommand of \"git multi-pack-index\" command\n creates new packfile(s), it does not call the \"git repack\"\n command but instead directly calls the \"git pack-objects\"\n command, and the configuration variables meant for the \"git\n repack\" command, like \"repack.usedaeltabaseoffset\", are ignored.\n\n Check the configuration variables used by \"git repack\" ourselves\n in \"git multi-index-pack\" and pass the corresponding options to\n underlying \"git pack-objects\".\n\n> Note that `repack.writeBitmaps` configuration is ignored, as the\n> pack bitmap facility is useful only with a single packfile.\n\nGood.\n\n> +\tint delta_base_offset = 1;\n> +\tint use_delta_islands = 0;\n\nThese give the default values for two configurations and over there\nbuiltin/repack.c has these lines:\n\n 17\tstatic int delta_base_offset = 1;\n 18\tstatic int pack_kept_objects = -1;\n 19\tstatic int write_bitmaps = -1;\n 20\tstatic int use_delta_islands;\n 21\tstatic char *packdir, *packtmp;\n\nWhen somebody is tempted to update these to change the default used\nby \"git repack\", it should be easy to notice that such a change must\nbe accompanied by a matching change to the lines you are introducing\nin this patch, or we'll be out of sync.\n\nThe easiest way to avoid such a problem may be to stop bypassing\n\"git repack\" and calling \"pack-objects\" ourselves. That is the\nreason why the configuration variables honored by \"git repack\" are\nignored in this codepath in the first place. But that is not the\napproach we are taking, so we need a reasonable way to tell those\nwho update this file and builtin/repack.c to make matching changes.\nAt the very least, perhaps we should give a comment above these two\nlines in this file, e.g.\n\n\t/*\n\t * when updating the default for these configuration\n\t * variables in builtin/repack.c, these must be adjusted\n\t * to match.\n\t */\n\tint delta_base_offset = 1;\n\tint use_delta_islands = 0;\n\nor something like that.\n\nWith that, the rest of the patch makes sense.\n\nThanks.\n"},{"id":"397458","messageId":"xmqq1rntvyhu.fsf@gitster.c.googlers.com","threadId":"53402","inReplyTo":"efeb3d7d1321e53e05079f296a5db5ab87f5fab2.1589034270.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] Ensured t5319 follows arith expansion guideline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-09T16:55:57Z","receivedAt":"2020-05-09T16:56:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Son Luong Ngoc via GitGitGadget\" writes:\n\n> From: Son Luong Ngoc \n>\n> As the old versions of dash is deprecated, dollar-sign inside\n> artihmetic expansion is no longer needed.\n> This ensures t5319 follows the coding guideline updated\n> in 'jk/arith-expansion-coding-guidelines' 6d4bf5813cd2c1a3b93fd4f0b231733f82133cce.\n\nThat does not match my understanding of the guideline. By removing\nthe \"dollar required\" rule and not adding a new \"dollar forbidden\"\nrule, we pretty much declared that \"we do not care much either way\"\n[*1*].\n\nEven if we cared, \"Once it _is_ in the tree, it's not really worth\nthe patch noise to go and fix it up.\" rule from the guidelines\napplies here.\n\nThanks.\n\n\n[Reference]\n\n*1* https://lore.kernel.org/git/20200505210741.GB645290@coredump.intra.peff.net/\n"},{"id":"397461","messageId":"xmqqlfm1ui6t.fsf@gitster.c.googlers.com","threadId":"53402","inReplyTo":"20200509161159.GA15146@danh.dev","subject":"Re: [PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-09T17:33:30Z","receivedAt":"2020-05-09T17:33:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh writes:\n\n> On 2020-05-09 14:24:29+0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>> \n>> +test_expect_success 'repack respects repack.packKeptObjects=false' '\n>> +\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n>> +\t(\n>> +\t\tcd dup &&\n>> +\t\tls .git/objects/pack/*idx >idx-list &&\n>\n> I think ls(1) is an overkill.\n> I think:\n>\n> \techo .git/objects/pack/*idx\n>\n> is more efficient.\n\nWhen there is no file whose name ends with idx, what happens?\n\n $ ls *idx && echo OK\n ls: cannot access '*idx': No such file or directory\n $ echo *idx && echo OK\n *idx\n OK\n\n>> +\t\ttest_line_count = 5 idx-list &&\n>> +\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n>\n> Likewise.\n\nLikewise.\n\n>> +\t\tfor keep in $(cat keep-list)\n>> +\t\tdo\n>> +\t\t\ttouch $keep || return 1\n>\n> Is this intended?\n> Since touch(1) accepts multiple files as argument.\n\nGood suggestion, but doesn't .keep file record why the pack is kept\nin real life (i.e. not an empty file)?\n\n>> +\t\tdone &&\n>> +\t\tgit multi-pack-index repack --batch-size=0 &&\n>> +\t\tls .git/objects/pack/*idx >idx-list &&\n>> +\t\ttest_line_count = 5 idx-list &&\n>> +\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n>> +\t\ttest_line_count = 5 midx-list &&\n>> +\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n>\n> This line is overly long.\n> Should we write test-tool's output to temp file and process it?\n>\n> And I think either\n>\n> \tsed -n '3{p;q}'\n>\n> or:\n>\n> \tsed -n 3p\n>\n> is cleaner than\n>\n> \thead -n 3 | tail -n 1\n\n\"sed -n 3p\" is the only valid way to write it ;-)\n\n"},{"id":"397484","messageId":"20200510063844.GA14311@danh.dev","threadId":"53402","inReplyTo":"xmqqlfm1ui6t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-05-10T06:38:44Z","receivedAt":"2020-05-10T06:38:49Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-05-09 10:33:30-0700, Junio C Hamano wrote:\n> Đoàn Trần Công Danh writes:\n> \n> > On 2020-05-09 14:24:29+0000, Derrick Stolee via GitGitGadget wrote:\n> >> From: Derrick Stolee \n> >> \n> >> +test_expect_success 'repack respects repack.packKeptObjects=false' '\n> >> +\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n> >> +\t(\n> >> +\t\tcd dup &&\n> >> +\t\tls .git/objects/pack/*idx >idx-list &&\n> >\n> > I think ls(1) is an overkill.\n> > I think:\n> >\n> > \techo .git/objects/pack/*idx\n> >\n> > is more efficient.\n> \n> When there is no file whose name ends with idx, what happens?\n> \n> $ ls *idx && echo OK\n> ls: cannot access '*idx': No such file or directory\n> $ echo *idx && echo OK\n> *idx\n> OK\n\nYes, but I think the next line is checking for the number of lines.\nThis is better to fail faster.\n\n(My suggestion was wrong anyway, it should be \"printf \"%s\\\\n\" *idx)\n\n> >> +\t\ttest_line_count = 5 idx-list &&\n> >> +\t\tfor keep in $(cat keep-list)\n> >> +\t\tdo\n> >> +\t\t\ttouch $keep || return 1\n> >\n> > Is this intended?\n> > Since touch(1) accepts multiple files as argument.\n> \n> Good suggestion, but doesn't .keep file record why the pack is kept\n> in real life (i.e. not an empty file)?\n\nYes, in real life, we usually provide a reason in this .keep file.\nBut, we also allow empty file with git-index-pack --keep\nI think simple touch is fine for this test.\n\nMissing piece for my previous command:\nif `keep-list` is empty, we may want to fail fast,\ntouch with empty list will error out (at least in my system).\n\n\n-- \nDanh\n"},{"id":"397497","messageId":"20200510142712.GA27407@C02YX140LVDN","threadId":"53402","inReplyTo":"xmqq7dxlvypv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 1/3] midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-05-10T14:27:12Z","receivedAt":"2020-05-10T14:30:12Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"On Sat, May 09, 2020 at 09:51:08AM -0700, Junio C Hamano wrote:\n> \"Son Luong Ngoc via GitGitGadget\" writes:\n> \n> > From: Son Luong Ngoc \n> >\n> > Previously, when the \"repack\" subcommand of \"git multi-pack-index\" command\n> > creates new packfile(s), it does not call the \"git repack\" command but\n> > instead directly calls the \"git pack-objects\" command, and the\n> > configuration variables meant for the \"git repack\" command, like\n> > \"repack.usedaeltabaseoffset\", are ignored.\n> \n> When we talk about the current state of the code (i.e. before\n> applying this patch), we do not say \"previously\". It's not like you\n> are complaining about a recent breakage, e.g. \"previously X worked\n> like this but since change Y, it instead works like that, which\n> breaks Z\".\n> \n> > This patch ensured \"git multi-pack-index\" checks the configuration\n> > variables used by \"git repack\" and passes the corresponding options to\n> > the underlying \"git pack-objects\" command.\n> \n> We write this part in imperative mood, as if we are giving an order\n> to the codebase to \"become like so\". We do not give an observation\n> about the patch or the author (\"This patch does X, this patch also\n> does Y\", \"I do X, I do Y\").\n> \n> Taking these two together, perhaps like:\n> \n> When the \"repack\" subcommand of \"git multi-pack-index\" command\n> creates new packfile(s), it does not call the \"git repack\"\n> command but instead directly calls the \"git pack-objects\"\n> command, and the configuration variables meant for the \"git\n> repack\" command, like \"repack.usedaeltabaseoffset\", are ignored.\n> \n> Check the configuration variables used by \"git repack\" ourselves\n> in \"git multi-index-pack\" and pass the corresponding options to\n> underlying \"git pack-objects\".\n\nThanks for this, it will take me a bit to adjust to this style of\nwriting but I do find it to be a lot clearer and practical.\nWill update in next version.\n\n> \n> > Note that `repack.writeBitmaps` configuration is ignored, as the\n> > pack bitmap facility is useful only with a single packfile.\n> \n> Good.\n> \n> > +\tint delta_base_offset = 1;\n> > +\tint use_delta_islands = 0;\n> \n> These give the default values for two configurations and over there\n> builtin/repack.c has these lines:\n> \n> 17\tstatic int delta_base_offset = 1;\n> 18\tstatic int pack_kept_objects = -1;\n> 19\tstatic int write_bitmaps = -1;\n> 20\tstatic int use_delta_islands;\n> 21\tstatic char *packdir, *packtmp;\n> \n> When somebody is tempted to update these to change the default used\n> by \"git repack\", it should be easy to notice that such a change must\n> be accompanied by a matching change to the lines you are introducing\n> in this patch, or we'll be out of sync.\n> \n> The easiest way to avoid such a problem may be to stop bypassing\n> \"git repack\" and calling \"pack-objects\" ourselves. That is the\n> reason why the configuration variables honored by \"git repack\" are\n> ignored in this codepath in the first place. But that is not the\n> approach we are taking, so we need a reasonable way to tell those\n> who update this file and builtin/repack.c to make matching changes.\n> At the very least, perhaps we should give a comment above these two\n> lines in this file, e.g.\n> \n> \t/*\n> \t * when updating the default for these configuration\n> \t * variables in builtin/repack.c, these must be adjusted\n> \t * to match.\n> \t */\n> \tint delta_base_offset = 1;\n> \tint use_delta_islands = 0;\n> \n> or something like that.\n\nWill add the comments in next version.\n\n> \n> With that, the rest of the patch makes sense.\n> \n> Thanks.\n\nCheers,\nSon Luong\n"},{"id":"397498","messageId":"20200510155228.GB27407@C02YX140LVDN","threadId":"53402","inReplyTo":"20200510063844.GA14311@danh.dev","subject":"Re: [PATCH v3 2/3] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-05-10T15:52:28Z","receivedAt":"2020-05-10T15:52:33Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi,\n\nThanks Danh and Junio for the testing improvement suggestions.\nI think these are the points I will adopt into next version:\n\n- Remove the 3rd patch and keep the removal of dollar sign locally\n inside `repack respects repack.packKeptObjects=false`.\n\n- Change `head -n -3 | tail -n -1` to `sed -n 3p`\n\n- Apply test_line_count on keep-list for failing fast (before touch)\n\nCheers,\nSon Luong.\n"},{"id":"397499","messageId":"a8f75e34e5b3f3ffba9d6a3852f77d03d3352d95.1589126855.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v4.git.1589126855.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-10T16:07:33Z","receivedAt":"2020-05-10T16:07:39Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"From: Son Luong Ngoc \n\nWhen the \"repack\" subcommand of \"git multi-pack-index\" command\ncreates new packfile(s), it does not call the \"git repack\"\ncommand but instead directly calls the \"git pack-objects\"\ncommand, and the configuration variables meant for the \"git\nrepack\" command, like \"repack.usedaeltabaseoffset\", are ignored.\n\nCheck the configuration variables used by \"git repack\" ourselves\nin \"git multi-index-pack\" and pass the corresponding options to\nunderlying \"git pack-objects\".\n\nNote that `repack.writeBitmaps` configuration is ignored, as the\npack bitmap facility is useful only with a single packfile.\n\nSigned-off-by: Son Luong Ngoc \n---\n midx.c | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 9a61d3b37d9..d2a43bd1a38 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1370,6 +1370,14 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n \n+\t/*\n+\t * When updating the default for these configuration\n+\t * variables in builtin/repack.c, these must be adjusted\n+\t * to match.\n+\t */\n+\tint delta_base_offset = 1;\n+\tint use_delta_islands = 0;\n+\n \tif (!m)\n \t\treturn 0;\n \n@@ -1381,12 +1389,20 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t} else if (fill_included_packs_all(m, include_pack))\n \t\tgoto cleanup;\n \n+\trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\n+\trepo_config_get_bool(r, \"repack.usedeltaislands\", &use_delta_islands);\n+\n \targv_array_push(&cmd.args, \"pack-objects\");\n \n \tstrbuf_addstr(&base_name, object_dir);\n \tstrbuf_addstr(&base_name, \"/pack/pack\");\n \targv_array_push(&cmd.args, base_name.buf);\n \n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd.args, \"--delta-base-offset\");\n+\tif (use_delta_islands)\n+\t\targv_array_push(&cmd.args, \"--delta-islands\");\n+\n \tif (flags & MIDX_PROGRESS)\n \t\targv_array_push(&cmd.args, \"--progress\");\n \telse\n-- \ngitgitgadget\n\n"},{"id":"397500","messageId":"pull.626.v4.git.1589126855.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v3.git.1589034270.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] midx: apply gitconfig to midx repack","fromName":"Son Luong Ngoc via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-10T16:07:32Z","receivedAt":"2020-05-10T16:07:39Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Midx repack has largely been used in Microsoft Scalar on the client side to\noptimize the repository multiple packs state. However when I tried to apply\nthis onto the server-side, I realized that there are certain features that\nwere lacking compare to git repack. Most of these features are highly\ndesirable on the server-side to create the most optimized pack possible.\n\nOne of the example is delta_base_offset, comparing an midx repack\nwith/without delta_base_offset, we can observe significant size differences.\n\n> du objects/pack/*pack\n14536 objects/pack/pack-08a017b424534c88191addda1aa5dd6f24bf7a29.pack\n9435280 objects/pack/pack-8829c53ad1dca02e7311f8e5b404962ab242e8f1.pack\n\nLatest 2.26.2 (without delta_base_offset)\n> git multi-pack-index write\n> git multi-pack-index repack\n> git multi-pack-index expire\n> du objects/pack/*pack\n9446096 objects/pack/pack-366c75e2c2f987b9836d3bf0bf5e4a54b6975036.pack\n\nWith delta_base_offset\n> git version\ngit version 2.26.2.672.g232c24e857.dirty\n> git multi-pack-index write\n> git multi-pack-index repack\n> git multi-pack-index expire\n> du objects/pack/*pack\n9152512 objects/pack/pack-3bc8c1ec496ab95d26875f8367ff6807081e9e7d.pack\n\nNote that repack.writeBitmaps configuration is ignored, as the pack bitmap\nfacility is useful only with a single packfile.\n\nDerrick Stolee's following patch will address repack.packKeptObjects \nsupport.\n\nDerrick Stolee (1):\n multi-pack-index: respect repack.packKeptObjects=false\n\nSon Luong Ngoc (1):\n midx: teach \"git multi-pack-index repack\" honor \"git repack\"\n configurations\n\n Documentation/git-multi-pack-index.txt | 3 ++\n midx.c | 42 +++++++++++++++++++++++---\n t/t5319-multi-pack-index.sh | 27 +++++++++++++++++\n 3 files changed, 67 insertions(+), 5 deletions(-)\n\n\nbase-commit: b994622632154fc3b17fb40a38819ad954a5fb88\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-626%2Fsluongng%2Fsluongngoc%2Fmidx-config-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-626/sluongng/sluongngoc/midx-config-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/626\n\nRange-diff vs v3:\n\n 1: a925307d4c5 ! 1: a8f75e34e5b midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations\n @@ Metadata\n ## Commit message ##\n midx: teach \"git multi-pack-index repack\" honor \"git repack\" configurations\n \n - Previously, when the \"repack\" subcommand of \"git multi-pack-index\" command\n - creates new packfile(s), it does not call the \"git repack\" command but\n - instead directly calls the \"git pack-objects\" command, and the\n - configuration variables meant for the \"git repack\" command, like\n - \"repack.usedaeltabaseoffset\", are ignored.\n + When the \"repack\" subcommand of \"git multi-pack-index\" command\n + creates new packfile(s), it does not call the \"git repack\"\n + command but instead directly calls the \"git pack-objects\"\n + command, and the configuration variables meant for the \"git\n + repack\" command, like \"repack.usedaeltabaseoffset\", are ignored.\n \n - This patch ensured \"git multi-pack-index\" checks the configuration\n - variables used by \"git repack\" and passes the corresponding options to\n - the underlying \"git pack-objects\" command.\n + Check the configuration variables used by \"git repack\" ourselves\n + in \"git multi-index-pack\" and pass the corresponding options to\n + underlying \"git pack-objects\".\n \n Note that `repack.writeBitmaps` configuration is ignored, as the\n pack bitmap facility is useful only with a single packfile.\n @@ Commit message\n \n ## midx.c ##\n @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n - \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct strbuf base_name = STRBUF_INIT;\n \tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n + \n ++\t/*\n ++\t * When updating the default for these configuration\n ++\t * variables in builtin/repack.c, these must be adjusted\n ++\t * to match.\n ++\t */\n +\tint delta_base_offset = 1;\n +\tint use_delta_islands = 0;\n - \n ++\n \tif (!m)\n \t\treturn 0;\n + \n @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t} else if (fill_included_packs_all(m, include_pack))\n \t\tgoto cleanup;\n 2: 988697dd512 ! 2: 192fc785382 multi-pack-index: respect repack.packKeptObjects=false\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'repack with minimum size does\n +\t\tls .git/objects/pack/*idx >idx-list &&\n +\t\ttest_line_count = 5 idx-list &&\n +\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n ++\t\ttest_line_count = 5 keep-list &&\n +\t\tfor keep in $(cat keep-list)\n +\t\tdo\n +\t\t\ttouch $keep || return 1\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'repack with minimum size does\n +\t\ttest_line_count = 5 idx-list &&\n +\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n +\t\ttest_line_count = 5 midx-list &&\n -+\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&\n -+\t\tBATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&\n ++\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | sed -n 3p) &&\n ++\t\tBATCH_SIZE=$((THIRD_SMALLEST_SIZE + 1)) &&\n +\t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n +\t\tls .git/objects/pack/*idx >idx-list &&\n +\t\ttest_line_count = 5 idx-list &&\n 3: efeb3d7d132 < -: ----------- Ensured t5319 follows arith expansion guideline\n\n-- \ngitgitgadget\n"},{"id":"397501","messageId":"192fc7853825013250552eda75957a290a9928eb.1589126855.git.gitgitgadget@gmail.com","threadId":"53402","inReplyTo":"pull.626.v4.git.1589126855.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] multi-pack-index: respect repack.packKeptObjects=false","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-10T16:07:34Z","receivedAt":"2020-05-10T16:07:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen selecting a batch of pack-files to repack in the \"git\nmulti-pack-index repack\" command, Git should respect the\nrepack.packKeptObjects config option. When false, this option says that\nthe pack-files with an associated \".keep\" file should not be repacked.\nThis config value is \"false\" by default.\n\nThere are two cases for selecting a batch of objects. The first is the\ncase where the input batch-size is zero, which specifies \"repack\neverything\". The second is with a non-zero batch size, which selects\npack-files using a greedy selection criteria. Both of these cases are\nupdated and tested.\n\nReported-by: Son Luong Ngoc \nSigned-off-by: Derrick Stolee \n---\n Documentation/git-multi-pack-index.txt | 3 +++\n midx.c | 26 ++++++++++++++++++++-----\n t/t5319-multi-pack-index.sh | 27 ++++++++++++++++++++++++++\n 3 files changed, 51 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt\nindex 642d9ac5b72..0c6619493c1 100644\n--- a/Documentation/git-multi-pack-index.txt\n+++ b/Documentation/git-multi-pack-index.txt\n@@ -56,6 +56,9 @@ repack::\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++\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 \n \n EXAMPLES\ndiff --git a/midx.c b/midx.c\nindex d2a43bd1a38..6d1584ca51d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1293,15 +1293,26 @@ static int compare_by_mtime(const void *a_, const void *b_)\n \treturn 0;\n }\n \n-static int fill_included_packs_all(struct multi_pack_index *m,\n+static int fill_included_packs_all(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n \t\t\t\t unsigned char *include_pack)\n {\n-\tuint32_t i;\n+\tuint32_t i, count = 0;\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n+\n+\tfor (i = 0; i < m->num_packs; i++) {\n+\t\tif (prepare_midx_pack(r, m, i))\n+\t\t\tcontinue;\n+\t\tif (!pack_kept_objects && m->packs[i]->pack_keep)\n+\t\t\tcontinue;\n \n-\tfor (i = 0; i < m->num_packs; i++)\n \t\tinclude_pack[i] = 1;\n+\t\tcount++;\n+\t}\n \n-\treturn m->num_packs < 2;\n+\treturn count < 2;\n }\n \n static int fill_included_packs_batch(struct repository *r,\n@@ -1312,6 +1323,9 @@ static int fill_included_packs_batch(struct repository *r,\n \tuint32_t i, packs_to_repack;\n \tsize_t total_size;\n \tstruct repack_info *pack_info = xcalloc(m->num_packs, sizeof(struct repack_info));\n+\tint pack_kept_objects = 0;\n+\n+\trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n \t\tpack_info[i].pack_int_id = i;\n@@ -1338,6 +1352,8 @@ static int fill_included_packs_batch(struct repository *r,\n \n \t\tif (!p)\n \t\t\tcontinue;\n+\t\tif (!pack_kept_objects && p->pack_keep)\n+\t\t\tcontinue;\n \t\tif (open_pack_index(p) || !p->num_objects)\n \t\t\tcontinue;\n \n@@ -1386,7 +1402,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tif (batch_size) {\n \t\tif (fill_included_packs_batch(r, m, include_pack, batch_size))\n \t\t\tgoto cleanup;\n-\t} else if (fill_included_packs_all(m, include_pack))\n+\t} else if (fill_included_packs_all(r, m, include_pack))\n \t\tgoto cleanup;\n \n \trepo_config_get_bool(r, \"repack.usedeltabaseoffset\", &delta_base_offset);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 030a7222b2a..7214cab36c0 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -538,6 +538,33 @@ test_expect_success 'repack with minimum size does not alter existing packs' '\n \t)\n '\n \n+test_expect_success 'repack respects repack.packKeptObjects=false' '\n+\ttest_when_finished rm -f dup/.git/objects/pack/*keep &&\n+\t(\n+\t\tcd dup &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\tls .git/objects/pack/*.pack | sed \"s/\\.pack/.keep/\" >keep-list &&\n+\t\ttest_line_count = 5 keep-list &&\n+\t\tfor keep in $(cat keep-list)\n+\t\tdo\n+\t\t\ttouch $keep || return 1\n+\t\tdone &&\n+\t\tgit multi-pack-index repack --batch-size=0 &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list &&\n+\t\tTHIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | sed -n 3p) &&\n+\t\tBATCH_SIZE=$((THIRD_SMALLEST_SIZE + 1)) &&\n+\t\tgit multi-pack-index repack --batch-size=$BATCH_SIZE &&\n+\t\tls .git/objects/pack/*idx >idx-list &&\n+\t\ttest_line_count = 5 idx-list &&\n+\t\ttest-tool read-midx .git/objects | grep idx >midx-list &&\n+\t\ttest_line_count = 5 midx-list\n+\t)\n+'\n+\n test_expect_success 'repack creates a new pack' '\n \t(\n \t\tcd dup &&\n-- \ngitgitgadget\n"}]}