short status: improve reporting for submodule changes

This comes as a series; first I'd like to refactor is_submodule_modified to take advantage of the new porcelain=2 plumbing switch to check for changes in the submodule.

On top of the refactoring comes the actual change, which moved the rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (3):
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c, is_submodule_modified: stricter checking for submodules short status: improve reporting for submodule changes

 Documentation/git-status.txt |  9 +++++++
 submodule.c                  | 58 +++++++++++++++++++++-----------------------
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 24 ++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 5 files changed, 84 insertions(+), 38 deletions(-)

2.12.1.432.gfe308fe33c.dirty

Migrate 'is_submodule_modified' to the new porcelain format of git-status.

As the old porcelain only reported ' M' for submodules, no matter what happened inside the submodule (untracked files, changes to tracked files or move of HEAD), the new API properly reports the different scenarios.

In a followup patch we will make use of these finer grained reporting for git-status.

---
 submodule.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

index 3200b7bb2b..d355ddb46b 100644
@@ -1041,17 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		NULL,
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
 
 	strbuf_addf(&buf, "%s/.git", path);

@@ -1066,42 +1058,45 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
+	argv_array_pushl(&cp.args, "status", "--porcelain=2", NULL);
 	if (ignore_untracked)
-		argv[2] = "-uno";
+		argv_array_push(&cp.args, "-uno");
 
-	cp.argv = argv;
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+		/* regular untracked files */
+		if (buf.buf[0] == '?')
 			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;
+
+		/* regular unmerged and renamed files */
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			if (buf.buf[5] == 'S') {
+				/* nested submodule handling */
+				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+				if (buf.buf[8] == 'U')
+					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			} else
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}
-		next_line = strchr(line, '\n');
-		if (!next_line)
-			break;
-		next_line++;
-		len -= (next_line - line);
-		line = next_line;
+
+		if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&
+		    dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+				break;
 	}
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.1.432.gfe308fe33c.dirty

[...]

Neat. Is this something that could be covered in tests, or should I be patient and rely on patch 3/3 for that?

I think this would be easier to understand if it were two patches: one that switched to --porcelain=2 with no change in behavior, and another that took advantage of --porcelain=2 to return richer information. As is, I had trouble verifying that this isn't going to break anything --- there's not enough local information here and in submodule.h to tell what callers may rely on and I didn't audit callers.

Thanks and hope that helps,
Jonathan

That sounds like a sensible organization.

I am not sure how to cover it in tests properly, as we do not expose this function to the outside directly.

This function is used in only one place (diff-lib.c, in match_stat_with_submodule,
which itself is used in run_diff_files and get_stat_data), which is deep down in the diff library.

I don't think so as it would double the code to review. I'll see if I can present this conversion in an easier way.

ok.

Thanks,
Stefan

By having a stricter check in the superproject we catch errors earlier, instead of spawning a child process to tell us.

---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index d355ddb46b..66335159ae 100644
@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	git_dir = read_gitfile(buf.buf);
 	if (!git_dir)
 		git_dir = buf.buf;
-	if (!is_directory(git_dir)) {
+	if (!is_git_directory(git_dir)) {
+		if (is_directory(git_dir))
+			die(_("'%s' not recognized as a git repository"), git_dir);
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
-
 	}
 	strbuf_reset(&buf);
 

2.12.1.432.gfe308fe33c.dirty

Plus the error message is clearer.

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |  9 +++++++++
 t/t3600-rm.sh                | 18 +++++++++++++-----
 t/t7506-status-submodule.sh  | 24 ++++++++++++++++++++++++
 wt-status.c                  | 13 +++++++++++--
 4 files changed, 57 insertions(+), 7 deletions(-)

index ba873657cf..01b457c322 100644
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..b58793448b 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index d31b34da83..98dff01947 100755
@@ -50,6 +50,15 @@ test_expect_success 'status with modified file in submodule (porcelain)' '
 	EOF
 '
 
+test_expect_success 'status with modified file in submodule (short)' '
+	(cd sub && git reset --hard) &&
+	echo "changed" >sub/foo &&
+	git status --short >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 &&

@@ -64,6 +73,14 @@ test_expect_success 'status with added file in submodule (porcelain)' '
 	EOF
 '
 
+test_expect_success 'status with added file in submodule (short)' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	git status --short >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 &&

@@ -83,6 +100,13 @@ test_expect_success 'status with untracked file in submodule (porcelain)' '
 	EOF
 '
 
+test_expect_success 'status with untracked file in submodule (short)' '
+	git status --short >output &&
+	diff output - <<-\EOF
+	 ? sub
+	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 &&

index 308cf3779e..9909fd0e57 100644
@@ -431,10 +431,19 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT) {
+				if (d->new_submodule_commits)
+					d->worktree_status = 'M';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+					d->worktree_status = 'm';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+					d->worktree_status = '?';
+			}
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.1.432.gfe308fe33c.dirty

Very nice.

[...]

v4:
* broken down in a lot of tiny patches.

This part is in the latest patch now.

Thanks,
Stefan

v3:
This comes as a series; first I'd like to refactor is_submodule_modified to take advantage of the new porcelain=2 plumbing switch to check for changes in the submodule.

On top of the refactoring comes the actual change, which moved the rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (8):
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: use argv_array in is_submodule_modified submodule.c: convert is_submodule_modified to use
strbuf_getwholeline_fd
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: factor out early loop termination in
is_submodule_modified
submodule.c: stricter checking for submodules in is_submodule_modified short status: improve reporting for submodule changes submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  9 +++++++
 submodule.c                  | 56 ++++++++++++++++++++-----------------------
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 5 files changed, 116 insertions(+), 37 deletions(-)

2.12.1.438.gb674c4c09c

Migrate 'is_submodule_modified' to the new porcelain format of git-status.

As the old porcelain only reported ' M' for submodules, no matter what happened inside the submodule (untracked files, changes to tracked files or move of HEAD), the new API properly reports the different scenarios.

In a followup patch we will make use of these finer grained reporting for git-status.

---
 submodule.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

index 3200b7bb2b..d355ddb46b 100644
@@ -1041,17 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		NULL,
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
 
 	strbuf_addf(&buf, "%s/.git", path);

@@ -1066,42 +1058,45 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
+	argv_array_pushl(&cp.args, "status", "--porcelain=2", NULL);
 	if (ignore_untracked)
