Performance optimization for detection of modified submodules

In the worst case is_submodule_modified() got called three times for
each submodule. The information we got from scanning the whole
submodule tree the first time can be reused instead.

New parameters have been added to diff_change() and diff_addremove(),
the information is stored in a new member of struct diff_filespec. Its
value is then reused instead of calling is_submodule_modified() again.

When no explicit "-dirty" is needed in the output the call to
is_submodule_modified() is not necessary when the submodules HEAD
already disagrees with the ref of the superproject, as this alone
marks it as modified. To achieve that, get_stat_data() got an extra
argument.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/diff-lib.c b/diff-lib.c
index 9cdf6daa9077e4c743822e461a08ff80530da38e..23e180eed13230fa234cdcb86db7dbd914abb4a5 100644 (file)
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -73,6 +73,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
                unsigned int oldmode, newmode;
                struct cache_entry *ce = active_cache[i];
                int changed;
+               unsigned dirty_submodule = 0;
 
                if (DIFF_OPT_TST(&revs->diffopt, QUICK) &&
                        DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
@@ -173,12 +174,16 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
                        if (silent_on_removed)
                                continue;
                        diff_addremove(&revs->diffopt, '-', ce->ce_mode,
-                                      ce->sha1, ce->name);
+                                      ce->sha1, ce->name, 0);
                        continue;
                }
                changed = ce_match_stat(ce, &st, ce_option);
-               if (S_ISGITLINK(ce->ce_mode) && !changed)
-                       changed = is_submodule_modified(ce->name);
+               if (S_ISGITLINK(ce->ce_mode)
+                   && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH))
+                   && is_submodule_modified(ce->name)) {
+                       changed = 1;
+                       dirty_submodule = 1;
+               }
                if (!changed) {
                        ce_mark_uptodate(ce);
                        if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
@@ -188,7 +193,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
                newmode = ce_mode_from_stat(ce, st.st_mode);
                diff_change(&revs->diffopt, oldmode, newmode,
                            ce->sha1, (changed ? null_sha1 : ce->sha1),
-                           ce->name);
+                           ce->name, 0, dirty_submodule);
 
        }
        diffcore_std(&revs->diffopt);
