Merge branch 'tg/diff-no-index-refactor'

"git diff ../else/where/A ../else/where/B" when ../else/where is
clearly outside the repository, and "git diff --no-index A B", do
not have to look at the index at all, but we used to read the index
unconditionally.

* tg/diff-no-index-refactor:
diff: avoid some nesting
diff: add test for --no-index executed outside repo
diff: don't read index when --no-index is given
diff: move no-index detection to builtin/diff.c

diff --git a/builtin/diff.c b/builtin/diff.c
index fe0cc7f1b5b1451f267b1d9e24a6e40cd6e55325..0f247d24008a4dc7ba5958e716c13f7582a820f1 100644 (file)
--- 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;
@@ -259,7 +262,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
        struct object_array ent = OBJECT_ARRAY_INIT;
        int blobs = 0, paths = 0;
        struct blobinfo blob[2];
-       int nongit;
+       int nongit = 0, no_index = 0;
        int result = 0;
 
        /*
@@ -285,14 +288,59 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
         * Other cases are errors.
         */
 
-       prefix = setup_git_directory_gently(&nongit);
-       gitmodules_config();
+       /* 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;
+       }
+
+       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
+        * outside of a git repository.  In this case it behaves
+        * the same way as "git diff --no-index <a> <b>", 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;
+
+       if (!no_index)
+               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 && 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 <path> <path>",
+                      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;

diff --git a/diff-no-index.c b/diff-no-index.c
index 00a8eefde9aeb96592f5ed6455dbf5d3b16747c4..33e5982a1c61a5726580f40df0a07dadc7e2aaf1 100644 (file)
--- 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 <path> <path>",
-                      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 760388fa174c7c8370463e2a9788ded6aab685c7..ce123fa06fee6fb6e5e76d6a92ea8284a5bbbcf1 100644 (file)
--- a/diff.h
+++ b/diff.h
@@ -332,7 +332,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);
 

diff --git a/t/perf/p4001-diff-no-index.sh b/t/perf/p4001-diff-no-index.sh
new file mode 100755 (executable)
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 979e98398bebc21fe664ca4a19770e5b251bfc97..2ab3c487340569d7f2f8164776907ef30df2d415 100755 (executable)
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -29,4 +29,30 @@ 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_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 <path> <path>" >expect.err &&
+               test_cmp expect.err actual.err
+       )
+'
+
 test_done