| Message ID | cover.1723533091.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin/maintenance: fix auto-detach with non-standard tasks | expand |
On Tue Aug 13, 2024 at 5:17 PM AEST, Patrick Steinhardt wrote: > Hi, > > I recently configured git-maintenance(1) to not use git-gc(1) anymore, > but instead to use git-multi-pack-index(1). I quickly noticed that the > behaviour here is somewhat broken because instead of auto-detaching when > `git maintenance run --auto` executes, we wait for the process to run to > completion. > > The root cause is that git-maintenance(1), probably by accident, > continues to rely on the auto-detaching mechanism in git-gc(1). So > instead of having git-maintenance(1) detach, it is git-gc(1) that > detaches and thus causes git-maintenance(1) to exit early. That of > course falls flat once any maintenance task other than git-gc(1) > executes, because these won't detach. > > Despite being a usability issue, this may also cause git-gc(1) to run > concurrently with any other enabled maintenance tasks. This shouldn't > lead to data loss, but it can certainly lead to processes stomping on > each others feet. > > This patch series fixes this by wiring up new `--detach` flags for both > git-gc(1) and git-maintenance(1). Like this, git-maintenance(1) now > knows to execute `git gc --auto --no-detach`, while our auto-maintenance > will execute `git mainteance run --auto --detach`. > > Patrick > > Patrick Steinhardt (7): > config: fix constness of out parameter for `git_config_get_expiry()` > builtin/gc: refactor to read config into structure > builtin/gc: fix leaking config values > builtin/gc: stop processing log file on signal > builtin/gc: add a `--detach` flag > builtin/maintenance: add a `--detach` flag > builtin/maintenance: fix auto-detach with non-standard tasks > > Documentation/git-gc.txt | 5 +- > builtin/gc.c | 384 ++++++++++++++++++++++++--------------- > config.c | 4 +- > config.h | 2 +- > read-cache.c | 12 +- > run-command.c | 12 +- > t/t5304-prune.sh | 1 + > t/t5616-partial-clone.sh | 6 +- > t/t6500-gc.sh | 39 ++++ > t/t7900-maintenance.sh | 82 ++++++++- > 10 files changed, 381 insertions(+), 166 deletions(-) Thanks Patrick! I've left a few replies, mostly non-blocking questions. Cheers, James
On 8/13/24 3:17 AM, Patrick Steinhardt wrote: > I recently configured git-maintenance(1) to not use git-gc(1) anymore, > but instead to use git-multi-pack-index(1). I quickly noticed that the > behaviour here is somewhat broken because instead of auto-detaching when > `git maintenance run --auto` executes, we wait for the process to run to > completion. > > The root cause is that git-maintenance(1), probably by accident, > continues to rely on the auto-detaching mechanism in git-gc(1). So > instead of having git-maintenance(1) detach, it is git-gc(1) that > detaches and thus causes git-maintenance(1) to exit early. That of > course falls flat once any maintenance task other than git-gc(1) > executes, because these won't detach. > > Despite being a usability issue, this may also cause git-gc(1) to run > concurrently with any other enabled maintenance tasks. This shouldn't > lead to data loss, but it can certainly lead to processes stomping on > each others feet. > > This patch series fixes this by wiring up new `--detach` flags for both > git-gc(1) and git-maintenance(1). Like this, git-maintenance(1) now > knows to execute `git gc --auto --no-detach`, while our auto-maintenance > will execute `git mainteance run --auto --detach`. Thank you for noticing this behavior, which is essentially an unintended regression from when the maintenance command was first introduced. It worked for most users because of the accidental detachment of the GC task, but now users can correctly customize their automatic maintenance to run in the background. This was my oversight, as I was focused on scheduled maintenance as being the primary way that users would customize their maintenance tasks. Thank you for unifying the concepts. I sprinkled in commentary, and most of it was just things I noticed while reading the series in order but then later patches or a careful read made my comments non-actionable. This v1 looks good to me. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > I sprinkled in commentary, and most of it was just things I noticed > while reading the series in order but then later patches or a careful > read made my comments non-actionable. > > This v1 looks good to me. Thanks for a "think aloud" review. Very much appreciated. I thought there was a minor reroll for phrasing planned, without any other change on significant part of the series? Let me mark the topic for 'next' when it happens. Thanks, both.
On Thu, Aug 15, 2024 at 10:04:10AM -0400, Derrick Stolee wrote: > On 8/13/24 3:17 AM, Patrick Steinhardt wrote: > > > I recently configured git-maintenance(1) to not use git-gc(1) anymore, > > but instead to use git-multi-pack-index(1). I quickly noticed that the > > behaviour here is somewhat broken because instead of auto-detaching when > > `git maintenance run --auto` executes, we wait for the process to run to > > completion. > > > > The root cause is that git-maintenance(1), probably by accident, > > continues to rely on the auto-detaching mechanism in git-gc(1). So > > instead of having git-maintenance(1) detach, it is git-gc(1) that > > detaches and thus causes git-maintenance(1) to exit early. That of > > course falls flat once any maintenance task other than git-gc(1) > > executes, because these won't detach. > > > > Despite being a usability issue, this may also cause git-gc(1) to run > > concurrently with any other enabled maintenance tasks. This shouldn't > > lead to data loss, but it can certainly lead to processes stomping on > > each others feet. > > > > This patch series fixes this by wiring up new `--detach` flags for both > > git-gc(1) and git-maintenance(1). Like this, git-maintenance(1) now > > knows to execute `git gc --auto --no-detach`, while our auto-maintenance > > will execute `git mainteance run --auto --detach`. > > Thank you for noticing this behavior, which is essentially an unintended > regression from when the maintenance command was first introduced. It > worked for most users because of the accidental detachment of the GC > task, but now users can correctly customize their automatic maintenance > to run in the background. > > This was my oversight, as I was focused on scheduled maintenance as > being the primary way that users would customize their maintenance tasks. > Thank you for unifying the concepts. > > I sprinkled in commentary, and most of it was just things I noticed > while reading the series in order but then later patches or a careful > read made my comments non-actionable. > > This v1 looks good to me. Thanks for your thorough review! Patrick