rerere: release lockfile in non-writing functionsjk/am-rerere-lock-fix

There's a bug in builtin/am.c in which we take a lock on
MERGE_RR recursively. But rather than fix am.c, this patch
fixes the confusing interface from rerere.c that caused the
bug. Read on for the gory details.

The setup_rerere() function both reads the existing MERGE_RR
file, and takes MERGE_RR.lock. In the rerere() and
rerere_forget() functions, we end up in write_rr(), which
will then commit the lock file.

But for functions like rerere_clear() that do not write to
MERGE_RR, we expect the caller to have handled
setup_rerere(). That caller would then need to release the
lockfile, but it can't; the lock struct is local to
rerere.c.

For builtin/rerere.c, this is OK. We run a single rerere
operation and then exit immediately, which has the side
effect of rolling back the lockfile.

But in builtin/am.c, this is actively wrong. If we run "git
am -3 --skip", we call setup-rerere twice without releasing
the lock:

  1. The "--skip" causes us to call am_rerere_clear(), which
     calls setup_rerere(), but never drops the lock.

  2. We then proceed to the next patch.

  3. The "--3way" may cause us to call rerere() to handle
     conflicts in that patch, but we are already holding the
     lock. The lockfile code dies with:

     BUG: prepare_tempfile_object called for active object

We could fix this by having rerere_clear() call
rollback_lock_file(). But it feels a bit odd for it to roll
back a lockfile that it did not itself take. So let's
simplify the interface further, and handle setup_rerere in
the function itself, taking away the question from the
caller over whether they need to do so.

We can give rerere_gc() the same treatment, as well (even
though it doesn't have any callers besides builtin/rerere.c
at this point). Note that these functions don't take flags
from their callers to pass along to setup_rerere; that's OK,
because the flags would not be meaningful for what they are
doing.

Both of those functions need to hold the lock because even
though they do not write to MERGE_RR, they are still writing
and should be protected from a simultaneous "rerere" run.
But rerere_remaining(), "rerere diff", and "rerere status"
are all read-only operations. They want to setup_rerere(),
but do not care about taking the lock in the first place.
Since our update of MERGE_RR is the usual atomic rename done
by commit_lock_file, they can just do a lockless read. For
that, we teach setup_rerere a READONLY flag to avoid the
lock.

As a bonus, this pushes builtin/rerere.c's setup_rerere call
closer to the functions that use it. Which means that "git
rerere totally-bogus-command" will no longer silently
exit(0) in a repository without rerere enabled.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

1 files changed, 15 insertions, 2 deletions

diff --git a/rerere.c b/rerere.c
index 6a517aa383..d90057b769 100644
--- a/rerere.c
+++ b/rerere.c
@@ -409,6 +409,8 @@ static int find_conflict(struct string_list *conflict)
 int rerere_remaining(struct string_list *merge_rr)
 {
 	int i;
+	if (setup_rerere(merge_rr, RERERE_READONLY))
+		return 0;
 	if (read_cache() < 0)
 		return error("Could not read index");
 
@@ -603,8 +605,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 
 	if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))
 		rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);
-	fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),
-				       LOCK_DIE_ON_ERROR);
+	if (flags & RERERE_READONLY)
+		fd = 0;
+	else
+		fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),
+					       LOCK_DIE_ON_ERROR);
 	read_rr(merge_rr);
 	return fd;
 }
@@ -701,6 +706,9 @@ void rerere_gc(struct string_list *rr)
 	int cutoff_noresolve = 15;
 	int cutoff_resolve = 60;
 
+	if (setup_rerere(rr, 0) < 0)
+		return;
+
 	git_config_get_int("gc.rerereresolved", &cutoff_resolve);
 	git_config_get_int("gc.rerereunresolved", &cutoff_noresolve);
 	git_config(git_default_config, NULL);
@@ -727,16 +735,21 @@ void rerere_gc(struct string_list *rr)
 	for (i = 0; i < to_remove.nr; i++)
 		unlink_rr_item(to_remove.items[i].string);
 	string_list_clear(&to_remove, 0);
+	rollback_lock_file(&write_lock);
 }
 
 void rerere_clear(struct string_list *merge_rr)
 {
 	int i;
 
+	if (setup_rerere(merge_rr, 0) < 0)
+		return;
+
 	for (i = 0; i < merge_rr->nr; i++) {
 		const char *name = (const char *)merge_rr->items[i].util;
 		if (!has_rerere_resolution(name))
 			unlink_rr_item(name);
 	}
 	unlink_or_warn(git_path_merge_rr());
+	rollback_lock_file(&write_lock);
 }