{"thread":{"id":"58263","subject":"[PATCH 0/2] let feature.experimental imply gc.cruftPacks=true","startedAt":"2022-08-03T20:57:34Z","lastAt":"2022-08-04T16:10:55Z","messageCount":9,"participants":["Emily Shaffer","Junio C Hamano","Ævar Arnfjörð Bjarmason","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"460571","messageId":"20220803205721.3686361-1-emilyshaffer@google.com","threadId":"58263","inReplyTo":null,"subject":"[PATCH 0/2] let feature.experimental imply gc.cruftPacks=true","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-08-03T20:57:19Z","receivedAt":"2022-08-03T20:57:34Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Cruft packs seem to save us disk space during garbage collection. There\nmay still be some interesting races around mtime, but as I understand\nit, those are not particularly worse or better with or without cruft\npacks; so this setting primarily works to save us disk space. At least\nat Google, we see concerns around loose object explosion during gc\nfairly often, so this is a welcome change and we've turned it on for all\nGooglers. Because we did so, I wonder if it's going to make sense to for\ngc.cruftPacks to default to true in the future, with or without mtime\nrace fixes. With that in mind, I think it's a good idea to get any folks\nwho may be testing for us with feature.experimental to also try out\ncruft packs and complain to us if there is an issue.\n\nIn Monday's standup there was some discussion around remaining concerns\nwith mtime and cruft packs. As I understood it at least, it seems there\ncan be a loss of information around mtimes when switching between cruft\nrepacks and no-cruft repacks, for example:\n\n ab/cdef is unreachable and has mtime <last week>\n 'git gc --cruft'\n packs/<cruft>.pack contains abcdef\n packs/<cruft>.mtimes says abcdef has mtime <last week>\n 'git gc' (no cruft)\n ab/cdef is unreachable and has mtime <now>\n\nI could have misunderstood it. But it seems to be an issue primarily\naround switching between crufty and non-crufty gc.\n\nRead the full conversation around mtime races:\nhttps://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-01\n\nAs for the series, it's only a two-patch series because I noticed while\ntrying to add tests for feature.experimental => gc.cruftPacks=true that\nthere weren't any tests around gc.cruftPacks to begin with. It's\npossible that there are too many tests in patch 1 - gc --cruft is tested\nin t5329 'expiring cruft objects with git gc' - but it looked like the\ncontent of the test was different, in that t5329's test checks to make\nsure the cruft pack and associated metadata are deleted during\nexpiration, but the one I added checks that the cruft pack is generated\nduring a 'gc --cruft' which isn't ready to expire yet.\n\n - Emily\n\nEmily Shaffer (2):\n  gc: add tests for --cruft and friends\n  config: let feature.experimental imply gc.cruftPacks=true\n\n Documentation/config/feature.txt |  2 +\n builtin/gc.c                     |  6 +++\n t/t6500-gc.sh                    | 71 ++++++++++++++++++++++++++++++++\n 3 files changed, 79 insertions(+)\n\n-- \n2.37.1.455.g008518b4e5-goog\n\n"},{"id":"460572","messageId":"20220803205721.3686361-2-emilyshaffer@google.com","threadId":"58263","inReplyTo":"20220803205721.3686361-1-emilyshaffer@google.com","subject":"[PATCH 1/2] gc: add tests for --cruft and friends","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-08-03T20:57:20Z","receivedAt":"2022-08-03T20:57:36Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via\nloose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.\n'--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that\ndoesn't check whether a lone gc run generates these cruft packs.\n'gc.cruftPacks' is never exercised.\n\nAdd some tests to exercise these options to gc in the gc test suite.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex cd6c53360d..e4c2c3583d 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -202,6 +202,42 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n '\n \n+test_expect_success 'gc --cruft generates a cruft pack' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\t\ttest_commit base &&\n+\n+\t\ttest_commit --no-tag foo &&\n+\t\ttest_commit --no-tag bar &&\n+\t\tgit reset HEAD^^ &&\n+\n+\t\tgit gc --cruft &&\n+\n+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n+\t)\n+'\n+\n+test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\t\ttest_commit base &&\n+\n+\t\ttest_commit --no-tag foo &&\n+\t\ttest_commit --no-tag bar &&\n+\t\tgit reset HEAD^^ &&\n+\n+\t\tgit -c gc.cruftPacks=true gc &&\n+\n+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n+\t)\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9.  Furthermore, the\n-- \n2.37.1.455.g008518b4e5-goog\n\n"},{"id":"460573","messageId":"20220803205721.3686361-3-emilyshaffer@google.com","threadId":"58263","inReplyTo":"20220803205721.3686361-1-emilyshaffer@google.com","subject":"[PATCH 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-08-03T20:57:21Z","receivedAt":"2022-08-03T20:57:40Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"We are interested in exploring whether gc.cruftPacks=true should become\nthe default value; to determine whether it is safe to do so, let's\nencourage more users to try it out. Users who have set\nfeature.experimental=true have already volunteered to try new and\npossibly-breaking config changes, so let's try this new default with\nthat set of users.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\n Documentation/config/feature.txt |  2 ++\n builtin/gc.c                     |  6 ++++++\n t/t6500-gc.sh                    | 35 ++++++++++++++++++++++++++++++++\n 3 files changed, 43 insertions(+)\n\ndiff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt\nindex cdecd04e5b..f029c422be 100644\n--- a/Documentation/config/feature.txt\n+++ b/Documentation/config/feature.txt\n@@ -14,6 +14,8 @@ feature.experimental::\n +\n * `fetch.negotiationAlgorithm=skipping` may improve fetch negotiation times by\n skipping more commits at a time, reducing the number of round trips.\n+* `gc.cruftPacks=true` reduces disk space used by unreachable objects during\n+garbage collection.\n \n feature.manyFiles::\n \tEnable config options that optimize for repos with many files in the\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex eeff2b760e..919cc508c5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -136,6 +136,7 @@ static int gc_config_is_timestamp_never(const char *var)\n static void gc_config(void)\n {\n \tconst char *value;\n+\tint experimental = 0;\n \n \tif (!git_config_get_value(\"gc.packrefs\", &value)) {\n \t\tif (value && !strcmp(value, \"notbare\"))\n@@ -148,6 +149,11 @@ static void gc_config(void)\n \t    gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n \t\tprune_reflogs = 0;\n \n+\t/* feature.experimental implies gc.cruftPacks=true */\n+\tgit_config_get_bool(\"feature.experimental\", &experimental);\n+\tif (experimental)\n+\t\tcruft_packs = 1;\n+\n \tgit_config_get_int(\"gc.aggressivewindow\", &aggressive_window);\n \tgit_config_get_int(\"gc.aggressivedepth\", &aggressive_depth);\n \tgit_config_get_int(\"gc.auto\", &gc_auto_threshold);\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex e4c2c3583d..4ab6750111 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -238,6 +238,41 @@ test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n \t)\n '\n \n+test_expect_success 'feature.experimental=true generates a cruft pack' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\t\ttest_commit base &&\n+\n+\t\ttest_commit --no-tag foo &&\n+\t\ttest_commit --no-tag bar &&\n+\t\tgit reset HEAD^^ &&\n+\n+\t\tgit -c feature.experimental=true gc &&\n+\n+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n+\t)\n+'\n+\n+test_expect_success 'feature.experimental=false allows explicit cruft packs' '\n+\tgit init crufts &&\n+\ttest_when_finished \"rm -fr crufts\" &&\n+\t(\n+\t\tcd crufts &&\n+\t\ttest_commit base &&\n+\n+\t\ttest_commit --no-tag foo &&\n+\t\ttest_commit --no-tag bar &&\n+\t\tgit reset HEAD^^ &&\n+\n+\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n+\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n+\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n+\t)\n+'\n+\n run_and_wait_for_auto_gc () {\n \t# We read stdout from gc for the side effect of waiting until the\n \t# background gc process exits, closing its fd 9.  Furthermore, the\n-- \n2.37.1.455.g008518b4e5-goog\n\n"},{"id":"460576","messageId":"xmqqr11x800b.fsf@gitster.g","threadId":"58263","inReplyTo":"20220803205721.3686361-2-emilyshaffer@google.com","subject":"Re: [PATCH 1/2] gc: add tests for --cruft and friends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-03T21:56:04Z","receivedAt":"2022-08-03T21:56:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via\n> loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.\n> '--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that\n> doesn't check whether a lone gc run generates these cruft packs.\n> 'gc.cruftPacks' is never exercised.\n>\n> Add some tests to exercise these options to gc in the gc test suite.\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++\n>  1 file changed, 36 insertions(+)\n>\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index cd6c53360d..e4c2c3583d 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -202,6 +202,42 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n>  \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n>  '\n>  \n> +test_expect_success 'gc --cruft generates a cruft pack' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\t\ttest_commit base &&\n> +\n> +\t\ttest_commit --no-tag foo &&\n> +\t\ttest_commit --no-tag bar &&\n> +\t\tgit reset HEAD^^ &&\n> +\n> +\t\tgit gc --cruft &&\n> +\n> +\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n\nWhat guarantees that we will have one pack-*.mtimes?  \n\nI do not mind if we reliably diagnosed it as an error when \"git gc\n--cruft\" created two cruft packs, but I do mind if this call to\nbasename receives two files plus .mtimes suffix and misbehaves.\n\nIs the fact that it is accompanied by a .mtimes file the only clue\nthat a pack is a \"cruft\" pack?  Given that the usefulness of mtimes\nbased expiration approach is doubted, do we want to rely on it (and\nhaving to redesign the test)?\n\nI think the right test would be to\n\n * make a list of all \"in use\" objects;\n\n * see if there is one (or more) packfile that does not contain any\n   \"in use\" objects (look at their .idx file).\n\nIf all packfiles are packs with objects that are still in use, then\nwe did not create a cruft pack.\n\n> +\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n\nDQuote the whole thing, i.e.\n\n\t\ttest_path_is_file \".git/objects/pack/$cruft.pack\"\n\n"},{"id":"460578","messageId":"xmqqfsid7zk4.fsf@gitster.g","threadId":"58263","inReplyTo":"20220803205721.3686361-3-emilyshaffer@google.com","subject":"Re: [PATCH 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-03T22:05:47Z","receivedAt":"2022-08-03T22:05:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> +* `gc.cruftPacks=true` reduces disk space used by unreachable objects during\n> +garbage collection.\n\nOK.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index eeff2b760e..919cc508c5 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -136,6 +136,7 @@ static int gc_config_is_timestamp_never(const char *var)\n>  static void gc_config(void)\n>  {\n>  \tconst char *value;\n> +\tint experimental = 0;\n>  \n>  \tif (!git_config_get_value(\"gc.packrefs\", &value)) {\n>  \t\tif (value && !strcmp(value, \"notbare\"))\n> @@ -148,6 +149,11 @@ static void gc_config(void)\n>  \t    gc_config_is_timestamp_never(\"gc.reflogexpireunreachable\"))\n>  \t\tprune_reflogs = 0;\n>  \n> +\t/* feature.experimental implies gc.cruftPacks=true */\n> +\tgit_config_get_bool(\"feature.experimental\", &experimental);\n> +\tif (experimental)\n> +\t\tcruft_packs = 1;\n> +\n\nI suspect the whole thing can just be:\n\n\tgit_config_get_bool(\"feature.experimental\", &cruft_packs);\n\nIf there is no feature.experimental configuration, the call returns\nnon-zero (we do not check, though) without touching &cruft_packs, if\nthere is feature.experimental configuration, the call returns zero\n(we do not check, though) and cruft_packs is set to either true\n(when experimental) or false (otherwise).\n\nAnd this whole thing happens before we inspect what the more\nspecific configuration gc.cruftPacks says, so...\n\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index e4c2c3583d..4ab6750111 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -238,6 +238,41 @@ test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'feature.experimental=true generates a cruft pack' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\t\ttest_commit base &&\n> +\n> +\t\ttest_commit --no-tag foo &&\n> +\t\ttest_commit --no-tag bar &&\n> +\t\tgit reset HEAD^^ &&\n> +\n> +\t\tgit -c feature.experimental=true gc &&\n> +\n> +\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n> +\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n> +\t)\n> +'\n> +\n> +test_expect_success 'feature.experimental=false allows explicit cruft packs' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\t\ttest_commit base &&\n> +\n> +\t\ttest_commit --no-tag foo &&\n> +\t\ttest_commit --no-tag bar &&\n> +\t\tgit reset HEAD^^ &&\n> +\n> +\t\tgit -c gc.cruftPacks=true -c feature.experimental=false gc &&\n\nOK.  \n\nWhat is not tested is setting feature.experimental explicitly to\nfalse without touching gc.cruftPacks does not use the cruft pack.\n\n"},{"id":"460606","messageId":"220804.86a68ke9d5.gmgdl@evledraar.gmail.com","threadId":"58263","inReplyTo":"20220803205721.3686361-2-emilyshaffer@google.com","subject":"Re: [PATCH 1/2] gc: add tests for --cruft and friends","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-04T07:48:24Z","receivedAt":"2022-08-04T07:49:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Aug 03 2022, Emily Shaffer wrote:\n\n> In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via\n> loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'.\n> '--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that\n> doesn't check whether a lone gc run generates these cruft packs.\n> 'gc.cruftPacks' is never exercised.\n>\n> Add some tests to exercise these options to gc in the gc test suite.\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n>  t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++\n>  1 file changed, 36 insertions(+)\n>\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index cd6c53360d..e4c2c3583d 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -202,6 +202,42 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n>  \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n>  '\n>  \n> +test_expect_success 'gc --cruft generates a cruft pack' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n\nLet's \"test_when_finished\" first, then \"git init\", the point is to clean\nup the directory if we fail.\n\n> +\t(\n> +\t\tcd crufts &&\n> +\t\ttest_commit base &&\n> +\n> +\t\ttest_commit --no-tag foo &&\n> +\t\ttest_commit --no-tag bar &&\n> +\t\tgit reset HEAD^^ &&\n> +\n> +\t\tgit gc --cruft &&\n> +\n> +\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n> +\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n> +\t)\n> +'\n\n...this...\n\n> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n> +\tgit init crufts &&\n> +\ttest_when_finished \"rm -fr crufts\" &&\n> +\t(\n> +\t\tcd crufts &&\n> +\t\ttest_commit base &&\n> +\n> +\t\ttest_commit --no-tag foo &&\n> +\t\ttest_commit --no-tag bar &&\n> +\t\tgit reset HEAD^^ &&\n> +\n> +\t\tgit -c gc.cruftPacks=true gc &&\n> +\n> +\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n> +\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n> +\t)\n> +'\n> +\n\n...whole thing seems to be copy/pasted aside from the git options.\n\nIf so let's factor this into a trivial helper that invokes git \"$@\",\nthen call it with \"gc --cruft\" and \"-c ...\"?\n"},{"id":"460611","messageId":"6803b725-526e-a1c8-f15c-a9ed4a144d4c@github.com","threadId":"58263","inReplyTo":"20220803205721.3686361-3-emilyshaffer@google.com","subject":"Re: [PATCH 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-04T13:05:43Z","receivedAt":"2022-08-04T13:05:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/3/2022 4:57 PM, Emily Shaffer wrote:\n\n> +\t/* feature.experimental implies gc.cruftPacks=true */\n> +\tgit_config_get_bool(\"feature.experimental\", &experimental);\n> +\tif (experimental)\n> +\t\tcruft_packs = 1;\n> +\n\nThis should be grouped into prepare_repo_settings() in repo-settings.c\nso we have a single place to see what is updated by feature.experimental.\n\nThanks,\n-Stolee\n"},{"id":"460627","messageId":"xmqqtu6s6lda.fsf@gitster.g","threadId":"58263","inReplyTo":"220804.86a68ke9d5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] gc: add tests for --cruft and friends","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T16:09:53Z","receivedAt":"2022-08-04T16:10:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> +test_expect_success 'gc --cruft generates a cruft pack' '\n>> +\tgit init crufts &&\n>> +\ttest_when_finished \"rm -fr crufts\" &&\n>\n> Let's \"test_when_finished\" first, then \"git init\", the point is to clean\n> up the directory if we fail.\n\nGood advice.\n\nWe say \"rm -fr\" not \"rm -r\" there because we do not want to see a\nfailure to remove if \"git init\" failed before it manages to create\nthe directory ;-)\n\n>\n>> +\t(\n>> +\t\tcd crufts &&\n>> +\t\ttest_commit base &&\n>> +\n>> +\t\ttest_commit --no-tag foo &&\n>> +\t\ttest_commit --no-tag bar &&\n>> +\t\tgit reset HEAD^^ &&\n>> +\n>> +\t\tgit gc --cruft &&\n>> +\n>> +\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n>> +\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n>> +\t)\n>> +'\n>\n> ...this...\n>\n>> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' '\n>> +\tgit init crufts &&\n>> +\ttest_when_finished \"rm -fr crufts\" &&\n>> +\t(\n>> +\t\tcd crufts &&\n>> +\t\ttest_commit base &&\n>> +\n>> +\t\ttest_commit --no-tag foo &&\n>> +\t\ttest_commit --no-tag bar &&\n>> +\t\tgit reset HEAD^^ &&\n>> +\n>> +\t\tgit -c gc.cruftPacks=true gc &&\n>> +\n>> +\t\tcruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) &&\n>> +\t\ttest_path_is_file .git/objects/pack/$cruft.pack\n>> +\t)\n>> +'\n>> +\n>\n> ...whole thing seems to be copy/pasted aside from the git options.\n>\n> If so let's factor this into a trivial helper that invokes git \"$@\",\n> then call it with \"gc --cruft\" and \"-c ...\"?\n\nWith shell, passing \"here is a series of commands to be run in the\nmiddle of a boilerplate sequence\" is indeed easy to write, but it\ngets harder to follow and quote correctly, which is why I'd rather\nnot see that pattern overused.\n\nA pair of helper functions, one of which prepares a sample history\nto be used, and the other checks if we created one (or more) cruft\npacks, may achieve the same conciseness while remaining to be more\nreadable.  I.e.\n\n    test_when_finished \"rm -fr crufts\" &&\n    git init crufts &&\n    (\n\tcd crufts &&\n\tprepare_history &&\n\n\tgit -c gc.cruftPacks=true gc &&\n\n\tcruft_packs_exist\n    )\n\nperhaps?\n"},{"id":"460628","messageId":"xmqqpmhg6lbs.fsf@gitster.g","threadId":"58263","inReplyTo":"6803b725-526e-a1c8-f15c-a9ed4a144d4c@github.com","subject":"Re: [PATCH 2/2] config: let feature.experimental imply gc.cruftPacks=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-04T16:10:47Z","receivedAt":"2022-08-04T16:10:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> On 8/3/2022 4:57 PM, Emily Shaffer wrote:\n>\n>> +\t/* feature.experimental implies gc.cruftPacks=true */\n>> +\tgit_config_get_bool(\"feature.experimental\", &experimental);\n>> +\tif (experimental)\n>> +\t\tcruft_packs = 1;\n>> +\n>\n> This should be grouped into prepare_repo_settings() in repo-settings.c\n> so we have a single place to see what is updated by feature.experimental.\n\nExcellent.  I forgot about that.  Thanks.\n"}]}