Refactor dirty submodule detection in diff-lib.c

PATCH In review Jens Lehmann 2 participants 2 messages Jun 15, 2016

Share & tools

Jens Lehmann

author

2016-06-15 22:48 UTC #0 lore raw

PATCH 1/1 Refactor dirty submodule detection in diff-lib.c

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 <[email protected]>

---

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-lib.c +27 ~~-18~~

index 1ab286a..64be827 100644
@@ -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.c +6 -0

index dfdfa1a..5aefdcb 100644
@@ -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;

Signature · 1 line

1.7.0.2.385.g964e

Junio C Hamano

2016-06-15 22:48 UTC #1 lore raw

Quoted 8 lines · 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 <[email protected]>
---

Quoted 7 lines

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.