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

[Jeff King <peff@peff.net>]

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