Re: [PATCH v2] diff: don't read index when --no-index is given
Eric Sunshine writes: > On Mon, Dec 9, 2013 at 3:40 PM, Thomas Gummerer wrote: >> 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~3HEAD >> -- >> 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 >> --- >> 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. >> + >> test_done >> -- >> 1.8.5.4.g8639e57 -- Thomas -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff: don't read index when --no-index is given
On Mon, Dec 9, 2013 at 3:40 PM, Thomas Gummerer wrote: > 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~3HEAD > -- > 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 > --- > 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? > + > test_done > -- > 1.8.5.4.g8639e57 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff: don't read index when --no-index is given
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) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] diff: don't read index when --no-index is given
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~3HEAD -- 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 --- 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 000..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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html