@@ -204,16 +209,18 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 static void diff_index_show_file(struct rev_info *revs,
                                 const char *prefix,
                                 struct cache_entry *ce,
-                                const unsigned char *sha1, unsigned int mode)
+                                const unsigned char *sha1, unsigned int mode,
+                                unsigned dirty_submodule)
 {
        diff_addremove(&revs->diffopt, prefix[0], mode,
-                      sha1, ce->name);
+                      sha1, ce->name, dirty_submodule);
 }
 
 static int get_stat_data(struct cache_entry *ce,
                         const unsigned char **sha1p,
                         unsigned int *modep,
-                        int cached, int match_missing)
+                        int cached, int match_missing,
+                        unsigned *dirty_submodule, int output_format)
 {
        const unsigned char *sha1 = ce->sha1;
        unsigned int mode = ce->ce_mode;
@@ -233,8 +240,13 @@ static int get_stat_data(struct cache_entry *ce,
                        return -1;
                }
                changed = ce_match_stat(ce, &st, 0);
-               if (changed
-                   || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {
+               if (S_ISGITLINK(ce->ce_mode)
+                   && (!changed || (output_format & DIFF_FORMAT_PATCH))
+                   && is_submodule_modified(ce->name)) {
+                       changed = 1;
+                       *dirty_submodule = 1;
+               }
+               if (changed) {
                        mode = ce_mode_from_stat(ce, st.st_mode);
                        sha1 = null_sha1;
                }
@@ -251,15 +263,17 @@ static void show_new_file(struct rev_info *revs,
 {
        const unsigned char *sha1;
        unsigned int mode;
+       unsigned dirty_submodule = 0;
 
        /*
         * New file in the index: it might actually be different in
         * the working copy.
         */
-       if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0)
+       if (get_stat_data(new, &sha1, &mode, cached, match_missing,
+           &dirty_submodule, revs->diffopt.output_format) < 0)
                return;
 
-       diff_index_show_file(revs, "+", new, sha1, mode);
+       diff_index_show_file(revs, "+", new, sha1, mode, dirty_submodule);
 }
 
 static int show_modified(struct rev_info *revs,
@@ -270,11 +284,13 @@ static int show_modified(struct rev_info *revs,
 {
        unsigned int mode, oldmode;
        const unsigned char *sha1;
+       unsigned dirty_submodule = 0;
 
-       if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {
+       if (get_stat_data(new, &sha1, &mode, cached, match_missing,
+                         &dirty_submodule, revs->diffopt.output_format) < 0) {
                if (report_missing)
                        diff_index_show_file(revs, "-", old,
-                                            old->sha1, old->ce_mode);
+                                            old->sha1, old->ce_mode, 0);
                return -1;
        }
 
@@ -309,7 +325,7 @@ static int show_modified(struct rev_info *revs,
                return 0;
 
        diff_change(&revs->diffopt, oldmode, mode,
-                   old->sha1, sha1, old->name);
+                   old->sha1, sha1, old->name, 0, dirty_submodule);
        return 0;
 }
 
@@ -356,7 +372,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
         * Something removed from the tree?
         */
        if (!idx) {
-               diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+               diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode, 0);
                return;
        }
 

diff --git a/diff.c b/diff.c
index 750c066a5eb81200b9ebbcf958d5c6095eb5e1eb..8986873c0e9b1cd949a11dd1e59a85df080520d3 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -2032,7 +2032,7 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
        char *data = xmalloc(100), *dirty = "";
 
        /* Are we looking at the work tree? */
-       if (!s->sha1_valid && is_submodule_modified(s->path))
+       if (!s->sha1_valid && s->dirty_submodule)
                dirty = "-dirty";
 
        len = snprintf(data, 100,
@@ -3719,7 +3719,7 @@ int diff_result_code(struct diff_options *opt, int status)
 void diff_addremove(struct diff_options *options,
                    int addremove, unsigned mode,
                    const unsigned char *sha1,
-                   const char *concatpath)
+                   const char *concatpath, unsigned dirty_submodule)
 {
        struct diff_filespec *one, *two;
 
@@ -3751,8 +3751,10 @@ void diff_addremove(struct diff_options *options,
 
        if (addremove != '+')
                fill_filespec(one, sha1, mode);
-       if (addremove != '-')
+       if (addremove != '-') {
                fill_filespec(two, sha1, mode);
+               two->dirty_submodule = dirty_submodule;
+       }
 
        diff_queue(&diff_queued_diff, one, two);
        if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
@@ -3763,7 +3765,8 @@ void diff_change(struct diff_options *options,
                 unsigned old_mode, unsigned new_mode,
                 const unsigned char *old_sha1,
                 const unsigned char *new_sha1,
-                const char *concatpath)
+                const char *concatpath,
+                unsigned old_dirty_submodule, unsigned new_dirty_submodule)
 {
        struct diff_filespec *one, *two;
 
@@ -3776,6 +3779,8 @@ void diff_change(struct diff_options *options,
                const unsigned char *tmp_c;
                tmp = old_mode; old_mode = new_mode; new_mode = tmp;
                tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
+               tmp = old_dirty_submodule; old_dirty_submodule = new_dirty_submodule;
+                       new_dirty_submodule = tmp;
        }
 
        if (options->prefix &&
@@ -3786,6 +3791,8 @@ void diff_change(struct diff_options *options,
        two = alloc_filespec(concatpath);
        fill_filespec(one, old_sha1, old_mode);
        fill_filespec(two, new_sha1, new_mode);
+       one->dirty_submodule = old_dirty_submodule;
+       two->dirty_submodule = new_dirty_submodule;
 
        diff_queue(&diff_queued_diff, one, two);
        if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))

diff --git a/diff.h b/diff.h
index 6f6d0ed01d9212289784cf7250358320f64b72cc..968a8dce95b93d76e299c070ffa3587c31fcc965 100644 (file)
--- a/diff.h
+++ b/diff.h
@@ -14,12 +14,13 @@ typedef void (*change_fn_t)(struct diff_options *options,
                 unsigned old_mode, unsigned new_mode,
                 const unsigned char *old_sha1,
                 const unsigned char *new_sha1,
-                const char *fullpath);
+                const char *fullpath,
+                unsigned old_dirty_submodule, unsigned new_dirty_submodule);
 
 typedef void (*add_remove_fn_t)(struct diff_options *options,
                    int addremove, unsigned mode,
                    const unsigned char *sha1,
-                   const char *fullpath);
+                   const char *fullpath, unsigned dirty_submodule);
 
 typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
                struct diff_options *options, void *data);
@@ -177,13 +178,14 @@ extern void diff_addremove(struct diff_options *,
                           int addremove,
                           unsigned mode,
                           const unsigned char *sha1,
-                          const char *fullpath);
+                          const char *fullpath, unsigned dirty_submodule);
 
 extern void diff_change(struct diff_options *,
                        unsigned mode1, unsigned mode2,
                        const unsigned char *sha1,
                        const unsigned char *sha2,
-                       const char *fullpath);
+                       const char *fullpath,
+                       unsigned dirty_submodule1, unsigned dirty_submodule2);
 
 extern void diff_unmerge(struct diff_options *,
                         const char *path,

diff --git a/diffcore.h b/diffcore.h
index 5b634585e8970de81067b9807d8813c083b86b0a..66687c3fe5ea4552cff2b864b73696460ca40b1e 100644 (file)
--- a/diffcore.h
+++ b/diffcore.h
@@ -42,6 +42,7 @@ struct diff_filespec {
 #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)
        unsigned should_free : 1; /* data should be free()'ed */
        unsigned should_munmap : 1; /* data should be munmap()'ed */
+       unsigned dirty_submodule : 1;  /* For submodules: its work tree is dirty */
 
        struct userdiff_driver *driver;
        /* data should be considered "binary"; -1 means "don't know yet" */

diff --git a/revision.c b/revision.c
index 25fa14d93e04531e8d1dde3d9ac8408670adf32a..769cfd42514a7a6ea4e528d84e8ce8dee8820fe2 100644 (file)
--- a/revision.c
+++ b/revision.c
@@ -268,7 +268,7 @@ static int tree_difference = REV_TREE_SAME;
 static void file_add_remove(struct diff_options *options,
                    int addremove, unsigned mode,
                    const unsigned char *sha1,
-                   const char *fullpath)
+                   const char *fullpath, unsigned dirty_submodule)
 {
        int diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;
 
@@ -281,7 +281,8 @@ static void file_change(struct diff_options *options,
                 unsigned old_mode, unsigned new_mode,
                 const unsigned char *old_sha1,
                 const unsigned char *new_sha1,
-                const char *fullpath)
+                const char *fullpath,
+                unsigned old_dirty_submodule, unsigned new_dirty_submodule)
 {
        tree_difference = REV_TREE_DIFFERENT;
        DIFF_OPT_SET(options, HAS_CHANGES);

diff --git a/tree-diff.c b/tree-diff.c
index 7d745b44061955ced9ce4624a498647a1d8d32a5..fe9f52c4796512f40869e511b55a2d97fa84532e 100644 (file)
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -68,7 +68,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
                if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {
                        newbase[baselen + pathlen1] = 0;
                        opt->change(opt, mode1, mode2,
-                                   sha1, sha2, newbase);
+                                   sha1, sha2, newbase, 0, 0);
                        newbase[baselen + pathlen1] = '/';
                }
                retval = diff_tree_sha1(sha1, sha2, newbase, opt);
@@ -77,7 +77,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
        }
 
        fullname = malloc_fullname(base, baselen, path1, pathlen1);
-       opt->change(opt, mode1, mode2, sha1, sha2, fullname);
+       opt->change(opt, mode1, mode2, sha1, sha2, fullname, 0, 0);
        free(fullname);
        return 0;
 }
@@ -241,7 +241,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 
                if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {
                        newbase[baselen + pathlen] = 0;
-                       opt->add_remove(opt, *prefix, mode, sha1, newbase);
+                       opt->add_remove(opt, *prefix, mode, sha1, newbase, 0);
                        newbase[baselen + pathlen] = '/';
                }
 
@@ -252,7 +252,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
                free(newbase);
        } else {
                char *fullname = malloc_fullname(base, baselen, path, pathlen);
-               opt->add_remove(opt, prefix[0], mode, sha1, fullname);
+               opt->add_remove(opt, prefix[0], mode, sha1, fullname, 0);
                free(fullname);
        }
 }