Re: [PATCH] make commit --interactive lock index

[Junio C Hamano <gitster@pobox.com>]

Paolo Bonzini <bonzini@gnu.org> writes:

> I noticed that the way "git commit --interactive" sets up the commit
> is different from the way a normal "git commit" does it.  Commit
> 2888605c changed one, but not the other.  This makes the behavior
> equivalent in the two cases.

Sorry, you need to defend this change much better than that.

I fail to see why it is a bad thing to have differences among the
codepaths that do different things.  The quoted commit 2888605
(builtin-commit: fix partial-commit support, 2007-11-18) was _not_ about
"doing it the same way".  It was simply "commit has three cases, AS_IS,
NORMAL and PARTIAL; the codepath that implements PARTIAL case is not done
right, so fix it to behave exactly the same as the old scripted version".

When interactive_add() returns, the index has already been updated, and
there is no reason to take the index lock, refresh the index, nor anything
that the normal "as-is" commit codepath needs to do (let alone the
alternate index dance forced upon the partial commit codepath).

If your change were so that "git commit --interactive" reverts the index
when one of the hooks exited non-zero just like COMMIT_NORMAL case (as
opposed to the current code which does not revert the index), I would
understand the need to change what's inside "if (interactive)" block.  But
that is not what the patch is about.  Changing the codepath so that it
does not return from that block made me follow unnecessary and unrelated
codepath to convince myself that this patch is mostly a no-op, except that
you are doing an extra refresh_cache() on the index that interactive_add()
has already refreshed before giving the control back to you.

Can you explain why this is an improvement?