{"thread":{"id":"63182","subject":"[PATCH 0/2] Allow configuration for loose-objects maintenance task","startedAt":"2025-03-24T00:51:55Z","lastAt":"2025-03-24T06:11:31Z","messageCount":4,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"514889","messageId":"pull.1885.git.1742777512.gitgitgadget@gmail.com","threadId":"63182","inReplyTo":null,"subject":"[PATCH 0/2] Allow configuration for loose-objects maintenance task","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-24T00:51:49Z","receivedAt":"2025-03-24T00:51:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The loose-objects task for the git maintenance run command has a hard-coded\nlimit. The limit exists by default for the purposes of background\nmaintenance, but can be misleading if users truly want to clean up all loose\nobjects in one command (and don't want to use git repack). This adds a new\nmaintenance.loose-objects.batchSize config option to help users adjust this\nvalue up or down.\n\nWhen testing, I noticed that progress indicators were not always provided\nwhen isatty(2) is false. This is because the --[no-]quiet option was not\nappropriately passing to child processes. A small change fixes this before\nthe config is added, so we can test the results using stderr output.\n\nThanks,\n\n * Stolee\n\nDerrick Stolee (2):\n maintenance: force progress/no-quiet to children\n maintenance: add loose-objects.batchSize config\n\n Documentation/config/maintenance.adoc | 5 +++++\n Documentation/git-maintenance.adoc | 18 ++++++++++-------\n builtin/gc.c | 20 +++++++++++++++++++\n t/t7900-maintenance.sh | 28 +++++++++++++++++++++++++++\n 4 files changed, 64 insertions(+), 7 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1885%2Fderrickstolee%2Floose-objects-batch-size-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1885/derrickstolee/loose-objects-batch-size-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1885\n-- \ngitgitgadget\n"},{"id":"514890","messageId":"8135c008325fcbdafe4767d02873deda5d873d2b.1742777512.git.gitgitgadget@gmail.com","threadId":"63182","inReplyTo":"pull.1885.git.1742777512.gitgitgadget@gmail.com","subject":"[PATCH 1/2] maintenance: force progress/no-quiet to children","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-24T00:51:50Z","receivedAt":"2025-03-24T00:51:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe --no-quiet option for 'git maintenance run' is supposed to indicate\nthat progress should happen even while ignoring the value of isatty(2).\nHowever, Git implicitly asks child processes to check isatty(2) since\nthese arguments are not passed through.\n\nThe pass through of --no-quiet will be useful in a test in the next\nchange.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 99431fd4674..6672f165bda 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1029,6 +1029,8 @@ static int run_write_commit_graph(struct maintenance_run_opts *opts)\n \n \tif (opts->quiet)\n \t\tstrvec_push(&child.args, \"--no-progress\");\n+\telse\n+\t\tstrvec_push(&child.args, \"--progress\");\n \n \treturn !!run_command(&child);\n }\n@@ -1185,6 +1187,8 @@ static int pack_loose(struct maintenance_run_opts *opts)\n \tstrvec_push(&pack_proc.args, \"pack-objects\");\n \tif (opts->quiet)\n \t\tstrvec_push(&pack_proc.args, \"--quiet\");\n+\telse\n+\t\tstrvec_push(&pack_proc.args, \"--no-quiet\");\n \tstrvec_pushf(&pack_proc.args, \"%s/pack/loose\", r->objects->odb->path);\n \n \tpack_proc.in = -1;\n@@ -1263,6 +1267,8 @@ static int multi_pack_index_write(struct maintenance_run_opts *opts)\n \n \tif (opts->quiet)\n \t\tstrvec_push(&child.args, \"--no-progress\");\n+\telse\n+\t\tstrvec_push(&child.args, \"--progress\");\n \n \tif (run_command(&child))\n \t\treturn error(_(\"failed to write multi-pack-index\"));\n@@ -1279,6 +1285,8 @@ static int multi_pack_index_expire(struct maintenance_run_opts *opts)\n \n \tif (opts->quiet)\n \t\tstrvec_push(&child.args, \"--no-progress\");\n+\telse\n+\t\tstrvec_push(&child.args, \"--progress\");\n \n \tif (run_command(&child))\n \t\treturn error(_(\"'git multi-pack-index expire' failed\"));\n@@ -1335,6 +1343,8 @@ static int multi_pack_index_repack(struct maintenance_run_opts *opts)\n \n \tif (opts->quiet)\n \t\tstrvec_push(&child.args, \"--no-progress\");\n+\telse\n+\t\tstrvec_push(&child.args, \"--progress\");\n \n \tstrvec_pushf(&child.args, \"--batch-size=%\"PRIuMAX,\n \t\t\t\t (uintmax_t)get_auto_pack_size());\n-- \ngitgitgadget\n\n"},{"id":"514891","messageId":"6ed537862fa7585928fde33d21b77367a7fb708d.1742777512.git.gitgitgadget@gmail.com","threadId":"63182","inReplyTo":"pull.1885.git.1742777512.gitgitgadget@gmail.com","subject":"[PATCH 2/2] maintenance: add loose-objects.batchSize config","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-24T00:51:51Z","receivedAt":"2025-03-24T00:51:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'loose-objects' task of 'git maintenance run' first deletes loose\nobjects that exit within packfiles and then collects loose objects into\na packfile. This second step uses an implicit limit of fifty thousand\nthat cannot be modified by users.\n\nAdd a new config option that allows this limit to be adjusted or ignored\nentirely.\n\nWhile creating tests for this option, I noticed that actually there was\nan off-by-one error due to the strict comparison in the limit check. I\nconsidered making the limit check turn true on equality, but instead I\nthought to use INT_MAX as a \"no limit\" barrier which should mean it's\nnever possible to hit the limit. Thus, a new decrement to the limit is\nprovided if the value is positive. (The restriction to positive values\nis to avoid underflow if INT_MIN is configured.)\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/config/maintenance.adoc | 5 +++++\n Documentation/git-maintenance.adoc | 18 ++++++++++-------\n builtin/gc.c | 10 ++++++++++\n t/t7900-maintenance.sh | 28 +++++++++++++++++++++++++++\n 4 files changed, 54 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/maintenance.adoc b/Documentation/config/maintenance.adoc\nindex 72a9d6cf816..42f9545da0e 100644\n--- a/Documentation/config/maintenance.adoc\n+++ b/Documentation/config/maintenance.adoc\n@@ -61,6 +61,11 @@ maintenance.loose-objects.auto::\n \tloose objects is at least the value of `maintenance.loose-objects.auto`.\n \tThe default value is 100.\n \n+maintenance.loose-objects.batchSize::\n+\tThis integer config option controls the maximum number of loose objects\n+\twritten into a packfile during the `loose-objects` task. The default is\n+\tfifty thousand. Use value `0` to indicate no limit.\n+\n maintenance.incremental-repack.auto::\n \tThis integer config option controls how often the `incremental-repack`\n \ttask should be run as part of `git maintenance run --auto`. If zero,\ndiff --git a/Documentation/git-maintenance.adoc b/Documentation/git-maintenance.adoc\nindex 0450d74aff1..c90b370b1fc 100644\n--- a/Documentation/git-maintenance.adoc\n+++ b/Documentation/git-maintenance.adoc\n@@ -126,13 +126,17 @@ loose-objects::\n \tobjects that already exist in a pack-file; concurrent Git processes\n \twill examine the pack-file for the object data instead of the loose\n \tobject. Second, it creates a new pack-file (starting with \"loose-\")\n-\tcontaining a batch of loose objects. The batch size is limited to 50\n-\tthousand objects to prevent the job from taking too long on a\n-\trepository with many loose objects. The `gc` task writes unreachable\n-\tobjects as loose objects to be cleaned up by a later step only if\n-\tthey are not re-added to a pack-file; for this reason it is not\n-\tadvisable to enable both the `loose-objects` and `gc` tasks at the\n-\tsame time.\n+\tcontaining a batch of loose objects.\n++\n+The batch size defaults to fifty thousand objects to prevent the job from\n+taking too long on a repository with many loose objects. Use the\n+`maintenance.loose-objects.batchSize` config option to adjust this size,\n+including a value of `0` to remove the limit.\n++\n+The `gc` task writes unreachable objects as loose objects to be cleaned up\n+by a later step only if they are not re-added to a pack-file; for this\n+reason it is not advisable to enable both the `loose-objects` and `gc`\n+tasks at the same time.\n \n incremental-repack::\n \tThe `incremental-repack` job repacks the object directory\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6672f165bda..817081e1a50 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1163,6 +1163,7 @@ static int write_loose_object_to_stdin(const struct object_id *oid,\n \n \tfprintf(d->in, \"%s\\n\", oid_to_hex(oid));\n \n+\t/* If batch_size is INT_MAX, then this will return 0 always. */\n \treturn ++(d->count) > d->batch_size;\n }\n \n@@ -1208,6 +1209,15 @@ static int pack_loose(struct maintenance_run_opts *opts)\n \tdata.count = 0;\n \tdata.batch_size = 50000;\n \n+\trepo_config_get_int(r, \"maintenance.loose-objects.batchSize\",\n+\t\t\t &data.batch_size);\n+\n+\t/* If configured as 0, then remove limit. */\n+\tif (!data.batch_size)\n+\t\tdata.batch_size = INT_MAX;\n+\telse if (data.batch_size > 0)\n+\t\tdata.batch_size--; /* Decrease for equality on limit. */\n+\n \tfor_each_loose_file_in_objdir(r->objects->odb->path,\n \t\t\t\t write_loose_object_to_stdin,\n \t\t\t\t NULL,\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 1909aed95e0..834ddb5ad68 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -306,6 +306,34 @@ test_expect_success 'maintenance.loose-objects.auto' '\n \ttest_subcommand git prune-packed --quiet err &&\n+\tgrep \"Enumerating objects: 50, done.\" err &&\n+\n+\tGIT_PROGRESS_DELAY=0 \\\n+\tgit -C loose-batch maintenance run --no-quiet --task=loose-objects 2>err &&\n+\tgrep \"Enumerating objects: 50, done.\" err &&\n+\n+\tGIT_PROGRESS_DELAY=0 \\\n+\tgit -C loose-batch maintenance run --no-quiet --task=loose-objects 2>err &&\n+\tgrep \"Enumerating objects: 2, done.\" err &&\n+\n+\tGIT_PROGRESS_DELAY=0 \\\n+\tgit -C loose-batch maintenance run --no-quiet --task=loose-objects 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'incremental-repack task' '\n \tpackDir=.git/objects/pack &&\n \tfor i in $(test_seq 1 5)\n-- \ngitgitgadget\n"},{"id":"514903","messageId":"xmqqbjtrot4e.fsf@gitster.g","threadId":"63182","inReplyTo":"6ed537862fa7585928fde33d21b77367a7fb708d.1742777512.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] maintenance: add loose-objects.batchSize config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-24T06:11:29Z","receivedAt":"2025-03-24T06:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The 'loose-objects' task of 'git maintenance run' first deletes loose\n> objects that exit within packfiles and then collects loose objects into\n\n\"exit\" -> \"exist\"? It may read better to also do \"collects\" ->\n\"collects remaining\".\n\n> a packfile. This second step uses an implicit limit of fifty thousand\n> that cannot be modified by users.\n>\n> Add a new config option that allows this limit to be adjusted or ignored\n> entirely.\n>\n> While creating tests for this option, I noticed that actually there was\n> an off-by-one error due to the strict comparison in the limit check.\n\nAhh, I, contrary to my usual routine, started reading from the code\nchange before reading the proposed log message and was wondering\nabout this exact point. \n\n> +\t/* If batch_size is INT_MAX, then this will return 0 always. */\n\nCute ;-).\n\n> \treturn ++(d->count) > d->batch_size;\n> }\n"}]}