[PATCH 0/5] roll back locks in various code paths

[PATCH 0/5] roll back locks in various code paths

[Martin Ågren]

Patches 2-4 are the actual fixes where I teach some functions to always
roll back the lock they're holding. Notably, these are all in "libgit".

Patch 1 is a "while at it" to use locks on the stack instead of having
them be static. Patch 5 removes code to roll back locks which are
already rolled back.

I've based this on maint. There's a conflict on pu, with c7d4394111
(sequencer: avoid using errno clobbered by rollback_lock_file(),
2018-02-11). The conflict resolution would be to take my version for the
"could not lock HEAD"-hunk.

Martin

Martin Ågren (5):
  sequencer: make lockfiles non-static
  sequencer: always roll back lock in `do_recursive_merge()`
  merge-recursive: always roll back lock in `merge_recursive_generic()`
  merge: always roll back lock in `checkout_fast_forward()`
  sequencer: do not roll back lockfile unnecessarily

 merge-recursive.c |  4 +++-
 merge.c           | 12 +++++++++---
 sequencer.c       | 32 ++++++++++++++------------------
 3 files changed, 26 insertions(+), 22 deletions(-)

-- 
2.16.2.246.ga4ee44448f

[PATCH 1/5] sequencer: make lockfiles non-static

[Martin Ågren]

After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can have lockfiles on the stack.

One of these functions fails to always roll back the lock. That will be
fixed in the next commit.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 sequencer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4d3f60594c..90807c4559 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -290,7 +290,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 static int write_message(const void *buf, size_t len, const char *filename,
 			 int append_eol)
 {
-	static struct lock_file msg_file;
+	struct lock_file msg_file = LOCK_INIT;
 
 	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
 	if (msg_fd < 0)
@@ -436,7 +436,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	int clean;
 	char **xopt;
-	static struct lock_file index_lock;
+	struct lock_file index_lock = LOCK_INIT;
 
 	if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)
 		return -1;
