{"thread":{"id":"61330","subject":"[PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)","startedAt":"2024-04-17T06:16:33Z","lastAt":"2024-04-18T05:48:14Z","messageCount":9,"participants":["Patrick Steinhardt","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"493057","messageId":"cover.1713334241.git.ps@pks.im","threadId":"61330","inReplyTo":null,"subject":"[PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-17T06:16:26Z","receivedAt":"2024-04-17T06:16:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series adapts git-receive-pack(1) to spawn `git\nmaintenance run --auto` instead of `git gc --auto` like all the other\nparts of our codebase do nowadays. This removes the last internal user\nof `git gc --auto`.\n\nPatrick\n\nPatrick Steinhardt (2):\n  run-command: introduce function to prepare auto-maintenance process\n  builtin/receive-pack: convert to use git-maintenance(1)\n\n Documentation/config/receive.txt |  2 +-\n builtin/receive-pack.c           | 21 ++++++++++-----------\n run-command.c                    | 19 +++++++++++++------\n run-command.h                    |  7 +++++++\n 4 files changed, 31 insertions(+), 18 deletions(-)\n\n-- \n2.44.GIT\n\n"},{"id":"493058","messageId":"929b6bfa08132523ee97f5adc376c3600f779a99.1713334241.git.ps@pks.im","threadId":"61330","inReplyTo":"cover.1713334241.git.ps@pks.im","subject":"[PATCH 1/2] run-command: introduce function to prepare auto-maintenance process","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-17T06:16:31Z","receivedAt":"2024-04-17T06:16:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `run_auto_maintenance()` function is responsible for spawning a new\n`git maintenance run --auto` process. To do so, it sets up the `sturct\nchild_process` and then runs it by executing `run_command()` directly.\nThis is rather inflexible in case callers want to modify the child\nprocess somewhat, e.g. to redirect stderr or stdout.\n\nIntroduce a new `prepare_auto_maintenance()` function to plug this gap.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n run-command.c | 19 +++++++++++++------\n run-command.h |  7 +++++++\n 2 files changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0e7435718a..1b821042b4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1793,20 +1793,27 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)\n \t\ttrace2_region_leave(tr2_category, tr2_label, NULL);\n }\n \n-int run_auto_maintenance(int quiet)\n+int prepare_auto_maintenance(int quiet, struct child_process *maint)\n {\n \tint enabled;\n-\tstruct child_process maint = CHILD_PROCESS_INIT;\n \n \tif (!git_config_get_bool(\"maintenance.auto\", &enabled) &&\n \t    !enabled)\n \t\treturn 0;\n \n-\tmaint.git_cmd = 1;\n-\tmaint.close_object_store = 1;\n-\tstrvec_pushl(&maint.args, \"maintenance\", \"run\", \"--auto\", NULL);\n-\tstrvec_push(&maint.args, quiet ? \"--quiet\" : \"--no-quiet\");\n+\tmaint->git_cmd = 1;\n+\tmaint->close_object_store = 1;\n+\tstrvec_pushl(&maint->args, \"maintenance\", \"run\", \"--auto\", NULL);\n+\tstrvec_push(&maint->args, quiet ? \"--quiet\" : \"--no-quiet\");\n+\n+\treturn 1;\n+}\n \n+int run_auto_maintenance(int quiet)\n+{\n+\tstruct child_process maint = CHILD_PROCESS_INIT;\n+\tif (!prepare_auto_maintenance(quiet, &maint))\n+\t\treturn 0;\n \treturn run_command(&maint);\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 1f22cc3827..55f6631a2a 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -217,6 +217,13 @@ int finish_command_in_signal(struct child_process *);\n  */\n int run_command(struct child_process *);\n \n+/*\n+ * Prepare a `struct child_process` to run auto-maintenance. Returns 1 if the\n+ * process has been prepared and is ready to run, or 0 in case auto-maintenance\n+ * should be skipped.\n+ */\n+int prepare_auto_maintenance(int quiet, struct child_process *maint);\n+\n /*\n  * Trigger an auto-gc\n  */\n-- \n2.44.GIT\n\n"},{"id":"493059","messageId":"c7bcc83d3a96b613db4fba9edfbb6d964b338ed5.1713334241.git.ps@pks.im","threadId":"61330","inReplyTo":"cover.1713334241.git.ps@pks.im","subject":"[PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-17T06:16:35Z","receivedAt":"2024-04-17T06:16:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 850b6edefa (auto-gc: extract a reusable helper from \"git fetch\",\n2020-05-06), we have introduced a helper function `run_auto_gc()` that\nkicks off `git gc --auto`. The intent of this function was to pass down\nthe \"--quiet\" flag to git-gc(1) as required without duplicating this at\nall callsites. In 7c3e9e8cfb (auto-gc: pass --quiet down from am,\ncommit, merge and rebase, 2020-05-06) we then converted callsites that\nneed to pass down this flag to use the new helper function. This has the\nnotable omission of git-receive-pack(1), which is the only remaining\nuser of `git gc --auto` that sets up the proccess manually. This is\nprobably because it unconditionally passes down the `--quiet` flag and\nthus didn't benefit much from the new helper function.\n\nIn a95ce12430 (maintenance: replace run_auto_gc(), 2020-09-17) we then\nreplaced `run_auto_gc()` with `run_auto_maintenance()` which invokes\ngit-maintenance(1) instead of git-gc(1). This command is the modern\nreplacement for git-gc(1) and is both more thorough and also more\nflexible because administrators can configure which tasks exactly to run\nduring maintenance.\n\nBut due to git-receive-pack(1) not using `run_auto_gc()` in the first\nplace it did not get converted to use git-maintenance(1) like we do\neverywhere else now. Address this oversight and start to use the newly\nintroduced function `prepare_auto_maintenance()`. This will also make it\neasier for us to adapt this code together with all the other callsites\nthat invoke auto-maintenance in the future.\n\nThis removes the last internal user of `git gc --auto`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/config/receive.txt |  2 +-\n builtin/receive-pack.c           | 21 ++++++++++-----------\n 2 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config/receive.txt b/Documentation/config/receive.txt\nindex c77e55b1cd..36a1e6f2d2 100644\n--- a/Documentation/config/receive.txt\n+++ b/Documentation/config/receive.txt\n@@ -8,7 +8,7 @@ receive.advertisePushOptions::\n \tcapability to its clients. False by default.\n \n receive.autogc::\n-\tBy default, git-receive-pack will run \"git-gc --auto\" after\n+\tBy default, git-receive-pack will run \"git maintenance run --auto\" after\n \treceiving data from git-push and updating refs.  You can stop\n \tit by setting this variable to false.\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 56d8a77ed7..e8d7df14b6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2585,17 +2585,16 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tif (auto_gc) {\n \t\t\tstruct child_process proc = CHILD_PROCESS_INIT;\n \n-\t\t\tproc.no_stdin = 1;\n-\t\t\tproc.stdout_to_stderr = 1;\n-\t\t\tproc.err = use_sideband ? -1 : 0;\n-\t\t\tproc.git_cmd = proc.close_object_store = 1;\n-\t\t\tstrvec_pushl(&proc.args, \"gc\", \"--auto\", \"--quiet\",\n-\t\t\t\t     NULL);\n-\n-\t\t\tif (!start_command(&proc)) {\n-\t\t\t\tif (use_sideband)\n-\t\t\t\t\tcopy_to_sideband(proc.err, -1, NULL);\n-\t\t\t\tfinish_command(&proc);\n+\t\t\tif (prepare_auto_maintenance(1, &proc)) {\n+\t\t\t\tproc.no_stdin = 1;\n+\t\t\t\tproc.stdout_to_stderr = 1;\n+\t\t\t\tproc.err = use_sideband ? -1 : 0;\n+\n+\t\t\t\tif (!start_command(&proc)) {\n+\t\t\t\t\tif (use_sideband)\n+\t\t\t\t\t\tcopy_to_sideband(proc.err, -1, NULL);\n+\t\t\t\t\tfinish_command(&proc);\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t\tif (auto_update_server_info)\n-- \n2.44.GIT\n\n"},{"id":"493098","messageId":"xmqq8r1c9ea2.fsf@gitster.g","threadId":"61330","inReplyTo":"929b6bfa08132523ee97f5adc376c3600f779a99.1713334241.git.ps@pks.im","subject":"Re: [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-17T15:53:25Z","receivedAt":"2024-04-17T15:53:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The `run_auto_maintenance()` function is responsible for spawning a new\n> `git maintenance run --auto` process. To do so, it sets up the `sturct\n> child_process` and then runs it by executing `run_command()` directly.\n> This is rather inflexible in case callers want to modify the child\n> process somewhat, e.g. to redirect stderr or stdout.\n>\n> Introduce a new `prepare_auto_maintenance()` function to plug this gap.\n\nI guess the mention of \"inflexible\" and \"redirection\" above refers\nto some incompatibile behaviour we would introduce if we just\nreplaced the manual spawning of \"gc --auto\" with a call to\nrun_auto_maintenance(), but I would have expected that will be\nsolved by making the interface to run_auto_maintenance() richer, not\nforcing the callers that would want to deviate from the norm to\nwrite the second half of the run_auto_maintenance() themselves.\n\n> +int run_auto_maintenance(int quiet)\n> +{\n> +\tstruct child_process maint = CHILD_PROCESS_INIT;\n> +\tif (!prepare_auto_maintenance(quiet, &maint))\n> +\t\treturn 0;\n>  \treturn run_command(&maint);\n>  }\n\nBut given that the \"second half\" is to just call run_command() on\nthe prepared child control structure, it is probably not a huge\ndeal.  It just felt somewhat an uneven API surface that 'quiet' can\nbe controlled with just a single bit and doing anything more than\nthat would require the caller to go into the structure to tweak.\n\nWill queue.  Thanks.\n"},{"id":"493099","messageId":"CAOLa=ZTRY6hmQqC2BozFkLmLQWnkQ4+sSug_8N1ZY9+aHNqCTg@mail.gmail.com","threadId":"61330","inReplyTo":"cover.1713334241.git.ps@pks.im","subject":"Re: [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-04-17T16:19:43Z","receivedAt":"2024-04-17T16:19:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Hi,\n>\n> this small patch series adapts git-receive-pack(1) to spawn `git\n> maintenance run --auto` instead of `git gc --auto` like all the other\n> parts of our codebase do nowadays. This removes the last internal user\n> of `git gc --auto`.\n>\n\nI don't have enough context here, so why do this?\n"},{"id":"493101","messageId":"CAOLa=ZQhv5m4RPwsQg1F2zLU86qjE9Ew6FhCpOsPWWM3FoHATw@mail.gmail.com","threadId":"61330","inReplyTo":"c7bcc83d3a96b613db4fba9edfbb6d964b338ed5.1713334241.git.ps@pks.im","subject":"Re: [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-04-17T16:50:46Z","receivedAt":"2024-04-17T16:50:48Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> In 850b6edefa (auto-gc: extract a reusable helper from \"git fetch\",\n> 2020-05-06), we have introduced a helper function `run_auto_gc()` that\n> kicks off `git gc --auto`. The intent of this function was to pass down\n> the \"--quiet\" flag to git-gc(1) as required without duplicating this at\n> all callsites. In 7c3e9e8cfb (auto-gc: pass --quiet down from am,\n> commit, merge and rebase, 2020-05-06) we then converted callsites that\n> need to pass down this flag to use the new helper function. This has the\n> notable omission of git-receive-pack(1), which is the only remaining\n> user of `git gc --auto` that sets up the proccess manually. This is\n> probably because it unconditionally passes down the `--quiet` flag and\n> thus didn't benefit much from the new helper function.\n>\n> In a95ce12430 (maintenance: replace run_auto_gc(), 2020-09-17) we then\n> replaced `run_auto_gc()` with `run_auto_maintenance()` which invokes\n> git-maintenance(1) instead of git-gc(1). This command is the modern\n> replacement for git-gc(1) and is both more thorough and also more\n> flexible because administrators can configure which tasks exactly to run\n> during maintenance.\n>\n> But due to git-receive-pack(1) not using `run_auto_gc()` in the first\n> place it did not get converted to use git-maintenance(1) like we do\n> everywhere else now. Address this oversight and start to use the newly\n> introduced function `prepare_auto_maintenance()`. This will also make it\n> easier for us to adapt this code together with all the other callsites\n> that invoke auto-maintenance in the future.\n\nThis commit explains my earlier question. Thanks.\n"},{"id":"493102","messageId":"xmqqwmow7wwp.fsf@gitster.g","threadId":"61330","inReplyTo":"CAOLa=ZTRY6hmQqC2BozFkLmLQWnkQ4+sSug_8N1ZY9+aHNqCTg@mail.gmail.com","subject":"Re: [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-17T16:53:58Z","receivedAt":"2024-04-17T16:58:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>> this small patch series adapts git-receive-pack(1) to spawn `git\n>> maintenance run --auto` instead of `git gc --auto` like all the other\n>> parts of our codebase do nowadays. This removes the last internal user\n>> of `git gc --auto`.\n>\n> I don't have enough context here, so why do this?\n\nI think the intent of a95ce124 (maintenance: replace run_auto_gc(),\n2020-09-17) was to update all codepaths that run \"git gc --auto\" to\ninstead run \"git maintenance --auto\", but only updated the ones that\nused to call run_auto_gc().  The codepath Patrick found runs \"git gc\n--auto\" without using run_auto_gc() and was left behind when the\nothers were converted.\n\nSo why do this?  I think \"To follow through a95ce124 started\" would\nprobably be a good enough reason, if a reader is on board with what\na95ce124 wanted to do.\n\nDo we have a handy reference that compares \"gc --auto\" and\n\"maintenance --auto\"?  Are they essentially the same thing these\ndays?  What are the things that is done by one but not by the other?\n\nTHanks.\n"},{"id":"493116","messageId":"ZiCzAodY-L0QvX8s@tanuki","threadId":"61330","inReplyTo":"xmqqwmow7wwp.fsf@gitster.g","subject":"Re: [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-18T05:43:30Z","receivedAt":"2024-04-18T05:43:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 17, 2024 at 09:53:58AM -0700, Junio C Hamano wrote:\n> Karthik Nayak <karthik.188@gmail.com> writes:\n> \n> >> this small patch series adapts git-receive-pack(1) to spawn `git\n> >> maintenance run --auto` instead of `git gc --auto` like all the other\n> >> parts of our codebase do nowadays. This removes the last internal user\n> >> of `git gc --auto`.\n> >\n> > I don't have enough context here, so why do this?\n\nFair enough, I was a bit lazy with the cover letter as I thought that\nthe second patch already explains it well enough. But true, I should've\nset the stage a bit better.\n\n> I think the intent of a95ce124 (maintenance: replace run_auto_gc(),\n> 2020-09-17) was to update all codepaths that run \"git gc --auto\" to\n> instead run \"git maintenance --auto\", but only updated the ones that\n> used to call run_auto_gc().  The codepath Patrick found runs \"git gc\n> --auto\" without using run_auto_gc() and was left behind when the\n> others were converted.\n\nYup, that was triggered this patch series. I was puzzled that we still\nrun `git gc --auto` at all.\n\n> So why do this?  I think \"To follow through a95ce124 started\" would\n> probably be a good enough reason, if a reader is on board with what\n> a95ce124 wanted to do.\n> \n> Do we have a handy reference that compares \"gc --auto\" and\n> \"maintenance --auto\"?  Are they essentially the same thing these\n> days?  What are the things that is done by one but not by the other?\n\ngit-maintenance(1) is a superset of what git-gc(1) does. It is\nstructured around different \"tasks\" that the user can enable or disable\nat a whim. By default, only a single \"gc\" task is enabled, which makes\nit a drop-in replacement for what we had before the conversion.\n\nBut users can ask git-maintenance(1) to perform housekeeping to their\nwill. Instead of running git-gc(1), they can configure indididual tasks\nto repack objects via multi-pack indices, pack only loose objects,\nupdate the commit-graph or repack references.\n\nSo overall, git-maintenance(1) is the modern and more flexible\nreplacement for git-gc(1) that gives more tuning knobs.\n\nPatrick\n"},{"id":"493117","messageId":"ZiC0GEnlxMOjst9w@tanuki","threadId":"61330","inReplyTo":"xmqq8r1c9ea2.fsf@gitster.g","subject":"Re: [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-18T05:48:08Z","receivedAt":"2024-04-18T05:48:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 17, 2024 at 08:53:25AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The `run_auto_maintenance()` function is responsible for spawning a new\n> > `git maintenance run --auto` process. To do so, it sets up the `sturct\n> > child_process` and then runs it by executing `run_command()` directly.\n> > This is rather inflexible in case callers want to modify the child\n> > process somewhat, e.g. to redirect stderr or stdout.\n> >\n> > Introduce a new `prepare_auto_maintenance()` function to plug this gap.\n> \n> I guess the mention of \"inflexible\" and \"redirection\" above refers\n> to some incompatibile behaviour we would introduce if we just\n> replaced the manual spawning of \"gc --auto\" with a call to\n> run_auto_maintenance(), but I would have expected that will be\n> solved by making the interface to run_auto_maintenance() richer, not\n> forcing the callers that would want to deviate from the norm to\n> write the second half of the run_auto_maintenance() themselves.\n> \n> > +int run_auto_maintenance(int quiet)\n> > +{\n> > +\tstruct child_process maint = CHILD_PROCESS_INIT;\n> > +\tif (!prepare_auto_maintenance(quiet, &maint))\n> > +\t\treturn 0;\n> >  \treturn run_command(&maint);\n> >  }\n> \n> But given that the \"second half\" is to just call run_command() on\n> the prepared child control structure, it is probably not a huge\n> deal.  It just felt somewhat an uneven API surface that 'quiet' can\n> be controlled with just a single bit and doing anything more than\n> that would require the caller to go into the structure to tweak.\n> \n> Will queue.  Thanks.\n\ngit-receive-pack(1) needs to do some magic with file descriptors and\nneeds to copy output of the command to the sideband. I first thought\nabout extending `run_auto_maintenance()` to support this, but found it\nto be messy as this really is quite a specific usecase. So I figured\nthat prepping the `struct child_process` like this is the nicer way to\napproach it.\n\nThanks.\n\nPatrick\n"}]}