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

[Junio C Hamano]

Jeff King  writes:

> On Tue, Aug 15, 2017 at 04:23:50AM +, 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.

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

[Jeff King]

On Tue, Aug 15, 2017 at 04:23:50AM +, 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.

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

Yeah, I tried to dig in the history and figure it out, but it was not
nearly as clear as I would have liked. :)

-Peff

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

[Kevin Willford]

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

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

[Jeff King]

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