Re: [PATCH v6 2/3] rerere: add "gc --skip-locked" for auto maintenance

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 9, 2026, 09:06 UTC
Message-ID: <asiugInq7YTj4Qbe@pks.im>
In-Reply-To: <2ef141410a1508477976f9e57cec05f1a7603264.1790939492.git.gitgitgadget@gmail.com>

On Fri, Oct 02, 2026 at 11:11:31AM +0000, Thomas Bachem via GitGitGadget wrote:

Show 6 quoted lines

> From: Thomas Bachem <mail@thomasbachem.com>
> 
> Since the previous commit, "git rerere gc" waits for MERGE_RR.lock
> like every other command that takes it, and fails only if the wait
> times out. That suits a user who runs it by hand and wants to know
> when nothing was pruned.

Two nits:

  - We already took the lock before the preceding commit, the only
    difference is that we now have a timeout. Your message sounds as if
    the whole lock were new.

  - The user don't necessarily care that nothing was pruned, but they do
    care that pruning has failed.

> But the user did not ask for the gc that auto
> maintenance starts after a commit, and the next commit starts another
> one.

And this reads quite awkward, too. How about:

  Starting with the preceding commit, processes that want to acquire
  the rerere cache's MERGE_RR.lock by default know to wait up to one
  second until that lock has been released. This is a sensible default
  for many commands that happen to write rerere entries, as we would
  otherwise die immediately when the lock is taken by another process.

  But for repository maintenance it's a bit more complicated, as there
  are two cases that we have to care about. When the user explicitly
  asks us to garbage collect rerere entries via `git rerere gc` they
  probably want us to try our best to perform this operation. It's thus
  sensible to wait for the lock and then die if we weren't able to
  acquire it.

  But we also prune rerere entries as part of auto-maintenance, which is
  only executed on a best-effort basis anyway. Delaying the whole
  operation to acquire the lock is somewhat heavy-handed, and neither
  does it make sense to die in case we haven't been able to garbage
  collect rerere entries as that would impede other housekeeping tasks.
  Furthermore, it's totally fine to skip the operation when the rerere
  cache is locked already, as we will retry during the next run anyway.

  But we do not have an easy way to tell `git rerere gc` to skip the
  operation in case the cache is locked already. Add a new
  "--skip-locked" flag to plug that gap and have auto-maintenance pass
  that flag.

Show 19 quoted lines

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 57a3520263..7ad3987b71 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -385,12 +385,14 @@ out:
>  	return should_prune;
>  }
>  
> -static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts UNUSED,
> +static int maintenance_task_rerere_gc(struct maintenance_run_opts *opts,
>  				      struct gc_config *cfg UNUSED)
>  {
>  	struct child_process rerere_cmd = CHILD_PROCESS_INIT;
>  	rerere_cmd.git_cmd = 1;
>  	strvec_pushl(&rerere_cmd.args, "rerere", "gc", NULL);
> +	if (opts->auto_flag)
> +		strvec_push(&rerere_cmd.args, "--skip-locked");
>  	return run_command(&rerere_cmd);
>  }

Makes sense, as this is what drives both `git gc --auto` and `git maintenance run --auto`.

Show 24 quoted lines

> diff --git a/rerere.c b/rerere.c
> index 64fac07c71..43c8eb04db 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -887,18 +887,30 @@ 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_NOWAIT does not apply with RERERE_READONLY");
>  	if (flags & RERERE_READONLY) {
>  		fd = 0;
>  	} else {
> +		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.
> +		 * it instead of dying right away.  The gc of an automatic
> +		 * maintenance run does not wait, since skipping one of
> +		 * its runs costs nothing.
>  		 */

This comment is basically a layering violation, as you now assume who passes `RERERE_NOWAIT`. It's a generic mechanism though, so I'd just drop that part.

Show 11 quoted lines

> +		if (flags & RERERE_NOWAIT) {
> +			lock_flags = 0;
> +			timeout_ms = 0;
> +		}
>  		fd = repo_hold_lock_file_for_update_timeout(r, &write_lock,
>  							    git_path_merge_rr(r),
> -							    LOCK_DIE_ON_ERROR,
> -							    rerere_lock_timeout_ms);
> +							    lock_flags, timeout_ms);
> +		if (fd < 0)
> +			return -1;

It's a tiny bit fishy that we return an error in the case where we have been asked to skip locking and we indeed weren't able to acquire the lock. To me it doesn't really indicate an error, as it matches the intent of the caller. But I guess that's debatable.

Show 10 quoted lines

> diff --git a/rerere.h b/rerere.h
> index feeb0e2c9f..d54c53d0d4 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -10,6 +10,8 @@ struct repository;
>  #define RERERE_AUTOUPDATE   01
>  #define RERERE_NOAUTOUPDATE 02
>  #define RERERE_READONLY     04
> +/* Take MERGE_RR.lock only if it is free, and return quietly otherwise */
> +#define RERERE_NOWAIT       010

"free" is a bit unusual for a term for a lock.

Show 6 quoted lines

> @@ -37,7 +39,7 @@ const char *rerere_path(struct strbuf *buf, const struct rerere_id *,
>  int rerere_forget(struct repository *, struct pathspec *);
>  int rerere_remaining(struct repository *, struct string_list *);
>  void rerere_clear(struct repository *, struct string_list *);
> -void rerere_gc(struct repository *, struct string_list *);
> +void rerere_gc(struct repository *, struct string_list *, int);

Given that these flags are new now, and given that none of the other flags apply to `rerere_gc`, shouldn't we instead have a separate list of flags specific to this function?

    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);

Show 28 quoted lines

> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
> index 7bd92235dc..28152bf456 100755
> --- a/t/t4200-rerere.sh
> +++ b/t/t4200-rerere.sh
> @@ -242,6 +242,27 @@ test_expect_success 'old records rest in peace' '
>  	test_path_is_missing $rr2/preimage
>  '
>  
> +test_expect_success 'gc --skip-locked 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 --skip-locked 2>err &&
> +	test_must_be_empty err &&
> +	test_path_is_file $rr2/preimage &&
> +
> +	rm .git/MERGE_RR.lock &&
> +	git rerere gc --skip-locked &&
> +	test_path_is_missing $rr2/preimage
> +'
> +
> +test_expect_success '--skip-locked is only accepted by gc' '
> +	test_must_fail git rerere --skip-locked clear 2>err &&
> +	test_grep "option .--skip-locked. requires .gc." err
> +'

You verify that --skip-locked skips when locked, but you don't verify that it doesn't skip when unlocked.

Patrick