On 27 February 2018 at 22:44, Jeff King <peff@xxxxxxxx> 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