-		argv[2] = "-uno";
+		argv_array_push(&cp.args, "-uno");
 
-	cp.argv = argv;
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+		/* regular untracked files */
+		if (buf.buf[0] == '?')
 			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;
+
+		/* regular unmerged and renamed files */
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			if (buf.buf[5] == 'S') {
+				/* nested submodule handling */
+				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+				if (buf.buf[8] == 'U')
+					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			} else
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}
-		next_line = strchr(line, '\n');
-		if (!next_line)
-			break;
-		next_line++;
-		len -= (next_line - line);
-		line = next_line;
+
+		if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&
+		    dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+				break;
 	}
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.1.438.gb674c4c09c

struct argv_array is easier to use and maintain

---
 submodule.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

index d355ddb46b..dc61ca7103 100644
@@ -1041,9 +1041,11 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
+	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
+	const char *line, *next_line;
 	const char *git_dir;
 
 	strbuf_addf(&buf, "%s/.git", path);

@@ -1068,35 +1070,32 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain=2' in submodule %s", path);
+		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
-		/* regular untracked files */
-		if (buf.buf[0] == '?')
+	len = strbuf_read(&buf, cp.out, 1024);
+	line = buf.buf;
+	while (len > 2) {
+		if ((line[0] == '?') && (line[1] == '?')) {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-
-		/* regular unmerged and renamed files */
-		if (buf.buf[0] == 'u' ||
-		    buf.buf[0] == '1' ||
-		    buf.buf[0] == '2') {
-			if (buf.buf[5] == 'S') {
-				/* nested submodule handling */
-				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
-					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-				if (buf.buf[8] == 'U')
-					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			} else
-				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-		}
-
-		if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&
-		    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))
-		die("'git status --porcelain=2' failed in submodule %s", path);
+		die("'git status --porcelain' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.1.438.gb674c4c09c

Instead of implementing line reading yet again, make use of our beautiful library functions.

---
 submodule.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

index dc61ca7103..f01e84faff 100644
@@ -1041,11 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
 
 	strbuf_addf(&buf, "%s/.git", path);

@@ -1072,10 +1070,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (start_command(&cp))
 		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+		if ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
 				break;

@@ -1085,12 +1081,6 @@ unsigned is_submodule_modified(const char *path, int 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);
 

2.12.1.438.gb674c4c09c

Migrate 'is_submodule_modified' to the new porcelain format of git-status. This conversion attempts to convert faithfully, i.e. the behavior ought to be exactly the same.

As the output in the parsing only distinguishes between untracked files and the rest, this is easy to port to the new format, as we only need to identify untracked files and the rest is handled in the "else" case.

untracked files are indicated by only a single question mark instead of two question marks, so the conversion is easy.

---
 submodule.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index f01e84faff..da1db90dda 100644
@@ -1068,10 +1068,11 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
 	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
-		if ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {
+		/* regular untracked files */
+		if (buf.buf[0] == '?') {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
 				break;

@@ -1085,7 +1086,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.1.438.gb674c4c09c

This makes it easier for a follow up patch.

---
 submodule.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

index da1db90dda..93d6f08b50 100644
@@ -1072,16 +1072,14 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 
 	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
 		/* regular untracked files */
-		if (buf.buf[0] == '?') {
+		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-				break;
-		} else {
+		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-			if (ignore_untracked ||
-			    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-				break;
-		}
+
+		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))
+			break;
 	}
 	close(cp.out);
 

2.12.1.438.gb674c4c09c

By having a stricter check in the superproject we catch errors earlier, instead of spawning a child process to tell us.

---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 93d6f08b50..e06e52b993 100644
@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	git_dir = read_gitfile(buf.buf);
 	if (!git_dir)
 		git_dir = buf.buf;
-	if (!is_directory(git_dir)) {
+	if (!is_git_directory(git_dir)) {
+		if (is_directory(git_dir))
+			die(_("'%s' not recognized as a git repository"), git_dir);
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
-
 	}
 	strbuf_reset(&buf);
 

2.12.1.438.gb674c4c09c

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |  9 +++++++
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 4 files changed, 90 insertions(+), 7 deletions(-)

index ba873657cf..01b457c322 100644
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..a6e5c5bd56 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 308cf3779e..9909fd0e57 100644
@@ -431,10 +431,19 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT) {
+				if (d->new_submodule_commits)
+					d->worktree_status = 'M';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+					d->worktree_status = 'm';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+					d->worktree_status = '?';
+			}
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.1.438.gb674c4c09c

---
 submodule.c                 | 16 ++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

index e06e52b993..0f477f3a4e 100644
@@ -1075,8 +1075,20 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		/* regular untracked files */
 		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-		else
-			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+
+		/* regular unmerged and renamed files */
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			if (buf.buf[5] == 'S') {
+				/* nested submodule handling */
+				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+				if (buf.buf[8] == 'U')
+					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			} else
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+		}
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
 		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))

index a6e5c5bd56..b58793448b 100755
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified_inside actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index ad46384064..e3cdcede72 100755
@@ -324,7 +324,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'
 test_expect_success 'status with untracked file in nested submodule (short)' '
 	git -C super status --short >output &&
 	diff output - <<-\EOF
-	 m sub1
+	 ? sub1
 	EOF
 '
 

2.12.1.438.gb674c4c09c

except for the first 2 patches, which are a rebase error :(

Please ignore.

v5:
* fixed rebase error in the first 2 patches
* the last 3 patches introduce behavior change outside the scope of is_modified_submodule whereas the first 4 ought to just be local changes.

Thanks,
Stefan

v4:
* broken down in a lot of tiny patches.

This part is in the latest patch now.

Thanks,
Stefan

v3:
This comes as a series; first I'd like to refactor is_submodule_modified to take advantage of the new porcelain=2 plumbing switch to check for changes in the submodule.

On top of the refactoring comes the actual change, which moved the rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (8):
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: use argv_array in is_submodule_modified submodule.c: convert is_submodule_modified to use
strbuf_getwholeline_fd
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: factor out early loop termination in
is_submodule_modified
submodule.c: stricter checking for submodules in is_submodule_modified short status: improve reporting for submodule changes submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  9 +++++++
 submodule.c                  | 56 ++++++++++++++++++++-----------------------
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 5 files changed, 116 insertions(+), 37 deletions(-)

2.12.1.438.gb674c4c09c

struct argv_array is easier to use and maintain

---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

index 3200b7bb2b..2c667ac95a 100644
@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		NULL,
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
 	const char *line, *next_line;

@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
+	argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
 	if (ignore_untracked)
-		argv[2] = "-uno";
+		argv_array_push(&cp.args, "-uno");
 
-	cp.argv = argv;
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

2.12.1.438.gb674c4c09c

Yes!

[...]

I also like the diffstat. :)

