git status: Fix false positive "new commits" output for dirty submodules

Testing if the output "new commits" should appear in the long format of
"git status" is done by comparing the hashes of the diffpair. This always
resulted in printing "new commits" for submodules that contained untracked
or modified content, even if they did not contain new commits. The reason
was that match_stat_with_submodule() did set the "changed" flag for dirty
submodules, resulting in two->sha1 being set to the null_sha1 at the call
sites, which indicates that new commits are present. This is changed so
that when no new commits are present, the same object names are in the
sha1 field for both sides of the filepair, and the working tree side will
have the "dirty_submodule" flag set when appropriate. For a submodule to
be seen as modified even when it just has a dirty work tree, some
conditions had to be extended to also check for the "dirty_submodule"
flag.

Unfortunately the test case that should have found this bug had been
changed incorrectly too. It is fixed and extended to test for other
combinations too.

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

3 files changed, 88 insertions, 9 deletions

diff --git a/diff-lib.c b/diff-lib.c
index ea9cf561c..87a259111 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -72,8 +72,6 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 	    && !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;
 }
@@ -202,7 +200,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		}
 		changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
 						    ce_option, &dirty_submodule);
-		if (!changed) {
+		if (!changed && !dirty_submodule) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 				continue;
@@ -333,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 240401b73..8fd020fa4 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,
@@ -3081,7 +3081,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. */
@@ -3216,6 +3217,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/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 1c8d32a99..dc9150a71 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 (new commits, modified content)" 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 (new commits, modified content)" output
+	grep "modified:   sub (modified content)" output
 '
 
 test_expect_success 'status with added file in submodule (porcelain)' '
@@ -64,7 +64,7 @@ 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 (new commits, untracked content)" output
+	grep "modified:   sub (untracked content)" output
 '
 
 test_expect_success 'status with untracked file in submodule (porcelain)' '
@@ -74,6 +74,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
 '