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

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

[Jens Lehmann]

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

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> 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?

I like this particular refactoring to remove duplicated obscure logic.

I am not sure if it is the right thing to do to return "changed" if the
change is only "submodule is dirty even though its HEAD is the same as
what is recorded in the index of the superproject", though.  See my
patches in response to your "[PATCH] git status: Fix false positive "new
commits" output for".  I think it would be conceptually the right thing to
do to feed the same object names in the sha1 field for both sides of the
filepair, with the working tree side marked with "dirty-submodule".  I
also suspect that you would not need to re-resolve the gitlinks in later
stages when you generate patches in textual form if you do it that way.