{"thread":{"id":"60162","subject":"[PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`","startedAt":"2023-08-28T22:49:52Z","lastAt":"2023-08-29T17:53:32Z","messageCount":8,"participants":["Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"481087","messageId":"b6d945197faaef8243bddf78f672a340404e6ac4.1693262936.git.me@ttaylorr.com","threadId":"60162","inReplyTo":"cover.1693262936.git.me@ttaylorr.com","subject":"[PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:49:07Z","receivedAt":"2023-08-28T22:49:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When pack-objects learned the `--cruft` option back in b757353676\n(builtin/pack-objects.c: --cruft without expiration, 2022-05-20), we\nexplicitly forbade `--cruft` with `--max-pack-size`.\n\nAt the time, there was no specific rationale given in the patch for not\nsupporting the `--max-pack-size` option with `--cruft`. (As best I can\nremember, it's because we were trying to push users towards only ever\nhaving a single cruft pack, but I cannot be sure).\n\nHowever, `--max-pack-size` is flexible enough that it already works with\n`--cruft` and can shard unreachable objects across multiple cruft packs,\ncreating separate \".mtimes\" files as appropriate. In fact, the\n`--max-pack-size` option worked with `--cruft` as far back as\nb757353676!\n\nThis is because we overwrite the `written_list`, and pass down the\nappropriate length, i.e. the number of objects written in each pack\nshard.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-pack-objects.txt |  4 +--\n builtin/pack-objects.c             |  7 ++--\n builtin/repack.c                   |  3 +-\n t/t5329-pack-objects-cruft.sh      | 54 ++++++++++++++++++++++++------\n 4 files changed, 49 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex a9995a932c..dea7eacb0f 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -116,9 +116,7 @@ unreachable object whose mtime is newer than the `--cruft-expiration`).\n +\n Incompatible with `--unpack-unreachable`, `--keep-unreachable`,\n `--pack-loose-unreachable`, `--stdin-packs`, as well as any other\n-options which imply `--revs`. Also incompatible with `--max-pack-size`;\n-when this option is set, the maximum pack size is not inferred from\n-`pack.packSizeLimit`.\n+options which imply `--revs`.\n \n --cruft-expiration=<approxidate>::\n \tIf specified, objects are eliminated from the cruft pack if they\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 868efe7e0f..a046994a43 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1190,8 +1190,7 @@ static void write_pack_file(void)\n \t\tunsigned char hash[GIT_MAX_RAWSZ];\n \t\tchar *pack_tmp_name = NULL;\n \n-\t\tif (pack_to_stdout)\n-\t\t\tf = hashfd_throughput(1, \"<stdout>\", progress_state);\n+\t\tif (pack_to_stdout) f = hashfd_throughput(1, \"<stdout>\", progress_state);\n \t\telse\n \t\t\tf = create_tmp_packfile(&pack_tmp_name);\n \n@@ -4382,7 +4381,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tif (!HAVE_THREADS && delta_search_threads != 1)\n \t\twarning(_(\"no threads support, ignoring --threads\"));\n-\tif (!pack_to_stdout && !pack_size_limit && !cruft)\n+\tif (!pack_to_stdout && !pack_size_limit)\n \t\tpack_size_limit = pack_size_limit_cfg;\n \tif (pack_to_stdout && pack_size_limit)\n \t\tdie(_(\"--max-pack-size cannot be used to build a pack for transfer\"));\n@@ -4414,8 +4413,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"cannot use internal rev list with --cruft\"));\n \t\tif (stdin_packs)\n \t\t\tdie(_(\"cannot use --stdin-packs with --cruft\"));\n-\t\tif (pack_size_limit)\n-\t\t\tdie(_(\"cannot use --max-pack-size with --cruft\"));\n \t}\n \n \t/*\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2b43a5be08..6943c5ba11 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -720,7 +720,6 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \n \tstrvec_push(&cmd.args, \"--honor-pack-keep\");\n \tstrvec_push(&cmd.args, \"--non-empty\");\n-\tstrvec_push(&cmd.args, \"--max-pack-size=0\");\n \n \tcmd.in = -1;\n \n@@ -1048,6 +1047,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tcruft_po_args.depth = po_args.depth;\n \t\tif (!cruft_po_args.threads)\n \t\t\tcruft_po_args.threads = po_args.threads;\n+\t\tif (!cruft_po_args.max_pack_size)\n+\t\t\tcruft_po_args.max_pack_size = po_args.max_pack_size;\n \n \t\tcruft_po_args.local = po_args.local;\n \t\tcruft_po_args.quiet = po_args.quiet;\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex 45667d4999..fc5fedbe9b 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -573,23 +573,54 @@ test_expect_success 'cruft repack with no reachable objects' '\n \t)\n '\n \n-test_expect_success 'cruft repack ignores --max-pack-size' '\n+write_blob () {\n+\ttest-tool genrandom \"$@\" >in &&\n+\tgit hash-object -w -t blob in\n+}\n+\n+find_pack () {\n+\tfor idx in $(ls $packdir/pack-*.idx)\n+\tdo\n+\t\tgit show-index <$idx >out &&\n+\t\tif grep -q \"$1\" out\n+\t\tthen\n+\t\t\techo $idx\n+\t\tfi || return 1\n+\tdone\n+}\n+\n+test_expect_success 'cruft repack with --max-pack-size' '\n \tgit init max-pack-size &&\n \t(\n \t\tcd max-pack-size &&\n \t\ttest_commit base &&\n+\n \t\t# two cruft objects which exceed the maximum pack size\n-\t\ttest-tool genrandom foo 1048576 | git hash-object --stdin -w &&\n-\t\ttest-tool genrandom bar 1048576 | git hash-object --stdin -w &&\n+\t\tfoo=$(write_blob foo 1048576) &&\n+\t\tbar=$(write_blob bar 1048576) &&\n+\t\ttest-tool chmtime --get -1000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path $foo)\" >foo.mtime &&\n+\t\ttest-tool chmtime --get -2000 \\\n+\t\t\t\"$objdir/$(test_oid_to_path $bar)\" >bar.mtime &&\n \t\tgit repack --cruft --max-pack-size=1M &&\n \t\tfind $packdir -name \"*.mtimes\" >cruft &&\n-\t\ttest_line_count = 1 cruft &&\n-\t\ttest-tool pack-mtimes \"$(basename \"$(cat cruft)\")\" >objects &&\n-\t\ttest_line_count = 2 objects\n+\t\ttest_line_count = 2 cruft &&\n+\n+\t\tfoo_mtimes=\"$(basename $(find_pack $foo) .idx).mtimes\" &&\n+\t\tbar_mtimes=\"$(basename $(find_pack $bar) .idx).mtimes\" &&\n+\t\ttest-tool pack-mtimes $foo_mtimes >foo.actual &&\n+\t\ttest-tool pack-mtimes $bar_mtimes >bar.actual &&\n+\n+\t\techo \"$foo $(cat foo.mtime)\" >foo.expect &&\n+\t\techo \"$bar $(cat bar.mtime)\" >bar.expect &&\n+\n+\t\ttest_cmp foo.expect foo.actual &&\n+\t\ttest_cmp bar.expect bar.actual &&\n+\t\ttest \"$foo_mtimes\" != \"$bar_mtimes\"\n \t)\n '\n \n-test_expect_success 'cruft repack ignores pack.packSizeLimit' '\n+test_expect_success 'cruft repack with pack.packSizeLimit' '\n \t(\n \t\tcd max-pack-size &&\n \t\t# repack everything back together to remove the existing cruft\n@@ -599,9 +630,12 @@ test_expect_success 'cruft repack ignores pack.packSizeLimit' '\n \t\t# ensure the same post condition is met when --max-pack-size\n \t\t# would otherwise be inferred from the configuration\n \t\tfind $packdir -name \"*.mtimes\" >cruft &&\n-\t\ttest_line_count = 1 cruft &&\n-\t\ttest-tool pack-mtimes \"$(basename \"$(cat cruft)\")\" >objects &&\n-\t\ttest_line_count = 2 objects\n+\t\ttest_line_count = 2 cruft &&\n+\t\tfor pack in $(cat cruft)\n+\t\tdo\n+\t\t\ttest-tool pack-mtimes \"$(basename $pack)\" >objects &&\n+\t\t\ttest_line_count = 1 objects || return 1\n+\t\tdone\n \t)\n '\n \n-- \n2.42.0.49.g03c54e21ee\n\n"},{"id":"481089","messageId":"03c54e21ee48028cd7c0d136671eb9d1e8b9546d.1693262936.git.me@ttaylorr.com","threadId":"60162","inReplyTo":"cover.1693262936.git.me@ttaylorr.com","subject":"[PATCH 4/4] Documentation/gitformat-pack.txt: drop mixed version section","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:49:12Z","receivedAt":"2023-08-28T22:49:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This section was added in 3d89a8c118 (Documentation/technical: add\ncruft-packs.txt, 2022-05-20) to highlight a potential pitfall when\ndeploying cruft packs in an environment where multiple versions of Git\nare GC-ing the same repository.\n\nNow that it has been more than a year since 3d89a8c118 was written,\nlet's drop this section as it is no longer relevant.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/gitformat-pack.txt | 26 --------------------------\n 1 file changed, 26 deletions(-)\n\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex 49bb09d7df..870e00f298 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -588,32 +588,6 @@ later on.\n It is linkgit:git-gc[1] that is typically responsible for removing expired\n unreachable objects.\n \n-=== Caution for mixed-version environments\n-\n-Repositories that have cruft packs in them will continue to work with any older\n-version of Git. Note, however, that previous versions of Git which do not\n-understand the `.mtimes` file will use the cruft pack's mtime as the mtime for\n-all of the objects in it. In other words, do not expect older (pre-cruft pack)\n-versions of Git to interpret or even read the contents of the `.mtimes` file.\n-\n-Note that having mixed versions of Git GC-ing the same repository can lead to\n-unreachable objects never being completely pruned. This can happen under the\n-following circumstances:\n-\n-  - An older version of Git running GC explodes the contents of an existing\n-    cruft pack loose, using the cruft pack's mtime.\n-  - A newer version running GC collects those loose objects into a cruft pack,\n-    where the .mtime file reflects the loose object's actual mtimes, but the\n-    cruft pack mtime is \"now\".\n-\n-Repeating this process will lead to unreachable objects not getting pruned as a\n-result of repeatedly resetting the objects' mtimes to the present time.\n-\n-If you are GC-ing repositories in a mixed version environment, consider omitting\n-the `--cruft` option when using linkgit:git-repack[1] and linkgit:git-gc[1], and\n-setting the `gc.cruftPacks` configuration to \"false\" until all writers\n-understand cruft packs.\n-\n === Alternatives\n \n Notable alternatives to this design include:\n-- \n2.42.0.49.g03c54e21ee\n"},{"id":"481090","messageId":"921a79c8bd6ab688fb9f403a59eeaed3176b630e.1693262936.git.me@ttaylorr.com","threadId":"60162","inReplyTo":"cover.1693262936.git.me@ttaylorr.com","subject":"[PATCH 1/4] builtin/pack-objects.c: remove unnecessary strbuf_reset()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:49:04Z","receivedAt":"2023-08-28T22:49:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When reading input with the `--cruft` option, `git pack-objects` reads\neach line into a strbuf, and then moves it to either the list of\ndiscarded or fresh packs, depending on whether or not the input line\nstarts with a '-' character.\n\nAt the beginning of each loop iteration, the next line of input is read\nwith `strbuf_getline()`, which calls `strbuf_reset()` (as a part of\n`strbuf_getwholeline()`) before reading the next line of input.\n\nThus, the call to `strbuf_reset()` (added back in b757353676\n(builtin/pack-objects.c: --cruft without expiration, 2022-05-20)) at the\nend of the loop is unnecessary, so let's remove it accordingly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d2a162d528..868efe7e0f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3603,7 +3603,6 @@ static void read_cruft_objects(void)\n \t\t\tstring_list_append(&discard_packs, buf.buf + 1);\n \t\telse\n \t\t\tstring_list_append(&fresh_packs, buf.buf);\n-\t\tstrbuf_reset(&buf);\n \t}\n \n \tstring_list_sort(&discard_packs);\n-- \n2.42.0.49.g03c54e21ee\n\n"},{"id":"481091","messageId":"cover.1693262936.git.me@ttaylorr.com","threadId":"60162","inReplyTo":null,"subject":"[PATCH 0/4] pack-objects: support `--max-pack-size` for cruft packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:48:58Z","receivedAt":"2023-08-28T22:49:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series comes from the beginning of a longer one with the goal\nof giving users more flexible options to manage a set of cruft packs.\n\nThe goal of that series is to be able to do things like:\n\n    $ git repack --cruft --max-cruft-size=10G\n\nand coalesce smaller cruft packs together until they reach the given\nthreshold.\n\nThis series takes a tiny step towards that direction by making\n`--max-pack-size` work with cruft packs. This will be necessary since we\nhave to guess the size of cruft packs when we combine two or more\nexisting cruft packs.\n\nThis accommodates situations like having an object which, when packed\nwith a cruft pack that is below the size threshold, crosses over and\ncauses the resulting pack to go above the size threshold. When\nspecifying `--max-pack-size`, we would split the pack appropriately, and\npack the aforementioned object separately.\n\nBut that is neither here nor there, since this series just makes a start\nin getting `--max-pack-size` to work with `--cruft`. Thanks in advance\nfor your review!\n\nTaylor Blau (4):\n  builtin/pack-objects.c: remove unnecessary strbuf_reset()\n  builtin/pack-objects.c: support `--max-pack-size` with `--cruft`\n  Documentation/gitformat-pack.txt: remove multi-cruft packs alternative\n  Documentation/gitformat-pack.txt: drop mixed version section\n\n Documentation/git-pack-objects.txt |  4 +--\n Documentation/gitformat-pack.txt   | 36 +-------------------\n builtin/pack-objects.c             |  8 ++---\n builtin/repack.c                   |  3 +-\n t/t5329-pack-objects-cruft.sh      | 54 ++++++++++++++++++++++++------\n 5 files changed, 50 insertions(+), 55 deletions(-)\n\n-- \n2.42.0.49.g03c54e21ee\n"},{"id":"481088","messageId":"c6ae0716c631ac4a3b5654c50d768d5ea920fa6d.1693262936.git.me@ttaylorr.com","threadId":"60162","inReplyTo":"cover.1693262936.git.me@ttaylorr.com","subject":"[PATCH 3/4] Documentation/gitformat-pack.txt: remove multi-cruft packs alternative","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-28T22:49:10Z","receivedAt":"2023-08-28T22:49:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This text, originally from 3d89a8c118 (Documentation/technical: add\ncruft-packs.txt, 2022-05-20) lists multiple cruft packs as a potential\nalternative to the design of cruft packs.\n\nWe have always supported multiple cruft packs (i.e. we use the most\nrecent mtime for a given object among all cruft packs which contain it,\netc.), but haven't encouraged its use.\n\nWe still aren't encouraging users to go out and generate multiple cruft\npacks, but let's take a step in that direction by dropping language that\nsuggests we aren't capable of working with multiple cruft packs.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/gitformat-pack.txt | 10 +---------\n 1 file changed, 1 insertion(+), 9 deletions(-)\n\ndiff --git a/Documentation/gitformat-pack.txt b/Documentation/gitformat-pack.txt\nindex 0c1be2dbe8..49bb09d7df 100644\n--- a/Documentation/gitformat-pack.txt\n+++ b/Documentation/gitformat-pack.txt\n@@ -618,21 +618,13 @@ understand cruft packs.\n \n Notable alternatives to this design include:\n \n-  - The location of the per-object mtime data, and\n-  - Storing unreachable objects in multiple cruft packs.\n+  - The location of the per-object mtime data.\n \n On the location of mtime data, a new auxiliary file tied to the pack was chosen\n to avoid complicating the `.idx` format. If the `.idx` format were ever to gain\n support for optional chunks of data, it may make sense to consolidate the\n `.mtimes` format into the `.idx` itself.\n \n-Storing unreachable objects among multiple cruft packs (e.g., creating a new\n-cruft pack during each repacking operation including only unreachable objects\n-which aren't already stored in an earlier cruft pack) is significantly more\n-complicated to construct, and so aren't pursued here. The obvious drawback to\n-the current implementation is that the entire cruft pack must be re-written from\n-scratch.\n-\n GIT\n ---\n Part of the linkgit:git[1] suite\n-- \n2.42.0.49.g03c54e21ee\n\n"},{"id":"481126","messageId":"xmqq4jkheor0.fsf@gitster.g","threadId":"60162","inReplyTo":"921a79c8bd6ab688fb9f403a59eeaed3176b630e.1693262936.git.me@ttaylorr.com","subject":"Re: [PATCH 1/4] builtin/pack-objects.c: remove unnecessary strbuf_reset()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-29T17:34:43Z","receivedAt":"2023-08-29T17:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> When reading input with the `--cruft` option, `git pack-objects` reads\n> each line into a strbuf, and then moves it to either the list of\n> discarded or fresh packs, depending on whether or not the input line\n> starts with a '-' character.\n>\n> At the beginning of each loop iteration, the next line of input is read\n> with `strbuf_getline()`, which calls `strbuf_reset()` (as a part of\n> `strbuf_getwholeline()`) before reading the next line of input.\n\nYup, the reset() at the end of each iteration is not needed to\nprepare for the next interation (which getline() is perfectly\ncapable of doing it itself) but helps to release the resource\nthat is no longer used after the loop finishes iterating.  If I were\ncleaning up this code, I would probably move the _reset() after the\nloop instead *and* keep the _release() at the end, so that future\nchanges can add new (re)uses of buf in between.\n\nBut it is probalby not worth worrying about keeping a single line\nfrom the cruft object list a little longer than absolutely needed.\n\nWill queue.  Thanks.\n\n>\n> Thus, the call to `strbuf_reset()` (added back in b757353676\n> (builtin/pack-objects.c: --cruft without expiration, 2022-05-20)) at the\n> end of the loop is unnecessary, so let's remove it accordingly.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  builtin/pack-objects.c | 1 -\n>  1 file changed, 1 deletion(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index d2a162d528..868efe7e0f 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -3603,7 +3603,6 @@ static void read_cruft_objects(void)\n>  \t\t\tstring_list_append(&discard_packs, buf.buf + 1);\n>  \t\telse\n>  \t\t\tstring_list_append(&fresh_packs, buf.buf);\n> -\t\tstrbuf_reset(&buf);\n>  \t}\n>  \n>  \tstring_list_sort(&discard_packs);\n"},{"id":"481127","messageId":"xmqqr0nld9u9.fsf@gitster.g","threadId":"60162","inReplyTo":"b6d945197faaef8243bddf78f672a340404e6ac4.1693262936.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-29T17:42:06Z","receivedAt":"2023-08-29T17:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> When pack-objects learned the `--cruft` option back in b757353676\n> (builtin/pack-objects.c: --cruft without expiration, 2022-05-20), we\n> explicitly forbade `--cruft` with `--max-pack-size`.\n>\n> At the time, there was no specific rationale given in the patch for not\n> supporting the `--max-pack-size` option with `--cruft`. (As best I can\n> remember, it's because we were trying to push users towards only ever\n> having a single cruft pack, but I cannot be sure).\n\nI am reasonably sure it was the case but then I do not recall we\never discussing how the second cruft pack gets consolidated into one\nby combining it with the existing one.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 868efe7e0f..a046994a43 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1190,8 +1190,7 @@ static void write_pack_file(void)\n>  \t\tunsigned char hash[GIT_MAX_RAWSZ];\n>  \t\tchar *pack_tmp_name = NULL;\n>  \n> -\t\tif (pack_to_stdout)\n> -\t\t\tf = hashfd_throughput(1, \"<stdout>\", progress_state);\n> +\t\tif (pack_to_stdout) f = hashfd_throughput(1, \"<stdout>\", progress_state);\n>  \t\telse\n>  \t\t\tf = create_tmp_packfile(&pack_tmp_name);\n\nAn unintended change, I am sure ;-)\n\nIt is very surprising that absolutely no real change is needed to\nallow cruft packs to honor the settings, other than removing the\nseemingly artificial inter-option-compatibility roadblocks (all\nhunks for it omitted above as they were trivially obvious).  I am\nsure the first hunk to fold an \"if\" statement onto a single line is\nnot what makes the feature to actually work ;-)\n\n> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\n> index 45667d4999..fc5fedbe9b 100755\n> --- a/t/t5329-pack-objects-cruft.sh\n> +++ b/t/t5329-pack-objects-cruft.sh\n> @@ -573,23 +573,54 @@ test_expect_success 'cruft repack with no reachable objects' '\n>  \t)\n>  '\n>  \n> -test_expect_success 'cruft repack ignores --max-pack-size' '\n> +write_blob () {\n> +\ttest-tool genrandom \"$@\" >in &&\n> +\tgit hash-object -w -t blob in\n> +}\n> +\n> +find_pack () {\n> +\tfor idx in $(ls $packdir/pack-*.idx)\n> +\tdo\n> +\t\tgit show-index <$idx >out &&\n> +\t\tif grep -q \"$1\" out\n> +\t\tthen\n> +\t\t\techo $idx\n> +\t\tfi || return 1\n> +\tdone\n> +}\n> +\n> +test_expect_success 'cruft repack with --max-pack-size' '\n>  \tgit init max-pack-size &&\n>  \t(\n>  \t\tcd max-pack-size &&\n>  \t\ttest_commit base &&\n> +\n>  \t\t# two cruft objects which exceed the maximum pack size\n> -\t\ttest-tool genrandom foo 1048576 | git hash-object --stdin -w &&\n> -\t\ttest-tool genrandom bar 1048576 | git hash-object --stdin -w &&\n> +\t\tfoo=$(write_blob foo 1048576) &&\n> +\t\tbar=$(write_blob bar 1048576) &&\n> +\t\ttest-tool chmtime --get -1000 \\\n> +\t\t\t\"$objdir/$(test_oid_to_path $foo)\" >foo.mtime &&\n> +\t\ttest-tool chmtime --get -2000 \\\n> +\t\t\t\"$objdir/$(test_oid_to_path $bar)\" >bar.mtime &&\n>  \t\tgit repack --cruft --max-pack-size=1M &&\n>  \t\tfind $packdir -name \"*.mtimes\" >cruft &&\n> -\t\ttest_line_count = 1 cruft &&\n> -\t\ttest-tool pack-mtimes \"$(basename \"$(cat cruft)\")\" >objects &&\n> -\t\ttest_line_count = 2 objects\n> +\t\ttest_line_count = 2 cruft &&\n> +\n> +\t\tfoo_mtimes=\"$(basename $(find_pack $foo) .idx).mtimes\" &&\n> +\t\tbar_mtimes=\"$(basename $(find_pack $bar) .idx).mtimes\" &&\n> +\t\ttest-tool pack-mtimes $foo_mtimes >foo.actual &&\n> +\t\ttest-tool pack-mtimes $bar_mtimes >bar.actual &&\n> +\n> +\t\techo \"$foo $(cat foo.mtime)\" >foo.expect &&\n> +\t\techo \"$bar $(cat bar.mtime)\" >bar.expect &&\n> +\n> +\t\ttest_cmp foo.expect foo.actual &&\n> +\t\ttest_cmp bar.expect bar.actual &&\n> +\t\ttest \"$foo_mtimes\" != \"$bar_mtimes\"\n>  \t)\n>  '\n>  \n> -test_expect_success 'cruft repack ignores pack.packSizeLimit' '\n> +test_expect_success 'cruft repack with pack.packSizeLimit' '\n>  \t(\n>  \t\tcd max-pack-size &&\n>  \t\t# repack everything back together to remove the existing cruft\n> @@ -599,9 +630,12 @@ test_expect_success 'cruft repack ignores pack.packSizeLimit' '\n>  \t\t# ensure the same post condition is met when --max-pack-size\n>  \t\t# would otherwise be inferred from the configuration\n>  \t\tfind $packdir -name \"*.mtimes\" >cruft &&\n> -\t\ttest_line_count = 1 cruft &&\n> -\t\ttest-tool pack-mtimes \"$(basename \"$(cat cruft)\")\" >objects &&\n> -\t\ttest_line_count = 2 objects\n> +\t\ttest_line_count = 2 cruft &&\n> +\t\tfor pack in $(cat cruft)\n> +\t\tdo\n> +\t\t\ttest-tool pack-mtimes \"$(basename $pack)\" >objects &&\n> +\t\t\ttest_line_count = 1 objects || return 1\n> +\t\tdone\n>  \t)\n>  '\n"},{"id":"481129","messageId":"ZO4wZdaR/I06n+E8@nand.local","threadId":"60162","inReplyTo":"xmqqr0nld9u9.fsf@gitster.g","subject":"Re: [PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-08-29T17:52:37Z","receivedAt":"2023-08-29T17:53:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 29, 2023 at 10:42:06AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > When pack-objects learned the `--cruft` option back in b757353676\n> > (builtin/pack-objects.c: --cruft without expiration, 2022-05-20), we\n> > explicitly forbade `--cruft` with `--max-pack-size`.\n> >\n> > At the time, there was no specific rationale given in the patch for not\n> > supporting the `--max-pack-size` option with `--cruft`. (As best I can\n> > remember, it's because we were trying to push users towards only ever\n> > having a single cruft pack, but I cannot be sure).\n>\n> I am reasonably sure it was the case but then I do not recall we\n> ever discussing how the second cruft pack gets consolidated into one\n> by combining it with the existing one.\n\nYeah. We write the combined cruft pack just like we would any other, and\nrecord each packed object's most recent mtime available from either:\n\n  - a loose copy of that object, if one exists\n  - the mtime of the .pack file for any packed copies of that object\n    which may exist\n  - the mtime of that object as recorded in an .mtimes file (if that\n    file was packed as cruft).\n\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index 868efe7e0f..a046994a43 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -1190,8 +1190,7 @@ static void write_pack_file(void)\n> >  \t\tunsigned char hash[GIT_MAX_RAWSZ];\n> >  \t\tchar *pack_tmp_name = NULL;\n> >\n> > -\t\tif (pack_to_stdout)\n> > -\t\t\tf = hashfd_throughput(1, \"<stdout>\", progress_state);\n> > +\t\tif (pack_to_stdout) f = hashfd_throughput(1, \"<stdout>\", progress_state);\n> >  \t\telse\n> >  \t\t\tf = create_tmp_packfile(&pack_tmp_name);\n>\n> An unintended change, I am sure ;-)\n>\n> It is very surprising that absolutely no real change is needed to\n> allow cruft packs to honor the settings, other than removing the\n> seemingly artificial inter-option-compatibility roadblocks (all\n> hunks for it omitted above as they were trivially obvious).  I am\n> sure the first hunk to fold an \"if\" statement onto a single line is\n> not what makes the feature to actually work ;-)\n\nHah, this made me laugh. Indeed, a whitespace change around this\nif-statement is not the make-or-break change we needed to make this\nfeature work!\n\nI'm happy to clean this up and resubmit it, but you may have already\ndone so, in which case I'll leave this as-is.\n\nThanks,\nTaylor\n"}]}