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

[Kevin Willford <kewillf@microsoft.com>]

> On Mon, Aug 14, 2017 at 03:54:25PM -0600, Kevin Willford wrote:
> 
> > If there is not a pre-commit hook, there is no reason to discard
> > the index and reread it.
> >
> > This change checks to presence of a pre-commit hook and then only
> > discards the index if there was one.
> >
> > Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> > ---
> >  builtin/commit.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> Thanks, this looks nice and simple.
> 
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index e7a2cb6285..ab71b93518 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -940,12 +940,15 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> >  		return 0;
> >  	}
> >
> > -	/*
> > -	 * Re-read the index as pre-commit hook could have updated it,
> > -	 * and write it out as a tree.  We must do this before we invoke
> > -	 * the editor and after we invoke run_status above.
> > -	 */
> > -	discard_cache();
> > +	if (!no_verify && find_hook("pre-commit")) {
> > +		/*
> > +		 * Re-read the index as pre-commit hook could have updated it,
> > +		 * and write it out as a tree.  We must do this before we invoke
> > +		 * the editor and after we invoke run_status above.
> > +		 */
> > +		discard_cache();
> > +	}
> > +
> >  	read_cache_from(index_file);
> 
> 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.
> 
> -Peff

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.

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.

Kevin