[PATCH v4 2/2] rerere: go on at a conflict when the lock stays busy

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

From: Thomas Bachem <mail@thomasbachem.com>

When a merge, rebase, cherry-pick, revert, am, stash or apply stops
at a conflict, it runs rerere right before it returns to the user.
If MERGE_RR.lock is still held when rerere.lockTimeout runs out, the
command dies there. For a rebase that is worse than a lost
recording: the sequencer has not yet written the state that
"git rebase --continue" needs, so the rebase cannot continue, and
following the "git commit --amend" advice folds the conflicted pick
into the previous commit.

So print a warning and go on instead. The conflict is still in
place, and the warning tells the user to run "git rerere" before
resolving it. That records the preimage, or replays a known
resolution, just as the command would have done.

All other callers still fail when the timeout runs out. "git commit"
and "git am --continue" record the resolution and then move on to
the next commit or patch. A warning would come too late there, and
the next rerere run would record whatever the file contains by then.
The "rerere clear" that am and rebase run for --skip and --abort
would leave the same stale entry behind. "git rerere", "git rerere
forget" and "git rerere clear" fail because the user asked for that
state explicitly.

Assisted-by: Claude Fable 5.1
Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
---
 Documentation/config/rerere.adoc |  10 ++-
 apply.c                          |   2 +-
 builtin/am.c                     |   3 +-
 builtin/merge.c                  |   2 +-
 builtin/stash.c                  |   2 +-
 rerere.c                         |  16 ++++-
 rerere.h                         |   2 +
 sequencer.c                      |   4 +-
 t/t4200-rerere.sh                | 105 ++++++++++++++++++++++++++++++-
 9 files changed, 132 insertions(+), 14 deletions(-)

diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
index cc9dd0c37b..81deefa006 100644
--- a/Documentation/config/rerere.adoc
+++ b/Documentation/config/rerere.adoc
@@ -16,6 +16,10 @@ 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.  `git rerere gc` never
-	waits and skips its run while the lock is held.
+	second).  When the time is up, a command that stops at a
+	conflict, such as `git merge` or `git rebase`, prints a
+	warning and goes on without rerere; run `git rerere` before
+	resolving the conflict to record it after all.  Any other
+	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.
diff --git a/apply.c b/apply.c
index f00b7ba4d3..3b8502535b 100644
--- a/apply.c
+++ b/apply.c
@@ -4865,7 +4865,7 @@ static int write_out_results(struct apply_state *state, struct patch *list)
 		 * tree with conflict markers, but that isn't written with --cached.
 		 */
 		if (!state->cached)
-			repo_rerere(state->repo, 0);
+			repo_rerere(state->repo, RERERE_SKIP_LOCKED);
 	}
 
 	return errs;
diff --git a/builtin/am.c b/builtin/am.c
index e9623b8307..bcb93db515 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1649,7 +1649,8 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 		o.verbosity = 0;
 
 	if (merge_ort_generic(&o, &our_tree, &their_tree, 1, bases, &result)) {
-		repo_rerere(the_repository, state->allow_rerere_autoupdate);
+		repo_rerere(the_repository, state->allow_rerere_autoupdate |
+			    RERERE_SKIP_LOCKED);
 		free(their_tree_name);
 		return error(_("Failed to merge in the changes."));
 	}
diff --git a/builtin/merge.c b/builtin/merge.c
index 5b4eb23a83..1ed5959bfd 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1061,7 +1061,7 @@ static int suggest_conflicts(void)
 	fputs(msgbuf.buf, fp);
 	strbuf_release(&msgbuf);
 	fclose(fp);
-	repo_rerere(the_repository, allow_rerere_auto);
+	repo_rerere(the_repository, allow_rerere_auto | RERERE_SKIP_LOCKED);
 	printf(_("Automatic merge failed; "
 			"fix conflicts and then commit the result.\n"));
 	return 1;
diff --git a/builtin/stash.c b/builtin/stash.c
index 72c52571f8..83b6e1be72 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -729,7 +729,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
 		ret = error(_("could not write index"));
 
 	if (ret) {
-		repo_rerere(the_repository, 0);
+		repo_rerere(the_repository, RERERE_SKIP_LOCKED);
 
 		if (index)
 			fprintf_ln(stderr, _("Index was not unstashed."));
diff --git a/rerere.c b/rerere.c
index 7d44f3937c..a996d39159 100644
--- a/rerere.c
+++ b/rerere.c
@@ -3,6 +3,7 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
+#include "advice.h"
 #include "config.h"
 #include "copy.h"
 #include "environment.h"
@@ -887,8 +888,9 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags)
 
 	if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))
 		rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);
-	if ((flags & RERERE_READONLY) && (flags & RERERE_NOWAIT))
-		BUG("RERERE_READONLY takes no lock, so RERERE_NOWAIT does not apply");
+	if ((flags & RERERE_READONLY) &&
+	    (flags & (RERERE_NOWAIT | RERERE_SKIP_LOCKED)))
+		BUG("RERERE_READONLY takes no lock, so no lock flag applies");
 	if (flags & RERERE_READONLY) {
 		fd = 0;
 	} else {
@@ -900,18 +902,26 @@ int setup_rerere(struct repository *r, struct string_list *merge_rr, int flags)
 		 * 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.
+		 * waits: skipping one of its runs costs nothing.  A
+		 * command that stops at a conflict must not die here
+		 * either, so it warns and goes on without rerere.
 		 */
 		if (flags & RERERE_NOWAIT) {
 			lock_flags = 0;
 			timeout_ms = 0;
 		}
+		if (flags & RERERE_SKIP_LOCKED)
+			lock_flags = 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'"), path);
+			if (flags & RERERE_SKIP_LOCKED)
+				advise(_("run \"git rerere\" before resolving "
+					 "the conflict to record or replay "
+					 "its resolution"));
 			return -1;
 		}
 	}