[...]

Instead of implementing line reading yet again, make use of our beautiful library functions.

---
 submodule.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

index 2c667ac95a..c1b7b78260 100644
@@ -1041,11 +1041,9 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
 
 	strbuf_addf(&buf, "%s/.git", path);

@@ -1072,10 +1070,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (start_command(&cp))
 		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+		if ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
 				break;

@@ -1085,12 +1081,6 @@ unsigned is_submodule_modified(const char *path, int 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);
 

2.12.1.438.gb674c4c09c

This changes buffering behavior in two ways:

- by using strbuf_getwholeline_fd instead of strbuf_read, we avoid having to allocate memory for the entire child process output at once. That is, we limit maximum memory usage (good).

- by using strbuf_getwholeline_fd instead of strbuf_read, we xread one byte at a time instead of larger chunks. That means more overhead due to context switches (bad).

Some callers of getwholeline_fd need the one-byte-at-a-time thing to avoid waiting too long for input, and in some cases the alternative is deadlock. We know this caller doesn't fall into that category because it was doing fine slurping the entire file at once. As the getwholeline_fd API doc comment explains:

* It reads one character at a time, so it is very slow. Do not
* use it unless you need the correct position in the file
* descriptor.

Can this caller use xfdopen and strbuf_getwholeline instead to get back the benefit of buffering (i.e., something like the below)?

Another consequence of switching to streaming is that we may close before the child finishes. Do we have to worry about handling SIGPIPE in the child? I haven't checked how this handles that --- a test might be useful.

Thanks and hope that helps,
Jonathan

index c1b7b78260..184d5739fc 100644
@@ -1043,6 +1043,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
+	FILE *fp;
 	unsigned dirty_submodule = 0;
 	const char *git_dir;
 

@@ -1070,7 +1071,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (start_command(&cp))
 		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+	fp = xfdopen(cp.out, "r");
+	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
 		if ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)

@@ -1082,7 +1084,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 				break;
 		}
 	}
-	close(cp.out);
+	fclose(fp);
 
 	if (finish_command(&cp))
 		die("'git status --porcelain' failed in submodule %s", path);

Thanks for careful reading!

The code below makes sense to me.

We could have had closing before the child finished before as well:
* the first read happens with strbuf_read(&buf, cp.out, 1024);
* it contains a line indicating a modified file
* The condition breaks out of while:

        if (ignore_untracked ||
            (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
                break;

* after the while loop we have the close();
* but we only had one read, which is not the complete output of the child.

This patch doesn't make it worse in that respect.

Thanks,
Stefan

The 1024 is only a hint. So it actually reads the output in full.

So I guess I'll come up with a test for SIGPIPE, then.

Migrate 'is_submodule_modified' to the new porcelain format of git-status. This conversion attempts to convert faithfully, i.e. the behavior ought to be exactly the same.

As the output in the parsing only distinguishes between untracked files and the rest, this is easy to port to the new format, as we only need to identify untracked files and the rest is handled in the "else" case.

untracked files are indicated by only a single question mark instead of two question marks, so the conversion is easy.

---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index c1b7b78260..da1db90dda 100644
@@ -1058,7 +1058,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
-	argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+	argv_array_pushl(&cp.args, "status", "--porcelain=2", NULL);
 	if (ignore_untracked)
 		argv_array_push(&cp.args, "-uno");
 

@@ -1068,10 +1068,11 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
 	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
-		if ((buf.buf[0] == '?') && (buf.buf[1] == '?')) {
+		/* regular untracked files */
+		if (buf.buf[0] == '?') {
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
 				break;

@@ -1085,7 +1086,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.1.438.gb674c4c09c

This makes it easier for a follow up patch.

---
 submodule.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

index da1db90dda..93d6f08b50 100644
@@ -1072,16 +1072,14 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 
 	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
 		/* regular untracked files */
-		if (buf.buf[0] == '?') {
+		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-				break;
-		} else {
+		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-			if (ignore_untracked ||
-			    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-				break;
-		}
+
+		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))
+			break;
 	}
 	close(cp.out);
 

2.12.1.438.gb674c4c09c

By having a stricter check in the superproject we catch errors earlier, instead of spawning a child process to tell us.

---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 93d6f08b50..e06e52b993 100644
@@ -1050,11 +1050,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	git_dir = read_gitfile(buf.buf);
 	if (!git_dir)
 		git_dir = buf.buf;
-	if (!is_directory(git_dir)) {
+	if (!is_git_directory(git_dir)) {
+		if (is_directory(git_dir))
+			die(_("'%s' not recognized as a git repository"), git_dir);
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
-
 	}
 	strbuf_reset(&buf);
 

2.12.1.438.gb674c4c09c

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |  9 +++++++
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 4 files changed, 90 insertions(+), 7 deletions(-)

index ba873657cf..01b457c322 100644
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..a6e5c5bd56 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 308cf3779e..9909fd0e57 100644
@@ -431,10 +431,19 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT) {
+				if (d->new_submodule_commits)
+					d->worktree_status = 'M';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+					d->worktree_status = 'm';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+					d->worktree_status = '?';
+			}
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.1.438.gb674c4c09c

v6:
* kill the child once we know all information that we ask for, as an optimization
* reordered the patches for that
* strbuf_getwholeline instead of its _fd version.

v5:
* fixed rebase error in the first 2 patches
* the last 3 patches introduce behavior change outside the scope of is_modified_submodule whereas the first 4 ought to just be local changes.

Thanks,
Stefan

v4:
* broken down in a lot of tiny patches.

This part is in the latest patch now.

Thanks,
Stefan

v3:
This comes as a series; first I'd like to refactor is_submodule_modified to take advantage of the new porcelain=2 plumbing switch to check for changes in the submodule.

On top of the refactoring comes the actual change, which moved the rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (8):
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: use argv_array in is_submodule_modified submodule.c: convert is_submodule_modified to use
strbuf_getwholeline_fd
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: factor out early loop termination in
is_submodule_modified
submodule.c: stricter checking for submodules in is_submodule_modified short status: improve reporting for submodule changes submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  9 +++++++
 submodule.c                  | 56 ++++++++++++++++++++-----------------------
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 5 files changed, 116 insertions(+), 37 deletions(-)

2.12.1.438.gb674c4c09c

struct argv_array is easier to use and maintain

---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

index 3200b7bb2b..2c667ac95a 100644
@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		NULL,
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
 	const char *line, *next_line;

@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
+	argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
 	if (ignore_untracked)
-		argv[2] = "-uno";
+		argv_array_push(&cp.args, "-uno");
 
-	cp.argv = argv;
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

2.12.1.437.g2b7623d507

Missing '.' at end of sentence.

With or without that tweak, I still like this as much as last time. :)

Thanks.

This makes it easier for a follow up patch.

---
 submodule.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

index 2c667ac95a..e52cb8a958 100644
@@ -1075,16 +1075,15 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	len = strbuf_read(&buf, cp.out, 1024);
 	line = buf.buf;
 	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+		if ((line[0] == '?') && (line[1] == '?'))
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-				break;
-		} else {
+		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-			if (ignore_untracked ||
-			    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-				break;
-		}
+
+		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))
+			break;
+
 		next_line = strchr(line, '\n');
 		if (!next_line)
 			break;

