[PATCHv3 0/3] short status: improve reporting for submodule changes

[PATCHv3 0/3] short status: improve reporting for submodule changes

[Stefan Beller]

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

[PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..d355ddb46b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 2/3] submodule.c, is_submodule_modified: stricter checking for submodules

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index d355ddb46b..66335159ae 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 3/3] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34da83..98dff01947 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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 &&
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..9909fd0e57 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Jonathan Nieder]

Stefan Beller wrote:

> 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.
[...]
>  submodule.c | 53 ++++++++++++++++++++++++-----------------------------
>  1 file changed, 24 insertions(+), 29 deletions(-)

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

Re: [PATCH 2/3] submodule.c, is_submodule_modified: stricter checking for submodules

[Jonathan Nieder]

Stefan Beller wrote:

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

Plus the error message is clearer.

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 3/3] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

>  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(-)

Very nice.

[...]
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -181,6 +181,13 @@ in which case `XY` are `!!`.

The documentation is clear.

[...]
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh

The updated behavior shown in this test makes sense.

[...]
> --- a/t/t7506-status-submodule.sh
> +++ b/t/t7506-status-submodule.sh

This test has good coverage and describes a good behavior.

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -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';

Can these be new DIFF_STATUS_* letters in diff.h?

I hadn't realized before that the worktree_status usually is taken
verbatim from diff_filepair.  Now I understand better what you were
talking about offline earlier today (sorry for the confusion).

We probably don't want the diff machinery to have to be aware of the
various status_format modes, so doing this here seems sensible to me.
Unfortunately it's also a reminder that "git diff --name-status" and
--diff-filter could benefit from a similar change.  (Unfortunate
because that's a harder change to make without breaking scripts.)

> +				else if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
> +					d->worktree_status = 'm';
> +				else if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
> +					d->worktree_status = '?';
> +			}
> +		}

Given the above, this implementation looks good. So for what it's worth,
this patch is
Reviewed-by: Jonathan Nieder <jrineder@gmail.com>

Patch 1/3 is the one that gives me pause, since I hadn't been able to
chase down all callers.  Would it be feasible to split that into two:
one patch to switch to --porcelain=2 but not change behavior, and a
separate patch to improve what is stored in the dirty_submodule flags?

Thanks,
Jonathan

Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Stefan Beller wrote:
>
>> 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.
> [...]
>>  submodule.c | 53 ++++++++++++++++++++++++-----------------------------
>>  1 file changed, 24 insertions(+), 29 deletions(-)
>
> 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.  

That sounds like a sensible organization.

> 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.

Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

On Wed, Mar 22, 2017 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Stefan Beller wrote:
>>
>>> 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.
>> [...]
>>>  submodule.c | 53 ++++++++++++++++++++++++-----------------------------
>>>  1 file changed, 24 insertions(+), 29 deletions(-)
>>
>> Neat.  Is this something that could be covered in tests, or should I
>> be patient and rely on patch 3/3 for that?

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 think this would be easier to understand if it were two patches: one
>> that switched to --porcelain=2 with no change in behavior,

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.

> That sounds like a sensible organization.

ok.

Thanks,
Stefan

[PATCH 0/8] short status: improve reporting for submodule changes

[Stefan Beller]

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

Jonathan wrote:
> Patch 1/3 is the one that gives me pause, since I hadn't been able to
> chase down all callers.  Would it be feasible to split that into two:
> one patch to switch to --porcelain=2 but not change behavior, and a
> separate patch to improve what is stored in the dirty_submodule flags?

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

[PATCH 1/8] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..d355ddb46b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 2/8] submodule.c: use argv_array in is_submodule_modified

[Stefan Beller]

struct argv_array is easier to use and maintain

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/submodule.c b/submodule.c
index d355ddb46b..dc61ca7103 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 3/8] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/submodule.c b/submodule.c
index dc61ca7103..f01e84faff 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 4/8] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/submodule.c b/submodule.c
index f01e84faff..da1db90dda 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 5/8] submodule.c: factor out early loop termination in is_submodule_modified

[Stefan Beller]

This makes it easier for a follow up patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index da1db90dda..93d6f08b50 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 6/8] submodule.c: stricter checking for submodules in is_submodule_modified

[Stefan Beller]

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 93d6f08b50..e06e52b993 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 7/8] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34da83..ad46384064 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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 &&
@@ -271,4 +295,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+test_expect_success 'setup superproject with untracked file in nested submodule' '
+	(
+		cd super &&
+		git clean -dfx &&
+		rm .gitmodules &&
+		git submodule add -f ./sub1 &&
+		git submodule add -f ./sub2 &&
+		git commit -a -m "messy merge in superproject" &&
+		(
+			cd sub1 &&
+			git submodule add ../sub2 &&
+			git commit -a -m "add sub2 to sub1"
+		) &&
+		git add sub1 &&
+		git commit -a -m "update sub1 to contain nested sub"
+	) &&
+	echo untracked >super/sub1/sub2/untracked
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	EOF
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..9909fd0e57 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 8/8] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                 | 16 ++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index e06e52b993..0f477f3a4e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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))
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index ad46384064..e3cdcede72 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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

