Re: [PATCH v2] rerere: keep a background gc from killing a rebase

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Thomas

On 04/09/2026 08:44, Thomas Bachem via GitGitGadget wrote:
> From: Thomas Bachem <mail@thomasbachem.com>
> 
> Since 2.54 unscheduled maintenance uses the "geometric" strategy, so
> the "git maintenance run --auto --detach" behind every "git commit"
> runs "git rerere gc" in the background whenever rr-cache has an entry.

With Patricks patches that's no-longer true I think. I think a better 
motivation, as the cache is per-repository, rather than per-worktree, is 
concurrent writers running in different worktrees. That makes the 
timeout much more sensible as we expect writing a conflict resolution to 
be much faster than gc.

Overall, this commit message is rather long and it would be helpful if 
you could distill it to remove unnecessary and unrelated details.

> 
>   Documentation/config/rerere.adoc | 10 ++++
>   Documentation/git-rerere.adoc    |  4 +-
>   builtin/am.c                     |  2 +-
>   builtin/rebase.c                 |  6 +-
>   builtin/rerere.c                 |  7 ++-
>   rerere.c                         | 42 ++++++++++----
>   rerere.h                         |  8 ++-
>   t/t4200-rerere.sh                | 96 ++++++++++++++++++++++++++++++++
>   t/t7900-maintenance.sh           |  8 +++
>   9 files changed, 163 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/config/rerere.adoc b/Documentation/config/rerere.adoc
> index 3a78b5ebb1..b67323fc46 100644
> --- a/Documentation/config/rerere.adoc
> +++ b/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, and
> +	`git rerere`, `git rerere forget` and `git rerere clear` fail
> +	instead of going on.

Why do those commands fail rather than wait?

> @@ -908,12 +911,31 @@ 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_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;

It might be worth adding a check above here that BUG()s out if the 
caller passes an incompatible set of flags.

> +		/*
> +		 * 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));

A background job that the user did not explicitly start printing to the 
terminal is rather confusing as it is likely to get mixed in with the 
output of whatever is running in the foreground.

Thanks

Phillip

> +			return -1;
> +		}
> +	}
>   	read_rr(r, merge_rr);
>   	return fd;
>   }
> @@ -1124,7 +1146,7 @@ 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;
> @@ -1133,7 +1155,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 +1259,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 +1311,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++) {
> diff --git a/rerere.h b/rerere.h
> index d4b5f7c932..3a9f58acd9 100644
> --- a/rerere.h
> +++ b/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), \
> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 1717f407c8..243b3ebed3 100755
> --- a/t/t4200-rerere.sh
> +++ b/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)" '
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d7f82e1bec..a55ca2e829 100755
> --- a/t/t7900-maintenance.sh
> +++ b/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
> 
> base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc