rerere: release lockfile in non-writing functions

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>

diff --git a/builtin/am.c b/builtin/am.c
index 27165a6730ef04da2b4c22ee03eccadfa2e517b6..83b3d86e67f1ef335d0d3011ef0205909b315692 100644 (file)
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2057,11 +2057,6 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)
 static void am_rerere_clear(void)
 {
        struct string_list merge_rr = STRING_LIST_INIT_DUP;
-       int fd = setup_rerere(&merge_rr, 0);
-
-       if (fd < 0)
-               return;
-
        rerere_clear(&merge_rr);
        string_list_clear(&merge_rr, 1);
 }

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 7afadd2eadd59d8d3f4b77ad902bc24c94af9ec6..12535c9b4f48cd196943c1914fefee7c47cecec2 100644 (file)
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -50,7 +50,7 @@ static int diff_two(const char *file1, const char *label1,
 int cmd_rerere(int argc, const char **argv, const char *prefix)
 {
        struct string_list merge_rr = STRING_LIST_INIT_DUP;
-       int i, fd, autoupdate = -1, flags = 0;
+       int i, autoupdate = -1, flags = 0;
 
        struct option options[] = {
                OPT_SET_INT(0, "rerere-autoupdate", &autoupdate,
@@ -79,18 +79,16 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
                return rerere_forget(&pathspec);
        }
 
-       fd = setup_rerere(&merge_rr, flags);
-       if (fd < 0)
-               return 0;
-
        if (!strcmp(argv[0], "clear")) {
                rerere_clear(&merge_rr);
        } else if (!strcmp(argv[0], "gc"))
                rerere_gc(&merge_rr);
-       else if (!strcmp(argv[0], "status"))
+       else if (!strcmp(argv[0], "status")) {
+               if (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)
+                       return 0;
                for (i = 0; i < merge_rr.nr; i++)
                        printf("%s\n", merge_rr.items[i].string);
-       else if (!strcmp(argv[0], "remaining")) {
+       } else if (!strcmp(argv[0], "remaining")) {
                rerere_remaining(&merge_rr);
                for (i = 0; i < merge_rr.nr; i++) {
                        if (merge_rr.items[i].util != RERERE_RESOLVED)
@@ -100,13 +98,15 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
                                 * string_list_clear() */
                                merge_rr.items[i].util = NULL;
                }
-       } else if (!strcmp(argv[0], "diff"))
+       } else if (!strcmp(argv[0], "diff")) {
+               if (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)
+                       return 0;
                for (i = 0; i < merge_rr.nr; i++) {
                        const char *path = merge_rr.items[i].string;
                        const char *name = (const char *)merge_rr.items[i].util;
                        diff_two(rerere_path(name, "preimage"), path, path, path);
                }
-       else
+       } else
                usage_with_options(rerere_usage, options);
 
        string_list_clear(&merge_rr, 1);

diff --git a/rerere.c b/rerere.c
index 6a517aa383b75512ba77592616e084d74647b491..d90057b7695918e260e1778a522011dabaa3a37b 100644 (file)
--- 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);
 }

diff --git a/rerere.h b/rerere.h
index 2956c2edf2f0f88bd9fb15682e31e3478df44a03..407d59996d97ae307d5b4a124f78ef79666e1feb 100644 (file)
--- a/rerere.h
+++ b/rerere.h
@@ -7,6 +7,7 @@ struct pathspec;
 
 #define RERERE_AUTOUPDATE   01
 #define RERERE_NOAUTOUPDATE 02
+#define RERERE_READONLY     04
 
 /*
  * Marks paths that have been hand-resolved and added to the

diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index dd627c42d3f5b73a088f91dd6ab8c1ce9ddcb88c..af6053a2429785c3d9981194177f05aa4beb57d2 100755 (executable)
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -873,4 +873,40 @@ test_expect_success 'am --message-id -s signs off after the message id' '
        test_cmp expected actual
 '
 
+test_expect_success 'am -3 works with rerere' '
+       rm -fr .git/rebase-apply &&
+       git reset --hard &&
+
+       # make patches one->two and two->three...
+       test_commit one file &&
+       test_commit two file &&
+       test_commit three file &&
+       git format-patch -2 --stdout >seq.patch &&
+
+       # and create a situation that conflicts...
+       git reset --hard one &&
+       test_commit other file &&
+
+       # enable rerere...
+       test_config rerere.enabled true &&
+       test_when_finished "rm -rf .git/rr-cache" &&
+
+       # ...and apply. Our resolution is to skip the first
+       # patch, and the rerere the second one.
+       test_must_fail git am -3 seq.patch &&
+       test_must_fail git am --skip &&
+       echo resolved >file &&
+       git add file &&
+       git am --resolved &&
+
+       # now apply again, and confirm that rerere engaged (we still
+       # expect failure from am because rerere does not auto-commit
+       # for us).
+       git reset --hard other &&
+       test_must_fail git am -3 seq.patch &&
+       test_must_fail git am --skip &&
+       echo resolved >expect &&
+       test_cmp expect file
+'
+
 test_done