Re: [PATCH 0/8] short status: improve reporting for submodule changes

[Stefan Beller]

On Thu, Mar 23, 2017 at 2:09 PM, Stefan Beller <sbeller@google.com> wrote:
> v4:
> * broken down in a lot of tiny patches.
>

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

Please ignore.

[PATCH v5 0/7] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Jonathan wrote:
> Patch 1/3 is the one that gives me pause, since I hadn't been able to
> chase down all callers.  Would it be feasible to split that into two:
> one patch to switch to --porcelain=2 but not change behavior, and a
> separate patch to improve what is stored in the dirty_submodule flags?

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

[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Stefan Beller]

struct argv_array is easier to use and maintain

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..2c667ac95a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd

[Stefan Beller]

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

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2c667ac95a..c1b7b78260 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 3/7] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index c1b7b78260..da1db90dda 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 4/7] submodule.c: factor out early loop termination in is_submodule_modified

[Stefan Beller]

This makes it easier for a follow up patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index da1db90dda..93d6f08b50 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Stefan Beller]

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 93d6f08b50..e06e52b993 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 6/7] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34da83..ad46384064 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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 &&
@@ -271,4 +295,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+test_expect_success 'setup superproject with untracked file in nested submodule' '
+	(
+		cd super &&
+		git clean -dfx &&
+		rm .gitmodules &&
+		git submodule add -f ./sub1 &&
+		git submodule add -f ./sub2 &&
+		git commit -a -m "messy merge in superproject" &&
+		(
+			cd sub1 &&
+			git submodule add ../sub2 &&
+			git commit -a -m "add sub2 to sub1"
+		) &&
+		git add sub1 &&
+		git commit -a -m "update sub1 to contain nested sub"
+	) &&
+	echo untracked >super/sub1/sub2/untracked
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	EOF
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..9909fd0e57 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

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".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                 | 16 ++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index e06e52b993..0f477f3a4e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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))
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index ad46384064..e3cdcede72 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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

Re: [PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> struct argv_array is easier to use and maintain

Yes!

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

I also like the diffstat. :)

[...]
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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,
> -	};

and the avoidance of this kind of fixed-size magic.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thank you.

Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd

[Jonathan Nieder]

Stefan Beller wrote:

> Instead of implementing line reading yet again, make use of our beautiful
> library functions.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)

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

diff --git i/submodule.c w/submodule.c
index c1b7b78260..184d5739fc 100644
--- i/submodule.c
+++ w/submodule.c
@@ -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);

Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd

[Stefan Beller]

On Thu, Mar 23, 2017 at 3:50 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Stefan Beller wrote:
>
>> Instead of implementing line reading yet again, make use of our beautiful
>> library functions.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  submodule.c | 14 ++------------
>>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> 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).

Thanks for careful reading!

>
> 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)?

The code below makes sense to me.

>
> Another consequence of switching to streaming is that we may close
> before the child finishes.

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.

>  Do we have to worry about handling SIGPIPE
> in the child?  I haven't checked how this handles that --- a test
> might be useful.

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

Thanks,
Stefan

Re: [PATCH 2/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline_fd

[Stefan Beller]

On Thu, Mar 23, 2017 at 4:04 PM, Stefan Beller <sbeller@google.com> wrote:
>
> We could have had closing before the child finished before as well:
> * the first read happens with strbuf_read(&buf, cp.out, 1024);

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.

[PATCH v6 0/7] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Jonathan wrote:
> Patch 1/3 is the one that gives me pause, since I hadn't been able to
> chase down all callers.  Would it be feasible to split that into two:
> one patch to switch to --porcelain=2 but not change behavior, and a
> separate patch to improve what is stored in the dirty_submodule flags?

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

[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Stefan Beller]

struct argv_array is easier to use and maintain

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..2c667ac95a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified

[Stefan Beller]

This makes it easier for a follow up patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2c667ac95a..e52cb8a958 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index e52cb8a958..0c43f9f2b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 0c43f9f2b1..256f15fde1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Stefan Beller]

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 256f15fde1..467f1de763 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 6/7] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34da83..ad46384064 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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 &&
@@ -271,4 +295,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+test_expect_success 'setup superproject with untracked file in nested submodule' '
+	(
+		cd super &&
+		git clean -dfx &&
+		rm .gitmodules &&
+		git submodule add -f ./sub1 &&
+		git submodule add -f ./sub2 &&
+		git commit -a -m "messy merge in superproject" &&
+		(
+			cd sub1 &&
+			git submodule add ../sub2 &&
+			git commit -a -m "add sub2 to sub1"
+		) &&
+		git add sub1 &&
+		git commit -a -m "update sub1 to contain nested sub"
+	) &&
+	echo untracked >super/sub1/sub2/untracked
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	EOF
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..9909fd0e57 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

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".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                 | 16 ++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 467f1de763..ec7e9b1b06 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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)) {
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index ad46384064..e3cdcede72 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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

