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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 3, 2017, 23:38 UTC
Message-ID: <xmqqh94adev3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <DA0A42D68346B1469147552440A645039A9C9988@ORSMSX115.amr.corp.intel.com>

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

Show 8 quoted lines

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

Show 20 quoted lines

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

Show 11 quoted lines

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