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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Dec 9, 2013, 19:14 UTC
Message-ID: <87siu1n8g6.fsf@gmail.com>
In-Reply-To: <20131209151609.GA14841@google.com>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 18 quoted lines

> Thomas Gummerer wrote:
>
>> git diff --no-index ... currently reads the index, during setup, when
>> calling gitmodules_config().  In the usual case this gives us some
>> performance drawbacks,
>
> Makes sense.
>
>>                        but it's especially annoying if there is a broken
>> index file.
>
> Is this really a normal case?  It makes sense that as a side-effect it
> is easier to use "git diff --no-index" as a general-purpose tool while
> investigating a broken repo, but I would have thought that quickly
> learning a repo is broken is a good thing in any case.
>
> A little more information about the context where this came up would
> be helpful, I guess.

It came up while I was working on index-v5, where I had to investigate quite a few repositories where the index was broken, especially when I was changing the index format slightly. For example I would take one version, use test-dump-cache-tree to dump the cache tree to a file, change the format slightly, use test-dump-cache-tree again, and check the difference with "git diff --no-index".

This might not be a very common use case, but maybe the patch might help someone else too. (In addition to the performance improvements)

I'm not sure how much diff --no-index is used normally, but when the index is broken that would be detected relatively soon anyway. I'm not so worried about one more command working when it's broken.

Show 19 quoted lines

> [...]
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
> [...]
>> @@ -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;
>> +		}
>
> setup_revisions() uses the same logic that doesn't handle options that
> take arguments in their "unstuck" form (e.g., "--word-diff-regex --"),
> so this is probably not a regression, though I haven't checked. :)

The same logic is used in diff_no_index(), so I think it should be fine.

Show 14 quoted lines

> [...]
>>  	prefix = setup_git_directory_gently(&nongit);
>> -	gitmodules_config();
>> +	if (!no_index)
>> +		gitmodules_config();
>
> Perhaps we can improve performance and behavior by skipping the
> setup_git_directory_gently() call, too?
>
> That would help with the repairing-broken-repository case by
> working even if .git/config or .git/HEAD is broken, and I think
> it is more intuitive that the repository-local configuration is
> ignored by "git diff --no-index".  It would also help with
> performance by avoiding some filesystem access.

Yes, I think that would make sense, thanks. I tested it, and it didn't change the performance, but it's still a good change for the broken repository case. Will change in the re-roll and add a test for that.

Show 9 quoted lines

> [...]
>> +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
>
> Clever.  I wouldn't use "test_expect_code 0", since that's
> already implied by including the "git diff --no-index" call
> in the && chain.

Thanks, will change. Thanks a lot for your review. Will send a re-roll soon.

> Thanks and hope that helps,
> Jonathan

-- Thomas