Re: [PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> struct argv_array is easier to use and maintain

Missing '.' at end of sentence.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)

With or without that tweak, I still like this as much as last time. :)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> --- a/submodule.c
> +++ b/submodule.c
> @@ -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))

nit: long line

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/submodule.c w/submodule.c
index aba89661a7..8934d97359 100644
--- i/submodule.c
+++ w/submodule.c
@@ -1087,8 +1087,10 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 
 		if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
-		    ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || ignore_untracked))
+		    (ignore_untracked
+		     || (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))) {
 			break;
+		}
 
 		next_line = strchr(line, '\n');
 		if (!next_line)

Re: [PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Jonathan Nieder]

Stefan Beller wrote:

> 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.

It also overlaps work a little better.

> 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.

Should this say something about SIGPIPE?

[...]
> +++ b/submodule.c
[...]
> @@ -1072,28 +1072,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
[...]
> +			/*
> +			 * We're not interested in any further information from
> +			 * the child any more, no output nor its exit code.
> +			 */

language nit: s/, no output/: neither output/

[...]
> -	if (finish_command(&cp))
> +	if (finish_command(&cp) && !ignore_cp_exit_code)

finish_command complains if the child dies of SIGTERM:

		error: status died of signal 15

wait_or_whine(cp->pid, cp->argv[0], 1) doesn't do that but is meant for
signal handling.  Maybe we should rely on SIGPIPE instead (which
wait_or_whine always silences) and avoid the kill() call.

Can there be a test for this case (i.e. having lots of untracked files
in the submodule so the child process fills its pipe buffer and has to
exit early)?

Thanks,
Jonathan

Re: [PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2

[Jonathan Nieder]

Stefan Beller wrote:

[...]
> --- a/submodule.c
> +++ b/submodule.c
[...]
> @@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
[...]
>  	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;

At first I didn't believe it could be so simple. :)

Unlike ordinary files that use 1, 2, or u, ignored files use '!'.
You're not passing --ignored so you don't have to worry about them.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Very nice.

Re: [PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> By having a stricter check in the superproject we catch errors earlier,
> instead of spawning a child process to tell us.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Yep. :)

Re: [PATCH 6/7] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

> +++ b/wt-status.c
> @@ -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;

This is to simplify because dirty_submodule is just going to stay as 0
in the !S_ISGITLINK(p->two->mode) case, right?

>  			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 = '?';
> +			}
> +		}

Makes sense.

This patch also goes past the right margin.  Maybe this code to compute
d->worktree_status could be its own function?

With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/wt-status.c w/wt-status.c
index 9909fd0e57..0375484962 100644
--- i/wt-status.c
+++ w/wt-status.c
@@ -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)
@@ -435,14 +445,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 			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 = '?';
-			}
+			if (s->status_format == STATUS_FORMAT_SHORT)
+				d->worktree_status = short_submodule_status(d);
 		}
 
 		switch (p->status) {

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> 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".
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c                 | 16 ++++++++++++++--
>  t/t3600-rm.sh               |  2 +-
>  t/t7506-status-submodule.sh |  2 +-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 467f1de763..ec7e9b1b06 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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') {

Can this overflow the buffer?  Submodule state is supposed to be 4
characters, so could do

			/*
			 * 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);

			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
				/* untracked file */
				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;

			if (memcmp(buf.buf + 5, "S..U", 4))
				/* other change */
				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
		}

> +				/* 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;
> +		}

I get lost in these cases. What does it mean if we see S..., for
example?

As an example, if I am understanding correctly, before this patch, if
I have a submodule-in-submodule:

	$ find . -name .git
	.git
	sub/.git
	sub/subsub/.git

and I put a stray file in the sub-sub module:

	$ echo stray-file >sub/subsub/x.o

then status --porcelain=2 tells me that sub is modified:

	$ git status --porcelain=2
	1 .M S.M. [...] sub

What should it say after the patch?  Is the XY field ".." or ".M"?

Some tests covering these cases with --porcelain=2 and a brief mention
in documentation may help.

Thanks,
Jonathan

diff --git i/submodule.c w/submodule.c
index ec7e9b1b06..aec1b2cdca 100644
--- i/submodule.c
+++ w/submodule.c
@@ -1083,13 +1083,18 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		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
+			/*
+			 * 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);
+			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+				/* untracked file */
+				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			if (memcmp(buf.buf + 5, "S..U", 4))
+				/* other change */
 				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}
 

Re: [PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Stefan Beller]

On Fri, Mar 24, 2017 at 3:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> It also overlaps work a little better.

mentioned

>> 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.
>
> Should this say something about SIGPIPE?

done

> language nit: s/, no output/: neither output/

done

