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

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 <redacted>
---
 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;
+		}
+	}
 
 	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

-- 
1.8.4.2

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.

[...]

[...]

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

[...]

 	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.

[...]

+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 and hope that helps,
Jonathan

Am 09.12.2013 16:16, schrieb Jonathan Nieder:

Thomas Gummerer wrote:

quoted

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.

Hmm, but this will disable the submodule specific ignore configuration
options defined in the .gitmodules file, no? (E.g. when diffing two
directories containing submodules)

quoted

                       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.

But I agree that dying with "index file corrupt" is a bit strange when
calling diff with --no-index. Wouldn't adding a "gently" option (which
could then warn instead of dying) to gitmodules_config() be a better
solution here?

Jens Lehmann wrote:

Am 09.12.2013 16:16, schrieb Jonathan Nieder:

quoted

Thomas Gummerer wrote:

quoted

quoted

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.

Hmm, but this will disable the submodule specific ignore configuration
options defined in the .gitmodules file, no? (E.g. when diffing two
directories containing submodules)

Yes.  That seems like a good behavior.

"git diff --no-index" was invented as just a fancy version of 'diff
-uR", without any awareness of the current git repository.  That means
that at least in principle, .gitmodules and .gitignore should not
affect it.

[...]

                              Wouldn't adding a "gently" option (which
could then warn instead of dying) to gitmodules_config() be a better
solution here?

I don't think so.

Thanks and hope that helps,
Jonathan

Jonathan Nieder [off-list ref] writes:

Thomas Gummerer wrote:

quoted

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.

quoted

                       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.

[...]

quoted

--- a/builtin/diff.c
+++ b/builtin/diff.c

[...]

quoted

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

[...]

quoted

 	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.

[...]

quoted

+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

Thomas Gummerer wrote:

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

Makes a lot of sense.  Thanks for explaining.

Regards,
Jonathan

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

Thanks to Jonathan and Jens for comments on the previous round.
Changes:
 - Don't all setup_git_directory_gently when --no-index is given
 - Add performance test
 - Commit message improvements

 builtin/diff.c                | 16 +++++++++++++---
 t/perf/p4001-diff-no-index.sh | 17 +++++++++++++++++
 t/t4053-diff-no-index.sh      |  6 ++++++
 3 files changed, 36 insertions(+), 3 deletions(-)
 create mode 100755 t/perf/p4001-diff-no-index.sh

diff --git a/builtin/diff.c b/builtin/diff.c
index adb93a9..5f09a0b 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,19 @@ 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;
+		}
+	}
 
-	prefix = setup_git_directory_gently(&nongit);
-	gitmodules_config();
+	if (!no_index) {
+		prefix = setup_git_directory_gently(&nongit);
+		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..81c7aa0
--- /dev/null
+++ b/t/perf/p4001-diff-no-index.sh

@@ -0,0 +1,17 @@
+#!/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_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..d3dbf6b 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 &&
+	git diff --no-index a ../non/git/a &&
+'
+
 test_done

-- 
1.8.5.4.g8639e57

On 2013-12-09 21.40, Thomas Gummerer wrote:

 
+test_expect_success 'git diff --no-index with broken index' '
+	cd repo &&
+	echo broken >.git/index &&
+	git diff --no-index a ../non/git/a &&

                                           ^^
I'm confused: Does this work with the trailing && ?


(and when we use
"cd repo"
it could be good to use a sub-shell (even when this is the last test case)

On Mon, Dec 9, 2013 at 3:40 PM, Thomas Gummerer [off-list ref] wrote:

Stray && on the last line of the test.

Also, don't you want to do the 'cd' and subsequent commands inside a
subshell so that tests added after this one won't have to worry about
cd'ing back to the top-level?

+
 test_done
--
1.8.5.4.g8639e57

Eric Sunshine [off-list ref] writes:

On Mon, Dec 9, 2013 at 3:40 PM, Thomas Gummerer [off-list ref] wrote:

quoted

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

diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 979e983..d3dbf6b 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 &&
+       git diff --no-index a ../non/git/a &&
+'

Stray && on the last line of the test.

Also, don't you want to do the 'cd' and subsequent commands inside a
subshell so that tests added after this one won't have to worry about
cd'ing back to the top-level?

Thanks both to you and Torsten for catching both issues, I'll fix them
in a re-roll.

quoted

+
 test_done
--
1.8.5.4.g8639e57

--
Thomas