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

> 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.