> wait_or_whine(cp->pid, cp->argv[0], 1) doesn't do that but is meant for
> signal handling.  Maybe we should rely on SIGPIPE instead (which
> wait_or_whine always silences) and avoid the kill() call.

done

> Can there be a test for this case (i.e. having lots of untracked files
> in the submodule so the child process fills its pipe buffer and has to
> exit early)?

done

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

On Fri, Mar 24, 2017 at 4:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Can this overflow the buffer?  Submodule state is supposed to be 4
> characters, so could do
>
>                         /*
>                          * 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);
>
>                         if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
>                                 /* untracked file */
>                                 dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
>
>                         if (memcmp(buf.buf + 5, "S..U", 4))
>                                 /* other change */
>                                 dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

good idea, will use.


>                 }
>
>> +                             /* 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;
>> +             }
>
> I get lost in these cases. What does it mean if we see S..., for
> example?

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)

>
> Some tests covering these cases with --porcelain=2 and a brief mention
> in documentation may help.

ok, will do.

[PATCH v7 0/7] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Jonathan wrote:
> Patch 1/3 is the one that gives me pause, since I hadn't been able to
> chase down all callers.  Would it be feasible to split that into two:
> one patch to switch to --porcelain=2 but not change behavior, and a
> separate patch to improve what is stored in the dirty_submodule flags?

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

[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Stefan Beller]

struct argv_array is easier to use and maintain.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 submodule.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..2c667ac95a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified

[Stefan Beller]

This makes it easier for a follow up patch.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 submodule.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2c667ac95a..93e3fefd39 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Stefan Beller]

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).

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                 | 30 ++++++++++++++----------------
 t/t7506-status-submodule.sh | 18 +++++++++++++++++-
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/submodule.c b/submodule.c
index 93e3fefd39..e72781d9f2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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);
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34da83..51f8d0d034 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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

[PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 submodule.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index e72781d9f2..5865795b9f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Stefan Beller]

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 5865795b9f..fa21c7bb72 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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

[PATCH 6/7] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 51f8d0d034..6d3acb4a5a 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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 &&
@@ -287,4 +311,37 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+test_expect_success 'setup superproject with untracked file in nested submodule' '
+	(
+		cd super &&
+		git clean -dfx &&
+		rm .gitmodules &&
+		git submodule add -f ./sub1 &&
+		git submodule add -f ./sub2 &&
+		git commit -a -m "messy merge in superproject" &&
+		(
+			cd sub1 &&
+			git submodule add ../sub2 &&
+			git commit -a -m "add sub2 to sub1"
+		) &&
+		git add sub1 &&
+		git commit -a -m "update sub1 to contain nested sub"
+	) &&
+	echo untracked >super/sub1/sub2/untracked
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	EOF
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..0375484962 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

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".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                 | 23 +++++++++++++++++++++--
 t/t3600-rm.sh               |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index fa21c7bb72..730cc9513a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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) ||
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 6d3acb4a5a..ab822c79e6 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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

Re: [PATCH v7 0/7] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

> 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.
[...]
> 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(-)

Patches 1-6 are

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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".

I cannot quite parse the above.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c                 | 23 +++++++++++++++++++++--
>  t/t3600-rm.sh               |  2 +-
>  t/t7506-status-submodule.sh |  2 +-
>  3 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index fa21c7bb72..730cc9513a 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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;

OK, we have untracked one.

> +			if (memcmp(buf.buf + 5, "S..U", 4))
> +				/* other change */
> +				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

And for other cases like C at buf.buf[6] or M at buf.buf[7],
i.e. where the submodule not just has untracked files but has real
changes, we say it is truly MODIFIED here.

If there are changes to paths that is not a submodule but a tracked
file in the submodule in question would have N at buf.buf[5] and is
also caught with the same "not S..U so that's MODIFIED" logic.

OK.

Shouldn't this done as part of 4/7 where is_submodule_modified()
starts reading from the porcelain v2 output?  4/7 does adjust for
the change from double question mark (porcelain v1) to a single one
for untracked, but at the same time it needs to prepare for these
'u' (unmerged), '1' (normal modification) and '2' (mods with rename)
to appear in the output, no?

IOW, with 4/7 and 7/7 done as separate steps, isn't the system
broken between these steps?

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Jonathan Nieder]

Junio C Hamano wrote:

> Shouldn't this done as part of 4/7 where is_submodule_modified()
> starts reading from the porcelain v2 output?  4/7 does adjust for
> the change from double question mark (porcelain v1) to a single one
> for untracked, but at the same time it needs to prepare for these
> 'u' (unmerged), '1' (normal modification) and '2' (mods with rename)
> to appear in the output, no?
>
> IOW, with 4/7 and 7/7 done as separate steps, isn't the system
> broken between these steps?

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

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

On Mon, Mar 27, 2017 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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".
>
> I cannot quite parse the above.

