[PATCH v2 0/2] Show submodules as modified when they contain a dirty work tree

[PATCH v2 0/2] Show submodules as modified when they contain a dirty work tree

[Jens Lehmann]

Here is the updated version of the two patches.

This fixes all issues raised and passes the test suite.


Jens Lehmann (1):
  Show submodules as modified when they contain a dirty work tree

Junio C Hamano (1):
  Teach diff that modified submodule directory is dirty

 diff-lib.c                  |    8 +++-
 diff.c                      |    9 ++++-
 submodule.c                 |   49 +++++++++++++++++++++++++
 submodule.h                 |    1 +
 t/t4027-diff-submodule.sh   |   84 ++++++++++++++++++++++++++++++++++++++++++-
 t/t7506-status-submodule.sh |   83 ++++++++++++++++++++++++++++++++++--------
 6 files changed, 214 insertions(+), 20 deletions(-)

[PATCH v2 1/2] Show submodules as modified when they contain a dirty work tree

[Jens Lehmann]

Until now a submodule only then showed up as modified in the supermodule
when the last commit in the submodule differed from the one in the index
or the diffed against commit of the superproject. A dirty work tree
containing new untracked or modified files in a submodule was
undetectable when looking at it from the superproject.

Now git status and git diff (against the work tree) in the superproject
will also display submodules as modified when they contain untracked or
modified files, even if the compared ref matches the HEAD of the
submodule.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---

Changed since the last version:
- Added missing include (spotted by Junio)
- Made test 7506 more robust (thanks to Nanako)
- When diffing the work tree against a commit in the supermodule the
  submodule did not show up as modified when the refs matched


 diff-lib.c                  |    8 +++-
 submodule.c                 |   49 +++++++++++++++++++++++++
 submodule.h                 |    1 +
 t/t7506-status-submodule.sh |   83 +++++++++++++++++++++++++++++++++++--------
 4 files changed, 124 insertions(+), 17 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 274ff8c..29c5915 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -10,6 +10,7 @@
 #include "cache-tree.h"
 #include "unpack-trees.h"
 #include "refs.h"
+#include "submodule.h"

 /*
  * diff-files
@@ -159,7 +160,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 		}

-		if (ce_uptodate(ce) || ce_skip_worktree(ce))
+		if ((ce_uptodate(ce) && !S_ISGITLINK(ce->ce_mode)) || ce_skip_worktree(ce))
 			continue;

 		/* If CE_VALID is set, don't look at workdir for file removal */
@@ -176,6 +177,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			continue;
 		}
 		changed = ce_match_stat(ce, &st, ce_option);
+		if (S_ISGITLINK(ce->ce_mode) && !changed)
+			changed = is_submodule_modified(ce->name);
 		if (!changed) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
@@ -230,7 +233,8 @@ static int get_stat_data(struct cache_entry *ce,
 			return -1;
 		}
 		changed = ce_match_stat(ce, &st, 0);
-		if (changed) {
+		if (changed
+		    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			sha1 = null_sha1;
 		}
diff --git a/submodule.c b/submodule.c
index 86aad65..3f851de 100644
--- a/submodule.c
+++ b/submodule.c
@@ -4,6 +4,7 @@
 #include "diff.h"
 #include "commit.h"
 #include "revision.h"
+#include "run-command.h"

 int add_submodule_odb(const char *path)
 {
@@ -112,3 +113,51 @@ void show_submodule_summary(FILE *f, const char *path,
 	}
 	strbuf_release(&sb);
 }
+
+int is_submodule_modified(const char *path)
+{
+	int len;
+	struct child_process cp;
+	const char *argv[] = {
+		"status",
+		"--porcelain",
+		NULL,
+	};
+	char *env[3];
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "%s/.git/", path);
+	if (!is_directory(buf.buf)) {
+		strbuf_release(&buf);
+		/* The submodule is not checked out, so it is not modified */
+		return 0;
+
+	}
+	strbuf_reset(&buf);
+
+	strbuf_addf(&buf, "GIT_WORK_TREE=%s", path);
+	env[0] = strbuf_detach(&buf, NULL);
+	strbuf_addf(&buf, "GIT_DIR=%s/.git", path);
+	env[1] = strbuf_detach(&buf, NULL);
+	env[2] = NULL;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.argv = argv;
+	cp.env = (const char *const *)env;
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.out = -1;
+	if (start_command(&cp))
+		die("Could not run git status --porcelain");
+
+	len = strbuf_read(&buf, cp.out, 1024);
+	close(cp.out);
+
+	if (finish_command(&cp))
+		die("git status --porcelain failed");
+
+	free(env[0]);
+	free(env[1]);
+	strbuf_release(&buf);
+	return len != 0;
+}
diff --git a/submodule.h b/submodule.h
index 4c0269d..0773121 100644
--- a/submodule.h
+++ b/submodule.h
@@ -4,5 +4,6 @@
 void show_submodule_summary(FILE *f, const char *path,
 		unsigned char one[20], unsigned char two[20],
 		const char *del, const char *add, const char *reset);
+int is_submodule_modified(const char *path);

 #endif
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 3ca17ab..253c334 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -5,34 +5,87 @@ test_description='git status for submodule'
 . ./test-lib.sh

 test_expect_success 'setup' '
