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

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

Share & tools

Jens Lehmann

author

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

PATCH 1/1 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" was done by comparing the hashes of the diffpair. This always resulted in printing "new commits" for submodules that contained untracked or modified content, no matter if they also contained new commits. The reason was that the two->sha1 is always the null_sha1 when diffing against the work tree. This patch gets the correct ref to compare against by calling resolve_gitlink_ref(.., "HEAD", ..).

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

---

Another way to fix this bug would have been to enter the correct sha1 into two->sha1 for submodules with new commits. But i am not sure about the side effects such a change would have and thus fixed it this way so it could not hit anyone else.

 t/t7506-status-submodule.sh |   84 +++++++++++++++++++++++++++++++++++++++++--
 wt-status.c                 |    9 ++++-
 2 files changed, 88 insertions(+), 5 deletions(-)

t/t7506-status-submodule.sh +81 -3

index 1c8d32a..dc9150a 100755
@@ -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
 '

wt-status.c +7 -2

index e0e915e..d5d23de 100644
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "run-command.h"
 #include "remote.h"
+#include "refs.h"
 
 static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */

@@ -238,8 +239,12 @@ 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);
+		if (S_ISGITLINK(p->two->mode)) {
+			unsigned char sha1[20];
+
+			if (resolve_gitlink_ref(p->two->path, "HEAD", sha1) == 0)
+				d->new_submodule_commits = !!hashcmp(p->one->sha1, sha1);
+		}
 	}
 }

Signature · 1 line

1.7.0.2.387.g275c3b

Junio C Hamano

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

Quoted 2 lines · Jens Lehmann

Another way to fix this bug would have been to enter the correct sha1 into two->sha1 for submodules with new commits.

I didn't follow all the codepaths involved, but it might be worth a try. The beginning of the patch may look something like this, and then you would probably have to fix potential fallouts in diff.c from this change.

 diff-lib.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff-lib.c +1 -2

index d7e13cb..a147d9c 100644
@@ -182,10 +182,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		    && !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) {
+		if (!changed && !dirty_submodule) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 				continue;

Junio C Hamano

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

Quoted 8 lines · Junio C Hamano

Quoted 2 lines · Jens Lehmann
Another way to fix this bug would have been to enter the correct sha1 into two->sha1 for submodules with new commits.
I didn't follow all the codepaths involved, but it might be worth a try. The beginning of the patch may look something like this, and then you would probably have to fix potential fallouts in diff.c from this change.

Here is a fix of one of the fallouts.

 diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff.c +2 -0

index dfdfa1a..eefe836 100644
@@ -3221,6 +3221,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 {

Jens Lehmann

author

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

PATCH v2 1/1 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 <[email protected]>

---

Am 11.03.2010 21:16, schrieb Junio C Hamano:

Quoted 4 lines

Quoted 2 lines · Junio C Hamano
Jens Lehmann <[email protected]> writes:

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Another way to fix this bug would have been to enter the correct sha1 into two->sha1 for submodules with new commits.
I didn't follow all the codepaths involved, but it might be worth a try. The beginning of the patch may look something like this, and then you would probably have to fix potential fallouts in diff.c from this change.
Here is a fix of one of the fallouts.

Thanks for the pointers, here is version 2. This patch applies on top of the "Refactor dirty submodule detection in diff-lib.c" patch I sent recently. A few more eyeballs to make sure I touched all the relevant codepaths would be highly appreciated.

 diff-lib.c                  |    6 +--
 diff.c                      |    7 +++-
 t/t7506-status-submodule.sh |   84 +++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 88 insertions(+), 9 deletions(-)

diff-lib.c +2 -4

index 64be827..6d4922a 100644
@@ -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.c +5 -2

index 5aefdcb..f5d93e9 100644
@@ -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,

@@ -3092,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. */

@@ -3227,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 {

t/t7506-status-submodule.sh +81 -3

108 lines not on this page · raw message

Signature · 1 line

1.7.0.2.386.ge30d.dirty