I tried to describe the example Jonathan gave in his reply in a shorter form.
I'll
>> +                     /* regular unmerged and renamed files */
>> +                     if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
>> +                             /* nested untracked file */
>> +                             dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
>
> OK, we have untracked one.
>
>> +                     if (memcmp(buf.buf + 5, "S..U", 4))
>> +                             /* other change */
>> +                             dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
>
> And for other cases like C at buf.buf[6] or M at buf.buf[7],
> i.e. where the submodule not just has untracked files but has real
> changes, we say it is truly MODIFIED here.
>
> If there are changes to paths that is not a submodule but a tracked
> file in the submodule in question would have N at buf.buf[5] and is
> also caught with the same "not S..U so that's MODIFIED" logic.
>
> OK.

ok, thanks for checking.

>
> Shouldn't this done as part of 4/7 where is_submodule_modified()
> starts reading from the porcelain v2 output?

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.

>  4/7 does adjust for
> the change from double question mark (porcelain v1) to a single one
> for untracked, but at the same time it needs to prepare for these
> 'u' (unmerged), '1' (normal modification) and '2' (mods with rename)
> to appear in the output, no?

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.

> IOW, with 4/7 and 7/7 done as separate steps, isn't the system
> broken between these steps?

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

[PATCH v8 0/7] short status: improve reporting for submodule changes

[Stefan Beller]

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/20170325003610.15282-1-sbeller@google.com/

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 01b457c322..452c6eb875 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index ab822c79e6..4d6d8f6817 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -327,20 +327,65 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 		git add sub1 &&
 		git commit -a -m "update sub1 to contain nested sub"
 	) &&
-	echo untracked >super/sub1/sub2/untracked
+	echo "{ \$7=\"HASH\"; \$8=\"HASH\"; print }" >suppress_hashes.awk &&
+	echo "suppress_hashes.awk" >>.git/info/exclude &&
+	echo "output2" >>.git/info/exclude &&
+	echo content >super/sub1/sub2/file &&
+	echo content >super/sub2/file
 '
 
 test_expect_success 'status with untracked file in nested submodule (porcelain)' '
 	git -C super status --porcelain >output &&
 	diff output - <<-\EOF
 	 M sub1
+	 M sub2
+	EOF
+'
+
+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..U 160000 160000 160000 HASH HASH sub1
+	1 .M S..U 160000 160000 160000 HASH HASH sub2
 	EOF
 '
 
 test_expect_success 'status with untracked file in nested submodule (short)' '
 	git -C super status --short >output &&
 	diff output - <<-\EOF
-	 ? sub1
+	 m sub1
+	 ? sub2
+	EOF
+'
+
+test_expect_success 'setup superproject with modified file in nested submodule' '
+	git -C super/sub1/sub2 add file &&
+	git -C super/sub2 add file
+'
+
+test_expect_success 'status with added file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	EOF
+'
+
+test_expect_success 'status with added 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.M. 160000 160000 160000 HASH HASH sub2
+	EOF
+'
+
+test_expect_success 'status with added file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 m sub2
 	EOF
 '
 

[PATCH 1/2] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 51f8d0d034..fd057751df 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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 &&
@@ -287,4 +311,82 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+test_expect_success 'setup superproject with untracked file in nested submodule' '
+	(
+		cd super &&
+		git clean -dfx &&
+		rm .gitmodules &&
+		git submodule add -f ./sub1 &&
+		git submodule add -f ./sub2 &&
+		git commit -a -m "messy merge in superproject" &&
+		(
+			cd sub1 &&
+			git submodule add ../sub2 &&
+			git commit -a -m "add sub2 to sub1"
+		) &&
+		git add sub1 &&
+		git commit -a -m "update sub1 to contain nested sub"
+	) &&
+	echo "{ \$7=\"HASH\"; \$8=\"HASH\"; print }" >suppress_hashes.awk &&
+	echo "suppress_hashes.awk" >>.git/info/exclude &&
+	echo "output2" >>.git/info/exclude &&
+	echo content >super/sub1/sub2/file &&
+	echo content >super/sub2/file
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	EOF
+'
+
+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 sub2
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 ? sub2
+	EOF
+'
+
+test_expect_success 'setup superproject with modified file in nested submodule' '
+	git -C super/sub1/sub2 add file &&
+	git -C super/sub2 add file
+'
+
+test_expect_success 'status with added file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	EOF
+'
+
+test_expect_success 'status with added 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.M. 160000 160000 160000 HASH HASH sub2
+	EOF
+'
+
+test_expect_success 'status with added file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 m sub2
+	EOF
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..0375484962 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 01b457c322..452c6eb875 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 
diff --git a/submodule.c b/submodule.c
index fa21c7bb72..730cc9513a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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) ||
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index fd057751df..4d6d8f6817 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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

Re: [PATCH 1/2] short status: improve reporting for submodule changes

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

