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

> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
>> git diff --no-index ... currently reads the index, during setup, when
>> calling gitmodules_config().  In the usual case this gives us some
>> performance drawbacks, but it's especially annoying if there is a broken
>> index file.
>>
>> Avoid calling the unnecessary gitmodules_config() when the --no-index
>> option is given.  Also add a test to guard against similar breakages in the future.
>>
>> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
>> ---
>>  builtin/diff.c           | 13 +++++++++++--
>>  t/t4053-diff-no-index.sh |  6 ++++++
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/diff.c b/builtin/diff.c
>> index adb93a9..47c0833 100644
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -257,7 +257,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>  	int blobs = 0, paths = 0;
>>  	const char *path = NULL;
>>  	struct blobinfo blob[2];
>> -	int nongit;
>> +	int nongit, no_index = 0;
>>  	int result = 0;
>>
>>  	/*
>> @@ -282,9 +282,18 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>  	 *
>>  	 * Other cases are errors.
>>  	 */
>> +	for (i = 1; i < argc; i++) {
>> +		if (!strcmp(argv[i], "--"))
>> +			break;
>> +		if (!strcmp(argv[i], "--no-index")) {
>> +			no_index = 1;
>> +			break;
>> +		}
>> +	}
>
> This seems to duplicate only half the logic at the beginning of
> diff_no_index(), right?  E.g., running "git diff /var/tmp/[12]"
> inside a working tree that is controlled by a Git repository when
> /var/tmp/ is outside, we do want to behave as if the command line
> were "git diff --no-index /var/tmp/[12]", but this half duplication
> makes these two behave differently, no?

> I think the issue you are trying to address is worth tackling, but I
> wonder if a bit of preparatory refactoring is necessary to avoid the
> partial duplication.
>
>>  	prefix = setup_git_directory_gently(&nongit);
>> -	gitmodules_config();
>> +	if (!no_index)
>> +		gitmodules_config();
>>  	git_config(git_diff_ui_config, NULL);
>>
>>  	init_revisions(&rev, prefix);
>> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
>> index 979e983..a24ae4d 100755
>> --- a/t/t4053-diff-no-index.sh
>> +++ b/t/t4053-diff-no-index.sh
>> @@ -29,4 +29,10 @@ 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 &&
>> +	test_expect_code 0 git diff --no-index a ../non/git/a
>> +'
>> +
>>  test_done