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
