All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: git@vger.kernel.org
Cc: "Jonathan Nieder" <jrnieder@gmail.com>,
	"Jens Lehmann" <Jens.Lehmann@web.de>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Torsten Bögershausen" <tboegi@web.de>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Thomas Gummerer" <t.gummerer@gmail.com>
Subject: [PATCH v4 2/2] diff: don't read index when --no-index is given
Date: Wed, 11 Dec 2013 10:58:43 +0100	[thread overview]
Message-ID: <1386755923-22132-2-git-send-email-t.gummerer@gmail.com> (raw)
In-Reply-To: <1386755923-22132-1-git-send-email-t.gummerer@gmail.com>

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

  reply	other threads:[~2013-12-11  9:59 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-09 12:05 [PATCH] diff: don't read index when --no-index is given Thomas Gummerer
2013-12-09 15:16 ` Jonathan Nieder
2013-12-09 17:27   ` Jens Lehmann
2013-12-09 19:06     ` Jonathan Nieder
2013-12-09 19:14   ` Thomas Gummerer
2013-12-09 19:20     ` Jonathan Nieder
2013-12-09 20:40       ` [PATCH v2] " Thomas Gummerer
2013-12-09 20:55         ` Torsten Bögershausen
2013-12-09 20:57         ` Eric Sunshine
2013-12-09 21:17           ` Thomas Gummerer
2013-12-09 20:30 ` [PATCH] " Junio C Hamano
2013-12-09 21:13   ` Thomas Gummerer
2013-12-10 17:52   ` [PATCH v3 1/2] diff: move no-index detection to builtin/diff.c Thomas Gummerer
2013-12-10 17:52     ` [PATCH v3 2/2] diff: don't read index when --no-index is given Thomas Gummerer
2013-12-10 18:18       ` Jonathan Nieder
2013-12-10 18:55         ` Thomas Gummerer
2013-12-10 18:16     ` [PATCH v3 1/2] diff: move no-index detection to builtin/diff.c Jonathan Nieder
2013-12-10 18:46       ` Thomas Gummerer
2013-12-11  9:58       ` [PATCH v4 " Thomas Gummerer
2013-12-11  9:58         ` Thomas Gummerer [this message]
2013-12-12 20:25           ` [PATCH v4 2/2] diff: don't read index when --no-index is given Junio C Hamano
2013-12-14  0:44           ` Jonathan Nieder
2013-12-14  0:43         ` [PATCH v4 1/2] diff: move no-index detection to builtin/diff.c Jonathan Nieder
2013-12-14 10:42           ` Thomas Gummerer
2013-12-16 17:48             ` Junio C Hamano
2013-12-16 19:23               ` [PATCH 1/2] diff: add test for --no-index executed outside repo Thomas Gummerer
2013-12-16 19:23                 ` [PATCH 2/2] diff: avoid some nesting Thomas Gummerer
2013-12-16 19:42                 ` [PATCH 1/2] diff: add test for --no-index executed outside repo Junio C Hamano
2013-12-14 13:07           ` [PATCH v5 1/2] diff: move no-index detection to builtin/diff.c Thomas Gummerer
2013-12-14 13:07             ` [PATCH v5 2/2] diff: don't read index when --no-index is given Thomas Gummerer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1386755923-22132-2-git-send-email-t.gummerer@gmail.com \
    --to=t.gummerer@gmail.com \
    --cc=Jens.Lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=sunshine@sunshineco.com \
    --cc=tboegi@web.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.