diff --git a/rerere.h b/rerere.h
index a2712d543e..e91f3f4afa 100644
--- a/rerere.h
+++ b/rerere.h
@@ -12,6 +12,8 @@ struct repository;
 #define RERERE_READONLY     04
 /* Never wait for MERGE_RR.lock, and skip the run when it is held */
 #define RERERE_NOWAIT       010
+/* Warn and go on without rerere if MERGE_RR.lock cannot be taken in time */
+#define RERERE_SKIP_LOCKED  020
 
 /*
  * Marks paths that have been hand-resolved and added to the
diff --git a/sequencer.c b/sequencer.c
index 65afd100d9..49776e7c5a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2519,7 +2519,7 @@ static enum pick_result do_pick_commit(struct repository *r,
 		      : _("could not apply %s... %s"),
 		      short_commit_name(r, commit), msg.subject);
 		print_advice(r, res == 1, opts);
-		repo_rerere(r, opts->allow_rerere_auto);
+		repo_rerere(r, opts->allow_rerere_auto | RERERE_SKIP_LOCKED);
 		goto leave;
 	}
 
@@ -4448,7 +4448,7 @@ static int do_merge(struct repository *r,
 
 	rollback_lock_file(&lock);
 	if (ret)
-		repo_rerere(r, opts->allow_rerere_auto);
+		repo_rerere(r, opts->allow_rerere_auto | RERERE_SKIP_LOCKED);
 	else
 		/*
 		 * In case of problems, we now want to return a positive
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 27082a7676..d0a9363a1c 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -272,17 +272,118 @@ test_expect_success 'a held lock is waited out within rerere.lockTimeout' '
 	test_grep "^=======\$" $rr/preimage
 '
 
-test_expect_success 'merge fails once rerere.lockTimeout is up' '
+test_expect_success 'merge goes on without rerere 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 "Unable to create" err &&
+	test_grep "skipping rerere" err &&
+	test_grep "hint: .*git rerere" err &&
 	test_grep "^=======\$" a1 &&
 	test_path_is_missing $rr/preimage
 '
 
+test_expect_success 'rerere run at the stop records what was skipped' '
+	git reset --hard &&
+	rm -rf $rr &&
+	git checkout -b lock-held-catch-up third &&
+	test_when_finished "git checkout third && git branch -D lock-held-catch-up" &&
+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
+	>.git/MERGE_RR.lock &&
+	test_must_fail git -c rerere.lockTimeout=0 merge first &&
+	test_path_is_missing $rr/preimage &&
+	rm .git/MERGE_RR.lock &&
+	git rerere &&
+	test_grep "^=======\$" $rr/preimage &&
+	echo resolved >a1 &&
+	git add a1 &&
+	git commit -qm resolved &&
+	test_path_is_file $rr/postimage
+'
+
+test_expect_success 'rebase goes on without rerere once rerere.lockTimeout is up' '
+	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 &&
+	rm .git/MERGE_RR.lock &&
+	echo resolved >a1 &&
+	git add a1 &&
+	git rebase --continue &&
+	test_path_is_missing .git/rebase-merge &&
+	test_path_is_missing $rr/preimage
+'
+
+test_expect_success 'commit fails on a lock it cannot take' '
+	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 &&
+	echo resolved >a1 &&
+	git add a1 &&
+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
+	>.git/MERGE_RR.lock &&
+	test_must_fail git -c rerere.lockTimeout=0 commit -qm resolved 2>err &&
+	test_grep "Unable to create" err &&
+	test_path_is_missing $rr/postimage
+'
+
+test_expect_success 'am goes on without rerere once rerere.lockTimeout is up' '
+	git reset --hard &&
+	rm -rf $rr &&
+	git checkout -b lock-held-am third &&
+	test_when_finished "test_might_fail git am --abort &&
+		git checkout third && git branch -D lock-held-am" &&
+	git format-patch -1 --stdout first >first.patch &&
+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
+	>.git/MERGE_RR.lock &&
+	test_must_fail git -c rerere.lockTimeout=0 am --3way first.patch 2>err &&
+	test_grep "skipping rerere" err &&
+	test_path_is_dir .git/rebase-apply &&
+	rm .git/MERGE_RR.lock &&
+	git am --abort &&
+	test_path_is_missing .git/rebase-apply
+'
+
+test_expect_success 'stash pop goes on without rerere once rerere.lockTimeout is up' '
+	git reset --hard &&
+	rm -rf $rr &&
+	git checkout -b lock-held-stash third &&
+	test_when_finished "git reset --hard && git stash drop &&
+		git checkout third && git branch -D lock-held-stash" &&
+	echo stashed >>a1 &&
+	git stash &&
+	echo committed >>a1 &&
+	git commit -qam committed &&
+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
+	>.git/MERGE_RR.lock &&
+	test_must_fail git -c rerere.lockTimeout=0 stash pop 2>err &&
+	test_grep "skipping rerere" err &&
+	test_grep "^=======\$" a1
+'
+
+test_expect_success 'apply --3way goes on without rerere once rerere.lockTimeout is up' '
+	git reset --hard &&
+	rm -rf $rr &&
+	git checkout -b lock-held-apply third &&
+	test_when_finished "git reset --hard &&
+		git checkout third && git branch -D lock-held-apply" &&
+	git format-patch -1 --stdout first >first.patch &&
+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
+	>.git/MERGE_RR.lock &&
+	test_must_fail git -c rerere.lockTimeout=0 apply --3way first.patch 2>err &&
+	test_grep "skipping rerere" err &&
+	test_grep "^=======\$" a1
+'
+
 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 &&
-- 
gitgitgadget