From 470faf96544c1844ad775c695dfa370fe0ef3756 Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Wed, 11 Dec 2013 10:58:42 +0100 Subject: diff: move no-index detection to builtin/diff.c Currently the --no-index option is parsed in diff_no_index(). Move the detection if a no-index diff should be executed to builtin/diff.c, where we can use it for executing diff_no_index() conditionally. This will also allow us to execute other operations conditionally, which will be done in the next patch. There are no functional changes. Helped-by: Jonathan Nieder Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- builtin/diff.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- diff-no-index.c | 44 +------------------------------------------- diff.h | 2 +- 3 files changed, 51 insertions(+), 47 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index adb93a9efa..da69e4a3c8 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -16,6 +16,9 @@ #include "submodule.h" #include "sha1-array.h" +#define DIFF_NO_INDEX_EXPLICIT 1 +#define DIFF_NO_INDEX_IMPLICIT 2 + struct blobinfo { unsigned char sha1[20]; const char *name; @@ -257,7 +260,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 = 0, no_index = 0; int result = 0; /* @@ -283,14 +286,57 @@ int cmd_diff(int argc, const char **argv, const char *prefix) * Other cases are errors. */ + /* Were we asked to do --no-index explicitly? */ + for (i = 1; i < argc; i++) { + if (!strcmp(argv[i], "--")) { + i++; + break; + } + if (!strcmp(argv[i], "--no-index")) + no_index = DIFF_NO_INDEX_EXPLICIT; + if (argv[i][0] != '-') + break; + } + 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 + * outside of a git repository. In this case it behaves + * the same way as "git diff --no-index ", which acts + * as a colourful "diff" replacement. + */ + if (nongit || ((argc == i + 2) && + (!path_inside_repo(prefix, argv[i]) || + !path_inside_repo(prefix, argv[i + 1])))) + no_index = DIFF_NO_INDEX_IMPLICIT; + gitmodules_config(); git_config(git_diff_ui_config, NULL); init_revisions(&rev, prefix); - /* If this is a no-index diff, just run it and exit there. */ - diff_no_index(&rev, argc, argv, nongit, prefix); + if (no_index) { + if (argc != i + 2) { + if (no_index == DIFF_NO_INDEX_IMPLICIT) { + /* + * There was no --no-index and there were not two + * paths. It is possible that the user intended + * to do an inside-repository operation. + */ + fprintf(stderr, "Not a git repository\n"); + fprintf(stderr, + "To compare two paths outside a working tree:\n"); + } + /* Give the usage message for non-repository usage and exit. */ + usagef("git diff %s ", + no_index == DIFF_NO_INDEX_EXPLICIT ? + "--no-index" : "[--no-index]"); + + } + /* If this is a no-index diff, just run it and exit there. */ + diff_no_index(&rev, argc, argv, prefix); + } /* Otherwise, we are doing the usual "git" diff */ rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; diff --git a/diff-no-index.c b/diff-no-index.c index 00a8eefde9..33e5982a1c 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -183,54 +183,12 @@ static int queue_diff(struct diff_options *o, void diff_no_index(struct rev_info *revs, int argc, const char **argv, - int nongit, const char *prefix) + const char *prefix) { int i, prefixlen; - int no_index = 0; unsigned deprecated_show_diff_q_option_used = 0; const char *paths[2]; - /* Were we asked to do --no-index explicitly? */ - for (i = 1; i < argc; i++) { - if (!strcmp(argv[i], "--")) { - i++; - break; - } - if (!strcmp(argv[i], "--no-index")) - no_index = 1; - if (argv[i][0] != '-') - break; - } - - if (!no_index && !nongit) { - /* - * Inside a git repository, without --no-index. Only - * when a path outside the repository is given, - * e.g. "git diff /var/tmp/[12]", or "git diff - * Makefile /var/tmp/Makefile", allow it to be used as - * a colourful "diff" replacement. - */ - if ((argc != i + 2) || - (path_inside_repo(prefix, argv[i]) && - path_inside_repo(prefix, argv[i+1]))) - return; - } - if (argc != i + 2) { - if (!no_index) { - /* - * There was no --no-index and there were not two - * paths. It is possible that the user intended - * to do an inside-repository operation. - */ - fprintf(stderr, "Not a git repository\n"); - fprintf(stderr, - "To compare two paths outside a working tree:\n"); - } - /* Give the usage message for non-repository usage and exit. */ - usagef("git diff %s ", - no_index ? "--no-index" : "[--no-index]"); - } - diff_setup(&revs->diffopt); for (i = 1; i < argc - 2; ) { int j; diff --git a/diff.h b/diff.h index e34232501e..de105d392a 100644 --- a/diff.h +++ b/diff.h @@ -330,7 +330,7 @@ extern int diff_flush_patch_id(struct diff_options *, unsigned char *); extern int diff_result_code(struct diff_options *, int); -extern void diff_no_index(struct rev_info *, int, const char **, int, const char *); +extern void diff_no_index(struct rev_info *, int, const char **, const char *); extern int index_differs_from(const char *def, int diff_flags); -- cgit v1.2.3 From 6df5762db354ca55a0cf77451d06b332b7de0b82 Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Wed, 11 Dec 2013 10:58:43 +0100 Subject: 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~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 Signed-off-by: Junio C Hamano --- 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 da69e4a3c8..ea1dd65a2f 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 0000000000..683be6984f --- /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 979e98398b..077c775e77 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 -- cgit v1.2.3 From 8a19dfa1aa5bbbc6e3ea8553e418ad4f78448cb3 Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Mon, 16 Dec 2013 21:19:23 +0100 Subject: diff: add test for --no-index executed outside repo 470faf9 diff: move no-index detection to builtin/diff.c breaks the error message for "git diff --no-index", when the command is executed outside of a git repository and the wrong number of arguments are given. 6df5762 diff: don't read index when --no-index is given fixes the problem. Add a test to guard against similar breakages in the future. Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- t/t4053-diff-no-index.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 077c775e77..2ab3c48734 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -44,4 +44,15 @@ test_expect_success 'git diff outside repo with broken index' ' ) ' +test_expect_success 'git diff --no-index executed outside repo gives correct error message' ' + ( + GIT_CEILING_DIRECTORIES=$TRASH_DIRECTORY/non && + export GIT_CEILING_DIRECTORIES && + cd non/git && + test_must_fail git diff --no-index a 2>actual.err && + echo "usage: git diff --no-index " >expect.err && + test_cmp expect.err actual.err + ) +' + test_done -- cgit v1.2.3 From aad90e85f8e6368533aa30c072b8e2bd7adafa53 Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Mon, 16 Dec 2013 21:19:24 +0100 Subject: diff: avoid some nesting Avoid some nesting in builtin/diff.c, to make the code easier to read. There are no functional changes. Helped-by: Jonathan Nieder Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- builtin/diff.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index ea1dd65a2f..24d6271625 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -319,27 +319,26 @@ int cmd_diff(int argc, const char **argv, const char *prefix) init_revisions(&rev, prefix); - if (no_index) { - if (argc != i + 2) { - if (no_index == DIFF_NO_INDEX_IMPLICIT) { - /* - * There was no --no-index and there were not two - * paths. It is possible that the user intended - * to do an inside-repository operation. - */ - fprintf(stderr, "Not a git repository\n"); - fprintf(stderr, - "To compare two paths outside a working tree:\n"); - } - /* Give the usage message for non-repository usage and exit. */ - usagef("git diff %s ", - no_index == DIFF_NO_INDEX_EXPLICIT ? - "--no-index" : "[--no-index]"); - + if (no_index && argc != i + 2) { + if (no_index == DIFF_NO_INDEX_IMPLICIT) { + /* + * There was no --no-index and there were not two + * paths. It is possible that the user intended + * to do an inside-repository operation. + */ + fprintf(stderr, "Not a git repository\n"); + fprintf(stderr, + "To compare two paths outside a working tree:\n"); } + /* Give the usage message for non-repository usage and exit. */ + usagef("git diff %s ", + no_index == DIFF_NO_INDEX_EXPLICIT ? + "--no-index" : "[--no-index]"); + + } + if (no_index) /* If this is a no-index diff, just run it and exit there. */ diff_no_index(&rev, argc, argv, prefix); - } /* Otherwise, we are doing the usual "git" diff */ rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index; -- cgit v1.2.3