[...]
> +++ b/t/t7506-status-submodule.sh
[...]
> @@ -287,4 +311,82 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
>  	test_cmp diff_submodule_actual diff_submodule_expect
>  '
>  
> +test_expect_success 'setup superproject with untracked file in nested submodule' '
> +	(
> +		cd super &&
> +		git clean -dfx &&
> +		rm .gitmodules &&
> +		git submodule add -f ./sub1 &&
> +		git submodule add -f ./sub2 &&
> +		git commit -a -m "messy merge in superproject" &&
> +		(
> +			cd sub1 &&
> +			git submodule add ../sub2 &&
> +			git commit -a -m "add sub2 to sub1"
> +		) &&
> +		git add sub1 &&
> +		git commit -a -m "update sub1 to contain nested sub"
> +	) &&
> +	echo "{ \$7=\"HASH\"; \$8=\"HASH\"; print }" >suppress_hashes.awk &&
> +	echo "suppress_hashes.awk" >>.git/info/exclude &&

I had some trouble following what suppress_hashes.awk does at first.

Some other examples that could be worth imitating:

- diff-lib.sh has some sanitize_diff functions using 'sed'
- t4202 has a sanitize_output functino, also using 'sed'
- grepping for $_x40 finds some other examples (these will be fun to
  change when the hash function changes, but at least they're easy to
  find).

The main general idea I have in mind is that providing a function at the
top of the file for the test to use instead of a script that interacts
with what git is tracking can make things easier to understand.

Also, could there be a comment with a diagram describing the setup
(sub1 vs sub2, etc)?

[...]
> +test_expect_success 'status with untracked file in nested submodule (porcelain)' '
> +	git -C super status --porcelain >output &&
> +	diff output - <<-\EOF
> +	 M sub1
> +	 M sub2
> +	EOF
> +'
> +
> +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 sub2
> +	EOF
> +'
> +
> +test_expect_success 'status with untracked file in nested submodule (short)' '
> +	git -C super status --short >output &&
> +	diff output - <<-\EOF
> +	 m sub1
> +	 ? sub2
> +	EOF
> +'
> +
> +test_expect_success 'setup superproject with modified file in nested submodule' '
> +	git -C super/sub1/sub2 add file &&
> +	git -C super/sub2 add file
> +'
> +
> +test_expect_success 'status with added file in nested submodule (porcelain)' '
> +	git -C super status --porcelain >output &&
> +	diff output - <<-\EOF
> +	 M sub1
> +	 M sub2
> +	EOF
> +'
> +
> +test_expect_success 'status with added 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.M. 160000 160000 160000 HASH HASH sub2
> +	EOF
> +'
> +
> +test_expect_success 'status with added file in nested submodule (short)' '
> +	git -C super status --short >output &&
> +	diff output - <<-\EOF
> +	 m sub1
> +	 m sub2
> +	EOF

How does ordinary non --short "git status" handle these cases?

What should happen when there is a new untracked repository within a
submodule?

What should happen if there is both a modified tracked file and an
untracked file in a sub-submodule?


> +'
> +
>  test_done

Very nice.  Thanks --- this was exactly what I was looking for.

The rest looks good.

Sincerely,
Jonathan

Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified

[Jonathan Nieder]

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.

s/turned into/displayed as/

>     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.

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)?

[...]
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -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.

Language nits:

* Can simplify by leaving out "Note that ".
* s/, e\.g\./. For example,/

Should this say a brief word about rationale?  The obvious way to
describe it would involve "git add --recurse-submodules", which alas
doesn't exist yet.  But we could say

                                              this is reported as '?' as well,
   since the change cannot be added using "git add -u" from within any of the
   submodules.

[...]
> --- a/submodule.c
> +++ b/submodule.c
> @@ -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)

optional: Can shorten:

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

That produces the same code at run time because compilers are able to
inline the strlen of a constant.  What you already have also seems
sensible, though.

[...]
> +				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;

sanity check: What does this do for a "2" line indicating a sub-submodule
that has been renamed that contains an untracked file?  Do we need to
rely on some other indication to show this as a change?

Enumerating some more cases, since I keep finding myself getting lost:

 - if the HEAD commit of "sub" changes, we show this as " M sub".
   What should we show if the HEAD commit of "sub/subsub" changes?
   I think this should be " m".

 - if "sub" is renamed, we show this as "R  sub -> newname".
   What should we show if "sub/subsub" is renamed?  It is tempting
   to show this as " m".

 - if "sub" is deleted, we show this as "D  sub". What should we
   show if "sub/subsub" is deleted? I think this is " m".

It keeps getting more complicated, but I think this is making sense.

Thanks and hope that helps,
Jonathan

Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

> sanity check: What does this do for a "2" line indicating a sub-submodule
> that has been renamed that contains an untracked file?  Do we need to
> rely on some other indication to show this as a change?

