rerere: keep a background gc from killing a rebase

Documentation/config/rerere.adoc

@@ -10,3 +10,13 @@ rerere.enabled::
 	enabled if there is an `rr-cache` directory under the
 	`$GIT_DIR`, e.g. if "rerere" was previously used in the
 	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.

Documentation/git-rerere.adoc

@@ -70,7 +70,9 @@ occurred a long time ago.  By default, unresolved conflicts older
 than 15 days and resolved conflicts older than 60
 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.
 
 
 DISCUSSION

builtin/am.c

@@ -2112,7 +2112,7 @@ 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

@@ -367,7 +367,7 @@ 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: {
@@ -1382,7 +1382,7 @@ 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)
@@ -1396,7 +1396,7 @@ 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

@@ -74,7 +74,7 @@ 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;
@@ -85,14 +85,15 @@ 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

@@ -32,6 +32,7 @@ static int rerere_enabled = -1;
 
 /* automatically update cleanly resolved paths to the index */
 static int rerere_autoupdate;
+static int rerere_lock_timeout_ms = 1000;
 
 #define RR_HAS_POSTIMAGE 1
 #define RR_HAS_PREIMAGE 2
@@ -876,6 +877,8 @@ static void git_rerere_config(void)
 {
 	repo_config_get_bool(the_repository, "rerere.enabled", &rerere_enabled);
 	repo_config_get_bool(the_repository, "rerere.autoupdate", &rerere_autoupdate);
+	repo_config_get_int(the_repository, "rerere.locktimeout",
+			    &rerere_lock_timeout_ms);
 	repo_config(the_repository, git_default_config, NULL);
 }
 
@@ -908,12 +911,36 @@ 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)
+	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) {
 		fd = 0;
-	else
-		fd = hold_lock_file_for_update(&write_lock,
-					       git_path_merge_rr(r),
-					       LOCK_DIE_ON_ERROR);
+	} else {
+		int lock_flags = 0;
+		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.
+		 */
+		fd = hold_lock_file_for_update_timeout(&write_lock,
+						       git_path_merge_rr(r),
+						       lock_flags, timeout_ms);
+		if (fd < 0) {
+			warning_errno(_("skipping rerere, unable to create '%s.lock'"),
+				      git_path_merge_rr(r));
+			return -1;
+		}
+	}
 	read_rr(r, merge_rr);
 	return fd;
 }
@@ -1124,7 +1151,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 	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;
@@ -1133,7 +1160,7 @@ 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;
 
@@ -1237,7 +1264,7 @@ void rerere_gc(struct repository *r, struct string_list *rr)
 	timestamp_t cutoff_resolve = now - 60 * 86400;
 	struct strbuf buf = STRBUF_INIT;
 
-	if (setup_rerere(r, rr, 0) < 0)
+	if (setup_rerere(r, rr, RERERE_NOWAIT) < 0)
 		return;
 
 	repo_config_get_expiry_in_days(the_repository, "gc.rerereresolved",
@@ -1289,11 +1316,11 @@ 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

@@ -10,6 +10,10 @@ 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 */
+#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
@@ -34,9 +38,9 @@ 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

@@ -242,6 +242,102 @@ test_expect_success 'old records rest in peace' '
 	test_path_is_missing $rr2/preimage
 '
 
+test_expect_success 'gc does nothing while MERGE_RR is locked' '
+	mkdir -p $rr2 &&
+	echo Hello >$rr2/preimage &&
+	test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
+
+	test_when_finished "rm -f .git/MERGE_RR.lock" &&
+	>.git/MERGE_RR.lock &&
+	git rerere gc 2>err &&
+	test_grep "MERGE_RR.lock" err &&
+	test_path_is_file $rr2/preimage &&
+
+	rm .git/MERGE_RR.lock &&
+	git rerere gc &&
+	test_path_is_missing $rr2/preimage
+'
+
+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" &&
+	>.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 "^=======\$" $rr/preimage
+'
+
+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 "skipping rerere" 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 &&
+	test_must_fail git -c rerere.lockTimeout=0 rerere 2>err &&
+	test_grep "Unable to create" err &&
+	test_must_fail git -c rerere.lockTimeout=0 rerere forget a1 2>err &&
+	test_grep "Unable to create" err &&
+	test_must_fail git -c rerere.lockTimeout=0 rerere clear 2>err &&
+	test_grep "Unable to create" err
+'
+
+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 &&
+	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_path_is_missing .git/rebase-merge
+'
+
 rerere_gc_custom_expiry_test () {
 	five_days="$1" right_now="$2"
 	test_expect_success "rerere gc with custom expiry ($five_days, $right_now)" '

t/t7900-maintenance.sh

@@ -885,6 +885,14 @@ test_expect_success 'rerere-gc task with --auto honors maintenance.rerere-gc.aut
 	test_expect_rerere_gc ! git -c maintenance.rerere-gc.auto=0 maintenance run --auto --task=rerere-gc
 '
 
+test_expect_success 'rerere-gc task succeeds while MERGE_RR is locked' '
+	test_when_finished "rm -rf .git/rr-cache .git/MERGE_RR.lock" &&
+	mkdir .git/rr-cache &&
+	: >.git/rr-cache/entry &&
+	>.git/MERGE_RR.lock &&
+	test_expect_rerere_gc git maintenance run --task=rerere-gc
+'
+
 test_expect_success '--auto and --schedule incompatible' '
 	test_must_fail git maintenance run --auto --schedule=daily 2>err &&
 	test_grep "cannot be used together" err