{"thread":{"id":"40246","subject":"[PATCH] rerere: release lockfile in non-writing functions","startedAt":"2015-09-01T22:14:09Z","lastAt":"2015-09-02T19:43:49Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"269164","messageId":"20150901221409.GA23274@sigill.intra.peff.net","threadId":"40246","inReplyTo":null,"subject":"[PATCH] rerere: release lockfile in non-writing functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T22:14:09Z","receivedAt":"2015-09-01T22:14:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a bug in builtin/am.c in which we take a lock on\nMERGE_RR recursively. But rather than fix am.c, this patch\nfixes the confusing interface from rerere.c that caused the\nbug. Read on for the gory details.\n\nThe setup_rerere() function both reads the existing MERGE_RR\nfile, and takes MERGE_RR.lock. In the rerere() and\nrerere_forget() functions, we end up in write_rr(), which\nwill then commit the lock file.\n\nBut for functions like rerere_clear() that do not write to\nMERGE_RR, we expect the caller to have handled\nsetup_rerere(). That caller would then need to release the\nlockfile, but it can't; the lock struct is local to\nrerere.c.\n\nFor builtin/rerere.c, this is OK. We run a single rerere\noperation and then exit immediately, which has the side\neffect of rolling back the lockfile.\n\nBut in builtin/am.c, this is actively wrong. If we run \"git\nam -3 --skip\", we call setup-rerere twice without releasing\nthe lock:\n\n  1. The \"--skip\" causes us to call am_rerere_clear(), which\n     calls setup_rerere(), but never drops the lock.\n\n  2. We then proceed to the next patch.\n\n  3. The \"--3way\" may cause us to call rerere() to handle\n     conflicts in that patch, but we are already holding the\n     lock. The lockfile code dies with:\n\n     BUG: prepare_tempfile_object called for active object\n\nWe could fix this by having rerere_clear() call\nrollback_lock_file(). But it feels a bit odd for it to roll\nback a lockfile that it did not itself take. So let's\nsimplify the interface further, and handle setup_rerere in\nthe function itself, taking away the question from the\ncaller over whether they need to do so.\n\nWe can give rerere_gc() the same treatment, as well (even\nthough it doesn't have any callers besides builtin/rerere.c\nat this point). Note that these functions don't take flags\nfrom their callers to pass along to setup_rerere; that's OK,\nbecause the flags would not be meaningful for what they are\ndoing.\n\nBoth of those functions need to hold the lock because even\nthough they do not write to MERGE_RR, they are still writing\nand should be protected from a simultaneous \"rerere\" run.\nBut rerere_remaining(), \"rerere diff\", and \"rerere status\"\nare all read-only operations. They want to setup_rerere(),\nbut do not care about taking the lock in the first place.\nSince our update of MERGE_RR is the usual atomic rename done\nby commit_lock_file, they can just do a lockless read. For\nthat, we teach setup_rerere a READONLY flag to avoid the\nlock.\n\nAs a bonus, this pushes builtin/rerere.c's setup_rerere call\ncloser to the functions that use it. Which means that \"git\nrerere totally-bogus-command\" will no longer silently\nexit(0) in a repository without rerere enabled.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c     |  5 -----\n builtin/rerere.c | 18 +++++++++---------\n rerere.c         | 17 +++++++++++++++--\n rerere.h         |  1 +\n t/t4150-am.sh    | 36 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 61 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 27165a6..83b3d86 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2057,11 +2057,6 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)\n static void am_rerere_clear(void)\n {\n \tstruct string_list merge_rr = STRING_LIST_INIT_DUP;\n-\tint fd = setup_rerere(&merge_rr, 0);\n-\n-\tif (fd < 0)\n-\t\treturn;\n-\n \trerere_clear(&merge_rr);\n \tstring_list_clear(&merge_rr, 1);\n }\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 7afadd2..12535c9 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -50,7 +50,7 @@ static int diff_two(const char *file1, const char *label1,\n int cmd_rerere(int argc, const char **argv, const char *prefix)\n {\n \tstruct string_list merge_rr = STRING_LIST_INIT_DUP;\n-\tint i, fd, autoupdate = -1, flags = 0;\n+\tint i, autoupdate = -1, flags = 0;\n \n \tstruct option options[] = {\n \t\tOPT_SET_INT(0, \"rerere-autoupdate\", &autoupdate,\n@@ -79,18 +79,16 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)\n \t\treturn rerere_forget(&pathspec);\n \t}\n \n-\tfd = setup_rerere(&merge_rr, flags);\n-\tif (fd < 0)\n-\t\treturn 0;\n-\n \tif (!strcmp(argv[0], \"clear\")) {\n \t\trerere_clear(&merge_rr);\n \t} else if (!strcmp(argv[0], \"gc\"))\n \t\trerere_gc(&merge_rr);\n-\telse if (!strcmp(argv[0], \"status\"))\n+\telse if (!strcmp(argv[0], \"status\")) {\n+\t\tif (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)\n+\t\t\treturn 0;\n \t\tfor (i = 0; i < merge_rr.nr; i++)\n \t\t\tprintf(\"%s\\n\", merge_rr.items[i].string);\n-\telse if (!strcmp(argv[0], \"remaining\")) {\n+\t} else if (!strcmp(argv[0], \"remaining\")) {\n \t\trerere_remaining(&merge_rr);\n \t\tfor (i = 0; i < merge_rr.nr; i++) {\n \t\t\tif (merge_rr.items[i].util != RERERE_RESOLVED)\n@@ -100,13 +98,15 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)\n \t\t\t\t * string_list_clear() */\n \t\t\t\tmerge_rr.items[i].util = NULL;\n \t\t}\n-\t} else if (!strcmp(argv[0], \"diff\"))\n+\t} else if (!strcmp(argv[0], \"diff\")) {\n+\t\tif (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)\n+\t\t\treturn 0;\n \t\tfor (i = 0; i < merge_rr.nr; i++) {\n \t\t\tconst char *path = merge_rr.items[i].string;\n \t\t\tconst char *name = (const char *)merge_rr.items[i].util;\n \t\t\tdiff_two(rerere_path(name, \"preimage\"), path, path, path);\n \t\t}\n-\telse\n+\t} else\n \t\tusage_with_options(rerere_usage, options);\n \n \tstring_list_clear(&merge_rr, 1);\ndiff --git a/rerere.c b/rerere.c\nindex 6a517aa..d90057b 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -409,6 +409,8 @@ static int find_conflict(struct string_list *conflict)\n int rerere_remaining(struct string_list *merge_rr)\n {\n \tint i;\n+\tif (setup_rerere(merge_rr, RERERE_READONLY))\n+\t\treturn 0;\n \tif (read_cache() < 0)\n \t\treturn error(\"Could not read index\");\n \n@@ -603,8 +605,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)\n \n \tif (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))\n \t\trerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);\n-\tfd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),\n-\t\t\t\t       LOCK_DIE_ON_ERROR);\n+\tif (flags & RERERE_READONLY)\n+\t\tfd = 0;\n+\telse\n+\t\tfd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),\n+\t\t\t\t\t       LOCK_DIE_ON_ERROR);\n \tread_rr(merge_rr);\n \treturn fd;\n }\n@@ -701,6 +706,9 @@ void rerere_gc(struct string_list *rr)\n \tint cutoff_noresolve = 15;\n \tint cutoff_resolve = 60;\n \n+\tif (setup_rerere(rr, 0) < 0)\n+\t\treturn;\n+\n \tgit_config_get_int(\"gc.rerereresolved\", &cutoff_resolve);\n \tgit_config_get_int(\"gc.rerereunresolved\", &cutoff_noresolve);\n \tgit_config(git_default_config, NULL);\n@@ -727,16 +735,21 @@ void rerere_gc(struct string_list *rr)\n \tfor (i = 0; i < to_remove.nr; i++)\n \t\tunlink_rr_item(to_remove.items[i].string);\n \tstring_list_clear(&to_remove, 0);\n+\trollback_lock_file(&write_lock);\n }\n \n void rerere_clear(struct string_list *merge_rr)\n {\n \tint i;\n \n+\tif (setup_rerere(merge_rr, 0) < 0)\n+\t\treturn;\n+\n \tfor (i = 0; i < merge_rr->nr; i++) {\n \t\tconst char *name = (const char *)merge_rr->items[i].util;\n \t\tif (!has_rerere_resolution(name))\n \t\t\tunlink_rr_item(name);\n \t}\n \tunlink_or_warn(git_path_merge_rr());\n+\trollback_lock_file(&write_lock);\n }\ndiff --git a/rerere.h b/rerere.h\nindex 2956c2e..407d599 100644\n--- a/rerere.h\n+++ b/rerere.h\n@@ -7,6 +7,7 @@ struct pathspec;\n \n #define RERERE_AUTOUPDATE   01\n #define RERERE_NOAUTOUPDATE 02\n+#define RERERE_READONLY     04\n \n /*\n  * Marks paths that have been hand-resolved and added to the\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex dd627c4..af6053a 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -873,4 +873,40 @@ test_expect_success 'am --message-id -s signs off after the message id' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'am -3 works with rerere' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\n+\t# make patches one->two and two->three...\n+\ttest_commit one file &&\n+\ttest_commit two file &&\n+\ttest_commit three file &&\n+\tgit format-patch -2 --stdout >seq.patch &&\n+\n+\t# and create a situation that conflicts...\n+\tgit reset --hard one &&\n+\ttest_commit other file &&\n+\n+\t# enable rerere...\n+\ttest_config rerere.enabled true &&\n+\ttest_when_finished \"rm -rf .git/rr-cache\" &&\n+\n+\t# ...and apply. Our resolution is to skip the first\n+\t# patch, and the rerere the second one.\n+\ttest_must_fail git am -3 seq.patch &&\n+\ttest_must_fail git am --skip &&\n+\techo resolved >file &&\n+\tgit add file &&\n+\tgit am --resolved &&\n+\n+\t# now apply again, and confirm that rerere engaged (we still\n+\t# expect failure from am because rerere does not auto-commit\n+\t# for us).\n+\tgit reset --hard other &&\n+\ttest_must_fail git am -3 seq.patch &&\n+\ttest_must_fail git am --skip &&\n+\techo resolved >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_done\n-- \n2.5.1.739.g7891f6b\n"},{"id":"269229","messageId":"xmqqpp20mwsq.fsf@gitster.mtv.corp.google.com","threadId":"40246","inReplyTo":"20150901221409.GA23274@sigill.intra.peff.net","subject":"Re: [PATCH] rerere: release lockfile in non-writing functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-02T19:43:49Z","receivedAt":"2015-09-02T19:43:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There's a bug in builtin/am.c in which we take a lock on\n> MERGE_RR recursively. But rather than fix am.c, this patch\n> fixes the confusing interface from rerere.c that caused the\n> bug. Read on for the gory details.\n>\n> The setup_rerere() function both reads the existing MERGE_RR\n> file, and takes MERGE_RR.lock. In the rerere() and\n> rerere_forget() functions, we end up in write_rr(), which\n> will then commit the lock file.\n>\n> But for functions like rerere_clear() that do not write to\n> MERGE_RR, we expect the caller to have handled\n> setup_rerere(). That caller would then need to release the\n> lockfile, but it can't; the lock struct is local to\n> rerere.c.\n>\n> For builtin/rerere.c, this is OK. We run a single rerere\n> operation and then exit immediately, which has the side\n> effect of rolling back the lockfile.\n>\n> But in builtin/am.c, this is actively wrong. If we run \"git\n> am -3 --skip\", we call setup-rerere twice without releasing\n> the lock:\n>\n>   1. The \"--skip\" causes us to call am_rerere_clear(), which\n>      calls setup_rerere(), but never drops the lock.\n>\n>   2. We then proceed to the next patch.\n>\n>   3. The \"--3way\" may cause us to call rerere() to handle\n>      conflicts in that patch, but we are already holding the\n>      lock. The lockfile code dies with:\n>\n>      BUG: prepare_tempfile_object called for active object\n\nThanks.  I've been re-acquainting with the internals of rerere and\nagree with your approach to this issue.\n\nLet's merge this before -rc1.\n"}]}