Oh. :(

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

>
> Enumerating some more cases, since I keep finding myself getting lost:
>
>  - if the HEAD commit of "sub" changes, we show this as " M sub".
>    What should we show if the HEAD commit of "sub/subsub" changes?
>    I think this should be " m".
>
>  - if "sub" is renamed, we show this as "R  sub -> newname".
>    What should we show if "sub/subsub" is renamed?  It is tempting
>    to show this as " m".
>
>  - if "sub" is deleted, we show this as "D  sub". What should we
>    show if "sub/subsub" is deleted? I think this is " m".

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

[PATCHv9 (6,7)/7] short status: improve reporting for submodule changes

[Stefan Beller]

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/20170328230938.9887-1-sbeller@google.com/

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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 01b457c322..d70abc6afe 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 
diff --git a/submodule.c b/submodule.c
index 730cc9513a..3da65100e3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -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;
 		}
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index ab822c79e6..055c90736e 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -17,6 +17,12 @@ test_create_repo_with_commit () {
 	)
 }
 
+sanitize_output () {
+	sed -e "s/$_x40/HASH/" -e "s/$_x40/HASH/" output >output2 &&
+	mv output2 output
+}
+
+
 test_expect_success 'setup' '
 	test_create_repo_with_commit sub &&
 	echo output > .gitignore &&
@@ -311,6 +317,10 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+# We'll setup different cases for further testing:
+# sub1 will contain a nested submodule,
+# sub2 will have an untracked file
+# sub3 will have an untracked repository
 test_expect_success 'setup superproject with untracked file in nested submodule' '
 	(
 		cd super &&
@@ -318,6 +328,7 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 		rm .gitmodules &&
 		git submodule add -f ./sub1 &&
 		git submodule add -f ./sub2 &&
+		git submodule add -f ./sub1 sub3 &&
 		git commit -a -m "messy merge in superproject" &&
 		(
 			cd sub1 &&
@@ -327,13 +338,27 @@ test_expect_success 'setup superproject with untracked file in nested submodule'
 		git add sub1 &&
 		git commit -a -m "update sub1 to contain nested sub"
 	) &&
-	echo untracked >super/sub1/sub2/untracked
+	echo content >super/sub1/sub2/file &&
+	echo content >super/sub2/file &&
+	git -C super/sub3 clone ../../sub2 untracked_repository
 '
 
 test_expect_success 'status with untracked file in nested submodule (porcelain)' '
 	git -C super status --porcelain >output &&
 	diff output - <<-\EOF
 	 M sub1
+	 M sub2
+	 M sub3
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '
+	git -C super status --porcelain=2 >output &&
+	sanitize_output output &&
+	diff output - <<-\EOF
+	1 .M S..U 160000 160000 160000 HASH HASH sub1
+	1 .M S..U 160000 160000 160000 HASH HASH sub2
+	1 .M S..U 160000 160000 160000 HASH HASH sub3
 	EOF
 '
 
@@ -341,6 +366,41 @@ test_expect_success 'status with untracked file in nested submodule (short)' '
 	git -C super status --short >output &&
 	diff output - <<-\EOF
 	 ? sub1
+	 ? sub2
+	 ? sub3
+	EOF
+'
+
+test_expect_success 'setup superproject with modified file in nested submodule' '
+	git -C super/sub1/sub2 add file &&
+	git -C super/sub2 add file
+'
+
+test_expect_success 'status with added file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	 M sub3
+	EOF
+'
+
+test_expect_success 'status with added file in nested submodule (porcelain=2)' '
+	git -C super status --porcelain=2 >output &&
+	sanitize_output output &&
+	diff output - <<-\EOF
+	1 .M S.M. 160000 160000 160000 HASH HASH sub1
+	1 .M S.M. 160000 160000 160000 HASH HASH sub2
+	1 .M S..U 160000 160000 160000 HASH HASH sub3
+	EOF
+'
+
+test_expect_success 'status with added file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 m sub2
+	 ? sub3
 	EOF
 '
 

[PATCH 1/2] short status: improve reporting for submodule changes

[Stefan Beller]

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.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..67f1a910f3 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 51f8d0d034..1fa2ff2909 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -17,6 +17,12 @@ test_create_repo_with_commit () {
 	)
 }
 
+sanitize_output () {
+	sed -e "s/$_x40/HASH/" -e "s/$_x40/HASH/" output >output2 &&
+	mv output2 output
+}
+
+
 test_expect_success 'setup' '
 	test_create_repo_with_commit sub &&
 	echo output > .gitignore &&
@@ -50,6 +56,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 +79,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 +106,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 &&
@@ -287,4 +317,91 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
 	test_cmp diff_submodule_actual diff_submodule_expect
 '
 