@@ -1183,7 +1183,7 @@ static int prepare_revs(struct replay_opts *opts)
 
 static int read_and_refresh_cache(struct replay_opts *opts)
 {
-	static struct lock_file index_lock;
+	struct lock_file index_lock = LOCK_INIT;
 	int index_fd = hold_locked_index(&index_lock, 0);
 	if (read_index_preload(&the_index, NULL) < 0) {
 		rollback_lock_file(&index_lock);
@@ -1577,7 +1577,7 @@ static int create_seq_dir(void)
 
 static int save_head(const char *head)
 {
-	static struct lock_file head_lock;
+	struct lock_file head_lock = LOCK_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 	ssize_t written;
@@ -1702,7 +1702,7 @@ int sequencer_rollback(struct replay_opts *opts)
 
 static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 {
-	static struct lock_file todo_lock;
+	struct lock_file todo_lock = LOCK_INIT;
 	const char *todo_path = get_todo_path(opts);
 	int next = todo_list->current, offset, fd;
 
-- 
2.16.2.246.ga4ee44448f

[PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

[Martin Ågren]

If we return early, we forget to roll back the lockfile. Do so.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 90807c4559..e6bac4692a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		fputs(o.obuf.buf, stdout);
 	strbuf_release(&o.obuf);
 	diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
-	if (clean < 0)
+	if (clean < 0) {
+		rollback_lock_file(&index_lock);
 		return clean;
+	}
 
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
-- 
2.16.2.246.ga4ee44448f

[PATCH 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`

[Martin Ågren]

If we return early, we forget to roll back the lockfile. Do so.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 merge-recursive.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cc5fa0a949..7345125691 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2198,8 +2198,10 @@ int merge_recursive_generic(struct merge_options *o,
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
 			result);
-	if (clean < 0)
+	if (clean < 0) {
+		rollback_lock_file(&lock);
 		return clean;
+	}
 
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
-- 
2.16.2.246.ga4ee44448f

[PATCH 4/5] merge: always roll back lock in `checkout_fast_forward()`

[Martin Ågren]

This function originated in builtin/merge.c. It was moved to merge.c in
commit db699a8a1f (Move try_merge_command and checkout_fast_forward to
libgit.a, 2012-10-26), but was used from sequencer.c even before that.

If a problem occurs, the function returns without rolling back the
lockfile. Teach it to do so.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 merge.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/merge.c b/merge.c
index 195b578700..f06a4773d4 100644
--- a/merge.c
+++ b/merge.c
@@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head,
 	setup_unpack_trees_porcelain(&opts, "merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
-	if (!trees[nr_trees++])
+	if (!trees[nr_trees++]) {
+		rollback_lock_file(&lock_file);
 		return -1;
+	}
 	trees[nr_trees] = parse_tree_indirect(remote);
-	if (!trees[nr_trees++])
+	if (!trees[nr_trees++]) {
+		rollback_lock_file(&lock_file);
 		return -1;
+	}
 	for (i = 0; i < nr_trees; i++) {
 		parse_tree(trees[i]);
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
+	if (unpack_trees(nr_trees, t, &opts)) {
+		rollback_lock_file(&lock_file);
 		return -1;
+	}
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		return error(_("unable to write new index file"));
 	return 0;
-- 
2.16.2.246.ga4ee44448f

[PATCH 5/5] sequencer: do not roll back lockfile unnecessarily

[Martin Ågren]

If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is
no need to call `rollback_lock_file()` on the lockfile. It doesn't hurt
either, but it does make different callers in this file inconsistent,
which might be confusing.

While at it, remove a trailing '.' from a recurring error message.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 sequencer.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e6bac4692a..0e6d04e4ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,10 +303,8 @@ static int write_message(const void *buf, size_t len, const char *filename,
 		rollback_lock_file(&msg_file);
 		return error_errno(_("could not write eol to '%s'"), filename);
 	}
-	if (commit_lock_file(&msg_file) < 0) {
-		rollback_lock_file(&msg_file);
-		return error(_("failed to finalize '%s'."), filename);
-	}
+	if (commit_lock_file(&msg_file) < 0)
+		return error(_("failed to finalize '%s'"), filename);
 
 	return 0;
 }
@@ -1585,10 +1583,8 @@ static int save_head(const char *head)
 	ssize_t written;
 
 	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
-	if (fd < 0) {
-		rollback_lock_file(&head_lock);
+	if (fd < 0)
 		return error_errno(_("could not lock HEAD"));
-	}
 	strbuf_addf(&buf, "%s\n", head);
 	written = write_in_full(fd, buf.buf, buf.len);
 	strbuf_release(&buf);
@@ -1597,10 +1593,8 @@ static int save_head(const char *head)
 		return error_errno(_("could not write to '%s'"),
 				   git_path_head_file());
 	}
-	if (commit_lock_file(&head_lock) < 0) {
-		rollback_lock_file(&head_lock);
-		return error(_("failed to finalize '%s'."), git_path_head_file());
-	}
+	if (commit_lock_file(&head_lock) < 0)
+		return error(_("failed to finalize '%s'"), git_path_head_file());
 	return 0;
 }
 
@@ -1724,7 +1718,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 			todo_list->buf.len - offset) < 0)
 		return error_errno(_("could not write to '%s'"), todo_path);
 	if (commit_lock_file(&todo_lock) < 0)
-		return error(_("failed to finalize '%s'."), todo_path);
+		return error(_("failed to finalize '%s'"), todo_path);
 
 	if (is_rebase_i(opts)) {
 		const char *done_path = rebase_path_done();
-- 
2.16.2.246.ga4ee44448f

Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

[Jeff King]

On Tue, Feb 27, 2018 at 10:30:10PM +0100, Martin Ågren wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 90807c4559..e6bac4692a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  		fputs(o.obuf.buf, stdout);
>  	strbuf_release(&o.obuf);
>  	diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
> -	if (clean < 0)
> +	if (clean < 0) {
> +		rollback_lock_file(&index_lock);
>  		return clean;
> +	}
>  
>  	if (active_cache_changed &&
>  	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))

This addition is obviously correct.

I want to note one thing that confused me while reviewing. While looking
to see if there were other returns, I noticed that the lines right near
the end of your context are funny:

        if (active_cache_changed &&
              write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
                  /*
                   * TRANSLATORS: %s will be "revert", "cherry-pick" or
                   * "rebase -i".
                   */
                  return error(_("%s: Unable to write new index file"),
                          _(action_name(opts)));
          rollback_lock_file(&index_lock);

At first I thought that rollback was a noop, since write_locked_index()
would always either commit or rollback. But it's needed for the case
when we active_cache_changed isn't true.

So I think it's correct as-is, but I wonder if writing it as:

  if (!active_cache_changed)
	rollback_lock_file(&index_lock);
  else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
	return error(...);

might be easier to follow. I'm OK with leaving it, too, but thought I'd
mention it in case it confused other reviewers.

-Peff

Re: [PATCH 0/5] roll back locks in various code paths

[Jeff King]

On Tue, Feb 27, 2018 at 10:30:08PM +0100, Martin Ågren wrote:

> Patches 2-4 are the actual fixes where I teach some functions to always
> roll back the lock they're holding. Notably, these are all in "libgit".
> 
> Patch 1 is a "while at it" to use locks on the stack instead of having
> them be static. Patch 5 removes code to roll back locks which are
> already rolled back.

These all look good to me. Happy to see us dropping the "static" from
more lockfiles, too.

-Peff

Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

[Martin Ågren]

On 27 February 2018 at 22:44, Jeff King <peff@peff.net> wrote:
> I want to note one thing that confused me while reviewing. While looking
> to see if there were other returns, I noticed that the lines right near
> the end of your context are funny:
>
>         if (active_cache_changed &&
>               write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
>                   /*
>                    * TRANSLATORS: %s will be "revert", "cherry-pick" or
>                    * "rebase -i".
>                    */
>                   return error(_("%s: Unable to write new index file"),
>                           _(action_name(opts)));
>           rollback_lock_file(&index_lock);
>
> At first I thought that rollback was a noop, since write_locked_index()
> would always either commit or rollback. But it's needed for the case
> when we active_cache_changed isn't true.
>
> So I think it's correct as-is, but I wonder if writing it as:
>
>   if (!active_cache_changed)
>         rollback_lock_file(&index_lock);
>   else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
>         return error(...);
>
> might be easier to follow. I'm OK with leaving it, too, but thought I'd
> mention it in case it confused other reviewers.

I also hesitated at that one. There are some similar instances elsewhere, e.g.,
in builtin/merge.c. There's also rerere.c, which does a variant of your
suggestion.

Martin

Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`

[Jeff King]

On Tue, Feb 27, 2018 at 11:08:12PM +0100, Martin Ågren wrote:

> > So I think it's correct as-is, but I wonder if writing it as:
> >
> >   if (!active_cache_changed)
> >         rollback_lock_file(&index_lock);
> >   else if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> >         return error(...);
> >
> > might be easier to follow. I'm OK with leaving it, too, but thought I'd
> > mention it in case it confused other reviewers.
> 
> I also hesitated at that one. There are some similar instances elsewhere, e.g.,
> in builtin/merge.c. There's also rerere.c, which does a variant of your
> suggestion.

Hmm, yeah, grepping shows quite a few of various forms.

I wonder if it is worth a helper like:

  /* like write_locked_index(), but optimize out unchanged writes */
  static int maybe_write_locked_index(struct index *index,
                                      struct lock_file *lock,
				      unsigned flags)
  {
	if (!index->cached_changed) {
		if (flags & COMMIT_LOCK)
			rollback_lock_file(lock);
		return 0;
	}
	return write_locked_index(index, lock, flags);
  }

Alternatively, it could just be a flag to write_locked_index() to enable
the optimization.

I actually suspect that most callers would prefer to have it kick in by
default (with an optional flag to disable it if some caller really needs
to), but that would possibly be a subtle breakage for the caller that
needs the flag.

-Peff

Re: [PATCH 0/5] roll back locks in various code paths

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> Patches 2-4 are the actual fixes where I teach some functions to always
> roll back the lock they're holding. Notably, these are all in "libgit".
>
> Patch 1 is a "while at it" to use locks on the stack instead of having
> them be static. Patch 5 removes code to roll back locks which are
> already rolled back.
>
> I've based this on maint. There's a conflict on pu, with c7d4394111
> (sequencer: avoid using errno clobbered by rollback_lock_file(),
> 2018-02-11). The conflict resolution would be to take my version for the
> "could not lock HEAD"-hunk.

Thanks for running a trial merge before sending your patches out.  I
wish there were more contributors like you ;-)

The changes looked reasonable.

Re: [PATCH 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`

[Martin Ågren]

On 27 February 2018 at 22:30, Martin Ågren <martin.agren@gmail.com> wrote:
> If we return early, we forget to roll back the lockfile. Do so.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  merge-recursive.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index cc5fa0a949..7345125691 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2198,8 +2198,10 @@ int merge_recursive_generic(struct merge_options *o,
>         hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>         clean = merge_recursive(o, head_commit, next_commit, ca,
>                         result);
> -       if (clean < 0)
> +       if (clean < 0) {
> +               rollback_lock_file(&lock);
>                 return clean;
> +       }
>
>         if (active_cache_changed &&
>             write_locked_index(&the_index, &lock, COMMIT_LOCK))

This is incomplete. If "!active_cache_changed", this function fails to
roll back the lock. I'll reroll this series.

This also highlights the point Peff made in [1] about how that
"active_cache_changed &&" can be confusing. I'll look into the added or
extended API he proposed, although perhaps not as part of the reroll.

Martin

[1] https://public-inbox.org/git/20180227214430.GC6899@sigill.intra.peff.net/

[PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

This is v2 of my series to always release locks. As before, there's a
conflict with pu, where the correct resolution is to take my version of
the conflicting hunk.

The only difference to v1 is in patch 3. I'll follow up with a patch to
address the confusing pattern which Peff mentioned and which fooled me
when I prepared v1.

Martin

Martin Ågren (5):
  sequencer: make lockfiles non-static
  sequencer: always roll back lock in `do_recursive_merge()`
  merge-recursive: always roll back lock in `merge_recursive_generic()`
  merge: always roll back lock in `checkout_fast_forward()`
  sequencer: do not roll back lockfile unnecessarily

 merge-recursive.c |  5 ++++-
 merge.c           | 12 +++++++++---
 sequencer.c       | 32 ++++++++++++++------------------
 3 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.16.2.246.ga4ee44448f

[PATCH v2 1/5] sequencer: make lockfiles non-static

[Martin Ågren]

After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
we can have lockfiles on the stack.

One of these functions fails to always roll back the lock. That will be
fixed in the next commit.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 sequencer.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4d3f60594c..90807c4559 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -290,7 +290,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 static int write_message(const void *buf, size_t len, const char *filename,
 			 int append_eol)
 {
-	static struct lock_file msg_file;
+	struct lock_file msg_file = LOCK_INIT;
 
 	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
 	if (msg_fd < 0)
@@ -436,7 +436,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	int clean;
 	char **xopt;
-	static struct lock_file index_lock;
+	struct lock_file index_lock = LOCK_INIT;
 
 	if (hold_locked_index(&index_lock, LOCK_REPORT_ON_ERROR) < 0)
 		return -1;
@@ -1183,7 +1183,7 @@ static int prepare_revs(struct replay_opts *opts)
 
 static int read_and_refresh_cache(struct replay_opts *opts)
 {
-	static struct lock_file index_lock;
+	struct lock_file index_lock = LOCK_INIT;
 	int index_fd = hold_locked_index(&index_lock, 0);
 	if (read_index_preload(&the_index, NULL) < 0) {
 		rollback_lock_file(&index_lock);
@@ -1577,7 +1577,7 @@ static int create_seq_dir(void)
 
 static int save_head(const char *head)
 {
-	static struct lock_file head_lock;
+	struct lock_file head_lock = LOCK_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 	ssize_t written;
@@ -1702,7 +1702,7 @@ int sequencer_rollback(struct replay_opts *opts)
 
 static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 {
-	static struct lock_file todo_lock;
+	struct lock_file todo_lock = LOCK_INIT;
 	const char *todo_path = get_todo_path(opts);
 	int next = todo_list->current, offset, fd;
 
-- 
2.16.2.246.ga4ee44448f

[PATCH v2 2/5] sequencer: always roll back lock in `do_recursive_merge()`

[Martin Ågren]

If we return early, we forget to roll back the lockfile. Do so.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 sequencer.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 90807c4559..e6bac4692a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -465,8 +465,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		fputs(o.obuf.buf, stdout);
 	strbuf_release(&o.obuf);
 	diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
-	if (clean < 0)
+	if (clean < 0) {
+		rollback_lock_file(&index_lock);
 		return clean;
+	}
 
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
-- 
2.16.2.246.ga4ee44448f

[PATCH v2 3/5] merge-recursive: always roll back lock in `merge_recursive_generic()`

[Martin Ågren]

If we return early, or if `active_cache_changed` is false, we forget to
roll back the lockfile.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 merge-recursive.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cc5fa0a949..002fb82cf1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2198,12 +2198,15 @@ int merge_recursive_generic(struct merge_options *o,
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
 			result);
-	if (clean < 0)
+	if (clean < 0) {
+		rollback_lock_file(&lock);
 		return clean;
+	}
 
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
 		return err(o, _("Unable to write index."));
+	rollback_lock_file(&lock);
 
 	return clean ? 0 : 1;
 }
-- 
2.16.2.246.ga4ee44448f

[PATCH v2 4/5] merge: always roll back lock in `checkout_fast_forward()`

[Martin Ågren]

This function originated in builtin/merge.c. It was moved to merge.c in
commit db699a8a1f (Move try_merge_command and checkout_fast_forward to
libgit.a, 2012-10-26), but was used from sequencer.c even before that.

If a problem occurs, the function returns without rolling back the
lockfile. Teach it to do so.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 merge.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/merge.c b/merge.c
index 195b578700..f06a4773d4 100644
--- a/merge.c
+++ b/merge.c
@@ -113,17 +113,23 @@ int checkout_fast_forward(const struct object_id *head,
 	setup_unpack_trees_porcelain(&opts, "merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
-	if (!trees[nr_trees++])
+	if (!trees[nr_trees++]) {
+		rollback_lock_file(&lock_file);
 		return -1;
+	}
 	trees[nr_trees] = parse_tree_indirect(remote);
-	if (!trees[nr_trees++])
+	if (!trees[nr_trees++]) {
+		rollback_lock_file(&lock_file);
 		return -1;
+	}
 	for (i = 0; i < nr_trees; i++) {
 		parse_tree(trees[i]);
 		init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
 	}
-	if (unpack_trees(nr_trees, t, &opts))
+	if (unpack_trees(nr_trees, t, &opts)) {
+		rollback_lock_file(&lock_file);
 		return -1;
+	}
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		return error(_("unable to write new index file"));
 	return 0;
-- 
2.16.2.246.ga4ee44448f

[PATCH v2 5/5] sequencer: do not roll back lockfile unnecessarily

[Martin Ågren]

If `commit_lock_file()` or `hold_lock_file_for_update()` fail, there is
no need to call `rollback_lock_file()` on the lockfile. It doesn't hurt
either, but it does make different callers in this file inconsistent,
which might be confusing.

While at it, remove a trailing '.' from a recurring error message.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 sequencer.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e6bac4692a..0e6d04e4ce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,10 +303,8 @@ static int write_message(const void *buf, size_t len, const char *filename,
 		rollback_lock_file(&msg_file);
 		return error_errno(_("could not write eol to '%s'"), filename);
 	}
-	if (commit_lock_file(&msg_file) < 0) {
-		rollback_lock_file(&msg_file);
-		return error(_("failed to finalize '%s'."), filename);
-	}
+	if (commit_lock_file(&msg_file) < 0)
+		return error(_("failed to finalize '%s'"), filename);
 
 	return 0;
 }
@@ -1585,10 +1583,8 @@ static int save_head(const char *head)
 	ssize_t written;
 
 	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), 0);
-	if (fd < 0) {
-		rollback_lock_file(&head_lock);
+	if (fd < 0)
 		return error_errno(_("could not lock HEAD"));
-	}
 	strbuf_addf(&buf, "%s\n", head);
 	written = write_in_full(fd, buf.buf, buf.len);
 	strbuf_release(&buf);
@@ -1597,10 +1593,8 @@ static int save_head(const char *head)
 		return error_errno(_("could not write to '%s'"),
 				   git_path_head_file());
 	}
-	if (commit_lock_file(&head_lock) < 0) {
-		rollback_lock_file(&head_lock);
-		return error(_("failed to finalize '%s'."), git_path_head_file());
-	}
+	if (commit_lock_file(&head_lock) < 0)
+		return error(_("failed to finalize '%s'"), git_path_head_file());
 	return 0;
 }
 
@@ -1724,7 +1718,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 			todo_list->buf.len - offset) < 0)
 		return error_errno(_("could not write to '%s'"), todo_path);
 	if (commit_lock_file(&todo_lock) < 0)
-		return error(_("failed to finalize '%s'."), todo_path);
+		return error(_("failed to finalize '%s'"), todo_path);
 
 	if (is_rebase_i(opts)) {
 		const char *done_path = rebase_path_done();
-- 
2.16.2.246.ga4ee44448f

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

> I'll follow up with a patch to
> address the confusing pattern which Peff mentioned and which fooled me
> when I prepared v1.

Here is such a patch on top of the others. I'm not particularly proud of the
name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.

I think this makes the current users a bit more obvious, and should help future
users get this optimization right.

Martin

-- >8 --
Subject: write_locked_index(): add flag to avoid writing unchanged index

We have several callers like

	if (active_cache_changed && write_locked_index(...))
		handle_error();
	rollback_lock_file(...);

where the final rollback is needed because "!active_cache_changed"
shortcuts the if-expression. There are also a few variants of this,
including some if-else constructs that make it more clear when the
explicit rollback is really needed.

Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and
simplify the callers. The most complicated of the callers is in
builtin/update-index.c, where we need to check with `force_write` to
decide whether we should pass the new flag or not.

A further upshot of this patch is that `active_cache_changed`, which is
defined as `the_index.cache_changed`, now only has a few users left.

We could have made the new flag behave the other way round
("FORCE_WRITE"), but that could break existing users behind their backs.
Let's take the more conservative approach. We can still migrate existing
callers to use our new flag. Later we might even be able to flip the
default, possibly without entirely ignoring the risk to in-flight or
out-of-tree topics.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 cache.h                |  4 ++++
 builtin/add.c          |  7 +++----
 builtin/commit.c       | 10 +++-------
 builtin/merge.c        | 15 ++++++---------
 builtin/mv.c           |  4 ++--
 builtin/rm.c           |  7 +++----
 builtin/update-index.c | 16 +++++++---------
 merge-recursive.c      |  5 ++---
 read-cache.c           |  6 ++++++
 rerere.c               |  8 +++-----
 sequencer.c            | 11 +++++------
 11 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..905d8eb6cc 100644
--- a/cache.h
+++ b/cache.h
@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK		(1 << 0)
+#define SKIP_IF_UNCHANGED	(1 << 1)
 
 /*
  * Write the index while holding an already-taken lock. Close the lock,
@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.
+ *
+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);
 
diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e28..9e5a80cc6f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	unplug_bulk_checkin();
 
 finish:
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-			die(_("Unable to write new index file"));
-	}
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die(_("Unable to write new index file"));
 
 	UNLEAK(pathspec);
 	UNLEAK(dir);
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..8d9b4081c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (active_cache_changed
 		    || !cache_tree_fully_valid(active_cache_tree))
 			update_main_cache_tree(WRITE_TREE_SILENT);
-		if (active_cache_changed) {
-			if (write_locked_index(&the_index, &index_lock,
-					       COMMIT_LOCK))
-				die(_("unable to write new_index file"));
-		} else {
-			rollback_lock_file(&index_lock);
-		}
+		if (write_locked_index(&the_index, &index_lock,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+			die(_("unable to write new_index file"));
 		commit_style = COMMIT_AS_IS;
 		ret = get_index_file();
 		goto out;
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..30c65dfeff 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -651,10 +651,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		return error(_("Unable to write index."));
-	rollback_lock_file(&lock);
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
 		int clean, x;
@@ -691,10 +690,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 				remoteheads->item, reversed, &result);
 		if (clean < 0)
 			exit(128);
-		if (active_cache_changed &&
-		    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+		if (write_locked_index(&the_index, &lock,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die (_("unable to write %s"), get_index_file());
-		rollback_lock_file(&lock);
 		return clean ? 0 : 1;
 	} else {
 		return try_merge_command(strategy, xopts_nr, xopts,
@@ -810,10 +808,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		return error(_("Unable to write index."));
-	rollback_lock_file(&lock);
 
 	write_tree_trivial(&result_tree);
 	printf(_("Wonderful.\n"));
diff --git a/builtin/mv.c b/builtin/mv.c
index cf3684d907..ae013d6d20 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -293,8 +293,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (gitmodules_modified)
 		stage_updated_gitmodules(&the_index);
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		die(_("Unable to write new index file"));
 
 	return 0;
diff --git a/builtin/rm.c b/builtin/rm.c
index 4a2fcca27b..5d59a0aa65 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -385,10 +385,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			stage_updated_gitmodules(&the_index);
 	}
 
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-			die(_("Unable to write new index file"));
-	}
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die(_("Unable to write new index file"));
 
 	return 0;
 }
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 58d1c2d282..f2581904d0 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1182,17 +1182,15 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		report(_("fsmonitor disabled"));
 	}
 
-	if (active_cache_changed || force_write) {
-		if (newfd < 0) {
-			if (refresh_args.flags & REFRESH_QUIET)
-				exit(128);
-			unable_to_lock_die(get_index_file(), lock_error);
-		}
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-			die("Unable to write new index file");
+	if (newfd < 0 && (active_cache_changed || force_write)) {
+		if (refresh_args.flags & REFRESH_QUIET)
+			exit(128);
+		unable_to_lock_die(get_index_file(), lock_error);
 	}
 
-	rollback_lock_file(&lock_file);
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))
+		die("Unable to write new index file");
 
 	return has_errors ? 1 : 0;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 002fb82cf1..410cf4dca3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2203,10 +2203,9 @@ int merge_recursive_generic(struct merge_options *o,
 		return clean;
 	}
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		return err(o, _("Unable to write index."));
-	rollback_lock_file(&lock);
 
 	return clean ? 0 : 1;
 }
diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..f46485eb1e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2543,6 +2543,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	int new_shared_index, ret;
 	struct split_index *si = istate->split_index;
 
+	if ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {
+		if (flags & COMMIT_LOCK)
+			rollback_lock_file(lock);
+		return 0;
+	}
+
 	if (istate->fsmonitor_last_update)
 		fill_fsmonitor_bitmap(istate);
 
diff --git a/rerere.c b/rerere.c
index 1ce440f4bb..fe47ea50be 100644
--- a/rerere.c
+++ b/rerere.c
@@ -719,11 +719,9 @@ static void update_paths(struct string_list *update)
 			item->string);
 	}
 
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
-			die("Unable to write new index file");
-	} else
-		rollback_lock_file(&index_lock);
+	if (write_locked_index(&the_index, &index_lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die("Unable to write new index file");
 }
 
 static void remove_variant(struct rerere_id *id)
diff --git a/sequencer.c b/sequencer.c
index 0e6d04e4ce..1efcf1a2c0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -468,15 +468,14 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		return clean;
 	}
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &index_lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		/*
 		 * TRANSLATORS: %s will be "revert", "cherry-pick" or
 		 * "rebase -i".
 		 */
 		return error(_("%s: Unable to write new index file"),
 			_(action_name(opts)));
-	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
 		append_signoff(msgbuf, 0, 0);
@@ -1191,13 +1190,13 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 			_(action_name(opts)));
 	}
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-	if (the_index.cache_changed && index_fd >= 0) {
-		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
+	if (index_fd >= 0) {
+		if (write_locked_index(&the_index, &index_lock,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED)) {
 			return error(_("git %s: failed to refresh the index"),
 				_(action_name(opts)));
 		}
 	}
-	rollback_lock_file(&index_lock);
 	return 0;
 }
 
-- 
2.16.2.246.ga4ee44448f

Re: [PATCH v2 0/5] roll back locks in various code paths

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> A further upshot of this patch is that `active_cache_changed`, which is
> defined as `the_index.cache_changed`, now only has a few users left.

I am undecided if this is a *good* thing.  In a few codepaths where
we make a speculative update to the on-disk index file, I find that
the explicit mention of active_cache_changed clarifies what is going
on.  Perhaps once my (and other old timers') eyes get used to this
new SKIP_IF_UNCHANGED symbol, it would start serving the same
purpose ;-)

> We could have made the new flag behave the other way round
> ("FORCE_WRITE"), but that could break existing users behind their backs.

Yes, that would break topics in flight that add new calls to
write_locked_index(); they want to write out the index even when
active_cache_changed says there is no update.

Looking at a typical pre/post image of this change like this place:

>  	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>  	refresh_cache(REFRESH_QUIET);
> -	if (active_cache_changed &&
> -	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
> +	if (write_locked_index(&the_index, &lock,
> +			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
>  		return error(_("Unable to write index."));
> -	rollback_lock_file(&lock);

this certainly simplifies what the caller needs to do.

> @@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
>   * With `COMMIT_LOCK`, the lock is always committed or rolled back.
>   * Without it, the lock is closed, but neither committed nor rolled
>   * back.
> + *
> + * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
> + * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
>   */
>  extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);

Good.

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

On 1 March 2018 at 00:20, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> A further upshot of this patch is that `active_cache_changed`, which is
>> defined as `the_index.cache_changed`, now only has a few users left.
>
> I am undecided if this is a *good* thing.  In a few codepaths where
> we make a speculative update to the on-disk index file, I find that
> the explicit mention of active_cache_changed clarifies what is going
> on.  Perhaps once my (and other old timers') eyes get used to this
> new SKIP_IF_UNCHANGED symbol, it would start serving the same
> purpose ;-)

Right, you might say that this trades one symbol for another. What I
meant was, we only have a few "active_cache_changed" and soon (TM) they
might all be "the_index.cache_changed" or "index->cache_changed" and the
macro could be retired. My understanding of the history is limited, but
I was under the impression that this was like a transition macro (albeit
a very old one!).

Martin

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Wed, Feb 28, 2018 at 08:07:53PM +0100, Martin Ågren wrote:

> This is v2 of my series to always release locks. As before, there's a
> conflict with pu, where the correct resolution is to take my version of
> the conflicting hunk.
> 
> The only difference to v1 is in patch 3. I'll follow up with a patch to
> address the confusing pattern which Peff mentioned and which fooled me
> when I prepared v1.

This looks good to me. And I'm glad my rambling helped find something
useful. ;)

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Wed, Feb 28, 2018 at 08:58:09PM +0100, Martin Ågren wrote:

> > I'll follow up with a patch to
> > address the confusing pattern which Peff mentioned and which fooled me
> > when I prepared v1.
> 
> Here is such a patch on top of the others. I'm not particularly proud of the
> name SKIP_IF_UNCHANGED, but don't think it's any worse than, e.g.,
> IGNORE_UNCHANGED or NO_UNCHANGED_WRITE. :-/ Suggestions welcome.
> 
> I think this makes the current users a bit more obvious, and should help future
> users get this optimization right.

IMHO the result is easier to follow. Except for one case:

> -	if (active_cache_changed || force_write) {
> -		if (newfd < 0) {
> -			if (refresh_args.flags & REFRESH_QUIET)
> -				exit(128);
> -			unable_to_lock_die(get_index_file(), lock_error);
> -		}
> -		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> -			die("Unable to write new index file");
> +	if (newfd < 0 && (active_cache_changed || force_write)) {
> +		if (refresh_args.flags & REFRESH_QUIET)
> +			exit(128);
> +		unable_to_lock_die(get_index_file(), lock_error);
>  	}
>  
> -	rollback_lock_file(&lock_file);
> +	if (write_locked_index(&the_index, &lock_file,
> +			       COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))
> +		die("Unable to write new index file");

where I think the logic just ends up repeating itself. I guess you were
anxious to try to get rid of active_cached_changed, but I don't think
keeping it around is really that big a deal (and certainly another trick
is to just say "the_index.cache_changed").

-Peff

Re: [PATCH v2 0/5] roll back locks in various code paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> IMHO the result is easier to follow. Except for one case:
>
>> -	if (active_cache_changed || force_write) {
>> -		if (newfd < 0) {
>> -			if (refresh_args.flags & REFRESH_QUIET)
>> -				exit(128);
>> -			unable_to_lock_die(get_index_file(), lock_error);
>> -		}
>> -		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>> -			die("Unable to write new index file");
>> +	if (newfd < 0 && (active_cache_changed || force_write)) {
>> +		if (refresh_args.flags & REFRESH_QUIET)
>> +			exit(128);
>> +		unable_to_lock_die(get_index_file(), lock_error);
>>  	}
>>  
>> -	rollback_lock_file(&lock_file);
>> +	if (write_locked_index(&the_index, &lock_file,
>> +			       COMMIT_LOCK | (force_write ? 0 : SKIP_IF_UNCHANGED)))
>> +		die("Unable to write new index file");
>
> where I think the logic just ends up repeating itself.

Yup, this one I also had a bit of trouble with.

Re: [PATCH v2 0/5] roll back locks in various code paths

[Martin Ågren]

On 1 March 2018 at 20:24, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> IMHO the result is easier to follow. Except for one case:
>>
>>> [...]
>>
>> where I think the logic just ends up repeating itself.
>
> Yup, this one I also had a bit of trouble with.

Thanks for your feedback, both of you. It's much appreciated.

After thinking about it, I tend to agree. That rewrite loses an
indentation level and makes it a bit clearer that we have two steps,
"maybe bail" and "write". But at the cost of duplicating logic -- after
all, those two steps are very closely related, so there's no need to
artificially separate them.

Here it is again, without that hunk, and without the commit message
claim that it'd be a good thing to have just a few uses of
"active_cache_changed" remaining.

This could go as a patch 6/5 onto ma/roll-back-lockfiles. I keep
debating myself whether this would do better earlier in such a six-pack,
where it would be "also, this releases a lock where we used to forget
to". Right now, there is overlap between patch 3/5 (which adds a line)
and this patch (which removes it). This patch doesn't entirely negate
the need for patch 3/5 though..

Martin

-- >8 --
Subject: write_locked_index(): add flag to avoid writing unchanged index

We have several callers like

	if (active_cache_changed && write_locked_index(...))
		handle_error();
	rollback_lock_file(...);

where the final rollback is needed because "!active_cache_changed"
shortcuts the if-expression. There are also a few variants of this,
including some if-else constructs that make it more clear when the
explicit rollback is really needed.

Teach `write_locked_index()` to take a new flag SKIP_IF_UNCHANGED and
simplify the callers. Leave the most complicated of the callers (in
builtin/update-index.c) unchanged. Rewriting it to use this new flag
would end up duplicating logic.

We could have made the new flag behave the other way round
("FORCE_WRITE"), but that could break existing users behind their backs.
Let's take the more conservative approach. We can still migrate existing
callers to use our new flag. Later we might even be able to flip the
default, possibly without entirely ignoring the risk to in-flight or
out-of-tree topics.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 cache.h           |  4 ++++
 builtin/add.c     |  7 +++----
 builtin/commit.c  | 10 +++-------
 builtin/merge.c   | 15 ++++++---------
 builtin/mv.c      |  4 ++--
 builtin/rm.c      |  7 +++----
 merge-recursive.c |  5 ++---
 read-cache.c      |  6 ++++++
 rerere.c          |  8 +++-----
 sequencer.c       | 11 +++++------
 10 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..905d8eb6cc 100644
--- a/cache.h
+++ b/cache.h
@@ -622,6 +622,7 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK		(1 << 0)
+#define SKIP_IF_UNCHANGED	(1 << 1)
 
 /*
  * Write the index while holding an already-taken lock. Close the lock,
@@ -638,6 +639,9 @@ extern int read_index_unmerged(struct index_state *);
  * With `COMMIT_LOCK`, the lock is always committed or rolled back.
  * Without it, the lock is closed, but neither committed nor rolled
  * back.
+ *
+ * If `SKIP_IF_UNCHANGED` is given and the index is unchanged, nothing
+ * is written (and the lock is rolled back if `COMMIT_LOCK` is given).
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags);
 
diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e28..9e5a80cc6f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,10 +534,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	unplug_bulk_checkin();
 
 finish:
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-			die(_("Unable to write new index file"));
-	}
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die(_("Unable to write new index file"));
 
 	UNLEAK(pathspec);
 	UNLEAK(dir);
diff --git a/builtin/commit.c b/builtin/commit.c
index 8a87701414..8d9b4081c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -423,13 +423,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (active_cache_changed
 		    || !cache_tree_fully_valid(active_cache_tree))
 			update_main_cache_tree(WRITE_TREE_SILENT);
-		if (active_cache_changed) {
-			if (write_locked_index(&the_index, &index_lock,
-					       COMMIT_LOCK))
-				die(_("unable to write new_index file"));
-		} else {
-			rollback_lock_file(&index_lock);
-		}
+		if (write_locked_index(&the_index, &index_lock,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+			die(_("unable to write new_index file"));
 		commit_style = COMMIT_AS_IS;
 		ret = get_index_file();
 		goto out;
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..30c65dfeff 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -651,10 +651,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		return error(_("Unable to write index."));
-	rollback_lock_file(&lock);
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
 		int clean, x;
@@ -691,10 +690,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 				remoteheads->item, reversed, &result);
 		if (clean < 0)
 			exit(128);
-		if (active_cache_changed &&
-		    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+		if (write_locked_index(&the_index, &lock,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die (_("unable to write %s"), get_index_file());
-		rollback_lock_file(&lock);
 		return clean ? 0 : 1;
 	} else {
 		return try_merge_command(strategy, xopts_nr, xopts,
@@ -810,10 +808,9 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 
 	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
 	refresh_cache(REFRESH_QUIET);
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		return error(_("Unable to write index."));
-	rollback_lock_file(&lock);
 
 	write_tree_trivial(&result_tree);
 	printf(_("Wonderful.\n"));
diff --git a/builtin/mv.c b/builtin/mv.c
index cf3684d907..ae013d6d20 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -293,8 +293,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (gitmodules_modified)
 		stage_updated_gitmodules(&the_index);
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		die(_("Unable to write new index file"));
 
 	return 0;
diff --git a/builtin/rm.c b/builtin/rm.c
index 4a2fcca27b..5d59a0aa65 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -385,10 +385,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 			stage_updated_gitmodules(&the_index);
 	}
 
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-			die(_("Unable to write new index file"));
-	}
+	if (write_locked_index(&the_index, &lock_file,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die(_("Unable to write new index file"));
 
 	return 0;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 002fb82cf1..410cf4dca3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2203,10 +2203,9 @@ int merge_recursive_generic(struct merge_options *o,
 		return clean;
 	}
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		return err(o, _("Unable to write index."));
-	rollback_lock_file(&lock);
 
 	return clean ? 0 : 1;
 }
diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..f46485eb1e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2543,6 +2543,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	int new_shared_index, ret;
 	struct split_index *si = istate->split_index;
 
+	if ((flags & SKIP_IF_UNCHANGED) && !istate->cache_changed) {
+		if (flags & COMMIT_LOCK)
+			rollback_lock_file(lock);
+		return 0;
+	}
+
 	if (istate->fsmonitor_last_update)
 		fill_fsmonitor_bitmap(istate);
 
diff --git a/rerere.c b/rerere.c
index 1ce440f4bb..fe47ea50be 100644
--- a/rerere.c
+++ b/rerere.c
@@ -719,11 +719,9 @@ static void update_paths(struct string_list *update)
 			item->string);
 	}
 
-	if (active_cache_changed) {
-		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
-			die("Unable to write new index file");
-	} else
-		rollback_lock_file(&index_lock);
+	if (write_locked_index(&the_index, &index_lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
+		die("Unable to write new index file");
 }
 
 static void remove_variant(struct rerere_id *id)
diff --git a/sequencer.c b/sequencer.c
index 0e6d04e4ce..1efcf1a2c0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -468,15 +468,14 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 		return clean;
 	}
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+	if (write_locked_index(&the_index, &index_lock,
+			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 		/*
 		 * TRANSLATORS: %s will be "revert", "cherry-pick" or
 		 * "rebase -i".
 		 */
 		return error(_("%s: Unable to write new index file"),
 			_(action_name(opts)));
-	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
 		append_signoff(msgbuf, 0, 0);
@@ -1191,13 +1190,13 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 			_(action_name(opts)));
 	}
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-	if (the_index.cache_changed && index_fd >= 0) {
-		if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK)) {
+	if (index_fd >= 0) {
+		if (write_locked_index(&the_index, &index_lock,
+				       COMMIT_LOCK | SKIP_IF_UNCHANGED)) {
 			return error(_("git %s: failed to refresh the index"),
 				_(action_name(opts)));
 		}
 	}
-	rollback_lock_file(&index_lock);
 	return 0;
 }
 
-- 
2.16.2.246.ga4ee44448f

Re: [PATCH v2 0/5] roll back locks in various code paths

[Jeff King]

On Thu, Mar 01, 2018 at 09:40:20PM +0100, Martin Ågren wrote:

> After thinking about it, I tend to agree. That rewrite loses an
> indentation level and makes it a bit clearer that we have two steps,
> "maybe bail" and "write". But at the cost of duplicating logic -- after
> all, those two steps are very closely related, so there's no need to
> artificially separate them.
> 
> Here it is again, without that hunk, and without the commit message
> claim that it'd be a good thing to have just a few uses of
> "active_cache_changed" remaining.

Thanks, this version looks good to me. The name SKIP_IF_UNCHANGED is
generic and may result in clashes down the road. But then so is the name
COMMIT_LOCK. I'm OK to punt on that until we do see such a collision, at
which point we may want to provide a consistent namespace for these
flags.

-Peff