Re: [PATCH 2/5] sequencer: always roll back lock in `do_recursive_merge()`
flat view
From: Jeff King <hidden>
Date: 2018-02-27 21:44:37
On Tue, Feb 27, 2018 at 10:30:10PM +0100, Martin Ågren wrote:
quoted hunk ↗ jump to hunk
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