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

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2008, 05:29 UTC
Message-ID: <7vr6bkmkbm.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <E1K1eXH-00063c-Bt@fencepost.gnu.org>

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?