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

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

Jeff King <peff@peff.net> writes:

> 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.