[v3,0/7] builtin/maintenance: fix auto-detach with non-standard tasks

Message

Patrick Steinhardt Aug. 16, 2024, 10:44 a.m. UTC

Hi,

this is the second version of my patch series that fixes how
git-maintenance(1) detaches: instead of letting its child process
git-gc(1) detach, we now optionally ask git-maintenance(1) itself to
detach when running via our auto maintenance mechanism. This fixes
behaviour of git-maintenance(1) when configured to run non-standard
tasks like the "incremental" task.

Changes compared to v2:

  - Fix leaking git-gc(1) process in t6500.

  - Add missing documentation for `maintenance.autoDetach`.

Thanks!

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
  run-command: fix detaching when running auto maintenance

 Documentation/config/gc.txt          |   3 +-
 Documentation/config/maintenance.txt |  11 +
 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                        |  45 +++-
 t/t7900-maintenance.sh               |  82 +++++-
 12 files changed, 396 insertions(+), 171 deletions(-)

Range-diff against v2:
1:  040453f27f = 1:  040453f27f config: fix constness of out parameter for `git_config_get_expiry()`
2:  ff6aa9d7ba = 2:  ff6aa9d7ba builtin/gc: refactor to read config into structure
3:  310e361371 = 3:  310e361371 builtin/gc: fix leaking config values
4:  812c61c9b6 = 4:  812c61c9b6 builtin/gc: stop processing log file on signal
5:  ca78d3dc7c ! 5:  b934b23889 builtin/gc: add a `--detach` flag
    @@ builtin/gc.c: static int maintenance_run(int argc, const char **argv, const char
      	for (i = 0; i < TASK__COUNT; i++)
     
      ## t/t6500-gc.sh ##
    +@@ t/t6500-gc.sh: test_expect_success 'gc.maxCruftSize sets appropriate repack options' '
    + 	test_subcommand $cruft_max_size_opts --max-cruft-size=3145728 <trace2.txt
    + '
    + 
    +-run_and_wait_for_auto_gc () {
    ++run_and_wait_for_gc () {
    + 	# We read stdout from gc for the side effect of waiting until the
    + 	# background gc process exits, closing its fd 9.  Furthermore, the
    + 	# variable assignment from a command substitution preserves the
    + 	# exit status of the main gc process.
    + 	# Note: this fd trickery doesn't work on Windows, but there is no
    + 	# need to, because on Win the auto gc always runs in the foreground.
    +-	doesnt_matter=$(git gc --auto 9>&1)
    ++	doesnt_matter=$(git gc "$@" 9>&1)
    + }
    + 
    + test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '
    +@@ t/t6500-gc.sh: test_expect_success 'background auto gc does not run if gc.log is present and re
    + 	test-tool chmtime =-345600 .git/gc.log &&
    + 	git gc --auto &&
    + 	test_config gc.logexpiry 2.days &&
    +-	run_and_wait_for_auto_gc &&
    ++	run_and_wait_for_gc --auto &&
    + 	ls .git/objects/pack/pack-*.pack >packs &&
    + 	test_line_count = 1 packs
    + '
     @@ t/t6500-gc.sh: test_expect_success 'background auto gc respects lock for all operations' '
    + 	printf "%d %s" "$shell_pid" "$hostname" >.git/gc.pid &&
    + 
    + 	# our gc should exit zero without doing anything
    +-	run_and_wait_for_auto_gc &&
    ++	run_and_wait_for_gc --auto &&
    + 	(ls -1 .git/refs/heads .git/reftable >actual || true) &&
      	test_cmp expect actual
      '
      
    @@ t/t6500-gc.sh: test_expect_success 'background auto gc respects lock for all ope
     +		git config gc.autodetach false &&
     +		git config gc.auto 2 &&
     +
    -+		cat >expect <<-EOF &&
    -+		Auto packing the repository in background for optimum performance.
    -+		See "git help gc" for manual housekeeping.
    -+		EOF
    -+		GIT_PROGRESS_DELAY=0 git gc --auto --detach 2>actual &&
    -+		test_cmp expect actual
    ++		# Note that we cannot use `test_cmp` here to compare stderr
    ++		# because it may contain output from `set -x`.
    ++		run_and_wait_for_gc --auto --detach 2>actual &&
    ++		test_grep "Auto packing the repository in background for optimum performance." actual
     +	)
     +'
     +
6:  06dbb73425 = 6:  347d0a2002 builtin/maintenance: add a `--detach` flag
7:  6bc170ff05 ! 7:  9befef7c1f run-command: fix detaching when running auto maintenance
    @@ Commit message
     
         Signed-off-by: Patrick Steinhardt <[e-mail]>
     
    + ## Documentation/config/gc.txt ##
    +@@ Documentation/config/gc.txt: use, it'll affect how the auto pack limit works.
    + 
    + gc.autoDetach::
    + 	Make `git gc --auto` return immediately and run in the background
    +-	if the system supports it. Default is true.
    ++	if the system supports it. Default is true. This config variable acts
    ++	as a fallback in case `maintenance.autoDetach` is not set.
    + 
    + gc.bigPackThreshold::
    + 	If non-zero, all non-cruft packs larger than this limit are kept
    +
    + ## Documentation/config/maintenance.txt ##
    +@@ Documentation/config/maintenance.txt: maintenance.auto::
    + 	`git maintenance run --auto` after doing their normal work. Defaults
    + 	to true.
    + 
    ++maintenance.autoDetach::
    ++	Many Git commands trigger automatic maintenance after they have
    ++	written data into the repository. This boolean config option
    ++	controls whether this automatic maintenance shall happen in the
    ++	foreground or whether the maintenance process shall detach and
    ++	continue to run in the background.
    +++
    ++If unset, the value of `gc.autoDetach` is used as a fallback. Defaults
    ++to true if both are unset, meaning that the maintenance process will
    ++detach.
    ++
    + maintenance.strategy::
    + 	This string config option provides a way to specify one of a few
    + 	recommended schedules for background maintenance. This only affects
    +
      ## builtin/gc.c ##
     @@ builtin/gc.c: static int maintenance_task_gc(struct maintenance_run_opts *opts,
      		strvec_push(&child.args, "--quiet");