@matheusbsbs / matheus-bagliano / commits / 9dd330e6ca

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>

Jeff King committed Sep 1, 2015 at 18:14 UTC 9dd330e6cad6ce11557acb18f35136c549d8ac1b

5 files changed +61 -16

builtin/am.c

-5

@@ -2057,11 +2057,6 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)

2057 static void am_rerere_clear(void)

2058 {

2059 struct string_list merge_rr = STRING_LIST_INIT_DUP;

2060 - int fd = setup_rerere(&merge_rr, 0);

2061 -

2062 - if (fd < 0)

2063 - return;

2064 -

2060 rerere_clear(&merge_rr);

2061 string_list_clear(&merge_rr, 1);

2062 }

builtin/rerere.c

+9 -9

@@ -50,7 +50,7 @@ static int diff_two(const char *file1, const char *label1,

50 int cmd_rerere(int argc, const char **argv, const char *prefix)

51 {

52 struct string_list merge_rr = STRING_LIST_INIT_DUP;

53 - int i, fd, autoupdate = -1, flags = 0;

53 + int i, autoupdate = -1, flags = 0;

55 struct option options[] = {

56 OPT_SET_INT(0, "rerere-autoupdate", &autoupdate,

@@ -79,18 +79,16 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)

79 return rerere_forget(&pathspec);

80 }

82 - fd = setup_rerere(&merge_rr, flags);

83 - if (fd < 0)

84 - return 0;

85 -

82 if (!strcmp(argv[0], "clear")) {

83 rerere_clear(&merge_rr);

84 } else if (!strcmp(argv[0], "gc"))

85 rerere_gc(&merge_rr);

90 - else if (!strcmp(argv[0], "status"))

86 + else if (!strcmp(argv[0], "status")) {

87 + if (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)

88 + return 0;

89 for (i = 0; i < merge_rr.nr; i++)

90 printf("%s\n", merge_rr.items[i].string);

93 - else if (!strcmp(argv[0], "remaining")) {

91 + } else if (!strcmp(argv[0], "remaining")) {

92 rerere_remaining(&merge_rr);

93 for (i = 0; i < merge_rr.nr; i++) {

94 if (merge_rr.items[i].util != RERERE_RESOLVED)

@@ -100,13 +98,15 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)

98 * string_list_clear() */

99 merge_rr.items[i].util = NULL;

100 }

103 - } else if (!strcmp(argv[0], "diff"))

101 + } else if (!strcmp(argv[0], "diff")) {

102 + if (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)

103 + return 0;

104 for (i = 0; i < merge_rr.nr; i++) {

105 const char *path = merge_rr.items[i].string;

106 const char *name = (const char *)merge_rr.items[i].util;

107 diff_two(rerere_path(name, "preimage"), path, path, path);

108 }

109 - else

109 + } else

110 usage_with_options(rerere_usage, options);

111

112 string_list_clear(&merge_rr, 1);

rerere.c

+15 -2

@@ -409,6 +409,8 @@ static int find_conflict(struct string_list *conflict)

409 int rerere_remaining(struct string_list *merge_rr)

410 {

411 int i;

412 + if (setup_rerere(merge_rr, RERERE_READONLY))

413 + return 0;

414 if (read_cache() < 0)

415 return error("Could not read index");

416

@@ -603,8 +605,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)

605

606 if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))

607 rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);

606 - fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),

607 - LOCK_DIE_ON_ERROR);

608 + if (flags & RERERE_READONLY)

609 + fd = 0;

610 + else

611 + fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),

612 + LOCK_DIE_ON_ERROR);

613 read_rr(merge_rr);

614 return fd;

615 }

@@ -701,6 +706,9 @@ void rerere_gc(struct string_list *rr)

706 int cutoff_noresolve = 15;

707 int cutoff_resolve = 60;

708

709 + if (setup_rerere(rr, 0) < 0)

710 + return;

711 +

712 git_config_get_int("gc.rerereresolved", &cutoff_resolve);

713 git_config_get_int("gc.rerereunresolved", &cutoff_noresolve);

714 git_config(git_default_config, NULL);

@@ -727,16 +735,21 @@ void rerere_gc(struct string_list *rr)

735 for (i = 0; i < to_remove.nr; i++)

736 unlink_rr_item(to_remove.items[i].string);

737 string_list_clear(&to_remove, 0);

738 + rollback_lock_file(&write_lock);

739 }

740

741 void rerere_clear(struct string_list *merge_rr)

742 {

743 int i;

744

745 + if (setup_rerere(merge_rr, 0) < 0)

746 + return;

747 +

748 for (i = 0; i < merge_rr->nr; i++) {

749 const char *name = (const char *)merge_rr->items[i].util;

750 if (!has_rerere_resolution(name))

751 unlink_rr_item(name);

752 }

753 unlink_or_warn(git_path_merge_rr());

754 + rollback_lock_file(&write_lock);

755 }

rerere.h

@@ -7,6 +7,7 @@ struct pathspec;

8 #define RERERE_AUTOUPDATE 01

9 #define RERERE_NOAUTOUPDATE 02

10 +#define RERERE_READONLY 04

12 /*

13 * Marks paths that have been hand-resolved and added to the

t/t4150-am.sh

+36

@@ -873,4 +873,40 @@ test_expect_success 'am --message-id -s signs off after the message id' '

873 test_cmp expected actual

874 '

875

876 +test_expect_success 'am -3 works with rerere' '

877 + rm -fr .git/rebase-apply &&

878 + git reset --hard &&

879 +

880 + # make patches one->two and two->three...

881 + test_commit one file &&

882 + test_commit two file &&

883 + test_commit three file &&

884 + git format-patch -2 --stdout >seq.patch &&

885 +

886 + # and create a situation that conflicts...

887 + git reset --hard one &&

888 + test_commit other file &&

889 +

890 + # enable rerere...

891 + test_config rerere.enabled true &&

892 + test_when_finished "rm -rf .git/rr-cache" &&

893 +

894 + # ...and apply. Our resolution is to skip the first

895 + # patch, and the rerere the second one.

896 + test_must_fail git am -3 seq.patch &&

897 + test_must_fail git am --skip &&

898 + echo resolved >file &&

899 + git add file &&

900 + git am --resolved &&

901 +

902 + # now apply again, and confirm that rerere engaged (we still

903 + # expect failure from am because rerere does not auto-commit

904 + # for us).

905 + git reset --hard other &&

906 + test_must_fail git am -3 seq.patch &&

907 + test_must_fail git am --skip &&

908 + echo resolved >expect &&

909 + test_cmp expect file

910 +'

911 +

912 test_done