Re: [PATCH v2] commit: skip discarding the index if there is no pre-commit hook

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2017, 18:05 UTC
Message-ID: <xmqq60doso3x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170815045313.njpq5wf7iuredhoe@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Tue, Aug 15, 2017 at 04:23:50AM +0000, Kevin Willford wrote:
>
>> > This read_cache_from() should be a noop, right, because it immediately
>> > sees istate->initialized is set? So it shouldn't matter that it is not
>> > in the conditional with discard_cache(). Though if its only purpose is
>> > to re-read the just-discarded contents, perhaps it makes sense to put it
>> > there for readability.
>> 
>> I thought about that and didn't know if there were cases when this would be called
>> and the cache has not been loaded.  It didn't look like it since it is only called from 
>> cmd_commit and prepare_index is called before it.  Also if in the future this call would
>> be made when it had not read the index yet so thought it was safest just to leave
>> this as always being called since it is basically a noop if the istate->initialized is set.
>
> Yeah, I agree it might be safer as you have it. And hopefully the
> comment above the discard would point anybody in the right direction.

I agree that it would not hurt to have it outside the conditional, because read_cache_from() is a no-op when it is already populated. I however do not think it is sensible to call prepare_to_commit() without having populated the in-core index---in the function, nobody conditionally reads the in-core index, expecting that the caller might not have prepared the in-core index, when we need to do the wt-status thing, for example.

>> Also based on this commit
>> https://github.com/git/git/commit/2888605c649ccd423232161186d72c0e6c458a48
>> it looked like the discard_cache was added independent of the read_cache_from call,
>> which made me think that the two were not tied together.

That one comes from the first commit of the C reimplementation, f5bbc322 ("Port git commit to C.", 2007-11-08).

If I am reading the old code correctly, read_cache_from() is called for entirely different reasons. Back in the old code, when doing "commit --patch", prepare_index() called interactive_add(), which would return to us after updating the index file in the filesystem. The caller of prepare_index(), which was cmd_commit(), needed to read that off of the filesystem hence the call is there.

When not doing "--patch", prepare_index() called read_cache(), so the read_cache_from() there was a no-op.

2888605c ("builtin-commit: fix partial-commit support", 2007-11-18) was to fix the mistake of not re-reading the index from the file at that point for non "--patch" cases. Having read_cache_from() that is most of the time no-op was not sufficient and needed additional discard_cache() there.