Re: [PATCH v4 2/2] diff: don't read index when --no-index is given

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

Thomas Gummerer <t.gummerer@gmail.com> writes:

> git diff --no-index ... currently reads the index, during setup, when
> calling gitmodules_config().  This results in worse performance when the
> index is not actually needed.  This patch avoids calling
> gitmodules_config() when the --no-index option is given.  The times for
> executing "git diff --no-index" in the WebKit repository are improved as
> follows:
>
> Test                      HEAD~3            HEAD
> ------------------------------------------------------------------
> 4001.1: diff --no-index   0.24(0.15+0.09)   0.01(0.00+0.00) -95.8%
>
> An additional improvement of this patch is that "git diff --no-index" no
> longer breaks when the index file is corrupt, which makes it possible to
> use it for investigating the broken repository.
>
> To improve the possible usage as investigation tool for broken
> repositories, setup_git_directory_gently() is also not called when the
> --no-index option is given.
>
> Also add a test to guard against future breakages, and a performance
> test to show the improvements.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---

Looks reasonable.

Thanks.  Will queue.

>  builtin/diff.c                |  7 +++++--
>  t/perf/p4001-diff-no-index.sh | 22 ++++++++++++++++++++++
>  t/t4053-diff-no-index.sh      | 15 +++++++++++++++
>  3 files changed, 42 insertions(+), 2 deletions(-)
>  create mode 100755 t/perf/p4001-diff-no-index.sh
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index da69e4a..ea1dd65 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -298,7 +298,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  			break;
>  	}
>  
> -	prefix = setup_git_directory_gently(&nongit);
> +	if (!no_index)
> +		prefix = setup_git_directory_gently(&nongit);
> +
>  	/*
>  	 * Treat git diff with at least one path outside of the
>  	 * repo the same as if the command would have been executed
> @@ -311,7 +313,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  			!path_inside_repo(prefix, argv[i + 1]))))
>  		no_index = DIFF_NO_INDEX_IMPLICIT;
>  
> -	gitmodules_config();
> +	if (!no_index)
> +		gitmodules_config();
>  	git_config(git_diff_ui_config, NULL);
>  
>  	init_revisions(&rev, prefix);
> diff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh
> new file mode 100755
> index 0000000..683be69
> --- /dev/null
> +++ b/t/perf/p4001-diff-no-index.sh
> @@ -0,0 +1,22 @@
> +#!/bin/sh
> +
> +test_description="Test diff --no-index performance"
> +
> +. ./perf-lib.sh
> +
> +test_perf_large_repo
> +test_checkout_worktree
> +
> +file1=$(git ls-files | tail -n 2 | head -1)
> +file2=$(git ls-files | tail -n 1 | head -1)
> +
> +test_expect_success "empty files, so they take no time to diff" "
> +	echo >$file1 &&
> +	echo >$file2
> +"
> +
> +test_perf "diff --no-index" "
> +	git diff --no-index $file1 $file2 >/dev/null
> +"
> +
> +test_done
> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
> index 979e983..077c775 100755
> --- a/t/t4053-diff-no-index.sh
> +++ b/t/t4053-diff-no-index.sh
> @@ -29,4 +29,19 @@ test_expect_success 'git diff --no-index relative path outside repo' '
>  	)
>  '
>  
> +test_expect_success 'git diff --no-index with broken index' '
> +	(
> +		cd repo &&
> +		echo broken >.git/index &&
> +		git diff --no-index a ../non/git/a
> +	)
> +'
> +
> +test_expect_success 'git diff outside repo with broken index' '
> +	(
> +		cd repo &&
> +		git diff ../non/git/a ../non/git/b
> +	)
> +'
> +
>  test_done