2.12.1.437.g2b7623d507

Instead of implementing line reading yet again, make use of our beautiful library function to read one line. By using strbuf_getwholeline instead of strbuf_read, we avoid having to allocate memory for the entire child process output at once. That is, we limit maximum memory usage. Once we know all information that we care about, we can terminate the child early. In that case we do not care about its exit code as well.

---
 submodule.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

index e52cb8a958..0c43f9f2b1 100644
@@ -1041,12 +1041,12 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
+	FILE *fp;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
+	int ignore_cp_exit_code = 0;
 
 	strbuf_addf(&buf, "%s/.git", path);
 	git_dir = read_gitfile(buf.buf);

@@ -1072,28 +1072,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (start_command(&cp))
 		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?'))
+	fp = xfdopen(cp.out, "r");
+	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
+		if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
-		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))
-			break;
-
-		next_line = strchr(line, '\n');
-		if (!next_line)
+		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked)) {
+			/*
+			 * We're not interested in any further information from
+			 * the child any more, no output nor its exit code.
+			 */
+			kill(cp.pid, SIGTERM);
+			ignore_cp_exit_code = 1;
 			break;
-		next_line++;
-		len -= (next_line - line);
-		line = next_line;
+		}
 	}
-	close(cp.out);
+	fclose(fp);
 
-	if (finish_command(&cp))
+	if (finish_command(&cp) && !ignore_cp_exit_code)
 		die("'git status --porcelain' failed in submodule %s", path);
 
 	strbuf_release(&buf);

2.12.1.437.g2b7623d507

It also overlaps work a little better.

Should this say something about SIGPIPE?

[...]

[...]

[...]

mentioned

done

done

done

done

Migrate 'is_submodule_modified' to the new porcelain format of git-status. This conversion attempts to convert faithfully, i.e. the behavior ought to be exactly the same.

As the output in the parsing only distinguishes between untracked files and the rest, this is easy to port to the new format, as we only need to identify untracked files and the rest is handled in the "else" case.

untracked files are indicated by only a single question mark instead of two question marks, so the conversion is easy.

---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index 0c43f9f2b1..256f15fde1 100644
@@ -1060,7 +1060,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
-	argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+	argv_array_pushl(&cp.args, "status", "--porcelain=2", NULL);
 	if (ignore_untracked)
 		argv_array_push(&cp.args, "-uno");
 

@@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
 	fp = xfdopen(cp.out, "r");
 	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
-		if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
+		/* regular untracked files */
+		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

@@ -1093,7 +1094,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	fclose(fp);
 
 	if (finish_command(&cp) && !ignore_cp_exit_code)
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.1.437.g2b7623d507

Stefan Beller wrote:

[...]

[...]

[...]

By having a stricter check in the superproject we catch errors earlier, instead of spawning a child process to tell us.

---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 256f15fde1..467f1de763 100644
@@ -1052,11 +1052,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	git_dir = read_gitfile(buf.buf);
 	if (!git_dir)
 		git_dir = buf.buf;
-	if (!is_directory(git_dir)) {
+	if (!is_git_directory(git_dir)) {
+		if (is_directory(git_dir))
+			die(_("'%s' not recognized as a git repository"), git_dir);
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
-
 	}
 	strbuf_reset(&buf);
 

2.12.1.437.g2b7623d507

Yep. :)

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |  9 +++++++
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 4 files changed, 90 insertions(+), 7 deletions(-)

index ba873657cf..01b457c322 100644
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..a6e5c5bd56 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 308cf3779e..9909fd0e57 100644
@@ -431,10 +431,19 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT) {
+				if (d->new_submodule_commits)
+					d->worktree_status = 'M';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+					d->worktree_status = 'm';
+				else if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+					d->worktree_status = '?';
+			}
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.1.437.g2b7623d507

When a nested submodule has untracked files, it would be reported as "modified submodule" in the superproject, because submodules are not parsed correctly in is_submodule_modified as they are bucketed into the modified pile as "they are not an untracked file".

---
 submodule.c                 | 16 ++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

index 467f1de763..ec7e9b1b06 100644
@@ -1078,8 +1078,20 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		/* regular untracked files */
 		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-		else
