[PATCH v4 0/2] rerere: wait for MERGE_RR.lock, and go on at a conflict

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

This is now two patches, as Patrick asked: the wait first, the skip on top,
so that the second can be discussed on its own.

Patch 1 makes every writer wait rerere.lockTimeout for MERGE_RR.lock before
it fails as it does today, and lets "git rerere gc" skip its run instead of
waiting. That alone keeps a rebase alive through the gc that a commit's auto
maintenance starts: the gc gives way whenever it comes second, and the one
second default is longer than pruning a few thousand entries takes.

Patch 2 is what I proposed in my 6 September reply to Junio: a command that
stops at a conflict warns and goes on when the lock is still held after the
wait, and the warning tells the user to run "git rerere" before resolving.
Everything else keeps failing: the recordings after a resolution, the user's
own rerere commands, and the clears that am and rebase run.

Changes since v3:

 * Based on master instead of maint. On master setup_rerere() already uses
   the repository-scoped lock helpers, so the series merges cleanly into
   next and seen, including ps/tune-rerere-gc (which makes the gc rarer, not
   the race) and tb/rerere-lock-grace (which only stops the commands a
   rebase spawns from starting the gc).

 * The first log message says that the race is as old as the lock and that
   the 2.54 maintenance default only made it easy to hit (Patrick).

 * Only the six callers that stop at a conflict go on without rerere: the
   sequencer's pick and merge, "git merge", the 3-way fallback of "git am",
   "git stash" and "git apply --3way". "git commit" and "git am --continue"
   fail after the wait, as in 2.55 (Junio). Merge, rebase, am, stash and
   apply each have a test for this.

 * The config entry describes the outcome per command and no longer says
   "retry" (Junio).

 * The "rerere clear" that am and rebase run for --skip and --abort waits
   and then fails like every other caller, so a held lock cannot leave a
   stale MERGE_RR entry behind. The signatures of rerere_clear() and
   rerere_forget() are back to what upstream has.

 * A new test runs "git rerere" at the stop after a skipped recording and
   checks that the preimage, and after the resolution the postimage, get
   recorded.

Thomas Bachem (2):
  rerere: wait for MERGE_RR.lock, and let the gc skip it
  rerere: go on at a conflict when the lock stays busy

 Documentation/config/rerere.adoc |  13 +++
 Documentation/git-rerere.adoc    |   4 +-
 apply.c                          |   2 +-
 builtin/am.c                     |   3 +-
 builtin/merge.c                  |   2 +-
 builtin/stash.c                  |   2 +-
 rerere.c                         |  49 +++++++--
 rerere.h                         |   4 +
 sequencer.c                      |   4 +-
 t/t4200-rerere.sh                | 168 +++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh           |   8 ++
 11 files changed, 246 insertions(+), 13 deletions(-)


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

