patchcommit: Optimize number of lstat() calls

> When making a --only commit, original behavior was to do a full cache
> update for the purposes of giving the pre-commit hook an up-to-date set 
> of stat data. That would result in long runtime for git-commit in a big 
> repo on NFS (>60s for a 54k-file repo).
>
> With this change, when doing a --only commit and no pre-commit hook is
> present, the cache update is skipped since it is known a priori which
> files are to be committed.

> Signed-off-by: Matthew K. Gumbel <matthew.k.gumbel@intel.com>
> ---
> builtin/commit.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2de5f6c..1df3d71 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -470,7 +470,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>
>     hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
>     add_remove_files(&partial);
> -   refresh_cache(REFRESH_QUIET);
> +    if (find_hook("pre-commit")) {
> +        refresh_cache(REFRESH_QUIET);
> +    }
>     update_main_cache_tree(WRITE_TREE_SILENT);
>     if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
>         die(_("unable to write new_index file"));

> @@ -482,7 +484,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>
>     create_base_index(current_head);
>     add_remove_files(&partial);
> -   refresh_cache(REFRESH_QUIET);
> +    if (find_hook("pre-commit")) {
> +        refresh_cache(REFRESH_QUIET);
> +    }
>
>     if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK))
>         die(_("unable to write temporary index file"));

> Aside from whitespace breakage, I think we should never skip the
> refreshing of the real index that is left after "git commit"
> finishes.
> 
> Between these two calls to refresh_cache(), the one that writes out
> a temporary index that contains the contents of HEAD plus the
> contents of the working tree for the specified paths may be fine
> without refreshing, unless somebody else (like the pre-commit hook)
> looks at it.  But the other one refreshes the real index file that
> will be used after "git commit" returns the control.  Users and
> scripts that run "git commit" inside expect that the entries in the
> resulting index are refreshed after "git commit" returns, and I do
> not think of a safe way to optimizing it out; unlike the other one,
> to which we can say "as long as there is no pre-commit hook, nobody
> will look at it after we are done", there does not an easy-to-check
> set of conditions that we can use to decide when it is safe to skip
> refreshing.
> 
> Besides, leaving the main index not refreshed would mean the user
> has to pay the refreshing cost when s/he runs other commands "git
> diff", "git status", etc. after "git commit" for the first time;
> so...

>> Besides, leaving the main index not refreshed would mean the user
>> has to pay the refreshing cost when s/he runs other commands "git
>> diff", "git status", etc. after "git commit" for the first time;
>> so...
>
> I am not sure... when you run `git diff` and `git status`, the index is
> refreshed *anyway*, so with the patch under discussion we would save one
> round of lstat() calls, for the same effect.

> I could imagine that there is a third option we should consider, too: only
> lstat() and update the paths that match the pathspec(s) provided on the
> command line (this is the semantic meaning of the --only option, after
> all: "I am only interested in these paths as far as this commit is
> concerned"). What do you think?

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I could imagine that there is a third option we should consider, too: only
> > lstat() and update the paths that match the pathspec(s) provided on the
> > command line (this is the semantic meaning of the --only option, after
> > all: "I am only interested in these paths as far as this commit is
> > concerned"). What do you think?
> 
> I wondered that myself when I read the first message from Matthew
> and noticed that we always refresh the entire index.  But if it is
> OK to leave the entire index un-refreshed, that would even be
> simpler ;-)