+# We'll setup different cases for further testing:
+# sub1 will contain a nested submodule,
+# sub2 will have an untracked file
+# sub3 will have an untracked repository
+test_expect_success 'setup superproject with untracked file in nested submodule' '
+	(
+		cd super &&
+		git clean -dfx &&
+		rm .gitmodules &&
+		git submodule add -f ./sub1 &&
+		git submodule add -f ./sub2 &&
+		git submodule add -f ./sub1 sub3 &&
+		git commit -a -m "messy merge in superproject" &&
+		(
+			cd sub1 &&
+			git submodule add ../sub2 &&
+			git commit -a -m "add sub2 to sub1"
+		) &&
+		git add sub1 &&
+		git commit -a -m "update sub1 to contain nested sub"
+	) &&
+	echo content >super/sub1/sub2/file &&
+	echo content >super/sub2/file &&
+	git -C super/sub3 clone ../../sub2 untracked_repository
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	 M sub3
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (porcelain=2)' '
+	git -C super status --porcelain=2 >output &&
+	sanitize_output output &&
+	diff output - <<-\EOF
+	1 .M S.M. 160000 160000 160000 HASH HASH sub1
+	1 .M S..U 160000 160000 160000 HASH HASH sub2
+	1 .M S..U 160000 160000 160000 HASH HASH sub3
+	EOF
+'
+
+test_expect_success 'status with untracked file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 ? sub2
+	 ? sub3
+	EOF
+'
+
+test_expect_success 'setup superproject with modified file in nested submodule' '
+	git -C super/sub1/sub2 add file &&
+	git -C super/sub2 add file
+'
+
+test_expect_success 'status with added file in nested submodule (porcelain)' '
+	git -C super status --porcelain >output &&
+	diff output - <<-\EOF
+	 M sub1
+	 M sub2
+	 M sub3
+	EOF
+'
+
+test_expect_success 'status with added file in nested submodule (porcelain=2)' '
+	git -C super status --porcelain=2 >output &&
+	sanitize_output output &&
+	diff output - <<-\EOF
+	1 .M S.M. 160000 160000 160000 HASH HASH sub1
+	1 .M S.M. 160000 160000 160000 HASH HASH sub2
+	1 .M S..U 160000 160000 160000 HASH HASH sub3
+	EOF
+'
+
+test_expect_success 'status with added file in nested submodule (short)' '
+	git -C super status --short >output &&
+	diff output - <<-\EOF
+	 m sub1
+	 m sub2
+	 ? sub3
+	EOF
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..0375484962 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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

[PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 67f1a910f3..d70abc6afe 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -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
 
diff --git a/submodule.c b/submodule.c
index fa21c7bb72..3da65100e3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1078,8 +1078,25 @@ 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 = line type, XY = status, SSSS = submodule state */
+			if (buf.len < strlen("T XY SSSS"))
+				die("BUG: invalid status --porcelain=2 line %s",
+				    buf.buf);
+
+			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+				/* nested untracked file */
+				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+			if (buf.buf[0] == 'u' ||
+			    buf.buf[0] == '2' ||
+			    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) ||
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 1fa2ff2909..055c90736e 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -356,7 +356,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2
 	git -C super status --porcelain=2 >output &&
 	sanitize_output output &&
 	diff output - <<-\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
 	1 .M S..U 160000 160000 160000 HASH HASH sub3
 	EOF
@@ -365,7 +365,7 @@ test_expect_success 'status with untracked file in nested submodule (porcelain=2
 test_expect_success 'status with untracked file in nested submodule (short)' '
 	git -C super status --short >output &&
 	diff output - <<-\EOF
-	 m sub1
+	 ? sub1
 	 ? sub2
 	 ? sub3
 	EOF
-- 
2.12.1.442.ge9452a8fbc

Re: [PATCH 1/2] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  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(-)

Yes, this looks good.

Thank you,
Jonathan

Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

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

Do you have an example?  (In just the commit message would be fine, in
tests would be even better.)

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

but I would be a lot more comfortable after looking at the change to
"git status" output.  (E.g. a test demonstrating it can happen in a
followup change if that's simpler.)

Thanks for your patient work on this.

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> Shouldn't this done as part of 4/7 where is_submodule_modified()
>> starts reading from the porcelain v2 output?  4/7 does adjust for
>> the change from double question mark (porcelain v1) to a single one
>> for untracked, but at the same time it needs to prepare for these
>> 'u' (unmerged), '1' (normal modification) and '2' (mods with rename)
>> to appear in the output, no?
>>
>> IOW, with 4/7 and 7/7 done as separate steps, isn't the system
>> broken between these steps?
>
> No.  Both before and after patch 4, this code has to determine two
> details from a submodule:
> ...
> 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.

Re: [PATCH 2/2] submodule.c: correctly handle nested submodules in is_submodule_modified

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Stefan Beller wrote:
>
>> This bug fix also affects the default output (non-short, non-porcelain)
>> of git-status, which is not tested here.
>
> Do you have an example?  (In just the commit message would be fine, in
> tests would be even better.)
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> but I would be a lot more comfortable after looking at the change to
> "git status" output.  (E.g. a test demonstrating it can happen in a
> followup change if that's simpler.)
>
> Thanks for your patient work on this.

Thank you both.  I re-read the whole thing and it feels to me that
the topic is now ready for 'next'.