-			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+
+		/* regular unmerged and renamed files */
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			if (buf.buf[5] == 'S') {
+				/* nested submodule handling */
+				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+				if (buf.buf[8] == 'U')
+					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			} else
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+		}
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
 		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked)) {

index a6e5c5bd56..b58793448b 100755
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified_inside actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index ad46384064..e3cdcede72 100755
@@ -324,7 +324,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'
 test_expect_success 'status with untracked file in nested submodule (short)' '
 	git -C super status --short >output &&
 	diff output - <<-\EOF
-	 m sub1
+	 ? sub1
 	EOF
 '
 

2.12.1.437.g2b7623d507

good idea, will use.

That means there is a submodule with no new commits, no modified files and no untracked files. ("Why did we even output this?", oh because of it is in either 2 (moved) or because the hashes are different, i.e. we already added the new HEAD of the submodule)

ok, will do.

v7:
* taken all of Jonathan minor nits, so patch 1..6 should be good to go
* patch 7 lacks tests and documentation (according to Jonathan...) but as it is the last patch, just fixing a minor detail we can leave it off.

Junio, please take patch 1-6 as usual, I will be out until next Wednesday.

Thanks,
Stefan

v6:
* kill the child once we know all information that we ask for, as an optimization
* reordered the patches for that
* strbuf_getwholeline instead of its _fd version.

v5:
* fixed rebase error in the first 2 patches
* the last 3 patches introduce behavior change outside the scope of is_modified_submodule whereas the first 4 ought to just be local changes.

Thanks,
Stefan

v4:
* broken down in a lot of tiny patches.

This part is in the latest patch now.

Thanks,
Stefan

v3:
This comes as a series; first I'd like to refactor is_submodule_modified to take advantage of the new porcelain=2 plumbing switch to check for changes in the submodule.

On top of the refactoring comes the actual change, which moved the rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (8):
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: use argv_array in is_submodule_modified submodule.c: convert is_submodule_modified to use
strbuf_getwholeline_fd
submodule.c: port is_submodule_modified to use porcelain 2 submodule.c: factor out early loop termination in
is_submodule_modified
submodule.c: stricter checking for submodules in is_submodule_modified short status: improve reporting for submodule changes submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  9 +++++++
 submodule.c                  | 56 ++++++++++++++++++++-----------------------
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 13 ++++++++--
 5 files changed, 116 insertions(+), 37 deletions(-)

2.12.1.438.gb674c4c09c

struct argv_array is easier to use and maintain.

---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

index 3200b7bb2b..2c667ac95a 100644
@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
 	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		NULL,
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
 	const char *line, *next_line;

@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
+	argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
 	if (ignore_untracked)
-		argv[2] = "-uno";
+		argv_array_push(&cp.args, "-uno");
 
-	cp.argv = argv;
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;

2.12.0.rc1.49.gdeb397943c.dirty

This makes it easier for a follow up patch.

---
 submodule.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index 2c667ac95a..93e3fefd39 100644
@@ -1075,16 +1075,16 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	len = strbuf_read(&buf, cp.out, 1024);
 	line = buf.buf;
 	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+		if ((line[0] == '?') && (line[1] == '?'))
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-				break;
-		} else {
+		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-			if (ignore_untracked ||
-			    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-				break;
-		}
+
+		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
+		     ignore_untracked))
+			break;
+
 		next_line = strchr(line, '\n');
 		if (!next_line)
 			break;

2.12.0.rc1.49.gdeb397943c.dirty

Instead of implementing line reading yet again, make use of our beautiful library function to read one line. By using strbuf_getwholeline instead of strbuf_read, we avoid having to allocate memory for the entire child process output at once. That is, we limit maximum memory usage. Also we can start processing the output as it comes in, no need to wait for all of it.

Once we know all information that we care about, we can terminate the child early. In that case we do not care about its exit code as well. By just closing our side of the pipe the child process will get a SIGPIPE signal, which it will not report nor do we report it in finish_command, ac78663b0d (run-command: don't warn on SIGPIPE deaths, 2015-12-29).

---
 submodule.c                 | 30 ++++++++++++++----------------
 t/t7506-status-submodule.sh | 18 +++++++++++++++++-
 2 files changed, 31 insertions(+), 17 deletions(-)

index 93e3fefd39..e72781d9f2 100644
@@ -1041,12 +1041,12 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
+	FILE *fp;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
+	int ignore_cp_exit_code = 0;
 
 	strbuf_addf(&buf, "%s/.git", path);
 	git_dir = read_gitfile(buf.buf);

@@ -1072,29 +1072,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (start_command(&cp))
 		die("Could not run 'git status --porcelain' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?'))
+	fp = xfdopen(cp.out, "r");
+	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
+		if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
 		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
-		     ignore_untracked))
-			break;
-
-		next_line = strchr(line, '\n');
-		if (!next_line)
+		     ignore_untracked)) {
+			/*
+			 * We're not interested in any further information from
+			 * the child any more, neither output nor its exit code.
+			 */
+			ignore_cp_exit_code = 1;
 			break;
-		next_line++;
-		len -= (next_line - line);
-		line = next_line;
+		}
 	}
-	close(cp.out);
+	fclose(fp);
 
-	if (finish_command(&cp))
+	if (finish_command(&cp) && !ignore_cp_exit_code)
 		die("'git status --porcelain' failed in submodule %s", path);
 
 	strbuf_release(&buf);

index d31b34da83..51f8d0d034 100755
@@ -177,8 +177,24 @@ test_expect_success 'status with added file in modified submodule with .git file
 	test_i18ngrep "modified:   sub (new commits, modified content)" output
 '
 
+test_expect_success 'status with a lot of untracked files in the submodule' '
+	(
+		cd sub
+		i=0 &&
+		while test $i -lt 1024
+		do
+			>some-file-$i
+			i=$(( $i + 1 ))
+		done
+	) &&
+	git status --porcelain sub 2>err.actual &&
+	test_must_be_empty err.actual &&
+	rm err.actual
+'
+
 test_expect_success 'rm submodule contents' '
-	rm -rf sub/* sub/.git
+	rm -rf sub &&
+	mkdir sub
 '
 
 test_expect_success 'status clean (empty submodule dir)' '

2.12.0.rc1.49.gdeb397943c.dirty

Migrate 'is_submodule_modified' to the new porcelain format of git-status. This conversion attempts to convert faithfully, i.e. the behavior ought to be exactly the same.

As the output in the parsing only distinguishes between untracked files and the rest, this is easy to port to the new format, as we only need to identify untracked files and the rest is handled in the "else" case.

untracked files are indicated by only a single question mark instead of two question marks, so the conversion is easy.

---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index e72781d9f2..5865795b9f 100644
@@ -1060,7 +1060,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);
 
-	argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+	argv_array_pushl(&cp.args, "status", "--porcelain=2", NULL);
 	if (ignore_untracked)
 		argv_array_push(&cp.args, "-uno");
 

@@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
 	fp = xfdopen(cp.out, "r");
 	while (strbuf_getwholeline(&buf, fp, '\n') != EOF) {
-		if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
+		/* regular untracked files */
+		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 		else
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