-	test_create_repo sub
-	cd sub &&
-	: >bar &&
-	git add bar &&
-	git commit -m " Add bar" &&
-	cd .. &&
-	git add sub &&
+	test_create_repo sub &&
+	(
+		cd sub &&
+		: >bar &&
+		git add bar &&
+		git commit -m " Add bar" &&
+		: >foo &&
+		git add foo &&
+		git commit -m " Add foo"
+	) &&
+	echo output > .gitignore &&
+	git add sub .gitignore &&
 	git commit -m "Add submodule sub"
 '

 test_expect_success 'status clean' '
-	git status |
-	grep "nothing to commit"
+	git status >output &&
+	grep "nothing to commit" output
 '
+
 test_expect_success 'commit --dry-run -a clean' '
-	git commit --dry-run -a |
-	grep "nothing to commit"
+	test_must_fail git commit --dry-run -a >output &&
+	grep "nothing to commit" output
+'
+
+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
+'
+
+test_expect_success 'status with modified file in 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 submodule' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	git status >output &&
+	grep "modified:   sub" output
+'
+
+test_expect_success 'status with added file in 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 submodule' '
+	(cd sub && git reset --hard) &&
+	echo "content" >sub/new-file &&
+	git status >output &&
+	grep "modified:   sub" output
+'
+
+test_expect_success 'status with untracked file in submodule (porcelain)' '
+	git status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub
+	EOF
 '
+
 test_expect_success 'rm submodule contents' '
 	rm -rf sub/* sub/.git
 '
+
 test_expect_success 'status clean (empty submodule dir)' '
-	git status |
-	grep "nothing to commit"
+	git status >output &&
+	grep "nothing to commit" output
 '
+
 test_expect_success 'status -a clean (empty submodule dir)' '
-	git commit --dry-run -a |
-	grep "nothing to commit"
+	test_must_fail git commit --dry-run -a >output &&
+	grep "nothing to commit" output
 '

 test_done
-- 
1.6.6.332.g1985b

[PATCH v2 2/2] Teach diff that modified submodule directory is dirty

[Jens Lehmann]

From: Junio C Hamano <gitster@pobox.com>

A diff run in superproject only compares the name of the commit object
bound at the submodule paths.  When we compare with a work tree and the
checked out submodule directory is dirty (e.g. has either staged or
unstaged changes, or has new files the user forgot to add to the index),
show the work tree side as "dirty".

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

Changed since the last version:
- I added 3 tests for "git diff HEAD" when refs match (the HEAD in the
  submodule is the same as last committed in the superproject)


 diff.c                    |    9 ++++-
 t/t4027-diff-submodule.sh |   84 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 5060b48..012b3d3 100644
--- a/diff.c
+++ b/diff.c
@@ -2029,9 +2029,14 @@ static int populate_from_stdin(struct diff_filespec *s)
 static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
 {
 	int len;
-	char *data = xmalloc(100);
+	char *data = xmalloc(100), *dirty = "";
+
+	/* Are we looking at the work tree? */
+	if (!s->sha1_valid && is_submodule_modified(s->path))
+		dirty = "-dirty";
+
 	len = snprintf(data, 100,
-		"Subproject commit %s\n", sha1_to_hex(s->sha1));
+		       "Subproject commit %s%s\n", sha1_to_hex(s->sha1), dirty);
 	s->data = data;
 	s->size = len;
 	s->should_free = 1;
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 5cf8924..83c1914 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -32,7 +32,8 @@ test_expect_success setup '
 		cd sub &&
 		git rev-list HEAD
 	) &&
-	echo ":160000 160000 $3 $_z40 M	sub" >expect
+	echo ":160000 160000 $3 $_z40 M	sub" >expect &&
+	subtip=$3 subprev=$2
 '

 test_expect_success 'git diff --raw HEAD' '
@@ -50,6 +51,87 @@ test_expect_success 'git diff-files --raw' '
 	test_cmp expect actual.files
 '

+expect_from_to () {
+	printf "%sSubproject commit %s\n+Subproject commit %s\n" \
+		"-" "$1" "$2"
+}
+
+test_expect_success 'git diff HEAD' '
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (work tree)' '
+	echo >>sub/world &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev-dirty &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (index)' '
+	(
+		cd sub &&
+		git reset --hard &&
+		echo >>world &&
+		git add world
+	) &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev-dirty &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (untracked)' '
+	(
+		cd sub &&
+		git reset --hard &&
+		git clean -qfdx &&
+		>cruft
+	) &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subtip $subprev-dirty &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)' '
+	git commit -m "x" sub &&
+	echo >>sub/world &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subprev $subprev-dirty &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (index, refs match)' '
+	(
+		cd sub &&
+		git reset --hard &&
+		echo >>world &&
+		git add world
+	) &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subprev $subprev-dirty &&
+	test_cmp expect.body actual.body
+'
+
+test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' '
+	(
+		cd sub &&
+		git reset --hard &&
+		git clean -qfdx &&
+		>cruft
+	) &&
+	git diff HEAD >actual &&
+	sed -e "1,/^@@/d" actual >actual.body &&
+	expect_from_to >expect.body $subprev $subprev-dirty &&
+	test_cmp expect.body actual.body
+'
+
 test_expect_success 'git diff (empty submodule dir)' '
 	: >empty &&
 	rm -rf sub/* sub/.git &&
-- 
1.6.6.332.g1985b