{"thread":{"id":"47938","subject":"[PATCH 0/5] roll back locks in various code paths","startedAt":"2018-02-27T21:30:44Z","lastAt":"2018-03-02T10:49:14Z","messageCount":26,"participants":["Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"340452","messageId":"cover.1519763396.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":null,"subject":"[PATCH 0/5] roll back locks in various code paths","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T21:30:08Z","receivedAt":"2018-02-27T21:30:44Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Patches 2-4 are the actual fixes where I teach some functions to always\nroll back the lock they're holding. Notably, these are all in \"libgit\".\n\nPatch 1 is a \"while at it\" to use locks on the stack instead of having\nthem be static. Patch 5 removes code to roll back locks which are\nalready rolled back.\n\nI've based this on maint. There's a conflict on pu, with c7d4394111\n(sequencer: avoid using errno clobbered by rollback_lock_file(),\n2018-02-11). The conflict resolution would be to take my version for the\n\"could not lock HEAD\"-hunk.\n\nMartin\n\nMartin Ågren (5):\n sequencer: make lockfiles non-static\n sequencer: always roll back lock in `do_recursive_merge()`\n merge-recursive: always roll back lock in `merge_recursive_generic()`\n merge: always roll back lock in `checkout_fast_forward()`\n sequencer: do not roll back lockfile unnecessarily\n\n merge-recursive.c | 4 +++-\n merge.c | 12 +++++++++---\n sequencer.c | 32 ++++++++++++++------------------\n 3 files changed, 26 insertions(+), 22 deletions(-)\n\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340453","messageId":"4572a84cfdcb1897b67c271a9d06ca38802a2352.1519763396.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"[PATCH 1/5] sequencer: make lockfiles non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T21:30:09Z","receivedAt":"2018-02-27T21:30:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\nwe can have lockfiles on the stack.\n\nOne of these functions fails to always roll back the lock. That will be\nfixed in the next commit.\n\nSigned-off-by: Martin Ågren \n---\n sequencer.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4d3f60594c..90807c4559 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -290,7 +290,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n static int write_message(const void *buf, size_t len, const char *filename,\n \t\t\t int append_eol)\n {\n-\tstatic struct lock_file msg_file;\n+\tstruct lock_file msg_file = LOCK_INIT;\n \n \tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n \tif (msg_fd < 0)\n@@ -436,7 +436,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n \tint clean;\n \tchar **xopt;\n-\tstatic struct lock_file index_lock;\n+\tstruct lock_file index_lock = LOCK_INIT;\n \n \tif (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n@@ -1183,7 +1183,7 @@ static int prepare_revs(struct replay_opts *opts)\n \n static int read_and_refresh_cache(struct replay_opts *opts)\n {\n-\tstatic struct lock_file index_lock;\n+\tstruct lock_file index_lock = LOCK_INIT;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n \tif (read_index_preload(&the_index, NULL) < 0) {\n \t\trollback_lock_file(&index_lock);\n@@ -1577,7 +1577,7 @@ static int create_seq_dir(void)\n \n static int save_head(const char *head)\n {\n-\tstatic struct lock_file head_lock;\n+\tstruct lock_file head_lock = LOCK_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \tssize_t written;\n@@ -1702,7 +1702,7 @@ int sequencer_rollback(struct replay_opts *opts)\n \n static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)\n {\n-\tstatic struct lock_file todo_lock;\n+\tstruct lock_file todo_lock = LOCK_INIT;\n \tconst char *todo_path = get_todo_path(opts);\n \tint next = todo_list->current, offset, fd;\n \n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340454","messageId":"5d657ac87d1e71fd95175faed2b591fa609b8f1c.1519763396.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"[PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T21:30:10Z","receivedAt":"2018-02-27T21:30:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If we return early, we forget to roll back the lockfile. Do so.\n\nSigned-off-by: Martin Ågren \n---\n sequencer.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 90807c4559..e6bac4692a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\tfputs(o.obuf.buf, stdout);\n \tstrbuf_release(&o.obuf);\n \tdiff_warn_rename_limit(\"merge.renamelimit\", o.needed_rename_limit, 0);\n-\tif (clean < 0)\n+\tif (clean < 0) {\n+\t\trollback_lock_file(&index_lock);\n \t\treturn clean;\n+\t}\n \n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340455","messageId":"b19db05a8d8a98977788c04908134915eebc86a3.1519763396.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"[PATCH 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T21:30:11Z","receivedAt":"2018-02-27T21:30:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If we return early, we forget to roll back the lockfile. Do so.\n\nSigned-off-by: Martin Ågren \n---\n merge-recursive.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cc5fa0a949..7345125691 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2198,8 +2198,10 @@ int merge_recursive_generic(struct merge_options *o,\n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n-\tif (clean < 0)\n+\tif (clean < 0) {\n+\t\trollback_lock_file(&lock);\n \t\treturn clean;\n+\t}\n \n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340456","messageId":"8d2a1da0ad699c885b6ef335633c038ab7e354c4.1519763396.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"[PATCH 4/5] merge: always roll back lock in `checkout_fast_forward()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T21:30:12Z","receivedAt":"2018-02-27T21:30:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This function originated in builtin/merge.c. It was moved to merge.c in\ncommit db699a8a1f (Move try_merge_command and checkout_fast_forward to\nlibgit.a, 2012-10-26), but was used from sequencer.c even before that.\n\nIf a problem occurs, the function returns without rolling back the\nlockfile. Teach it to do so.\n\nSigned-off-by: Martin Ågren \n---\n merge.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/merge.c b/merge.c\nindex 195b578700..f06a4773d4 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head,\n \tsetup_unpack_trees_porcelain(&opts, \"merge\");\n \n \ttrees[nr_trees] = parse_tree_indirect(head);\n-\tif (!trees[nr_trees++])\n+\tif (!trees[nr_trees++]) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n+\t}\n \ttrees[nr_trees] = parse_tree_indirect(remote);\n-\tif (!trees[nr_trees++])\n+\tif (!trees[nr_trees++]) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n+\t}\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tparse_tree(trees[i]);\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n+\t}\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\treturn error(_(\"unable to write new index file\"));\n \treturn 0;\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340457","messageId":"d1dbf0b0a4742d8afeabd3acb0848e679c88eb50.1519763396.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"[PATCH 5/5] sequencer: do not roll back lockfile unnecessarily","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T21:30:13Z","receivedAt":"2018-02-27T21:31:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is\nno need to call `rollback_lock_file()` on the lockfile. It doesn't hurt\neither, but it does make different callers in this file inconsistent,\nwhich might be confusing.\n\nWhile at it, remove a trailing '.' from a recurring error message.\n\nSigned-off-by: Martin Ågren \n---\n sequencer.c | 18 ++++++------------\n 1 file changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e6bac4692a..0e6d04e4ce 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -303,10 +303,8 @@ static int write_message(const void *buf, size_t len, const char *filename,\n \t\trollback_lock_file(&msg_file);\n \t\treturn error_errno(_(\"could not write eol to '%s'\"), filename);\n \t}\n-\tif (commit_lock_file(&msg_file) < 0) {\n-\t\trollback_lock_file(&msg_file);\n-\t\treturn error(_(\"failed to finalize '%s'.\"), filename);\n-\t}\n+\tif (commit_lock_file(&msg_file) < 0)\n+\t\treturn error(_(\"failed to finalize '%s'\"), filename);\n \n \treturn 0;\n }\n@@ -1585,10 +1583,8 @@ static int save_head(const char *head)\n \tssize_t written;\n \n \tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n-\tif (fd < 0) {\n-\t\trollback_lock_file(&head_lock);\n+\tif (fd < 0)\n \t\treturn error_errno(_(\"could not lock HEAD\"));\n-\t}\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n \twritten = write_in_full(fd, buf.buf, buf.len);\n \tstrbuf_release(&buf);\n@@ -1597,10 +1593,8 @@ static int save_head(const char *head)\n \t\treturn error_errno(_(\"could not write to '%s'\"),\n \t\t\t\t git_path_head_file());\n \t}\n-\tif (commit_lock_file(&head_lock) < 0) {\n-\t\trollback_lock_file(&head_lock);\n-\t\treturn error(_(\"failed to finalize '%s'.\"), git_path_head_file());\n-\t}\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\treturn error(_(\"failed to finalize '%s'\"), git_path_head_file());\n \treturn 0;\n }\n \n@@ -1724,7 +1718,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t\ttodo_list->buf.len - offset) < 0)\n \t\treturn error_errno(_(\"could not write to '%s'\"), todo_path);\n \tif (commit_lock_file(&todo_lock) < 0)\n-\t\treturn error(_(\"failed to finalize '%s'.\"), todo_path);\n+\t\treturn error(_(\"failed to finalize '%s'\"), todo_path);\n \n \tif (is_rebase_i(opts)) {\n \t\tconst char *done_path = rebase_path_done();\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340461","messageId":"20180227214430.GC6899@sigill.intra.peff.net","threadId":"47938","inReplyTo":"5d657ac87d1e71fd95175faed2b591fa609b8f1c.1519763396.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T21:44:30Z","receivedAt":"2018-02-27T21:44:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2018 at 10:30:10PM +0100, Martin Ågren wrote:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 90807c4559..e6bac4692a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n> \t\tfputs(o.obuf.buf, stdout);\n> \tstrbuf_release(&o.obuf);\n> \tdiff_warn_rename_limit(\"merge.renamelimit\", o.needed_rename_limit, 0);\n> -\tif (clean < 0)\n> +\tif (clean < 0) {\n> +\t\trollback_lock_file(&index_lock);\n> \t\treturn clean;\n> +\t}\n> \n> \tif (active_cache_changed &&\n> \t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n\nThis addition is obviously correct.\n\nI want to note one thing that confused me while reviewing. While looking\nto see if there were other returns, I noticed that the lines right near\nthe end of your context are funny:\n\n if (active_cache_changed &&\n write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n /*\n * TRANSLATORS: %s will be \"revert\", \"cherry-pick\" or\n * \"rebase -i\".\n */\n return error(_(\"%s: Unable to write new index file\"),\n _(action_name(opts)));\n rollback_lock_file(&index_lock);\n\nAt first I thought that rollback was a noop, since write_locked_index()\nwould always either commit or rollback. But it's needed for the case\nwhen we active_cache_changed isn't true.\n\nSo I think it's correct as-is, but I wonder if writing it as:\n\n if (!active_cache_changed)\n\trollback_lock_file(&index_lock);\n else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n\treturn error(...);\n\nmight be easier to follow. I'm OK with leaving it, too, but thought I'd\nmention it in case it confused other reviewers.\n\n-Peff\n"},{"id":"340462","messageId":"20180227214712.GD6899@sigill.intra.peff.net","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/5] roll back locks in various code paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T21:47:13Z","receivedAt":"2018-02-27T21:47:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2018 at 10:30:08PM +0100, Martin Ågren wrote:\n\n> Patches 2-4 are the actual fixes where I teach some functions to always\n> roll back the lock they're holding. Notably, these are all in \"libgit\".\n> \n> Patch 1 is a \"while at it\" to use locks on the stack instead of having\n> them be static. Patch 5 removes code to roll back locks which are\n> already rolled back.\n\nThese all look good to me. Happy to see us dropping the \"static\" from\nmore lockfiles, too.\n\n-Peff\n"},{"id":"340468","messageId":"CAN0heSr_4Sd5-yAUQ-SG+=NZrwFrAFqY66aYk0DgMEZ_s53iGQ@mail.gmail.com","threadId":"47938","inReplyTo":"20180227214430.GC6899@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-27T22:08:12Z","receivedAt":"2018-02-27T22:08:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 27 February 2018 at 22:44, Jeff King wrote:\n> I want to note one thing that confused me while reviewing. While looking\n> to see if there were other returns, I noticed that the lines right near\n> the end of your context are funny:\n>\n> if (active_cache_changed &&\n> write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> /*\n> * TRANSLATORS: %s will be \"revert\", \"cherry-pick\" or\n> * \"rebase -i\".\n> */\n> return error(_(\"%s: Unable to write new index file\"),\n> _(action_name(opts)));\n> rollback_lock_file(&index_lock);\n>\n> At first I thought that rollback was a noop, since write_locked_index()\n> would always either commit or rollback. But it's needed for the case\n> when we active_cache_changed isn't true.\n>\n> So I think it's correct as-is, but I wonder if writing it as:\n>\n> if (!active_cache_changed)\n> rollback_lock_file(&index_lock);\n> else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> return error(...);\n>\n> might be easier to follow. I'm OK with leaving it, too, but thought I'd\n> mention it in case it confused other reviewers.\n\nI also hesitated at that one. There are some similar instances elsewhere, e.g.,\nin builtin/merge.c. There's also rerere.c, which does a variant of your\nsuggestion.\n\nMartin\n"},{"id":"340473","messageId":"20180227221547.GD11187@sigill.intra.peff.net","threadId":"47938","inReplyTo":"CAN0heSr_4Sd5-yAUQ-SG+=NZrwFrAFqY66aYk0DgMEZ_s53iGQ@mail.gmail.com","subject":"Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T22:15:48Z","receivedAt":"2018-02-27T22:15:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2018 at 11:08:12PM +0100, Martin Ågren wrote:\n\n> > So I think it's correct as-is, but I wonder if writing it as:\n> >\n> > if (!active_cache_changed)\n> > rollback_lock_file(&index_lock);\n> > else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n> > return error(...);\n> >\n> > might be easier to follow. I'm OK with leaving it, too, but thought I'd\n> > mention it in case it confused other reviewers.\n> \n> I also hesitated at that one. There are some similar instances elsewhere, e.g.,\n> in builtin/merge.c. There's also rerere.c, which does a variant of your\n> suggestion.\n\nHmm, yeah, grepping shows quite a few of various forms.\n\nI wonder if it is worth a helper like:\n\n /* like write_locked_index(), but optimize out unchanged writes */\n static int maybe_write_locked_index(struct index *index,\n struct lock_file *lock,\n\t\t\t\t unsigned flags)\n {\n\tif (!index->cached_changed) {\n\t\tif (flags & COMMIT_LOCK)\n\t\t\trollback_lock_file(lock);\n\t\treturn 0;\n\t}\n\treturn write_locked_index(index, lock, flags);\n }\n\nAlternatively, it could just be a flag to write_locked_index() to enable\nthe optimization.\n\nI actually suspect that most callers would prefer to have it kick in by\ndefault (with an optional flag to disable it if some caller really needs\nto), but that would possibly be a subtle breakage for the caller that\nneeds the flag.\n\n-Peff\n"},{"id":"340490","messageId":"xmqq1sh6ngof.fsf@gitster-ct.c.googlers.com","threadId":"47938","inReplyTo":"cover.1519763396.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/5] roll back locks in various code paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T23:42:08Z","receivedAt":"2018-02-27T23:42:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Patches 2-4 are the actual fixes where I teach some functions to always\n> roll back the lock they're holding. Notably, these are all in \"libgit\".\n>\n> Patch 1 is a \"while at it\" to use locks on the stack instead of having\n> them be static. Patch 5 removes code to roll back locks which are\n> already rolled back.\n>\n> I've based this on maint. There's a conflict on pu, with c7d4394111\n> (sequencer: avoid using errno clobbered by rollback_lock_file(),\n> 2018-02-11). The conflict resolution would be to take my version for the\n> \"could not lock HEAD\"-hunk.\n\nThanks for running a trial merge before sending your patches out. I\nwish there were more contributors like you ;-)\n\nThe changes looked reasonable.\n"},{"id":"340552","messageId":"CAN0heSqmWAN=rCO8busGtb0xNPiB5H_jkL664qH8xasPq1Sy0A@mail.gmail.com","threadId":"47938","inReplyTo":"b19db05a8d8a98977788c04908134915eebc86a3.1519763396.git.martin.agren@gmail.com","subject":"Re: [PATCH 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T11:02:02Z","receivedAt":"2018-02-28T11:02:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 27 February 2018 at 22:30, Martin Ågren wrote:\n> If we return early, we forget to roll back the lockfile. Do so.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> merge-recursive.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index cc5fa0a949..7345125691 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2198,8 +2198,10 @@ int merge_recursive_generic(struct merge_options *o,\n> hold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> clean = merge_recursive(o, head_commit, next_commit, ca,\n> result);\n> - if (clean < 0)\n> + if (clean < 0) {\n> + rollback_lock_file(&lock);\n> return clean;\n> + }\n>\n> if (active_cache_changed &&\n> write_locked_index(&the_index, &lock, COMMIT_LOCK))\n\nThis is incomplete. If \"!active_cache_changed\", this function fails to\nroll back the lock. I'll reroll this series.\n\nThis also highlights the point Peff made in [1] about how that\n\"active_cache_changed &&\" can be confusing. I'll look into the added or\nextended API he proposed, although perhaps not as part of the reroll.\n\nMartin\n\n[1] https://public-inbox.org/git/20180227214430.GC6899@sigill.intra.peff.net/\n"},{"id":"340598","messageId":"cover.1519843916.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"CAN0heSqmWAN=rCO8busGtb0xNPiB5H_jkL664qH8xasPq1Sy0A@mail.gmail.com","subject":"[PATCH v2 0/5] roll back locks in various code paths","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:07:53Z","receivedAt":"2018-02-28T19:08:17Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is v2 of my series to always release locks. As before, there's a\nconflict with pu, where the correct resolution is to take my version of\nthe conflicting hunk.\n\nThe only difference to v1 is in patch 3. I'll follow up with a patch to\naddress the confusing pattern which Peff mentioned and which fooled me\nwhen I prepared v1.\n\nMartin\n\nMartin Ågren (5):\n sequencer: make lockfiles non-static\n sequencer: always roll back lock in `do_recursive_merge()`\n merge-recursive: always roll back lock in `merge_recursive_generic()`\n merge: always roll back lock in `checkout_fast_forward()`\n sequencer: do not roll back lockfile unnecessarily\n\n merge-recursive.c | 5 ++++-\n merge.c | 12 +++++++++---\n sequencer.c | 32 ++++++++++++++------------------\n 3 files changed, 27 insertions(+), 22 deletions(-)\n\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340599","messageId":"4572a84cfdcb1897b67c271a9d06ca38802a2352.1519843916.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"[PATCH v2 1/5] sequencer: make lockfiles non-static","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:07:54Z","receivedAt":"2018-02-28T19:08:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),\nwe can have lockfiles on the stack.\n\nOne of these functions fails to always roll back the lock. That will be\nfixed in the next commit.\n\nSigned-off-by: Martin Ågren \n---\n sequencer.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4d3f60594c..90807c4559 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -290,7 +290,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n static int write_message(const void *buf, size_t len, const char *filename,\n \t\t\t int append_eol)\n {\n-\tstatic struct lock_file msg_file;\n+\tstruct lock_file msg_file = LOCK_INIT;\n \n \tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);\n \tif (msg_fd < 0)\n@@ -436,7 +436,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n \tint clean;\n \tchar **xopt;\n-\tstatic struct lock_file index_lock;\n+\tstruct lock_file index_lock = LOCK_INIT;\n \n \tif (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)\n \t\treturn -1;\n@@ -1183,7 +1183,7 @@ static int prepare_revs(struct replay_opts *opts)\n \n static int read_and_refresh_cache(struct replay_opts *opts)\n {\n-\tstatic struct lock_file index_lock;\n+\tstruct lock_file index_lock = LOCK_INIT;\n \tint index_fd = hold_locked_index(&index_lock, 0);\n \tif (read_index_preload(&the_index, NULL) < 0) {\n \t\trollback_lock_file(&index_lock);\n@@ -1577,7 +1577,7 @@ static int create_seq_dir(void)\n \n static int save_head(const char *head)\n {\n-\tstatic struct lock_file head_lock;\n+\tstruct lock_file head_lock = LOCK_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \tssize_t written;\n@@ -1702,7 +1702,7 @@ int sequencer_rollback(struct replay_opts *opts)\n \n static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)\n {\n-\tstatic struct lock_file todo_lock;\n+\tstruct lock_file todo_lock = LOCK_INIT;\n \tconst char *todo_path = get_todo_path(opts);\n \tint next = todo_list->current, offset, fd;\n \n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340600","messageId":"5d657ac87d1e71fd95175faed2b591fa609b8f1c.1519843916.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"[PATCH v2 2/5] sequencer: always roll back lock in `do_recursive_merge()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:07:55Z","receivedAt":"2018-02-28T19:08:25Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If we return early, we forget to roll back the lockfile. Do so.\n\nSigned-off-by: Martin Ågren \n---\n sequencer.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 90807c4559..e6bac4692a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\tfputs(o.obuf.buf, stdout);\n \tstrbuf_release(&o.obuf);\n \tdiff_warn_rename_limit(\"merge.renamelimit\", o.needed_rename_limit, 0);\n-\tif (clean < 0)\n+\tif (clean < 0) {\n+\t\trollback_lock_file(&index_lock);\n \t\treturn clean;\n+\t}\n \n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340601","messageId":"baaa7016b8812ca99d922c14547e23082bb6acba.1519843916.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"[PATCH v2 5/5] sequencer: do not roll back lockfile unnecessarily","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:07:58Z","receivedAt":"2018-02-28T19:08:35Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is\nno need to call `rollback_lock_file()` on the lockfile. It doesn't hurt\neither, but it does make different callers in this file inconsistent,\nwhich might be confusing.\n\nWhile at it, remove a trailing '.' from a recurring error message.\n\nSigned-off-by: Martin Ågren \n---\n sequencer.c | 18 ++++++------------\n 1 file changed, 6 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e6bac4692a..0e6d04e4ce 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -303,10 +303,8 @@ static int write_message(const void *buf, size_t len, const char *filename,\n \t\trollback_lock_file(&msg_file);\n \t\treturn error_errno(_(\"could not write eol to '%s'\"), filename);\n \t}\n-\tif (commit_lock_file(&msg_file) < 0) {\n-\t\trollback_lock_file(&msg_file);\n-\t\treturn error(_(\"failed to finalize '%s'.\"), filename);\n-\t}\n+\tif (commit_lock_file(&msg_file) < 0)\n+\t\treturn error(_(\"failed to finalize '%s'\"), filename);\n \n \treturn 0;\n }\n@@ -1585,10 +1583,8 @@ static int save_head(const char *head)\n \tssize_t written;\n \n \tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);\n-\tif (fd < 0) {\n-\t\trollback_lock_file(&head_lock);\n+\tif (fd < 0)\n \t\treturn error_errno(_(\"could not lock HEAD\"));\n-\t}\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n \twritten = write_in_full(fd, buf.buf, buf.len);\n \tstrbuf_release(&buf);\n@@ -1597,10 +1593,8 @@ static int save_head(const char *head)\n \t\treturn error_errno(_(\"could not write to '%s'\"),\n \t\t\t\t git_path_head_file());\n \t}\n-\tif (commit_lock_file(&head_lock) < 0) {\n-\t\trollback_lock_file(&head_lock);\n-\t\treturn error(_(\"failed to finalize '%s'.\"), git_path_head_file());\n-\t}\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\treturn error(_(\"failed to finalize '%s'\"), git_path_head_file());\n \treturn 0;\n }\n \n@@ -1724,7 +1718,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)\n \t\t\ttodo_list->buf.len - offset) < 0)\n \t\treturn error_errno(_(\"could not write to '%s'\"), todo_path);\n \tif (commit_lock_file(&todo_lock) < 0)\n-\t\treturn error(_(\"failed to finalize '%s'.\"), todo_path);\n+\t\treturn error(_(\"failed to finalize '%s'\"), todo_path);\n \n \tif (is_rebase_i(opts)) {\n \t\tconst char *done_path = rebase_path_done();\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340602","messageId":"45540e492e4b6498254f61601779076ff6d21d1d.1519843916.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"[PATCH v2 4/5] merge: always roll back lock in `checkout_fast_forward()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:07:57Z","receivedAt":"2018-02-28T19:08:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This function originated in builtin/merge.c. It was moved to merge.c in\ncommit db699a8a1f (Move try_merge_command and checkout_fast_forward to\nlibgit.a, 2012-10-26), but was used from sequencer.c even before that.\n\nIf a problem occurs, the function returns without rolling back the\nlockfile. Teach it to do so.\n\nSigned-off-by: Martin Ågren \n---\n merge.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/merge.c b/merge.c\nindex 195b578700..f06a4773d4 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head,\n \tsetup_unpack_trees_porcelain(&opts, \"merge\");\n \n \ttrees[nr_trees] = parse_tree_indirect(head);\n-\tif (!trees[nr_trees++])\n+\tif (!trees[nr_trees++]) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n+\t}\n \ttrees[nr_trees] = parse_tree_indirect(remote);\n-\tif (!trees[nr_trees++])\n+\tif (!trees[nr_trees++]) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n+\t}\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tparse_tree(trees[i]);\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\trollback_lock_file(&lock_file);\n \t\treturn -1;\n+\t}\n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\treturn error(_(\"unable to write new index file\"));\n \treturn 0;\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340603","messageId":"e902855072b5beb107c9dd5a5651c25c54386d03.1519843916.git.martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"[PATCH v2 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:07:56Z","receivedAt":"2018-02-28T19:08:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If we return early, or if `active_cache_changed` is false, we forget to\nroll back the lockfile.\n\nSigned-off-by: Martin Ågren \n---\n merge-recursive.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex cc5fa0a949..002fb82cf1 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2198,12 +2198,15 @@ int merge_recursive_generic(struct merge_options *o,\n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n-\tif (clean < 0)\n+\tif (clean < 0) {\n+\t\trollback_lock_file(&lock);\n \t\treturn clean;\n+\t}\n \n \tif (active_cache_changed &&\n \t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\treturn err(o, _(\"Unable to write index.\"));\n+\trollback_lock_file(&lock);\n \n \treturn clean ? 0 : 1;\n }\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340610","messageId":"20180228195809.9145-1-martin.agren@gmail.com","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-02-28T19:58:09Z","receivedAt":"2018-02-28T20:00:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"> I'll follow up with a patch to\n> address the confusing pattern which Peff mentioned and which fooled me\n> when I prepared v1.\n\nHere is such a patch on top of the others. I'm not particularly proud of the\nname SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,\nIGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.\n\nI think this makes the current users a bit more obvious, and should help future\nusers get this optimization right.\n\nMartin\n\n-- >8 --\nSubject: write_locked_index(): add flag to avoid writing unchanged index\n\nWe have several callers like\n\n\tif (active_cache_changed && write_locked_index(...))\n\t\thandle_error();\n\trollback_lock_file(...);\n\nwhere the final rollback is needed because \"!active_cache_changed\"\nshortcuts the if-expression. There are also a few variants of this,\nincluding some if-else constructs that make it more clear when the\nexplicit rollback is really needed.\n\nTeach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and\nsimplify the callers. The most complicated of the callers is in\nbuiltin/update-index.c, where we need to check with `force_write` to\ndecide whether we should pass the new flag or not.\n\nA further upshot of this patch is that `active_cache_changed`, which is\ndefined as `the_index.cache_changed`, now only has a few users left.\n\nWe could have made the new flag behave the other way round\n(\"FORCE_WRITE\"), but that could break existing users behind their backs.\nLet's take the more conservative approach. We can still migrate existing\ncallers to use our new flag. Later we might even be able to flip the\ndefault, possibly without entirely ignoring the risk to in-flight or\nout-of-tree topics.\n\nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n cache.h | 4 ++++\n builtin/add.c | 7 +++----\n builtin/commit.c | 10 +++-------\n builtin/merge.c | 15 ++++++---------\n builtin/mv.c | 4 ++--\n builtin/rm.c | 7 +++----\n builtin/update-index.c | 16 +++++++---------\n merge-recursive.c | 5 ++---\n read-cache.c | 6 ++++++\n rerere.c | 8 +++-----\n sequencer.c | 11 +++++------\n 11 files changed, 44 insertions(+), 49 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d8b975a571..905d8eb6cc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);\n \n /* For use with `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n+#define SKIP_IF_UNCHANGED\t(1 << 1)\n \n /*\n * Write the index while holding an already-taken lock. Close the lock,\n@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);\n * With `COMMIT_LOCK`, the lock is always committed or rolled back.\n * Without it, the lock is closed, but neither committed nor rolled\n * back.\n+ *\n+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing\n+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).\n */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n \ndiff --git a/builtin/add.c b/builtin/add.c\nindex bf01d89e28..9e5a80cc6f 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tunplug_bulk_checkin();\n \n finish:\n-\tif (active_cache_changed) {\n-\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\t\tdie(_(\"Unable to write new index file\"));\n-\t}\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(_(\"Unable to write new index file\"));\n \n \tUNLEAK(pathspec);\n \tUNLEAK(dir);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8a87701414..8d9b4081c3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (active_cache_changed\n \t\t || !cache_tree_fully_valid(active_cache_tree))\n \t\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\t\tif (active_cache_changed) {\n-\t\t\tif (write_locked_index(&the_index, &index_lock,\n-\t\t\t\t\t COMMIT_LOCK))\n-\t\t\t\tdie(_(\"unable to write new_index file\"));\n-\t\t} else {\n-\t\t\trollback_lock_file(&index_lock);\n-\t\t}\n+\t\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_AS_IS;\n \t\tret = get_index_file();\n \t\tgoto out;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 30264cfd7c..30c65dfeff 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -651,10 +651,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(&lock);\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n@@ -691,10 +690,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (clean < 0)\n \t\t\texit(128);\n-\t\tif (active_cache_changed &&\n-\t\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\t\tdie (_(\"unable to write %s\"), get_index_file());\n-\t\trollback_lock_file(&lock);\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(strategy, xopts_nr, xopts,\n@@ -810,10 +808,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n \n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(&lock);\n \n \twrite_tree_trivial(&result_tree);\n \tprintf(_(\"Wonderful.\\n\"));\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex cf3684d907..ae013d6d20 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -293,8 +293,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tif (gitmodules_modified)\n \t\tstage_updated_gitmodules(&the_index);\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n \n \treturn 0;\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4a2fcca27b..5d59a0aa65 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -385,10 +385,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tstage_updated_gitmodules(&the_index);\n \t}\n \n-\tif (active_cache_changed) {\n-\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\t\tdie(_(\"Unable to write new index file\"));\n-\t}\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(_(\"Unable to write new index file\"));\n \n \treturn 0;\n }\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 58d1c2d282..f2581904d0 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1182,17 +1182,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\treport(_(\"fsmonitor disabled\"));\n \t}\n \n-\tif (active_cache_changed || force_write) {\n-\t\tif (newfd < 0) {\n-\t\t\tif (refresh_args.flags & REFRESH_QUIET)\n-\t\t\t\texit(128);\n-\t\t\tunable_to_lock_die(get_index_file(), lock_error);\n-\t\t}\n-\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\t\tdie(\"Unable to write new index file\");\n+\tif (newfd < 0 && (active_cache_changed || force_write)) {\n+\t\tif (refresh_args.flags & REFRESH_QUIET)\n+\t\t\texit(128);\n+\t\tunable_to_lock_die(get_index_file(), lock_error);\n \t}\n \n-\trollback_lock_file(&lock_file);\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))\n+\t\tdie(\"Unable to write new index file\");\n \n \treturn has_errors ? 1 : 0;\n }\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 002fb82cf1..410cf4dca3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2203,10 +2203,9 @@ int merge_recursive_generic(struct merge_options *o,\n \t\treturn clean;\n \t}\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\treturn err(o, _(\"Unable to write index.\"));\n-\trollback_lock_file(&lock);\n \n \treturn clean ? 0 : 1;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b9..f46485eb1e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2543,6 +2543,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tint new_shared_index, ret;\n \tstruct split_index *si = istate->split_index;\n \n+\tif ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {\n+\t\tif (flags & COMMIT_LOCK)\n+\t\t\trollback_lock_file(lock);\n+\t\treturn 0;\n+\t}\n+\n \tif (istate->fsmonitor_last_update)\n \t\tfill_fsmonitor_bitmap(istate);\n \ndiff --git a/rerere.c b/rerere.c\nindex 1ce440f4bb..fe47ea50be 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -719,11 +719,9 @@ static void update_paths(struct string_list *update)\n \t\t\titem->string);\n \t}\n \n-\tif (active_cache_changed) {\n-\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-\t\t\tdie(\"Unable to write new index file\");\n-\t} else\n-\t\trollback_lock_file(&index_lock);\n+\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(\"Unable to write new index file\");\n }\n \n static void remove_variant(struct rerere_id *id)\ndiff --git a/sequencer.c b/sequencer.c\nindex 0e6d04e4ce..1efcf1a2c0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -468,15 +468,14 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\treturn clean;\n \t}\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\t/*\n \t\t * TRANSLATORS: %s will be \"revert\", \"cherry-pick\" or\n \t\t * \"rebase -i\".\n \t\t */\n \t\treturn error(_(\"%s: Unable to write new index file\"),\n \t\t\t_(action_name(opts)));\n-\trollback_lock_file(&index_lock);\n \n \tif (opts->signoff)\n \t\tappend_signoff(msgbuf, 0, 0);\n@@ -1191,13 +1190,13 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \t\t\t_(action_name(opts)));\n \t}\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n-\tif (the_index.cache_changed && index_fd >= 0) {\n-\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n+\tif (index_fd >= 0) {\n+\t\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED)) {\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\t_(action_name(opts)));\n \t\t}\n \t}\n-\trollback_lock_file(&index_lock);\n \treturn 0;\n }\n \n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340640","messageId":"xmqqr2p4itvh.fsf@gitster-ct.c.googlers.com","threadId":"47938","inReplyTo":"20180228195809.9145-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T23:20:34Z","receivedAt":"2018-02-28T23:20:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> A further upshot of this patch is that `active_cache_changed`, which is\n> defined as `the_index.cache_changed`, now only has a few users left.\n\nI am undecided if this is a *good* thing. In a few codepaths where\nwe make a speculative update to the on-disk index file, I find that\nthe explicit mention of active_cache_changed clarifies what is going\non. Perhaps once my (and other old timers') eyes get used to this\nnew SKIP_IF_UNCHANGED symbol, it would start serving the same\npurpose ;-)\n\n> We could have made the new flag behave the other way round\n> (\"FORCE_WRITE\"), but that could break existing users behind their backs.\n\nYes, that would break topics in flight that add new calls to\nwrite_locked_index(); they want to write out the index even when\nactive_cache_changed says there is no update.\n\nLooking at a typical pre/post image of this change like this place:\n\n> \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n> \trefresh_cache(REFRESH_QUIET);\n> -\tif (active_cache_changed &&\n> -\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n> +\tif (write_locked_index(&the_index, &lock,\n> +\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n> \t\treturn error(_(\"Unable to write index.\"));\n> -\trollback_lock_file(&lock);\n\nthis certainly simplifies what the caller needs to do.\n\n> @@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);\n> * With `COMMIT_LOCK`, the lock is always committed or rolled back.\n> * Without it, the lock is closed, but neither committed nor rolled\n> * back.\n> + *\n> + * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing\n> + * is written (and the lock is rolled back if `COMMIT_LOCK` is given).\n> */\n> extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n\nGood.\n"},{"id":"340685","messageId":"CAN0heSpcaR3bzHuD3AG48p8DagD_HDY8kVxYNctS1Qhoopcvqg@mail.gmail.com","threadId":"47938","inReplyTo":"xmqqr2p4itvh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-03-01T06:18:40Z","receivedAt":"2018-03-01T06:18:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 1 March 2018 at 00:20, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> A further upshot of this patch is that `active_cache_changed`, which is\n>> defined as `the_index.cache_changed`, now only has a few users left.\n>\n> I am undecided if this is a *good* thing. In a few codepaths where\n> we make a speculative update to the on-disk index file, I find that\n> the explicit mention of active_cache_changed clarifies what is going\n> on. Perhaps once my (and other old timers') eyes get used to this\n> new SKIP_IF_UNCHANGED symbol, it would start serving the same\n> purpose ;-)\n\nRight, you might say that this trades one symbol for another. What I\nmeant was, we only have a few \"active_cache_changed\" and soon (TM) they\nmight all be \"the_index.cache_changed\" or \"index->cache_changed\" and the\nmacro could be retired. My understanding of the history is limited, but\nI was under the impression that this was like a transition macro (albeit\na very old one!).\n\nMartin\n"},{"id":"340689","messageId":"20180301073855.GD31079@sigill.intra.peff.net","threadId":"47938","inReplyTo":"cover.1519843916.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-01T07:38:56Z","receivedAt":"2018-03-01T07:39:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 28, 2018 at 08:07:53PM +0100, Martin Ågren wrote:\n\n> This is v2 of my series to always release locks. As before, there's a\n> conflict with pu, where the correct resolution is to take my version of\n> the conflicting hunk.\n> \n> The only difference to v1 is in patch 3. I'll follow up with a patch to\n> address the confusing pattern which Peff mentioned and which fooled me\n> when I prepared v1.\n\nThis looks good to me. And I'm glad my rambling helped find something\nuseful. ;)\n\n-Peff\n"},{"id":"340690","messageId":"20180301074129.GE31079@sigill.intra.peff.net","threadId":"47938","inReplyTo":"20180228195809.9145-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-01T07:41:29Z","receivedAt":"2018-03-01T07:41:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:\n\n> > I'll follow up with a patch to\n> > address the confusing pattern which Peff mentioned and which fooled me\n> > when I prepared v1.\n> \n> Here is such a patch on top of the others. I'm not particularly proud of the\n> name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,\n> IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.\n> \n> I think this makes the current users a bit more obvious, and should help future\n> users get this optimization right.\n\nIMHO the result is easier to follow. Except for one case:\n\n> -\tif (active_cache_changed || force_write) {\n> -\t\tif (newfd < 0) {\n> -\t\t\tif (refresh_args.flags & REFRESH_QUIET)\n> -\t\t\t\texit(128);\n> -\t\t\tunable_to_lock_die(get_index_file(), lock_error);\n> -\t\t}\n> -\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> -\t\t\tdie(\"Unable to write new index file\");\n> +\tif (newfd < 0 && (active_cache_changed || force_write)) {\n> +\t\tif (refresh_args.flags & REFRESH_QUIET)\n> +\t\t\texit(128);\n> +\t\tunable_to_lock_die(get_index_file(), lock_error);\n> \t}\n> \n> -\trollback_lock_file(&lock_file);\n> +\tif (write_locked_index(&the_index, &lock_file,\n> +\t\t\t COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))\n> +\t\tdie(\"Unable to write new index file\");\n\nwhere I think the logic just ends up repeating itself. I guess you were\nanxious to try to get rid of active_cached_changed, but I don't think\nkeeping it around is really that big a deal (and certainly another trick\nis to just say \"the_index.cache_changed\").\n\n-Peff\n"},{"id":"340751","messageId":"xmqq4llzha5m.fsf@gitster-ct.c.googlers.com","threadId":"47938","inReplyTo":"20180301074129.GE31079@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-01T19:24:05Z","receivedAt":"2018-03-01T19:24:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> IMHO the result is easier to follow. Except for one case:\n>\n>> -\tif (active_cache_changed || force_write) {\n>> -\t\tif (newfd < 0) {\n>> -\t\t\tif (refresh_args.flags & REFRESH_QUIET)\n>> -\t\t\t\texit(128);\n>> -\t\t\tunable_to_lock_die(get_index_file(), lock_error);\n>> -\t\t}\n>> -\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n>> -\t\t\tdie(\"Unable to write new index file\");\n>> +\tif (newfd < 0 && (active_cache_changed || force_write)) {\n>> +\t\tif (refresh_args.flags & REFRESH_QUIET)\n>> +\t\t\texit(128);\n>> +\t\tunable_to_lock_die(get_index_file(), lock_error);\n>> \t}\n>> \n>> -\trollback_lock_file(&lock_file);\n>> +\tif (write_locked_index(&the_index, &lock_file,\n>> +\t\t\t COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))\n>> +\t\tdie(\"Unable to write new index file\");\n>\n> where I think the logic just ends up repeating itself.\n\nYup, this one I also had a bit of trouble with.\n"},{"id":"340758","messageId":"20180301204020.24406-1-martin.agren@gmail.com","threadId":"47938","inReplyTo":"xmqq4llzha5m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-03-01T20:40:20Z","receivedAt":"2018-03-01T20:40:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 1 March 2018 at 20:24, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> IMHO the result is easier to follow. Except for one case:\n>>\n>>> [...]\n>>\n>> where I think the logic just ends up repeating itself.\n>\n> Yup, this one I also had a bit of trouble with.\n\nThanks for your feedback, both of you. It's much appreciated.\n\nAfter thinking about it, I tend to agree. That rewrite loses an\nindentation level and makes it a bit clearer that we have two steps,\n\"maybe bail\" and \"write\". But at the cost of duplicating logic -- after\nall, those two steps are very closely related, so there's no need to\nartificially separate them.\n\nHere it is again, without that hunk, and without the commit message\nclaim that it'd be a good thing to have just a few uses of\n\"active_cache_changed\" remaining.\n\nThis could go as a patch 6/5 onto ma/roll-back-lockfiles. I keep\ndebating myself whether this would do better earlier in such a six-pack,\nwhere it would be \"also, this releases a lock where we used to forget\nto\". Right now, there is overlap between patch 3/5 (which adds a line)\nand this patch (which removes it). This patch doesn't entirely negate\nthe need for patch 3/5 though..\n\nMartin\n\n-- >8 --\nSubject: write_locked_index(): add flag to avoid writing unchanged index\n\nWe have several callers like\n\n\tif (active_cache_changed && write_locked_index(...))\n\t\thandle_error();\n\trollback_lock_file(...);\n\nwhere the final rollback is needed because \"!active_cache_changed\"\nshortcuts the if-expression. There are also a few variants of this,\nincluding some if-else constructs that make it more clear when the\nexplicit rollback is really needed.\n\nTeach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and\nsimplify the callers. Leave the most complicated of the callers (in\nbuiltin/update-index.c) unchanged. Rewriting it to use this new flag\nwould end up duplicating logic.\n\nWe could have made the new flag behave the other way round\n(\"FORCE_WRITE\"), but that could break existing users behind their backs.\nLet's take the more conservative approach. We can still migrate existing\ncallers to use our new flag. Later we might even be able to flip the\ndefault, possibly without entirely ignoring the risk to in-flight or\nout-of-tree topics.\n\nSuggested-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n cache.h | 4 ++++\n builtin/add.c | 7 +++----\n builtin/commit.c | 10 +++-------\n builtin/merge.c | 15 ++++++---------\n builtin/mv.c | 4 ++--\n builtin/rm.c | 7 +++----\n merge-recursive.c | 5 ++---\n read-cache.c | 6 ++++++\n rerere.c | 8 +++-----\n sequencer.c | 11 +++++------\n 10 files changed, 37 insertions(+), 40 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex d8b975a571..905d8eb6cc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);\n \n /* For use with `write_locked_index()`. */\n #define COMMIT_LOCK\t\t(1 << 0)\n+#define SKIP_IF_UNCHANGED\t(1 << 1)\n \n /*\n * Write the index while holding an already-taken lock. Close the lock,\n@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);\n * With `COMMIT_LOCK`, the lock is always committed or rolled back.\n * Without it, the lock is closed, but neither committed nor rolled\n * back.\n+ *\n+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing\n+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).\n */\n extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);\n \ndiff --git a/builtin/add.c b/builtin/add.c\nindex bf01d89e28..9e5a80cc6f 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tunplug_bulk_checkin();\n \n finish:\n-\tif (active_cache_changed) {\n-\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\t\tdie(_(\"Unable to write new index file\"));\n-\t}\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(_(\"Unable to write new index file\"));\n \n \tUNLEAK(pathspec);\n \tUNLEAK(dir);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8a87701414..8d9b4081c3 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (active_cache_changed\n \t\t || !cache_tree_fully_valid(active_cache_tree))\n \t\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n-\t\tif (active_cache_changed) {\n-\t\t\tif (write_locked_index(&the_index, &index_lock,\n-\t\t\t\t\t COMMIT_LOCK))\n-\t\t\t\tdie(_(\"unable to write new_index file\"));\n-\t\t} else {\n-\t\t\trollback_lock_file(&index_lock);\n-\t\t}\n+\t\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_AS_IS;\n \t\tret = get_index_file();\n \t\tgoto out;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 30264cfd7c..30c65dfeff 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -651,10 +651,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(&lock);\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n@@ -691,10 +690,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (clean < 0)\n \t\t\texit(128);\n-\t\tif (active_cache_changed &&\n-\t\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\t\tif (write_locked_index(&the_index, &lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\t\tdie (_(\"unable to write %s\"), get_index_file());\n-\t\trollback_lock_file(&lock);\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(strategy, xopts_nr, xopts,\n@@ -810,10 +808,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)\n \n \thold_locked_index(&lock, LOCK_DIE_ON_ERROR);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(&lock);\n \n \twrite_tree_trivial(&result_tree);\n \tprintf(_(\"Wonderful.\\n\"));\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex cf3684d907..ae013d6d20 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -293,8 +293,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tif (gitmodules_modified)\n \t\tstage_updated_gitmodules(&the_index);\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n \n \treturn 0;\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 4a2fcca27b..5d59a0aa65 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -385,10 +385,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tstage_updated_gitmodules(&the_index);\n \t}\n \n-\tif (active_cache_changed) {\n-\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n-\t\t\tdie(_(\"Unable to write new index file\"));\n-\t}\n+\tif (write_locked_index(&the_index, &lock_file,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(_(\"Unable to write new index file\"));\n \n \treturn 0;\n }\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 002fb82cf1..410cf4dca3 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2203,10 +2203,9 @@ int merge_recursive_generic(struct merge_options *o,\n \t\treturn clean;\n \t}\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\treturn err(o, _(\"Unable to write index.\"));\n-\trollback_lock_file(&lock);\n \n \treturn clean ? 0 : 1;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 2eb81a66b9..f46485eb1e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2543,6 +2543,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,\n \tint new_shared_index, ret;\n \tstruct split_index *si = istate->split_index;\n \n+\tif ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {\n+\t\tif (flags & COMMIT_LOCK)\n+\t\t\trollback_lock_file(lock);\n+\t\treturn 0;\n+\t}\n+\n \tif (istate->fsmonitor_last_update)\n \t\tfill_fsmonitor_bitmap(istate);\n \ndiff --git a/rerere.c b/rerere.c\nindex 1ce440f4bb..fe47ea50be 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -719,11 +719,9 @@ static void update_paths(struct string_list *update)\n \t\t\titem->string);\n \t}\n \n-\tif (active_cache_changed) {\n-\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n-\t\t\tdie(\"Unable to write new index file\");\n-\t} else\n-\t\trollback_lock_file(&index_lock);\n+\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n+\t\tdie(\"Unable to write new index file\");\n }\n \n static void remove_variant(struct rerere_id *id)\ndiff --git a/sequencer.c b/sequencer.c\nindex 0e6d04e4ce..1efcf1a2c0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -468,15 +468,14 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\treturn clean;\n \t}\n \n-\tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n+\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\t/*\n \t\t * TRANSLATORS: %s will be \"revert\", \"cherry-pick\" or\n \t\t * \"rebase -i\".\n \t\t */\n \t\treturn error(_(\"%s: Unable to write new index file\"),\n \t\t\t_(action_name(opts)));\n-\trollback_lock_file(&index_lock);\n \n \tif (opts->signoff)\n \t\tappend_signoff(msgbuf, 0, 0);\n@@ -1191,13 +1190,13 @@ static int read_and_refresh_cache(struct replay_opts *opts)\n \t\t\t_(action_name(opts)));\n \t}\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n-\tif (the_index.cache_changed && index_fd >= 0) {\n-\t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {\n+\tif (index_fd >= 0) {\n+\t\tif (write_locked_index(&the_index, &index_lock,\n+\t\t\t\t COMMIT_LOCK | SKIP_IF_UNCHANGED)) {\n \t\t\treturn error(_(\"git %s: failed to refresh the index\"),\n \t\t\t\t_(action_name(opts)));\n \t\t}\n \t}\n-\trollback_lock_file(&index_lock);\n \treturn 0;\n }\n \n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"340794","messageId":"20180302104904.GC11074@sigill.intra.peff.net","threadId":"47938","inReplyTo":"20180301204020.24406-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 0/5] roll back locks in various code paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-02T10:49:04Z","receivedAt":"2018-03-02T10:49:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 01, 2018 at 09:40:20PM +0100, Martin Ågren wrote:\n\n> After thinking about it, I tend to agree. That rewrite loses an\n> indentation level and makes it a bit clearer that we have two steps,\n> \"maybe bail\" and \"write\". But at the cost of duplicating logic -- after\n> all, those two steps are very closely related, so there's no need to\n> artificially separate them.\n> \n> Here it is again, without that hunk, and without the commit message\n> claim that it'd be a good thing to have just a few uses of\n> \"active_cache_changed\" remaining.\n\nThanks, this version looks good to me. The name SKIP_IF_UNCHANGED is\ngeneric and may result in clashes down the road. But then so is the name\nCOMMIT_LOCK. I'm OK to punt on that until we do see such a collision, at\nwhich point we may want to provide a consistent namespace for these\nflags.\n\n-Peff\n"}]}