@@ -1093,7 +1094,7 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	fclose(fp);
 
 	if (finish_command(&cp) && !ignore_cp_exit_code)
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;

2.12.0.rc1.49.gdeb397943c.dirty

By having a stricter check in the superproject we catch errors earlier, instead of spawning a child process to tell us.

---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 5865795b9f..fa21c7bb72 100644
@@ -1052,11 +1052,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	git_dir = read_gitfile(buf.buf);
 	if (!git_dir)
 		git_dir = buf.buf;
-	if (!is_directory(git_dir)) {
+	if (!is_git_directory(git_dir)) {
+		if (is_directory(git_dir))
+			die(_("'%s' not recognized as a git repository"), git_dir);
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;
-
 	}
 	strbuf_reset(&buf);
 

2.12.0.rc1.49.gdeb397943c.dirty

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |  9 +++++++
 t/t3600-rm.sh                | 18 ++++++++++----
 t/t7506-status-submodule.sh  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 17 +++++++++++--
 4 files changed, 94 insertions(+), 7 deletions(-)

index ba873657cf..01b457c322 100644
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..a6e5c5bd56 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 308cf3779e..0375484962 100644
@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
 	strbuf_release(&twobuf);
 }
 
+static char short_submodule_status(struct wt_status_change_data *d) {
+	if (d->new_submodule_commits)
+		return 'M';
+	if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+		return 'm';
+	if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+		return '?';
+	return d->worktree_status;
+}
+
 static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 					 struct diff_options *options,
 					 void *data)

@@ -431,10 +441,13 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT)
+				d->worktree_status = short_submodule_status(d);
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.0.rc1.49.gdeb397943c.dirty

When a nested submodule has untracked files, it would be reported as "modified submodule" in the superproject, because submodules are not parsed correctly in is_submodule_modified as they are bucketed into the modified pile as "they are not an untracked file".

---
 submodule.c                 | 23 +++++++++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 23 insertions(+), 4 deletions(-)

index fa21c7bb72..730cc9513a 100644
@@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		/* regular untracked files */
 		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-		else
-			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			/*
+			 * T XY SSSS:
+			 * T = line type, XY = status, SSSS = submodule state
+			 */
+			if (buf.len < 1 + 1 + 2 + 1 + 4)
+				die("BUG: invalid status --porcelain=2 line %s",
+				    buf.buf);
+
+			/* regular unmerged and renamed files */
+			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+				/* nested untracked file */
+				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+			if (memcmp(buf.buf + 5, "S..U", 4))
+				/* other change */
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+		}
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
 		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||

index a6e5c5bd56..b58793448b 100755
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified_inside actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 6d3acb4a5a..ab822c79e6 100755
@@ -340,7 +340,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain)'
 test_expect_success 'status with untracked file in nested submodule (short)' '
 	git -C super status --short >output &&
 	diff output - <<-\EOF
-	 m sub1
+	 ? sub1
 	EOF
 '
 

2.12.0.rc1.49.gdeb397943c.dirty

I cannot quite parse the above.

No. Both before and after patch 4, this code has to determine two details from a submodule:

1. Does it have untracked files?
2. Does it have any modifications to tracked files (including submodules)?

Using porcelain v1 format, (1) is represented by a "??" line and (2) is represented by any other line. Using porcelain v2 format, (1) is represented by a "u" line and (2) is represented by any other line.

So patch 4 does not intend to change behavior.

This patch 7 is trying to do something more subtle. Suppose I have a superproject 'parent', with a submodule 'parent/sub', which itself contains a submodule 'parent/sub/subsub'. Now suppose I run, from within 'parent':

echo hi >sub/subsub/stray-file

Both before and after patch 4, if I run "git status" from 'parent' then I will learn that "sub" was modified. "git status" within 'sub' would tell me that "subsub" has an untracked file.

But from the end user's point of view, even when running in "parent", what I want to know is that there is an untracked file. Treating it as a modification instead of untracked file is confusing and does not answer the user's actual question. That is what patch 7 tries to fix.

In other words, patch 7 is about changing that list of two questions from before. Something like

1. Does it or any submodule contained within it have untracked files, that I could add with "git add -N --recurse-submodules"?

2. Does it or any submodule contained within it have modified files, that I could add with "git add -u --recurse-submodules"?

3. Does it or any submodule contained within it have a changed HEAD, that I could also add with "git add -u --recurse-submodules"?

Question (3) didn't come up before because when there are no nested submodules, the diff machinery answers it (saving us from getting the answer from the status --porcelain we recurse to).

Thanks and hope that helps,
Jonathan

Thanks.

I tried to describe the example Jonathan gave in his reply in a shorter form. I'll

ok, thanks for checking.

I did that in an earlier version of the series. However the change from porcelain=1 to 2 should not be observable by the end user.

No, up to patch 5/7 we only had refactors, no user visible changes intended. And until then we had "has untracked files" and "everything else". The nice part of the conversion was to cover the "everything else" part easily.

This patch transforms it into "has untracked files or submodule reports untracked files (possibly nested)" and "everything else", but the former is more complicated to compute.

No, see Jonathans answer.

We could argue, that 6/7 and 7/7 done as separate steps is broken, because since 6/7 we promise to report untracked files inside submodules as " ?", but we do not report them properly for nested submodules.

Suppose I have a superproject 'parent', with two submodules 'parent/sub' and 'parent/sub1'. 'parent/sub' itself contains a submodule 'parent/sub/subsub'. Now suppose I run, from within 'parent':

echo hi >sub/subsub/stray-file
echo hi >sub1/stray-file

with patches 1..5:

    git status --short
     M sub
     M sub1

patch 6:

    git status --short
     m sub
     ? sub1

with patch 7:

    git status --short
     ? sub
     ? sub1

The documentation update in patch 6 is unclear what to expect from nested submodules, so that documentation is technically not wrong, but maybe misleading.

I want to resend patch 7 with a documentation update as well as tests.

Thanks,
Stefan

[...]

Patches 1-6 are

The effect of patch 7 on --porcelain=2 output is subtle enough that I don't feel I understand it. I think it heads in a good direction but indeed, some tests could help to illustrate the desired behavior.

Thanks for your patient work.
Jonathan

