Re: [PATCH] commit: Optimize number of lstat() calls

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

"Gumbel, Matthew K" <matthew.k.gumbel@intel.com> writes:

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

Did you determine that "pre-commit" hook is the only thing that
needs the special case and if so how?  With that stated in the log
message, we would feel safe to take this patch.

> 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"));

I wonder why this patch is full of 0xa0 bytes instead of spaces.
Somebody between your editor and mailing list is munging your
patches?

> @@ -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"));

I'd prefer if you introduced a small helper function, name it
can_cheat_on_refreshing() or something, and encapsulate the call to
find_hook("pre-commit") inside it---if there are other corner cases
you didn't think of, or if in the future more hooks are added that
need their incoming index file already refreshed, we do not want to
update these two places---rather we'd want to limit the need for
updating to the single helper function.