[PATCH v8 0/2] rerere: wait for MERGE_RR.lock, but not in auto maintenance

["Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com>]

A rebase dies at a conflict when a background "git rerere gc" holds
MERGE_RR.lock at that moment. With the first patch, rerere waits for the
lock instead of dying at once. With the second, the "git rerere gc" started
by auto maintenance does nothing while the lock is held.

For v8 I took Junio's suggestions on patch 1 and Patrick's on patch 2.
Changes since v7:

 * The rerere.lockTimeout entry no longer says that the command fails when
   the time is up (Junio).

 * The lock tests grep for the "Unable to create" message about
   MERGE_RR.lock (Junio).

 * The wait for the background unlocker is now in test_when_finished
   (Junio).

 * The rebase --abort test aborts its rebase in test_when_finished, so a
   failed check cannot break later tests.

 * The test "merge fails once rerere.lockTimeout is up" never waits, so it
   is now called "merge fails on a lock it cannot take".

 * In patch 2, rerere_gc() takes RERERE_NOWAIT again instead of its own enum
   rerere_gc_flags (Patrick).

 * The comment about waiting in setup_rerere() now says "by default", and as
   in patch 1 it sits directly above the call that takes the lock.

 * A comment in t7900 says why the tests with a held lock create rr-cache.

Thomas Bachem (2):
  rerere: wait for MERGE_RR.lock before giving up
  rerere: add "gc --skip-locked" for auto maintenance

 Documentation/config/rerere.adoc |  9 ++++
 builtin/gc.c                     |  4 +-
 builtin/rerere.c                 | 10 +++-
 rerere.c                         | 37 ++++++++++++---
 rerere.h                         |  4 +-
 t/t4200-rerere.sh                | 79 ++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh           | 26 ++++++++++-
 7 files changed, 157 insertions(+), 12 deletions(-)


base-commit: 34f06850c16c7f7ac822b1adc71354f11b0f2ca3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2214%2Fthomasbachem%2Frerere-gc-lock-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2214/thomasbachem/rerere-gc-lock-v8
Pull-Request: https://github.com/gitgitgadget/git/pull/2214

Range-diff vs v7:

 1:  3dc3d02f12 ! 1:  06f6c77439 rerere: wait for MERGE_RR.lock before giving up
     @@ Metadata
       ## Commit message ##
          rerere: wait for MERGE_RR.lock before giving up
      
     -    setup_rerere() takes MERGE_RR.lock with LOCK_DIE_ON_ERROR, so of two
     -    processes that want it at the same time the second one dies. That
     +    setup_rerere() takes MERGE_RR.lock with LOCK_DIE_ON_ERROR, so when two
     +    processes want it at the same time, the second one dies. That
          used to be rare. Since 452b12c2e0 (builtin/maintenance: use
          "geometric" strategy by default, 2026-02-24) the auto maintenance
          after a commit runs "git rerere gc" whenever rr-cache has enough
          stale entries, and the gc holds the lock while it prunes.
      
     -    A rebase whose next pick conflicts while that happens dies inside
     -    repo_rerere(), before the sequencer has written the state that
     +    If a rebase's next pick conflicts while the gc holds the lock, the
     +    rebase dies inside repo_rerere(), before the sequencer has written what
          "git rebase --continue" needs. Every later "git rebase --continue"
          then fails with "you have staged changes in your working tree".
      
     @@ Documentation/config/rerere.adoc: rerere.enabled::
      +	lock when another process holds it, typically a background
      +	`git rerere gc`.  Value 0 means not to wait at all; -1 means
      +	to wait indefinitely.  Default is 1000 (i.e., wait for 1
     -+	second).  When the time is up, the command fails as it does
     -+	for any other lock it cannot take.
     ++	second).
      
       ## rerere.c ##
      @@ rerere.c: static int rerere_enabled = -1;
     @@ t/t4200-rerere.sh: test_expect_success 'old records rest in peace' '
      +test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
      +	git reset --hard &&
      +	rm -rf $rr &&
     -+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
     ++	test_when_finished "wait && rm -f .git/MERGE_RR.lock" &&
      +	>.git/MERGE_RR.lock &&
      +	{
      +		( sleep 1 && rm -f .git/MERGE_RR.lock ) &
      +	} &&
      +	test_must_fail git -c rerere.lockTimeout=5000 merge first 2>err &&
     -+	wait &&
     -+	test_grep ! "MERGE_RR" err &&
     ++	test_grep ! "Unable to create.*MERGE_RR\.lock" err &&
      +	test_grep "^=======\$" $rr/preimage
      +'
      +
     -+test_expect_success 'merge fails once rerere.lockTimeout is up' '
     ++test_expect_success 'merge fails on a lock it cannot take' '
      +	git reset --hard &&
      +	rm -rf $rr &&
      +	test_when_finished "rm -f .git/MERGE_RR.lock" &&
      +	>.git/MERGE_RR.lock &&
      +	test_must_fail git -c rerere.lockTimeout=0 merge first 2>err &&
     -+	test_grep "Unable to create" err &&
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err &&
      +	test_grep "^=======\$" a1 &&
      +	test_path_is_missing $rr/preimage
      +'
     @@ t/t4200-rerere.sh: test_expect_success 'old records rest in peace' '
      +	test_when_finished "rm -f .git/MERGE_RR.lock" &&
      +	>.git/MERGE_RR.lock &&
      +	test_must_fail git -c rerere.lockTimeout=0 rerere 2>err &&
     -+	test_grep "Unable to create" err &&
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err &&
      +	test_must_fail git -c rerere.lockTimeout=0 rerere forget a1 2>err &&
     -+	test_grep "Unable to create" err &&
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err &&
      +	test_must_fail git -c rerere.lockTimeout=0 rerere clear 2>err &&
     -+	test_grep "Unable to create" err &&
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err &&
      +	test_must_fail git -c rerere.lockTimeout=0 rerere gc 2>err &&
     -+	test_grep "Unable to create" err
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err
      +'
      +
      +test_expect_success 'rebase --abort fails on a lock it cannot take' '
     @@ t/t4200-rerere.sh: test_expect_success 'old records rest in peace' '
      +	git checkout -b lock-held-abort third &&
      +	test_when_finished "git checkout third && git branch -D lock-held-abort" &&
      +	test_must_fail git rebase first &&
     ++	test_when_finished "test_might_fail git rebase --abort" &&
      +	test_when_finished "rm -f .git/MERGE_RR.lock" &&
      +	>.git/MERGE_RR.lock &&
      +	test_must_fail git -c rerere.lockTimeout=0 rebase --abort 2>err &&
     -+	test_grep "Unable to create" err &&
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err &&
      +	test_path_is_dir .git/rebase-merge &&
      +	rm .git/MERGE_RR.lock &&
      +	git rebase --abort &&
 2:  0002881196 ! 2:  4202361858 rerere: add "gc --skip-locked" for auto maintenance
     @@ Commit message
      
       ## Documentation/config/rerere.adoc ##
      @@ Documentation/config/rerere.adoc: rerere.lockTimeout::
     + 	lock when another process holds it, typically a background
       	`git rerere gc`.  Value 0 means not to wait at all; -1 means
       	to wait indefinitely.  Default is 1000 (i.e., wait for 1
     - 	second).  When the time is up, the command fails as it does
     --	for any other lock it cannot take.
     -+	for any other lock it cannot take.  A `git rerere gc` run by
     -+	`git maintenance run --auto` or `git gc --auto` does not wait
     -+	and does nothing while the lock is held.
     +-	second).
     ++	second).  A `git rerere gc` run by `git maintenance run --auto`
     ++	or `git gc --auto` does not wait and does nothing while the
     ++	lock is held.
      
       ## builtin/gc.c ##
      @@ builtin/gc.c: out:
     @@ builtin/rerere.c: int cmd_rerere(int argc,
       	} else if (!strcmp(argv[0], "gc"))
      -		rerere_gc(the_repository, &merge_rr);
      +		rerere_gc(the_repository, &merge_rr,
     -+			  skip_locked ? RERERE_GC_NOWAIT : 0);
     ++			  skip_locked ? RERERE_NOWAIT : 0);
       	else if (!strcmp(argv[0], "status")) {
       		if (setup_rerere(the_repository, &merge_rr,
       				 flags | RERERE_READONLY) < 0)
     @@ rerere.c: int setup_rerere(struct repository *r, struct string_list *merge_rr, i
      +		int lock_flags = LOCK_DIE_ON_ERROR;
      +		int timeout_ms = rerere_lock_timeout_ms;
      +
     - 		/*
     - 		 * Another process may hold the lock for a while, e.g.
     - 		 * "git rerere gc" while it prunes rr-cache, so wait for
     - 		 * it instead of dying right away.
     - 		 */
      +		if (flags & RERERE_NOWAIT) {
      +			lock_flags = 0;
      +			timeout_ms = 0;
      +		}
     ++
     + 		/*
     + 		 * Another process may hold the lock for a while, e.g.
     +-		 * "git rerere gc" while it prunes rr-cache, so wait for
     +-		 * it instead of dying right away.
     ++		 * "git rerere gc" while it prunes rr-cache, so by default
     ++		 * wait for it instead of dying right away.
     + 		 */
       		fd = repo_hold_lock_file_for_update_timeout(r, &write_lock,
       							    git_path_merge_rr(r),
      -							    LOCK_DIE_ON_ERROR,
     @@ rerere.c: out:
       }
       
      -void rerere_gc(struct repository *r, struct string_list *rr)
     -+void rerere_gc(struct repository *r, struct string_list *rr,
     -+	       enum rerere_gc_flags flags)
     ++void rerere_gc(struct repository *r, struct string_list *rr, int flags)
       {
       	struct string_list to_remove = STRING_LIST_INIT_DUP;
       	DIR *dir;
     @@ rerere.c: void rerere_gc(struct repository *r, struct string_list *rr)
       	struct strbuf buf = STRBUF_INIT;
       
      -	if (setup_rerere(r, rr, 0) < 0)
     -+	if (setup_rerere(r, rr,
     -+			 (flags & RERERE_GC_NOWAIT) ? RERERE_NOWAIT : 0) < 0)
     ++	if (setup_rerere(r, rr, flags) < 0)
       		return;
       
       	rerere_gc_cutoffs(r, &cutoff_resolve, &cutoff_noresolve);
     @@ rerere.h: const char *rerere_path(struct strbuf *buf, const struct rerere_id *,
       int rerere_remaining(struct repository *, struct string_list *);
       void rerere_clear(struct repository *, struct string_list *);
      -void rerere_gc(struct repository *, struct string_list *);
     -+
     -+enum rerere_gc_flags {
     -+	/* Skip the operation in case the MERGE_RR.lock is already taken. */
     -+	RERERE_GC_NOWAIT = (1 << 0),
     -+};
     -+
     -+void rerere_gc(struct repository *, struct string_list *,
     -+	       enum rerere_gc_flags flags);
     ++void rerere_gc(struct repository *, struct string_list *, int);
       
       /*
        * Check whether garbage collection for rerere entries is needed, which is
     @@ t/t7900-maintenance.sh: test_expect_success 'rerere-gc task with --auto honors m
       
      +test_expect_success 'rerere-gc task with --auto succeeds while MERGE_RR is locked' '
      +	test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" &&
     ++	# Without "rr-cache", rerere is disabled and never takes the lock.
      +	mkdir .git/rr-cache &&
      +	>.git/MERGE_RR.lock &&
      +	test_expect_rerere_gc git -c maintenance.rerere-gc.auto=-1 maintenance run --auto --task=rerere-gc
     @@ t/t7900-maintenance.sh: test_expect_success 'rerere-gc task with --auto honors m
      +	mkdir .git/rr-cache &&
      +	>.git/MERGE_RR.lock &&
      +	test_must_fail git -c rerere.lockTimeout=0 maintenance run --task=rerere-gc 2>err &&
     -+	test_grep "Unable to create" err
     ++	test_grep "Unable to create.*MERGE_RR\.lock" err
      +'
      +
       test_expect_success '--auto and --schedule incompatible' '

-- 
gitgitgadget