v8:
* This is a resend of the last two patches, i.e. these two patches apply at 5c896f7c3ec (origin/sb/submodule-short-status^^)
* below is a diff of this patch series against origin/sb/submodule-short-status
* add tests showing the subtle bug fix in case of nesting.
* add a bit of documentation

v7:
previous work at
https://public-inbox.org/git/[email protected]/

index 01b457c322..452c6eb875 100644
@@ -187,6 +187,8 @@ Submodules have more state and instead report
 		m    the submodule has modified content
 		?    the submodule has untracked files
 
+Note that 'm' and '?' are applied recursively, e.g. if a nested submodule
+in a submodule contains an untracked file, this is reported as '?' as well.
 
 If -b is used the short-format status is preceded by a line
 

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |   9 ++++
 t/t3600-rm.sh                |  18 +++++---
 t/t7506-status-submodule.sh  | 102 +++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  |  17 +++++++-
 4 files changed, 139 insertions(+), 7 deletions(-)

index ba873657cf..01b457c322 100644
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..a6e5c5bd56 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 308cf3779e..0375484962 100644
@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
 	strbuf_release(&twobuf);
 }
 
+static char short_submodule_status(struct wt_status_change_data *d) {
+	if (d->new_submodule_commits)
+		return 'M';
+	if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+		return 'm';
+	if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+		return '?';
+	return d->worktree_status;
+}
+
 static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 					 struct diff_options *options,
 					 void *data)

@@ -431,10 +441,13 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT)
+				d->worktree_status = short_submodule_status(d);
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.1.438.g67623a8358

Hi,

Stefan Beller wrote:

[...]

[...]

Suppose I have a superproject 'super', with two submodules 'super/sub' and 'super/sub1'. 'super/sub' itself contains a submodule 'super/sub/subsub'. Now suppose I run, from within 'super':

echo hi >sub/subsub/stray-file
echo hi >sub1/stray-file

Currently we get would see the following output in git-status:

    git status --short
     m sub
     ? sub1

With this patch applied, the untracked file in the nested submodule is turned into an untracked file on the 'super' level as well.

    git status --short
     ? sub
     ? sub1

This doesn't change the output of 'git status --porcelain=1' for nested submodules, because its output is always ' M' for either untracked files or local modifications no matter the nesting level of the submodule.

'git status --porcelain=2' is affected by this change in a nested submodule, though. Without this patch it would report the direct submodule as modified and having no untracked files. With this patch it would report untracked files. Chalk this up as a bug fix.

---
 Documentation/git-status.txt |  2 ++
 submodule.c                  | 23 +++++++++++++++++++++--
 t/t3600-rm.sh                |  2 +-
 t/t7506-status-submodule.sh  |  2 +-
 4 files changed, 25 insertions(+), 4 deletions(-)

index 01b457c322..452c6eb875 100644
@@ -187,6 +187,8 @@ Submodules have more state and instead report
 		m    the submodule has modified content
 		?    the submodule has untracked files
 
+Note that 'm' and '?' are applied recursively, e.g. if a nested submodule
+in a submodule contains an untracked file, this is reported as '?' as well.
 
 If -b is used the short-format status is preceded by a line
 

index fa21c7bb72..730cc9513a 100644
@@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		/* regular untracked files */
 		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-		else
-			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			/*
+			 * T XY SSSS:
+			 * T = line type, XY = status, SSSS = submodule state
+			 */
+			if (buf.len < 1 + 1 + 2 + 1 + 4)
+				die("BUG: invalid status --porcelain=2 line %s",
+				    buf.buf);
+
+			/* regular unmerged and renamed files */
+			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+				/* nested untracked file */
+				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+			if (memcmp(buf.buf + 5, "S..U", 4))
+				/* other change */
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+		}
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
 		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||

index a6e5c5bd56..b58793448b 100755
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified_inside actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index fd057751df..4d6d8f6817 100755
@@ -346,7 +346,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2
 	git -C super status --porcelain=2 >output &&
 	awk -f suppress_hashes.awk output >output2 &&
 	diff output2 - <<-\EOF
-	1 .M S.M. 160000 160000 160000 HASH HASH sub1
+	1 .M S..U 160000 160000 160000 HASH HASH sub1
 	1 .M S..U 160000 160000 160000 HASH HASH sub2
 	EOF
 '

2.12.1.438.g67623a8358

s/turned into/displayed as/

I think that's reasonable, and the documentation does a good job of describing it.

Does this have any effect on the default mode of 'git status' (without --short or --porcelain)?

[...]

			/* T = line type, XY = status, SSSS = submodule state */
			if (buf.len < strlen("T XY SSSS"))
				...