Range-diff vs v3:

 1:  5bfda65baa ! 1:  8a7a74d6aa rerere: keep a background gc from killing a rebase
     @@ Metadata
      Author: Thomas Bachem <mail@thomasbachem.com>
      
       ## Commit message ##
     -    rerere: keep a background gc from killing a rebase
     +    rerere: wait for MERGE_RR.lock, and let the gc skip it
      
     -    A "git rerere gc" holds MERGE_RR.lock for as long as pruning rr-cache
     -    takes, and since 2.54 the auto maintenance after every commit runs
     -    one whenever rr-cache has an entry. The commit a rebase spawns for a
     -    resolved pick starts it too, and the sequencer's repo_rerere() at the
     -    next conflict wants the lock a few milliseconds later. Both take it
     -    with LOCK_DIE_ON_ERROR, so whichever comes second dies. When it is
     -    the rebase, the index is written but the state for "git rebase
     -    --continue" is not, and every later continue refuses with "you have
     -    staged changes".
     +    setup_rerere() takes MERGE_RR.lock with LOCK_DIE_ON_ERROR. When two
     +    processes want the lock at the same time, the second one dies. This
     +    was always the case, but since 452b12c2e0 (builtin/maintenance: use
     +    "geometric" strategy by default, 2026-02-24) it is easy to hit: auto
     +    maintenance now runs "git rerere gc" after every commit whenever
     +    rr-cache contains at least one entry, and the gc holds the lock
     +    while it prunes.
      
     -    The gc needs the lock, since a rerere that has just created its
     -    directory looks like the empty ones it prunes. So wait for it
     -    instead, rerere.lockTimeout milliseconds, 1000 by default with the
     -    semantics of core.packedRefsTimeout, then warn and go on without
     -    rerere: a lost recording or replay is nothing next to a rebase that
     -    cannot continue. The gc itself never waits, and "git rerere", "git
     -    rerere forget" and "git rerere clear" wait but then die, since the
     -    state behind the lock is all they are for. The clearing "am" and
     -    "rebase" do on --abort, --skip and --quit goes on without it, and
     -    leaves the entries for the gc.
     +    A rebase whose next pick conflicts while the gc holds the lock dies
     +    inside repo_rerere(). That runs before the sequencer writes the state
     +    that "git rebase --continue" needs, so every later "git rebase
     +    --continue" fails with "you have staged changes".
     +
     +    Instead of dying right away, wait for the lock for up to
     +    rerere.lockTimeout milliseconds, 1000 by default, and only then fail
     +    as before. The gc itself does not wait: when the lock is held, it
     +    skips this run and leaves the pruning to the next one.
      
          Assisted-by: Claude Fable 5.1
          Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
     @@ Documentation/config/rerere.adoc: rerere.enabled::
       	repository.
      +
      +rerere.lockTimeout::
     -+	The length of time, in milliseconds, to retry when trying to
     -+	take the rerere lock while another process holds it, typically
     -+	a background `git rerere gc`.  When the time is up, the command
     -+	warns and goes on without rerere.  Value 0 means not to retry
     -+	at all; -1 means to try indefinitely.  Default is 1000 (i.e.,
     -+	retry for 1 second).  `git rerere gc` does not retry at all.
     -+	`git rerere`, `git rerere forget` and `git rerere clear` retry
     -+	the same way, but fail when the time is up instead of going on.
     ++	The length of time, in milliseconds, to wait for the rerere
     ++	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.  `git rerere gc` never
     ++	waits and skips its run while the lock is held.
      
       ## Documentation/git-rerere.adoc ##
      @@ Documentation/git-rerere.adoc: occurred a long time ago.  By default, unresolved conflicts older
     @@ Documentation/git-rerere.adoc: occurred a long time ago.  By default, unresolved
       days are pruned.  These defaults are controlled via the
       `gc.rerereUnresolved` and `gc.rerereResolved` configuration
      -variables respectively.
     -+variables respectively.  If another process holds the lock on the
     -+recorded resolutions, for example a merge or rebase that is recording
     -+a conflict, `gc` does nothing and reports so.
     ++variables respectively.  If another process holds the rerere lock,
     ++for example a merge or rebase that is recording a conflict, `gc`
     ++does nothing and says so.
       
       
       DISCUSSION
      
     - ## builtin/am.c ##
     -@@ builtin/am.c: static int clean_index(const struct object_id *head, const struct object_id *rem
     - static void am_rerere_clear(void)
     - {
     - 	struct string_list merge_rr = STRING_LIST_INIT_DUP;
     --	rerere_clear(the_repository, &merge_rr);
     -+	rerere_clear(the_repository, &merge_rr, 0);
     - 	string_list_clear(&merge_rr, 1);
     - }
     - 
     -
     - ## builtin/rebase.c ##
     -@@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
     - 	case ACTION_SKIP: {
     - 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
     - 
     --		rerere_clear(the_repository, &merge_rr);
     -+		rerere_clear(the_repository, &merge_rr, 0);
     - 	}
     - 		/* fallthrough */
     - 	case ACTION_CONTINUE: {
     -@@ builtin/rebase.c: int cmd_rebase(int argc,
     - 	case ACTION_SKIP: {
     - 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
     - 
     --		rerere_clear(the_repository, &merge_rr);
     -+		rerere_clear(the_repository, &merge_rr, 0);
     - 		string_list_clear(&merge_rr, 1);
     - 		ropts.flags = RESET_HEAD_HARD;
     - 		if (reset_head(the_repository, &ropts) < 0)
     -@@ builtin/rebase.c: int cmd_rebase(int argc,
     - 		struct string_list merge_rr = STRING_LIST_INIT_DUP;
     - 		struct strbuf head_msg = STRBUF_INIT;
     - 
     --		rerere_clear(the_repository, &merge_rr);
     -+		rerere_clear(the_repository, &merge_rr, 0);
     - 		string_list_clear(&merge_rr, 1);
     - 
     - 		if (read_basic_state(&options))
     -
     - ## builtin/rerere.c ##
     -@@ builtin/rerere.c: int cmd_rerere(int argc,
     - 		flags = RERERE_NOAUTOUPDATE;
     - 
     - 	if (argc < 1)
     --		return repo_rerere(the_repository, flags);
     -+		return repo_rerere(the_repository, flags | RERERE_LOCK_OR_DIE);
     - 
     - 	if (!strcmp(argv[0], "forget")) {
     - 		struct pathspec pathspec;
     -@@ builtin/rerere.c: int cmd_rerere(int argc,
     - 		parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
     - 			       prefix, argv + 1);
     - 
     --		ret = rerere_forget(the_repository, &pathspec);
     -+		ret = rerere_forget(the_repository, &pathspec,
     -+				    RERERE_LOCK_OR_DIE);
     - 
     - 		clear_pathspec(&pathspec);
     - 		return ret;
     - 	}
     - 
     - 	if (!strcmp(argv[0], "clear")) {
     --		rerere_clear(the_repository, &merge_rr);
     -+		rerere_clear(the_repository, &merge_rr, RERERE_LOCK_OR_DIE);
     - 	} else if (!strcmp(argv[0], "gc"))
     - 		rerere_gc(the_repository, &merge_rr);
     - 	else if (!strcmp(argv[0], "status")) {
     -
       ## rerere.c ##
      @@ rerere.c: static int rerere_enabled = -1;
     - 
       /* automatically update cleanly resolved paths to the index */
       static int rerere_autoupdate;
     -+static int rerere_lock_timeout_ms = 1000;
       
     ++/* how long to wait for MERGE_RR.lock, in milliseconds */
     ++static int rerere_lock_timeout_ms = 1000;
     ++
       #define RR_HAS_POSTIMAGE 1
       #define RR_HAS_PREIMAGE 2
     + struct rerere_dir {
      @@ rerere.c: static void git_rerere_config(void)
       {
       	repo_config_get_bool(the_repository, "rerere.enabled", &rerere_enabled);
     @@ rerere.c: int setup_rerere(struct repository *r, struct string_list *merge_rr, i
       	if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))
       		rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);
      -	if (flags & RERERE_READONLY)
     -+	if ((flags & RERERE_NOWAIT) && (flags & RERERE_LOCK_OR_DIE))
     -+		BUG("RERERE_NOWAIT and RERERE_LOCK_OR_DIE are mutually exclusive");
     -+	if ((flags & RERERE_READONLY) &&
     -+	    (flags & (RERERE_NOWAIT | RERERE_LOCK_OR_DIE)))
     -+		BUG("RERERE_READONLY takes no lock, so no lock flag applies");
     ++	if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT))
     ++		BUG("RERERE_READONLY takes no lock, so RERERE_NOWAIT does not apply");
      +	if (flags & RERERE_READONLY) {
       		fd = 0;
      -	else
     --		fd = hold_lock_file_for_update(&write_lock,
     --					       git_path_merge_rr(r),
     --					       LOCK_DIE_ON_ERROR);
     +-		fd = repo_hold_lock_file_for_update(r, &write_lock,
     +-						    git_path_merge_rr(r),
     +-						    LOCK_DIE_ON_ERROR);
      +	} else {
     -+		int lock_flags = 0;
     ++		const char *path = git_path_merge_rr(r);
     ++		int lock_flags = LOCK_DIE_ON_ERROR;
      +		long timeout_ms = rerere_lock_timeout_ms;
      +
     -+		if (flags & RERERE_LOCK_OR_DIE)
     -+			lock_flags = LOCK_DIE_ON_ERROR;
     -+		if (flags & RERERE_NOWAIT)
     -+			timeout_ms = 0;
      +		/*
     -+		 * A background "rerere gc" holds the lock for as long as it
     -+		 * takes to prune rr-cache, so wait it out rather than fail
     -+		 * at once.  The gc itself has nothing to lose from a skipped
     -+		 * run and never waits.
     ++		 * 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.  The gc itself never
     ++		 * waits: skipping one of its runs costs nothing.
      +		 */
     -+		fd = hold_lock_file_for_update_timeout(&write_lock,
     -+						       git_path_merge_rr(r),
     -+						       lock_flags, timeout_ms);
     ++		if (flags & RERERE_NOWAIT) {
     ++			lock_flags = 0;
     ++			timeout_ms = 0;
     ++		}
     ++		fd = repo_hold_lock_file_for_update_timeout(r, &write_lock,
     ++							    path, lock_flags,
     ++							    timeout_ms);
      +		if (fd < 0) {
     -+			warning_errno(_("skipping rerere, unable to create '%s.lock'"),
     -+				      git_path_merge_rr(r));
     ++			warning_errno(_("skipping rerere, "
     ++					"unable to create '%s.lock'"), path);
      +			return -1;
      +		}
      +	}
       	read_rr(r, merge_rr);
       	return fd;
       }
     -@@ rerere.c: fail_exit:
     - 	return -1;
     - }
     - 
     --int rerere_forget(struct repository *r, struct pathspec *pathspec)
     -+int rerere_forget(struct repository *r, struct pathspec *pathspec, int flags)
     - {
     - 	int i, fd, ret;
     - 	struct string_list conflict = STRING_LIST_INIT_DUP;
     -@@ rerere.c: int rerere_forget(struct repository *r, struct pathspec *pathspec)
     - 	if (repo_read_index(r) < 0)
     - 		return error(_("index file corrupt"));
     - 
     --	fd = setup_rerere(r, &merge_rr, RERERE_NOAUTOUPDATE);
     -+	fd = setup_rerere(r, &merge_rr, RERERE_NOAUTOUPDATE | flags);
     - 	if (fd < 0)
     - 		return 0;
     - 
      @@ rerere.c: void rerere_gc(struct repository *r, struct string_list *rr)
       	timestamp_t cutoff_resolve = now - 60 * 86400;
       	struct strbuf buf = STRBUF_INIT;
     @@ rerere.c: void rerere_gc(struct repository *r, struct string_list *rr)
       		return;
       
       	repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",
     -@@ rerere.c: void rerere_gc(struct repository *r, struct string_list *rr)
     -  *
     -  * NEEDSWORK: shouldn't we be calling this from "reset --hard"?
     -  */
     --void rerere_clear(struct repository *r, struct string_list *merge_rr)
     -+void rerere_clear(struct repository *r, struct string_list *merge_rr, int flags)
     - {
     - 	int i;
     - 
     --	if (setup_rerere(r, merge_rr, 0) < 0)
     -+	if (setup_rerere(r, merge_rr, flags) < 0)
     - 		return;
     - 
     - 	for (i = 0; i < merge_rr->nr; i++) {
      
       ## rerere.h ##
      @@ rerere.h: struct repository;
       #define RERERE_AUTOUPDATE   01
       #define RERERE_NOAUTOUPDATE 02
       #define RERERE_READONLY     04
     -+/* Do not wait for the lock when another process holds it */
     ++/* Never wait for MERGE_RR.lock, and skip the run when it is held */
      +#define RERERE_NOWAIT       010
     -+/* Die on a lock that cannot be taken instead of going on without rerere */
     -+#define RERERE_LOCK_OR_DIE  020
       
       /*
        * Marks paths that have been hand-resolved and added to the
     -@@ rerere.h: int repo_rerere(struct repository *, int);
     -  */
     - const char *rerere_path(struct strbuf *buf, const struct rerere_id *,
     - 			const char *file);
     --int rerere_forget(struct repository *, struct pathspec *);
     -+int rerere_forget(struct repository *, struct pathspec *, int);
     - int rerere_remaining(struct repository *, struct string_list *);
     --void rerere_clear(struct repository *, struct string_list *);
     -+void rerere_clear(struct repository *, struct string_list *, int);
     - void rerere_gc(struct repository *, struct string_list *);
     - 
     - #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
      
       ## t/t4200-rerere.sh ##
      @@ t/t4200-rerere.sh: test_expect_success 'old records rest in peace' '
     @@ t/t4200-rerere.sh: test_expect_success 'old records rest in peace' '
      +	test_grep "^=======\$" $rr/preimage
      +'
      +
     -+test_expect_success 'merge goes on without rerere once rerere.lockTimeout is up' '
     ++test_expect_success 'merge fails once rerere.lockTimeout is up' '
      +	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 "skipping rerere" err &&
     ++	test_grep "Unable to create" err &&
      +	test_grep "^=======\$" a1 &&
      +	test_path_is_missing $rr/preimage
      +'
      +
     -+test_expect_success 'commit goes on without rerere once rerere.lockTimeout is up' '
     -+	git reset --hard &&
     -+	rm -rf $rr &&
     -+	git checkout -b lock-held-commit third &&
     -+	test_when_finished "git checkout third && git branch -D lock-held-commit" &&
     -+	test_must_fail git merge first &&
     -+	test_path_is_file $rr/preimage &&
     -+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
     -+	>.git/MERGE_RR.lock &&
     -+	echo resolved >a1 &&
     -+	git add a1 &&
     -+	git -c rerere.lockTimeout=0 commit -qm resolved 2>err &&
     -+	test_grep "skipping rerere" err &&
     -+	test_path_is_missing $rr/postimage
     -+'
     -+
      +test_expect_success 'rerere, forget and clear fail on a lock they cannot take' '
      +	test_when_finished "rm -f .git/MERGE_RR.lock" &&
      +	>.git/MERGE_RR.lock &&
     @@ t/t4200-rerere.sh: test_expect_success 'old records rest in peace' '
      +	test_grep "Unable to create" err
      +'
      +
     -+test_expect_success 'rebase goes on without rerere once rerere.lockTimeout is up' '
     ++test_expect_success 'rebase --abort fails on a lock it cannot take' '
      +	git reset --hard &&
     -+	rm -rf $rr &&
     -+	git checkout -b lock-held third &&
     -+	test_when_finished "git checkout third && git branch -D lock-held" &&
     -+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
     -+	>.git/MERGE_RR.lock &&
     -+	test_must_fail git -c rerere.lockTimeout=0 rebase first 2>err &&
     -+	test_grep "skipping rerere" err &&
     -+	test_path_is_file .git/rebase-merge/stopped-sha &&
     -+	echo resolved >a1 &&
     -+	git add a1 &&
     -+	git -c rerere.lockTimeout=0 rebase --continue &&
     -+	test_path_is_missing .git/rebase-merge &&
     -+	test_path_is_missing $rr/preimage
     -+'
     -+
     -+test_expect_success 'rebase --abort goes on without rerere on a held lock' '
      +	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 "rm -f .git/MERGE_RR.lock" &&
      +	>.git/MERGE_RR.lock &&
     -+	git -c rerere.lockTimeout=0 rebase --abort 2>err &&
     -+	test_grep "skipping rerere" err &&
     ++	test_must_fail git -c rerere.lockTimeout=0 rebase --abort 2>err &&
     ++	test_grep "Unable to create" err &&
     ++	test_path_is_dir .git/rebase-merge &&
     ++	rm .git/MERGE_RR.lock &&
     ++	git rebase --abort &&
      +	test_path_is_missing .git/rebase-merge
      +'
      +
 -:  ---------- > 2:  1cce403113 rerere: go on at a conflict when the lock stays busy

-- 
gitgitgadget