* [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)
@ 2024-04-17 6:16 Patrick Steinhardt
2024-04-17 6:16 ` [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process Patrick Steinhardt
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Patrick Steinhardt @ 2024-04-17 6:16 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 692 bytes --]
Hi,
this small patch series adapts git-receive-pack(1) to spawn `git
maintenance run --auto` instead of `git gc --auto` like all the other
parts of our codebase do nowadays. This removes the last internal user
of `git gc --auto`.
Patrick
Patrick Steinhardt (2):
run-command: introduce function to prepare auto-maintenance process
builtin/receive-pack: convert to use git-maintenance(1)
Documentation/config/receive.txt | 2 +-
builtin/receive-pack.c | 21 ++++++++++-----------
run-command.c | 19 +++++++++++++------
run-command.h | 7 +++++++
4 files changed, 31 insertions(+), 18 deletions(-)
--
2.44.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process
2024-04-17 6:16 [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
@ 2024-04-17 6:16 ` Patrick Steinhardt
2024-04-17 15:53 ` Junio C Hamano
2024-04-17 6:16 ` [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
2024-04-17 16:19 ` [PATCH 0/2] " Karthik Nayak
2 siblings, 1 reply; 9+ messages in thread
From: Patrick Steinhardt @ 2024-04-17 6:16 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]
The `run_auto_maintenance()` function is responsible for spawning a new
`git maintenance run --auto` process. To do so, it sets up the `sturct
child_process` and then runs it by executing `run_command()` directly.
This is rather inflexible in case callers want to modify the child
process somewhat, e.g. to redirect stderr or stdout.
Introduce a new `prepare_auto_maintenance()` function to plug this gap.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
run-command.c | 19 +++++++++++++------
run-command.h | 7 +++++++
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/run-command.c b/run-command.c
index 0e7435718a..1b821042b4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1793,20 +1793,27 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
trace2_region_leave(tr2_category, tr2_label, NULL);
}
-int run_auto_maintenance(int quiet)
+int prepare_auto_maintenance(int quiet, struct child_process *maint)
{
int enabled;
- struct child_process maint = CHILD_PROCESS_INIT;
if (!git_config_get_bool("maintenance.auto", &enabled) &&
!enabled)
return 0;
- maint.git_cmd = 1;
- maint.close_object_store = 1;
- strvec_pushl(&maint.args, "maintenance", "run", "--auto", NULL);
- strvec_push(&maint.args, quiet ? "--quiet" : "--no-quiet");
+ maint->git_cmd = 1;
+ maint->close_object_store = 1;
+ strvec_pushl(&maint->args, "maintenance", "run", "--auto", NULL);
+ strvec_push(&maint->args, quiet ? "--quiet" : "--no-quiet");
+
+ return 1;
+}
+int run_auto_maintenance(int quiet)
+{
+ struct child_process maint = CHILD_PROCESS_INIT;
+ if (!prepare_auto_maintenance(quiet, &maint))
+ return 0;
return run_command(&maint);
}
diff --git a/run-command.h b/run-command.h
index 1f22cc3827..55f6631a2a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -217,6 +217,13 @@ int finish_command_in_signal(struct child_process *);
*/
int run_command(struct child_process *);
+/*
+ * Prepare a `struct child_process` to run auto-maintenance. Returns 1 if the
+ * process has been prepared and is ready to run, or 0 in case auto-maintenance
+ * should be skipped.
+ */
+int prepare_auto_maintenance(int quiet, struct child_process *maint);
+
/*
* Trigger an auto-gc
*/
--
2.44.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1)
2024-04-17 6:16 [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
2024-04-17 6:16 ` [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process Patrick Steinhardt
@ 2024-04-17 6:16 ` Patrick Steinhardt
2024-04-17 16:50 ` Karthik Nayak
2024-04-17 16:19 ` [PATCH 0/2] " Karthik Nayak
2 siblings, 1 reply; 9+ messages in thread
From: Patrick Steinhardt @ 2024-04-17 6:16 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]
In 850b6edefa (auto-gc: extract a reusable helper from "git fetch",
2020-05-06), we have introduced a helper function `run_auto_gc()` that
kicks off `git gc --auto`. The intent of this function was to pass down
the "--quiet" flag to git-gc(1) as required without duplicating this at
all callsites. In 7c3e9e8cfb (auto-gc: pass --quiet down from am,
commit, merge and rebase, 2020-05-06) we then converted callsites that
need to pass down this flag to use the new helper function. This has the
notable omission of git-receive-pack(1), which is the only remaining
user of `git gc --auto` that sets up the proccess manually. This is
probably because it unconditionally passes down the `--quiet` flag and
thus didn't benefit much from the new helper function.
In a95ce12430 (maintenance: replace run_auto_gc(), 2020-09-17) we then
replaced `run_auto_gc()` with `run_auto_maintenance()` which invokes
git-maintenance(1) instead of git-gc(1). This command is the modern
replacement for git-gc(1) and is both more thorough and also more
flexible because administrators can configure which tasks exactly to run
during maintenance.
But due to git-receive-pack(1) not using `run_auto_gc()` in the first
place it did not get converted to use git-maintenance(1) like we do
everywhere else now. Address this oversight and start to use the newly
introduced function `prepare_auto_maintenance()`. This will also make it
easier for us to adapt this code together with all the other callsites
that invoke auto-maintenance in the future.
This removes the last internal user of `git gc --auto`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/config/receive.txt | 2 +-
builtin/receive-pack.c | 21 ++++++++++-----------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/Documentation/config/receive.txt b/Documentation/config/receive.txt
index c77e55b1cd..36a1e6f2d2 100644
--- a/Documentation/config/receive.txt
+++ b/Documentation/config/receive.txt
@@ -8,7 +8,7 @@ receive.advertisePushOptions::
capability to its clients. False by default.
receive.autogc::
- By default, git-receive-pack will run "git-gc --auto" after
+ By default, git-receive-pack will run "git maintenance run --auto" after
receiving data from git-push and updating refs. You can stop
it by setting this variable to false.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56d8a77ed7..e8d7df14b6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2585,17 +2585,16 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
if (auto_gc) {
struct child_process proc = CHILD_PROCESS_INIT;
- proc.no_stdin = 1;
- proc.stdout_to_stderr = 1;
- proc.err = use_sideband ? -1 : 0;
- proc.git_cmd = proc.close_object_store = 1;
- strvec_pushl(&proc.args, "gc", "--auto", "--quiet",
- NULL);
-
- if (!start_command(&proc)) {
- if (use_sideband)
- copy_to_sideband(proc.err, -1, NULL);
- finish_command(&proc);
+ if (prepare_auto_maintenance(1, &proc)) {
+ proc.no_stdin = 1;
+ proc.stdout_to_stderr = 1;
+ proc.err = use_sideband ? -1 : 0;
+
+ if (!start_command(&proc)) {
+ if (use_sideband)
+ copy_to_sideband(proc.err, -1, NULL);
+ finish_command(&proc);
+ }
}
}
if (auto_update_server_info)
--
2.44.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process
2024-04-17 6:16 ` [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process Patrick Steinhardt
@ 2024-04-17 15:53 ` Junio C Hamano
2024-04-18 5:48 ` Patrick Steinhardt
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2024-04-17 15:53 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Derrick Stolee
Patrick Steinhardt <ps@pks.im> writes:
> The `run_auto_maintenance()` function is responsible for spawning a new
> `git maintenance run --auto` process. To do so, it sets up the `sturct
> child_process` and then runs it by executing `run_command()` directly.
> This is rather inflexible in case callers want to modify the child
> process somewhat, e.g. to redirect stderr or stdout.
>
> Introduce a new `prepare_auto_maintenance()` function to plug this gap.
I guess the mention of "inflexible" and "redirection" above refers
to some incompatibile behaviour we would introduce if we just
replaced the manual spawning of "gc --auto" with a call to
run_auto_maintenance(), but I would have expected that will be
solved by making the interface to run_auto_maintenance() richer, not
forcing the callers that would want to deviate from the norm to
write the second half of the run_auto_maintenance() themselves.
> +int run_auto_maintenance(int quiet)
> +{
> + struct child_process maint = CHILD_PROCESS_INIT;
> + if (!prepare_auto_maintenance(quiet, &maint))
> + return 0;
> return run_command(&maint);
> }
But given that the "second half" is to just call run_command() on
the prepared child control structure, it is probably not a huge
deal. It just felt somewhat an uneven API surface that 'quiet' can
be controlled with just a single bit and doing anything more than
that would require the caller to go into the structure to tweak.
Will queue. Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)
2024-04-17 6:16 [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
2024-04-17 6:16 ` [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process Patrick Steinhardt
2024-04-17 6:16 ` [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
@ 2024-04-17 16:19 ` Karthik Nayak
2024-04-17 16:53 ` Junio C Hamano
2 siblings, 1 reply; 9+ messages in thread
From: Karthik Nayak @ 2024-04-17 16:19 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Derrick Stolee, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 335 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this small patch series adapts git-receive-pack(1) to spawn `git
> maintenance run --auto` instead of `git gc --auto` like all the other
> parts of our codebase do nowadays. This removes the last internal user
> of `git gc --auto`.
>
I don't have enough context here, so why do this?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1)
2024-04-17 6:16 ` [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
@ 2024-04-17 16:50 ` Karthik Nayak
0 siblings, 0 replies; 9+ messages in thread
From: Karthik Nayak @ 2024-04-17 16:50 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Derrick Stolee, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> In 850b6edefa (auto-gc: extract a reusable helper from "git fetch",
> 2020-05-06), we have introduced a helper function `run_auto_gc()` that
> kicks off `git gc --auto`. The intent of this function was to pass down
> the "--quiet" flag to git-gc(1) as required without duplicating this at
> all callsites. In 7c3e9e8cfb (auto-gc: pass --quiet down from am,
> commit, merge and rebase, 2020-05-06) we then converted callsites that
> need to pass down this flag to use the new helper function. This has the
> notable omission of git-receive-pack(1), which is the only remaining
> user of `git gc --auto` that sets up the proccess manually. This is
> probably because it unconditionally passes down the `--quiet` flag and
> thus didn't benefit much from the new helper function.
>
> In a95ce12430 (maintenance: replace run_auto_gc(), 2020-09-17) we then
> replaced `run_auto_gc()` with `run_auto_maintenance()` which invokes
> git-maintenance(1) instead of git-gc(1). This command is the modern
> replacement for git-gc(1) and is both more thorough and also more
> flexible because administrators can configure which tasks exactly to run
> during maintenance.
>
> But due to git-receive-pack(1) not using `run_auto_gc()` in the first
> place it did not get converted to use git-maintenance(1) like we do
> everywhere else now. Address this oversight and start to use the newly
> introduced function `prepare_auto_maintenance()`. This will also make it
> easier for us to adapt this code together with all the other callsites
> that invoke auto-maintenance in the future.
This commit explains my earlier question. Thanks.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)
2024-04-17 16:19 ` [PATCH 0/2] " Karthik Nayak
@ 2024-04-17 16:53 ` Junio C Hamano
2024-04-18 5:43 ` Patrick Steinhardt
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2024-04-17 16:53 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git, Derrick Stolee
Karthik Nayak <karthik.188@gmail.com> writes:
>> this small patch series adapts git-receive-pack(1) to spawn `git
>> maintenance run --auto` instead of `git gc --auto` like all the other
>> parts of our codebase do nowadays. This removes the last internal user
>> of `git gc --auto`.
>
> I don't have enough context here, so why do this?
I think the intent of a95ce124 (maintenance: replace run_auto_gc(),
2020-09-17) was to update all codepaths that run "git gc --auto" to
instead run "git maintenance --auto", but only updated the ones that
used to call run_auto_gc(). The codepath Patrick found runs "git gc
--auto" without using run_auto_gc() and was left behind when the
others were converted.
So why do this? I think "To follow through a95ce124 started" would
probably be a good enough reason, if a reader is on board with what
a95ce124 wanted to do.
Do we have a handy reference that compares "gc --auto" and
"maintenance --auto"? Are they essentially the same thing these
days? What are the things that is done by one but not by the other?
THanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1)
2024-04-17 16:53 ` Junio C Hamano
@ 2024-04-18 5:43 ` Patrick Steinhardt
0 siblings, 0 replies; 9+ messages in thread
From: Patrick Steinhardt @ 2024-04-18 5:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, git, Derrick Stolee
[-- Attachment #1: Type: text/plain, Size: 2135 bytes --]
On Wed, Apr 17, 2024 at 09:53:58AM -0700, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> >> this small patch series adapts git-receive-pack(1) to spawn `git
> >> maintenance run --auto` instead of `git gc --auto` like all the other
> >> parts of our codebase do nowadays. This removes the last internal user
> >> of `git gc --auto`.
> >
> > I don't have enough context here, so why do this?
Fair enough, I was a bit lazy with the cover letter as I thought that
the second patch already explains it well enough. But true, I should've
set the stage a bit better.
> I think the intent of a95ce124 (maintenance: replace run_auto_gc(),
> 2020-09-17) was to update all codepaths that run "git gc --auto" to
> instead run "git maintenance --auto", but only updated the ones that
> used to call run_auto_gc(). The codepath Patrick found runs "git gc
> --auto" without using run_auto_gc() and was left behind when the
> others were converted.
Yup, that was triggered this patch series. I was puzzled that we still
run `git gc --auto` at all.
> So why do this? I think "To follow through a95ce124 started" would
> probably be a good enough reason, if a reader is on board with what
> a95ce124 wanted to do.
>
> Do we have a handy reference that compares "gc --auto" and
> "maintenance --auto"? Are they essentially the same thing these
> days? What are the things that is done by one but not by the other?
git-maintenance(1) is a superset of what git-gc(1) does. It is
structured around different "tasks" that the user can enable or disable
at a whim. By default, only a single "gc" task is enabled, which makes
it a drop-in replacement for what we had before the conversion.
But users can ask git-maintenance(1) to perform housekeeping to their
will. Instead of running git-gc(1), they can configure indididual tasks
to repack objects via multi-pack indices, pack only loose objects,
update the commit-graph or repack references.
So overall, git-maintenance(1) is the modern and more flexible
replacement for git-gc(1) that gives more tuning knobs.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process
2024-04-17 15:53 ` Junio C Hamano
@ 2024-04-18 5:48 ` Patrick Steinhardt
0 siblings, 0 replies; 9+ messages in thread
From: Patrick Steinhardt @ 2024-04-18 5:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Derrick Stolee
[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]
On Wed, Apr 17, 2024 at 08:53:25AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The `run_auto_maintenance()` function is responsible for spawning a new
> > `git maintenance run --auto` process. To do so, it sets up the `sturct
> > child_process` and then runs it by executing `run_command()` directly.
> > This is rather inflexible in case callers want to modify the child
> > process somewhat, e.g. to redirect stderr or stdout.
> >
> > Introduce a new `prepare_auto_maintenance()` function to plug this gap.
>
> I guess the mention of "inflexible" and "redirection" above refers
> to some incompatibile behaviour we would introduce if we just
> replaced the manual spawning of "gc --auto" with a call to
> run_auto_maintenance(), but I would have expected that will be
> solved by making the interface to run_auto_maintenance() richer, not
> forcing the callers that would want to deviate from the norm to
> write the second half of the run_auto_maintenance() themselves.
>
> > +int run_auto_maintenance(int quiet)
> > +{
> > + struct child_process maint = CHILD_PROCESS_INIT;
> > + if (!prepare_auto_maintenance(quiet, &maint))
> > + return 0;
> > return run_command(&maint);
> > }
>
> But given that the "second half" is to just call run_command() on
> the prepared child control structure, it is probably not a huge
> deal. It just felt somewhat an uneven API surface that 'quiet' can
> be controlled with just a single bit and doing anything more than
> that would require the caller to go into the structure to tweak.
>
> Will queue. Thanks.
git-receive-pack(1) needs to do some magic with file descriptors and
needs to copy output of the command to the sideband. I first thought
about extending `run_auto_maintenance()` to support this, but found it
to be messy as this really is quite a specific usecase. So I figured
that prepping the `struct child_process` like this is the nicer way to
approach it.
Thanks.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-18 5:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 6:16 [PATCH 0/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
2024-04-17 6:16 ` [PATCH 1/2] run-command: introduce function to prepare auto-maintenance process Patrick Steinhardt
2024-04-17 15:53 ` Junio C Hamano
2024-04-18 5:48 ` Patrick Steinhardt
2024-04-17 6:16 ` [PATCH 2/2] builtin/receive-pack: convert to use git-maintenance(1) Patrick Steinhardt
2024-04-17 16:50 ` Karthik Nayak
2024-04-17 16:19 ` [PATCH 0/2] " Karthik Nayak
2024-04-17 16:53 ` Junio C Hamano
2024-04-18 5:43 ` Patrick Steinhardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.