Oh. :(

In case of 'u' and '2' we need to set DIRTY_SUBMODULE_MODIFIED additionally. will fix in a reroll.

All these cases are ' m', which I agree with, as it is a "modification that cannot be git-add'ed in the superproject".

We might be inclined to later come up with ' d' for a deleted nested submodule, but I do not think it is worth the effort.

Thanks,
Stefan

v9:
* This is a resend of the last two patches, i.e. these two patches apply at 5c896f7c3ec (origin/sb/submodule-short-status^^)
* below is a diff of this patch series against origin/sb/submodule-short-status
* better tests, refined documentation, thanks for the review, Jonathan!

Thanks,
Stefan

previous work:
https://public-inbox.org/git/[email protected]/

Stefan Beller (2):
short status: improve reporting for submodule changes submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  13 +++++
 submodule.c                  |  21 +++++++-
 t/t3600-rm.sh                |  18 +++++--
 t/t7506-status-submodule.sh  | 117 +++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  |  17 ++++++-
 5 files changed, 177 insertions(+), 9 deletions(-)

index 01b457c322..d70abc6afe 100644
@@ -186,7 +186,11 @@ Submodules have more state and instead report
 		     recorded in the index
 		m    the submodule has modified content
 		?    the submodule has untracked files
+since modified content or untracked files in a submodule cannot be added
+via `git add` in the superproject to prepare a commit.
 
+'m' and '?' are applied recursively. For example if a nested submodule
+in a submodule contains an untracked file, this is reported as '?' as well.
 
 If -b is used the short-format status is preceded by a line
 

index 730cc9513a..3da65100e3 100644
@@ -1082,20 +1082,18 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		if (buf.buf[0] == 'u' ||
 		    buf.buf[0] == '1' ||
 		    buf.buf[0] == '2') {
-			/*
-			 * T XY SSSS:
-			 * T = line type, XY = status, SSSS = submodule state
-			 */
-			if (buf.len < 1 + 1 + 2 + 1 + 4)
+			/* T = line type, XY = status, SSSS = submodule state */
+			if (buf.len < strlen("T XY SSSS"))
 				die("BUG: invalid status --porcelain=2 line %s",
 				    buf.buf);
 
-			/* regular unmerged and renamed files */
 			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
 				/* nested untracked file */
 				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
 
-			if (memcmp(buf.buf + 5, "S..U", 4))
+			if (buf.buf[0] == 'u' ||
+			    buf.buf[0] == '2' ||
+			    memcmp(buf.buf + 5, "S..U", 4))
 				/* other change */
 				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}

If I add an untracked file to a submodule or modify a tracked file, currently "git status --short" treats the change in the same way as changes to the current HEAD of the submodule:

        $ git clone --quiet --recurse-submodules https://gerrit.googlesource.com/gerrit
        $ echo hello >gerrit/plugins/replication/stray-file
        $ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
        $ git -C gerrit status --short
         M plugins/replication

This is by analogy with ordinary files, where "M" represents a change that has not been added yet to the index. But this change cannot be added to the index without entering the submodule, "git add"-ing it, and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this. These are similar to the existing "??" and " M" but mean that the submodule (not the parent project) has new untracked files and modified files, respectively. The user can use "git add" and "git commit" from within the submodule to add them.

Changes to the submodule's HEAD commit can be recorded in the index with a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for the submodule. They represent increasing levels of change --- the last one that applies is shown (e.g., " m" if there are both modified files and untracked files in the submodule, or " M" if the submodule's HEAD has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain level 1, which shares code with "status --short". We only change "git status --short".

Non-short "git status" and "git status --porcelain=2" already handle these cases by showing more detail:

        $ git -C gerrit status --porcelain=2
        1 .M S.MU 160000 160000 160000 305c864db28eb0c77c8499bc04c87de3f849cf3c 305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
        $ git -C gerrit status
[...]
        modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

---
 Documentation/git-status.txt |  11 ++++
 t/t3600-rm.sh                |  18 +++++--
 t/t7506-status-submodule.sh  | 117 +++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  |  17 ++++++-
 4 files changed, 156 insertions(+), 7 deletions(-)

index ba873657cf..67f1a910f3 100644
@@ -181,6 +181,15 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state and instead report
+		M    the submodule has a different HEAD than
+		     recorded in the index
+		m    the submodule has modified content
+		?    the submodule has untracked files
+since modified content or untracked files in a submodule cannot be added
+via `git add` in the superproject to prepare a commit.
+
+
 If -b is used the short-format status is preceded by a line
 
     ## branchname tracking info

@@ -210,6 +219,8 @@ field from the first filename).  Third, filenames containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single `?`.
+
 Porcelain Format Version 2
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 

index 5aa6db584c..a6e5c5bd56 100755
@@ -268,6 +268,14 @@ cat >expect.modified <<EOF
  M submod
 EOF
 
+cat >expect.modified_inside <<EOF
+ m submod
+EOF
+
+cat >expect.modified_untracked <<EOF
+ ? submod
+EOF
+
 cat >expect.cached <<EOF
 D  submod
 EOF

@@ -421,7 +429,7 @@ test_expect_success 'rm of a populated submodule with modifications fails unless
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with untracked files fails unle
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule with different nested HE
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule with nested modification
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

@@ -651,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	test -d submod &&
 	test -f submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test_cmp expect.modified actual &&
+	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
 	test ! -d submod &&
 	git status -s -uno --ignore-submodules=none >actual &&

index 308cf3779e..0375484962 100644
@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct wt_status *s,
 	strbuf_release(&twobuf);
 }
 
+static char short_submodule_status(struct wt_status_change_data *d) {
+	if (d->new_submodule_commits)
+		return 'M';
+	if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+		return 'm';
+	if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+		return '?';
+	return d->worktree_status;
+}
+
 static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 					 struct diff_options *options,
 					 void *data)

@@ -431,10 +441,13 @@ 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))
+		if (S_ISGITLINK(p->two->mode)) {
+			d->dirty_submodule = p->two->dirty_submodule;
 			d->new_submodule_commits = !!oidcmp(&p->one->oid,
 							    &p->two->oid);
+			if (s->status_format == STATUS_FORMAT_SHORT)
+				d->worktree_status = short_submodule_status(d);
+		}
 
 		switch (p->status) {
 		case DIFF_STATUS_ADDED:

2.12.1.442.ge9452a8fbc

Yes, this looks good.

Thank you,
Jonathan

Suppose I have a superproject 'super', with two submodules 'super/sub' and 'super/sub1'. 'super/sub' itself contains a submodule 'super/sub/subsub'. Now suppose I run, from within 'super':

echo hi >sub/subsub/stray-file
echo hi >sub1/stray-file

Currently we get would see the following output in git-status:

    git status --short
     m sub
     ? sub1

With this patch applied, the untracked file in the nested submodule is displayed as an untracked file on the 'super' level as well.

    git status --short
     ? sub
     ? sub1

This doesn't change the output of 'git status --porcelain=1' for nested submodules, because its output is always ' M' for either untracked files or local modifications no matter the nesting level of the submodule.

'git status --porcelain=2' is affected by this change in a nested submodule, though. Without this patch it would report the direct submodule as modified and having no untracked files. With this patch it would report untracked files. Chalk this up as a bug fix.

This bug fix also affects the default output (non-short, non-porcelain) of git-status, which is not tested here.

---
 Documentation/git-status.txt |  2 ++
 submodule.c                  | 21 +++++++++++++++++++--
 t/t3600-rm.sh                |  2 +-
 t/t7506-status-submodule.sh  |  4 ++--
 4 files changed, 24 insertions(+), 5 deletions(-)

index 67f1a910f3..d70abc6afe 100644
@@ -189,6 +189,8 @@ Submodules have more state and instead report
 since modified content or untracked files in a submodule cannot be added
 via `git add` in the superproject to prepare a commit.
 
+'m' and '?' are applied recursively. For example if a nested submodule
+in a submodule contains an untracked file, this is reported as '?' as well.
 
 If -b is used the short-format status is preceded by a line