[PATCH] Refactor dirty submodule detection in diff-lib.c

[Jens Lehmann <Jens.Lehmann@web.de>]

Moving duplicated code into the new function match_stat_with_submodule().
Replacing the implicit activation of detailed checks for the dirtiness of
submodules when DIFF_FORMAT_PATCH was selected with explicitly setting
the recently added DIFF_OPT_DIRTY_SUBMODULES option in diff_setup_done().

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

All tests run fine with this patch. All other callsites where the option
DIFF_FORMAT_PATCH is set look like they don't deal with the work tree
or they call diff_setup_done() shortly after.
I am pretty sure about builtin/log.c, as this can't use the work tree
but only already commited stuff, no?
But then there is edit_patch() in buitin/add.c, I am not sure it makes
sense to set DIRTY_SUBMODULES there too. Opinions?


 diff-lib.c |   45 +++++++++++++++++++++++++++------------------
 diff.c     |    6 ++++++
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 1ab286a..64be827 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -55,6 +55,29 @@ static int check_removed(const struct cache_entry *ce, struct stat *st)
 	return 0;
 }

+/*
+ * Has a file changed or has a submodule new commits or a dirty work tree?
+ *
+ * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES
+ * option is set, the caller does not only want to know if a submodule is
+ * modified at all but wants to know all the conditions that are met (new
+ * commits, untracked content and/or modified content).
+ */
+static int match_stat_with_submodule(struct diff_options *diffopt,
+				      struct cache_entry *ce, struct stat *st,
+				      unsigned ce_option, unsigned *dirty_submodule)
+{
+	int changed = ce_match_stat(ce, st, ce_option);
+	if (S_ISGITLINK(ce->ce_mode)
+	    && !DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)
+	    && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES))) {
+		*dirty_submodule = is_submodule_modified(ce->name);
+		if (*dirty_submodule)
+			changed = 1;
+	}
+	return changed;
+}
+
 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
 	int entries, i;
@@ -177,15 +200,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				       ce->sha1, ce->name, 0);
 			continue;
 		}
-		changed = ce_match_stat(ce, &st, ce_option);
-		if (S_ISGITLINK(ce->ce_mode)
-		    && !DIFF_OPT_TST(&revs->diffopt, IGNORE_SUBMODULES)
-		    && (!changed || (revs->diffopt.output_format & DIFF_FORMAT_PATCH)
-			|| DIFF_OPT_TST(&revs->diffopt, DIRTY_SUBMODULES))) {
-			dirty_submodule = is_submodule_modified(ce->name);
-			if (dirty_submodule)
-				changed = 1;
-		}
+		changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
+    	    					    ce_option, &dirty_submodule);
 		if (!changed) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
@@ -241,15 +257,8 @@ static int get_stat_data(struct cache_entry *ce,
 			}
 			return -1;
 		}
-		changed = ce_match_stat(ce, &st, 0);
-		if (S_ISGITLINK(ce->ce_mode)
-		    && !DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)
-		    && (!changed || (diffopt->output_format & DIFF_FORMAT_PATCH)
-			|| DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES))) {
-			*dirty_submodule = is_submodule_modified(ce->name);
-			if (*dirty_submodule)
-				changed = 1;
-		}
+		changed = match_stat_with_submodule(diffopt, ce, &st,
+     	   					    0, dirty_submodule);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			sha1 = null_sha1;
diff --git a/diff.c b/diff.c
index dfdfa1a..5aefdcb 100644
--- a/diff.c
+++ b/diff.c
@@ -2628,6 +2628,12 @@ int diff_setup_done(struct diff_options *options)
 	 */
 	if (options->pickaxe)
 		DIFF_OPT_SET(options, RECURSIVE);
+	/*
+	 * When patches are generated, submodules diffed against the work tree
+	 * must be checked for dirtiness too so it can be shown in the output
+	 */
+	if (options->output_format & DIFF_FORMAT_PATCH)
+		DIFF_OPT_SET(options, DIRTY_SUBMODULES);

 	if (options->detect_rename && options->rename_limit < 0)
 		options->rename_limit = diff_rename_limit_default;
-- 
1.7.0.2.385.g964e