Merge branch 'jl/submodule-diff-dirtiness'

* jl/submodule-diff-dirtiness:
  git status: ignoring untracked files must apply to submodules too
  git status: Fix false positive "new commits" output for dirty submodules
  Refactor dirty submodule detection in diff-lib.c
  git status: Show detailed dirty status of submodules in long format
  git diff --submodule: Show detailed dirty status of submodules

10 files changed, 221 insertions, 43 deletions

diff --git a/diff-lib.c b/diff-lib.c
index 64976f921d..c9f6e05bad 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -55,6 +55,27 @@ 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, DIFF_OPT_TST(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES));
+	}
+	return changed;
+}
+
 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
 	int entries, i;
@@ -177,15 +198,9 @@ 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))
-		    && is_submodule_modified(ce->name)) {
-			changed = 1;
-			dirty_submodule = 1;
-		}
-		if (!changed) {
+		changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
+						    ce_option, &dirty_submodule);
+		if (!changed && !dirty_submodule) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 				continue;
@@ -240,14 +255,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))
-		    && is_submodule_modified(ce->name)) {
-			changed = 1;
-			*dirty_submodule = 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;
@@ -322,7 +331,7 @@ static int show_modified(struct rev_info *revs,
 	}
 
 	oldmode = old->ce_mode;
-	if (mode == oldmode && !hashcmp(sha1, old->sha1) &&
+	if (mode == oldmode && !hashcmp(sha1, old->sha1) && !dirty_submodule &&
 	    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 		return 0;
 
diff --git a/diff.c b/diff.c
index dfdfa1a813..f5d93e9694 100644
--- 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 && s->dirty_submodule)
+	if (s->dirty_submodule)
 		dirty = "-dirty";
 
 	len = snprintf(data, 100,
@@ -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;
@@ -3086,7 +3092,8 @@ int diff_unmodified_pair(struct diff_filepair *p)
 	 * dealing with a change.
 	 */
 	if (one->sha1_valid && two->sha1_valid &&
-	    !hashcmp(one->sha1, two->sha1))
+	    !hashcmp(one->sha1, two->sha1) &&
+	    !one->dirty_submodule && !two->dirty_submodule)
 		return 1; /* no change */
 	if (!one->sha1_valid && !two->sha1_valid)
 		return 1; /* both look at the same file on the filesystem. */
@@ -3221,6 +3228,8 @@ static void diff_resolve_rename_copy(void)
 		}
 		else if (hashcmp(p->one->sha1, p->two->sha1) ||
 			 p->one->mode != p->two->mode ||
+			 p->one->dirty_submodule ||
+			 p->two->dirty_submodule ||
 			 is_null_sha1(p->one->sha1))
 			p->status = DIFF_STATUS_MODIFIED;
 		else {
diff --git a/diff.h b/diff.h
index 2ef3341fb0..6a71013dc6 100644
--- a/diff.h
+++ b/diff.h
@@ -69,6 +69,8 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)
 #define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)
 #define DIFF_OPT_SUBMODULE_LOG       (1 << 23)
+#define DIFF_OPT_DIRTY_SUBMODULES    (1 << 24)
+#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
diff --git a/diffcore.h b/diffcore.h
index 66687c3fe5..fcd00bf27a 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -42,7 +42,9 @@ 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 */
+	unsigned dirty_submodule : 2;  /* For submodules: its work tree is dirty */
+#define DIRTY_SUBMODULE_UNTRACKED 1
+#define DIRTY_SUBMODULE_MODIFIED  2
 
 	struct userdiff_driver *driver;
 	/* data should be considered "binary"; -1 means "don't know yet" */
diff --git a/submodule.c b/submodule.c
index 5d286e409e..b3b8bc1479 100644
--- a/submodule.c
+++ b/submodule.c
@@ -5,6 +5,7 @@
 #include "commit.h"
 #include "revision.h"
 #include "run-command.h"
+#include "diffcore.h"
 
 static int add_submodule_odb(const char *path)
 {
@@ -85,13 +86,21 @@ void show_submodule_summary(FILE *f, const char *path,
 			message = "(revision walker failed)";
 	}
 
+	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+		fprintf(f, "Submodule %s contains untracked content\n", path);
+	if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+		fprintf(f, "Submodule %s contains modified content\n", path);
+
+	if (!hashcmp(one, two)) {
+		strbuf_release(&sb);
+		return;
+	}
+
 	strbuf_addf(&sb, "Submodule %s %s..", path,
 			find_unique_abbrev(one, DEFAULT_ABBREV));
 	if (!fast_backward && !fast_forward)
 		strbuf_addch(&sb, '.');
 	strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
-	if (dirty_submodule)
-		strbuf_add(&sb, "-dirty", 6);
 	if (message)
 		strbuf_addf(&sb, " %s\n", message);
 	else
@@ -121,17 +130,21 @@ void show_submodule_summary(FILE *f, const char *path,
 	strbuf_release(&sb);
 }
 
-int is_submodule_modified(const char *path)
+unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	int len, i;
+	int i;
+	ssize_t len;
 	struct child_process cp;
 	const char *argv[] = {
 		"status",
 		"--porcelain",
 		NULL,
+		NULL,
 	};
 	const char *env[LOCAL_REPO_ENV_SIZE + 3];
 	struct strbuf buf = STRBUF_INIT;
+	unsigned dirty_submodule = 0;
+	const char *line, *next_line;
 
 	for (i = 0; i < LOCAL_REPO_ENV_SIZE; i++)
 		env[i] = local_repo_env[i];
@@ -151,6 +164,9 @@ int is_submodule_modified(const char *path)
 	env[i++] = strbuf_detach(&buf, NULL);
 	env[i] = NULL;
 
+	if (ignore_untracked)
+		argv[2] = "-uno";
+
 	memset(&cp, 0, sizeof(cp));
 	cp.argv = argv;
 	cp.env = env;
@@ -161,6 +177,25 @@ int is_submodule_modified(const char *path)
 		die("Could not run git status --porcelain");
 
 	len = strbuf_read(&buf, cp.out, 1024);
+	line = buf.buf;
+	while (len > 2) {
+		if ((line[0] == '?') && (line[1] == '?')) {
+			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+				break;
+		} else {
+			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+			if (ignore_untracked ||
+			    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
+				break;
+		}
+		next_line = strchr(line, '\n');
+		if (!next_line)
+			break;
+		next_line++;
+		len -= (next_line - line);
+		line = next_line;
+	}
 	close(cp.out);
 
 	if (finish_command(&cp))
@@ -169,5 +204,5 @@ int is_submodule_modified(const char *path)
 	for (i = LOCAL_REPO_ENV_SIZE; env[i]; i++)
 		free((char *)env[i]);
 	strbuf_release(&buf);
-	return len != 0;
+	return dirty_submodule;
 }
diff --git a/submodule.h b/submodule.h
index 233696555e..dbda270873 100644
--- a/submodule.h
+++ b/submodule.h
@@ -5,6 +5,6 @@ void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
 		unsigned dirty_submodule,
 		const char *del, const char *add, const char *reset);
-int is_submodule_modified(const char *path);
+unsigned is_submodule_modified(const char *path, int ignore_untracked);
 
 #endif
diff --git a/t/t4041-diff-submodule.sh b/t/t4041-diff-submodule.sh
index 464305405a..11b19972ca 100755
--- a/t/t4041-diff-submodule.sh
+++ b/t/t4041-diff-submodule.sh
@@ -201,7 +201,7 @@ test_expect_success 'submodule contains untracked content' "
 	echo new > sm1/new-file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	diff actual - <<-EOF
-Submodule sm1 $head6..$head6-dirty:
+Submodule sm1 contains untracked content
 EOF
 "
 
@@ -209,7 +209,8 @@ test_expect_success 'submodule contains untracked and modifed content' "
 	echo new > sm1/foo6 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	diff actual - <<-EOF
-Submodule sm1 $head6..$head6-dirty:
+Submodule sm1 contains untracked content
+Submodule sm1 contains modified content
 EOF
 "
 
@@ -217,7 +218,7 @@ test_expect_success 'submodule contains modifed content' "
 	rm -f sm1/new-file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	diff actual - <<-EOF
-Submodule sm1 $head6..$head6-dirty:
+Submodule sm1 contains modified content
 EOF
 "
 
@@ -235,7 +236,8 @@ test_expect_success 'modified submodule contains untracked content' "
 	echo new > sm1/new-file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	diff actual - <<-EOF
-Submodule sm1 $head6..$head8-dirty:
+Submodule sm1 contains untracked content
+Submodule sm1 $head6..$head8:
   > change
 EOF
 "
@@ -244,7 +246,9 @@ test_expect_success 'modified submodule contains untracked and modifed content'
 	echo modification >> sm1/foo6 &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	diff actual - <<-EOF
-Submodule sm1 $head6..$head8-dirty:
+Submodule sm1 contains untracked content
+Submodule sm1 contains modified content
+Submodule sm1 $head6..$head8:
   > change
 EOF
 "
@@ -253,7 +257,8 @@ test_expect_success 'modified submodule contains modifed content' "
 	rm -f sm1/new-file &&
 	git diff-index -p --submodule=log HEAD >actual &&
 	diff actual - <<-EOF
-Submodule sm1 $head6..$head8-dirty:
+Submodule sm1 contains modified content
+Submodule sm1 $head6..$head8:
   > change
 EOF
 "
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 253c334319..aeec1f6142 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -34,7 +34,7 @@ test_expect_success 'status with modified file in submodule' '
 	(cd sub && git reset --hard) &&
 	echo "changed" >sub/foo &&
 	git status >output &&
-	grep "modified:   sub" output
+	grep "modified:   sub (modified content)" output
 '
 
 test_expect_success 'status with modified file in submodule (porcelain)' '
@@ -49,7 +49,7 @@ test_expect_success 'status with modified file in submodule (porcelain)' '
 test_expect_success 'status with added file in submodule' '
 	(cd sub && git reset --hard && echo >foo && git add foo) &&
 	git status >output &&
-	grep "modified:   sub" output
+	grep "modified:   sub (modified content)" output
 '
 
 test_expect_success 'status with added file in submodule (porcelain)' '
@@ -64,7 +64,12 @@ test_expect_success 'status with untracked file in submodule' '
 	(cd sub && git reset --hard) &&
 	echo "content" >sub/new-file &&
 	git status >output &&
-	grep "modified:   sub" output
+	grep "modified:   sub (untracked content)" output
+'
+
+test_expect_success 'status -uno with untracked file in submodule' '
+	git status -uno >output &&
+	grep "^nothing to commit" output
 '
 
 test_expect_success 'status with untracked file in submodule (porcelain)' '
@@ -74,6 +79,84 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '
 	EOF
 '
 
+test_expect_success 'status with added and untracked file in submodule' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	grep "modified:   sub (modified content, untracked content)" output
+'
+
+test_expect_success 'status with added and untracked file in submodule (porcelain)' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	echo "content" >sub/new-file &&
+	git status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub
+	EOF
+'
+
+test_expect_success 'status with modified file in modified submodule' '
+	(cd sub && git reset --hard) &&
+	rm sub/new-file &&
+	(cd sub && echo "next change" >foo && git commit -m "next change" foo) &&
+	echo "changed" >sub/foo &&
+	git status >output &&
+	grep "modified:   sub (new commits, modified content)" output
+'
+
+test_expect_success 'status with modified file in modified submodule (porcelain)' '
+	(cd sub && git reset --hard) &&
+	echo "changed" >sub/foo &&
+	git status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub
+	EOF
+'
+
+test_expect_success 'status with added file in modified submodule' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	git status >output &&
+	grep "modified:   sub (new commits, modified content)" output
+'
+
+test_expect_success 'status with added file in modified submodule (porcelain)' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	git status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub
+	EOF
+'
+
+test_expect_success 'status with untracked file in modified submodule' '
+	(cd sub && git reset --hard) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	grep "modified:   sub (new commits, untracked content)" output
+'
+
+test_expect_success 'status with untracked file in modified submodule (porcelain)' '
+	git status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub
+	EOF
+'
+
+test_expect_success 'status with added and untracked file in modified submodule' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	grep "modified:   sub (new commits, modified content, untracked content)" output
+'
+
+test_expect_success 'status with added and untracked file in modified submodule (porcelain)' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	echo "content" >sub/new-file &&
+	git status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub
+	EOF
+'
+
 test_expect_success 'rm submodule contents' '
 	rm -rf sub/* sub/.git
 '
diff --git a/wt-status.c b/wt-status.c
index dcaec7f09e..8ca59a2d2a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -78,7 +78,8 @@ static void wt_status_print_cached_header(struct wt_status *s)
 }
 
 static void wt_status_print_dirty_header(struct wt_status *s,
-					 int has_deleted)
+					 int has_deleted,
+					 int has_dirty_submodules)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
@@ -90,6 +91,8 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 	else
 		color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" to update what will be committed)");
 	color_fprintf_ln(s->fp, c, "#   (use \"git checkout -- <file>...\" to discard changes in working directory)");
+	if (has_dirty_submodules)
+		color_fprintf_ln(s->fp, c, "#   (commit or discard the untracked or modified content in submodules)");
 	color_fprintf_ln(s->fp, c, "#");
 }
 
@@ -144,6 +147,7 @@ static void wt_status_print_change_data(struct wt_status *s,
 	char *two_name;
 	const char *one, *two;
 	struct strbuf onebuf = STRBUF_INIT, twobuf = STRBUF_INIT;
+	struct strbuf extra = STRBUF_INIT;
 
 	one_name = two_name = it->string;
 	switch (change_type) {
@@ -153,6 +157,17 @@ static void wt_status_print_change_data(struct wt_status *s,
 			one_name = d->head_path;
 		break;
 	case WT_STATUS_CHANGED:
+		if (d->new_submodule_commits || d->dirty_submodule) {
+			strbuf_addstr(&extra, " (");
+			if (d->new_submodule_commits)
+				strbuf_addf(&extra, "new commits, ");
+			if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+				strbuf_addf(&extra, "modified content, ");
+			if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+				strbuf_addf(&extra, "untracked content, ");
+			strbuf_setlen(&extra, extra.len - 2);
+			strbuf_addch(&extra, ')');
+		}
 		status = d->worktree_status;
 		break;
 	}
@@ -189,6 +204,10 @@ static void wt_status_print_change_data(struct wt_status *s,
 	default:
 		die("bug: unhandled diff status %c", status);
 	}
+	if (extra.len) {
+		color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "%s", extra.buf);
+		strbuf_release(&extra);
+	}
 	fprintf(s->fp, "\n");
 	strbuf_release(&onebuf);
 	strbuf_release(&twobuf);
@@ -218,6 +237,9 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 		}
 		if (!d->worktree_status)
 			d->worktree_status = p->status;
+		d->dirty_submodule = p->two->dirty_submodule;
+		if (S_ISGITLINK(p->two->mode))
+			d->new_submodule_commits = !!hashcmp(p->one->sha1, p->two->sha1);
 	}
 }
 
@@ -281,6 +303,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	init_revisions(&rev, NULL);
 	setup_revisions(0, NULL, &rev, NULL);
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	DIFF_OPT_SET(&rev.diffopt, DIRTY_SUBMODULES);
+	if (!s->show_untracked_files)
+		DIFF_OPT_SET(&rev.diffopt, IGNORE_UNTRACKED_IN_SUBMODULES);
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
 	rev.diffopt.format_callback_data = s;
 	rev.prune_data = s->pathspec;
@@ -421,33 +446,39 @@ static void wt_status_print_updated(struct wt_status *s)
  *  0 : no change
  *  1 : some change but no delete
  */
-static int wt_status_check_worktree_changes(struct wt_status *s)
+static int wt_status_check_worktree_changes(struct wt_status *s,
+					     int *dirty_submodules)
 {
 	int i;
 	int changes = 0;
 
+	*dirty_submodules = 0;
+
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		d = s->change.items[i].util;
 		if (!d->worktree_status ||
 		    d->worktree_status == DIFF_STATUS_UNMERGED)
 			continue;
-		changes = 1;
+		if (!changes)
+			changes = 1;
+		if (d->dirty_submodule)
+			*dirty_submodules = 1;
 		if (d->worktree_status == DIFF_STATUS_DELETED)
-			return -1;
+			changes = -1;
 	}
 	return changes;
 }
 
 static void wt_status_print_changed(struct wt_status *s)
 {
-	int i;
-	int worktree_changes = wt_status_check_worktree_changes(s);
+	int i, dirty_submodules;
+	int worktree_changes = wt_status_check_worktree_changes(s, &dirty_submodules);
 
 	if (!worktree_changes)
 		return;
 
-	wt_status_print_dirty_header(s, worktree_changes < 0);
+	wt_status_print_dirty_header(s, worktree_changes < 0, dirty_submodules);
 
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
diff --git a/wt-status.h b/wt-status.h
index c60f40a34a..91206739f3 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -25,6 +25,8 @@ struct wt_status_change_data {
 	int index_status;
 	int stagemask;
 	char *head_path;
+	unsigned dirty_submodule       : 2;
+	unsigned new_submodule_commits : 1;
 };
 
 struct wt_status {