[RFC PATCH] short status: improve reporting for submodule changes

[RFC PATCH] short status: improve reporting for submodule changes

[Stefan Beller]

While we already have a porcelain2 layer for git-status, that is accurate
for submodules, users still like the way they are are used to of
'status -s'.

As a submodule has more state than a file potentially, we'll look at all
cases:

   ------ new submodule commits
 /  ----- modified files
 | /   -- untracked files
 | |  /
 | | |   current / proposed reporting
 0 0 0     "  "     "  "
 0 0 1     " M"     " ?"
 0 1 0     " M"     " m"
 0 1 1     " M"     " m"
 1 0 0     " M"     " M"
 1 0 1     " M"     " M"
 1 1 0     " M"     " M"
 1 1 1     " M"     " M"

So when there is no new commits in the submodule, we'd want to tell
what actually happend to the submodule. The lower case 'm' indicates
to the user that there is need to give a --recurse-submodules option
for e.g. adding or committing these changes. The " ?" also differentiates
an untracked file in the submodule from a regular untracked file.

While making these changes, we need to make sure to not break porcelain
level 1, which uses the same code as the short printing function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-status.txt |  8 ++++++++
 wt-status.c                  | 15 +++++++++++++--
 wt-status.h                  |  1 +
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..26c8d832cd 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -181,6 +181,12 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state to it, such that it reports instead:
+		M    the submodule has new commits
+		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 +216,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/wt-status.c b/wt-status.c
index a52d342695..080d97f272 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status);
 	else
 		putchar(' ');
-	if (d->worktree_status)
+	if (d->worktree_status) {
+		if (!s->submodule_porcelain1 &&
+		    (d->dirty_submodule || d->new_submodule_commits)) {
+			/* It is a submodule, and we're not in plumbing mode. */
+			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 = '?';
+		}
 		color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%c", d->worktree_status);
-	else
+	} else
 		putchar(' ');
 	putchar(' ');
 	if (s->null_termination) {
@@ -1811,6 +1821,7 @@ static void wt_porcelain_print(struct wt_status *s)
 	s->relative_paths = 0;
 	s->prefix = NULL;
 	s->no_gettext = 1;
+	s->submodule_porcelain1 = 1;
 	wt_shortstatus_print(s);
 }
 
diff --git a/wt-status.h b/wt-status.h
index 54fec77032..620e4d2ae4 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -70,6 +70,7 @@ struct wt_status {
 	int display_comment_prefix;
 	int relative_paths;
 	int submodule_summary;
+	int submodule_porcelain1;
 	int show_ignored_files;
 	enum untracked_status_type show_untracked_files;
 	const char *ignore_submodule_arg;
-- 
2.12.0.269.g1a05a5734c.dirty

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> While we already have a porcelain2 layer for git-status, that is accurate
> for submodules, users still like the way they are are used to of
> 'status -s'.
>
> As a submodule has more state than a file potentially, we'll look at all
> cases:
>
>    ------ new submodule commits
>  /  ----- modified files
>  | /   -- untracked files
>  | |  /
>  | | |   current / proposed reporting
>  0 0 0     "  "     "  "
>  0 0 1     " M"     " ?"
>  0 1 0     " M"     " m"
>  0 1 1     " M"     " m"
>  1 0 0     " M"     " M"
>  1 0 1     " M"     " M"
>  1 1 0     " M"     " M"
>  1 1 1     " M"     " M"

You are essentialy saying that there are three levels, 1. with
commit level difference, 2. the same commit with local mods, 3. no
mods but with crufts, and instead of wasting 8 letters to express
all combinations, the highest level is reported, right?  That sounds
OK to me.  I am not sure if "?" is a good letter to use (doesn't it
usually mean it is an untracked cruft?), though.

Does the commit level difference really mean "has new commits"?  It
probably is not new problem but an old mistake inherited from the
current code, but I suspect that you're just comparing the commit
bound in the index of the superproject and the HEAD commit in the
submodule, in which case "newness" does not matter a bit---"is it
the same or different?" is the question you are asking, I would
think.

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

[Stefan Beller]

On Wed, Mar 15, 2017 at 6:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> While we already have a porcelain2 layer for git-status, that is accurate
>> for submodules, users still like the way they are are used to of
>> 'status -s'.
>>
>> As a submodule has more state than a file potentially, we'll look at all
>> cases:
>>
>>    ------ new submodule commits
>>  /  ----- modified files
>>  | /   -- untracked files
>>  | |  /
>>  | | |   current / proposed reporting
>>  0 0 0     "  "     "  "
>>  0 0 1     " M"     " ?"
>>  0 1 0     " M"     " m"
>>  0 1 1     " M"     " m"
>>  1 0 0     " M"     " M"
>>  1 0 1     " M"     " M"
>>  1 1 0     " M"     " M"
>>  1 1 1     " M"     " M"
>
> You are essentialy saying that there are three levels, 1. with
> commit level difference, 2. the same commit with local mods, 3. no
> mods but with crufts, and instead of wasting 8 letters to express
> all combinations, the highest level is reported, right?  That sounds
> OK to me.  I am not sure if "?" is a good letter to use (doesn't it
> usually mean it is an untracked cruft?), though.

ok. it helped me, though, to picture all possibilities to come up with
what I consider best for each case. Yes in the end it can be described
as 'report highest bit'.

>
> Does the commit level difference really mean "has new commits"?  It
> probably is not new problem but an old mistake inherited from the
> current code, but I suspect that you're just comparing the commit
> bound in the index of the superproject and the HEAD commit in the
> submodule, in which case "newness" does not matter a bit---"is it
> the same or different?" is the question you are asking, I would
> think.

yes, I agree. That is the actual question asked.


>
>

[PATCH v2] short status: improve reporting for submodule changes

[Stefan Beller]

While we already have a porcelain2 layer for git-status, that is accurate
for submodules, users still like the way they are are used to of
'status -s'.

As a submodule has more state than a file potentially, we need more letters
indicating these states, we'll go with lower 'm' and single '?' for now.

When there the recorded commit doesn't differ from the submodule HEAD
(which we still want to treat as "M", because that can be dealt with
in the superproject), we can have different types of change in the
submodule. The lower case 'm' indicates that there are changes to tracked
files in the submodule. It signals that the --recurse-submodules option
is needed for e.g. adding or committing these changes. The " ?" also
differentiates an untracked file in the submodule from a regular
untracked file.

While making these changes, we need to make sure to not break porcelain
level 1, which uses the same code as the short printing function.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-status.txt |  8 ++++++++
 t/t7506-status-submodule.sh  | 24 ++++++++++++++++++++++++
 wt-status.c                  | 15 +++++++++++++--
 wt-status.h                  |  1 +
 4 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..b182b16c48 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -181,6 +181,12 @@ in which case `XY` are `!!`.
     !           !    ignored
     -------------------------------------------------
 
+Submodules have more state to it, such that it reports instead:
+		M    the submodule has a different HEAD than recorded
+		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 +216,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/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 a52d342695..080d97f272 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,
 		color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status);
 	else
 		putchar(' ');
-	if (d->worktree_status)
+	if (d->worktree_status) {
+		if (!s->submodule_porcelain1 &&
+		    (d->dirty_submodule || d->new_submodule_commits)) {
+			/* It is a submodule, and we're not in plumbing mode. */
+			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 = '?';
+		}
 		color_fprintf(s->fp, color(WT_STATUS_CHANGED, s), "%c", d->worktree_status);
-	else
+	} else
 		putchar(' ');
 	putchar(' ');
 	if (s->null_termination) {
@@ -1811,6 +1821,7 @@ static void wt_porcelain_print(struct wt_status *s)
 	s->relative_paths = 0;
 	s->prefix = NULL;
 	s->no_gettext = 1;
+	s->submodule_porcelain1 = 1;
 	wt_shortstatus_print(s);
 }
 
diff --git a/wt-status.h b/wt-status.h
index 54fec77032..620e4d2ae4 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -70,6 +70,7 @@ struct wt_status {
 	int display_comment_prefix;
 	int relative_paths;
 	int submodule_summary;
+	int submodule_porcelain1;
 	int show_ignored_files;
 	enum untracked_status_type show_untracked_files;
 	const char *ignore_submodule_arg;
-- 
2.12.0.269.g1a05a5734c.dirty

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

[Jonathan Nieder]

Hi,

Yay, I like the change this makes.  So I'll nitpick in the hope that
that makes the patch more likely to stick.

Stefan Beller wrote:

> While we already have a porcelain2 layer for git-status, that is accurate
> for submodules, users still like the way they are are used to of
> 'status -s'.

I had to read this a few times to understand it.  Maybe explaining it
from the user's point of view would work:

	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.

> As a submodule has more state than a file potentially, we need more letters
> indicating these states, we'll go with lower 'm' and single '?' for now.
>
> When there the recorded commit doesn't differ from the submodule HEAD
> (which we still want to treat as "M", because that can be dealt with
> in the superproject), we can have different types of change in the
> submodule. The lower case 'm' indicates that there are changes to tracked
> files in the submodule. It signals that the --recurse-submodules option
> is needed for e.g. adding or committing these changes. The " ?" also
> differentiates an untracked file in the submodule from a regular
> untracked file.

This could say

	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 uses the same code as the short printing function.

	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.

> Signed-off-by: Stefan Beller <sbeller@google.com>

Having written that, a few questions:

Is it important to avoid clutter by showing the submodule only once?
What would you think of showing whatever subset of those three
statuses apply to a given submodule as separate lines instead, to
match the information that long-form "git status" shows?

How should a new untracked file in a submodule of a submodule be
shown?

[...]
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -181,6 +181,12 @@ in which case `XY` are `!!`.
>      !           !    ignored
>      -------------------------------------------------
>  
> +Submodules have more state to it, such that it reports instead:

Language nits: s/ to it//; s/, such that it reports instead/ and instead report/

> +		M    the submodule has a different HEAD than recorded

than recorded in the index?

> +		m    the submodule has modified content
> +		?    the submodule has untracked files
[...]
> --- a/t/t7506-status-submodule.sh
> +++ b/t/t7506-status-submodule.sh

\o/

> @@ -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) &&

not about this patch: this could use "git -C sub" and avoid a
subshell.  But what you have here matches the existing style, so I
wouldn't suggest changing it here.

> +	echo "changed" >sub/foo &&
> +	git status --short >output &&
> +	diff output - <<-\EOF

similarly, this could use test_cmp, but that seems out of scope for
this change.

[...]
> +test_expect_success 'status with added file in submodule (short)' '
[...]
> +test_expect_success 'status with untracked file in submodule (short)' '

Test wishlist:
* --porcelain output for these cases
* behavior with nested submodules

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,
>  		color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status);
>  	else
>  		putchar(' ');
> -	if (d->worktree_status)
> +	if (d->worktree_status) {
> +		if (!s->submodule_porcelain1 &&

Isn't this 's->status_format != STATUS_FORMAT_PORCELAIN'?

[...]
> +		    (d->dirty_submodule || d->new_submodule_commits)) {
> +			/* It is a submodule, and we're not in plumbing mode. */
> +			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 = '?';
> +		}

Is this the right place to do this?  Could wt_status_collect_changed_cb set
worktree_status to the right value in the first place based on whether
'status_format == STATUS_FORMAT_SHORT' or would that have undesirable
knock-on effects elsewhere?

[...]
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -70,6 +70,7 @@ struct wt_status {
>  	int display_comment_prefix;
>  	int relative_paths;
>  	int submodule_summary;
> +	int submodule_porcelain1;

As described above, this shouldn't be needed since status_format
already has this information.

Thanks for this --- I like it.

Sincerely,
Jonathan

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

[Stefan Beller]

>
> Is it important to avoid clutter by showing the submodule only once?
> What would you think of showing whatever subset of those three
> statuses apply to a given submodule as separate lines instead, to
> match the information that long-form "git status" shows?

I considered it, but it would break the visual appeal of git status --short ?

>
> How should a new untracked file in a submodule of a submodule be
> shown?

The same way " ?" indicates that (1) there is an untracked file due to
the question mark and (2) that you need to recurse because it differs from
"??" for regular untracked files.

The problem here is that we do not know about these nested untracked files,
because we use --porcelain instead of --short for submodules in
submodule.c#is_submodule_modified(). I am rewriting that function anyway
for the "git-describe --dirty" bug, so maybe it's time to switch to porcelain=2
internally there, which can surface untracked files in nested subs.


>
> [...]
>> --- a/Documentation/git-status.txt
>> +++ b/Documentation/git-status.txt
>> @@ -181,6 +181,12 @@ in which case `XY` are `!!`.
>>      !           !    ignored
>>      -------------------------------------------------
>>
>> +Submodules have more state to it, such that it reports instead:
>
> Language nits: s/ to it//; s/, such that it reports instead/ and instead report/
>
>> +             M    the submodule has a different HEAD than recorded
>
> than recorded in the index?

ok, will fix.


>> +test_expect_success 'status with modified file in submodule (short)' '
>> +     (cd sub && git reset --hard) &&
>
> not about this patch: this could use "git -C sub" and avoid a
> subshell.  But what you have here matches the existing style, so I
> wouldn't suggest changing it here.
>
>> +     echo "changed" >sub/foo &&
>> +     git status --short >output &&
>> +     diff output - <<-\EOF
>
> similarly, this could use test_cmp, but that seems out of scope for
> this change.

Yes I considered if I want to redo the whole file to meet
'modern Git test suite style'. I'll think about it again.

> [...]
>> +test_expect_success 'status with added file in submodule (short)' '
> [...]
>> +test_expect_success 'status with untracked file in submodule (short)' '
>
> Test wishlist:
> * --porcelain output for these cases
> * behavior with nested submodules

Look at the test file, please.
I copied the tests cases (porcelain) and replaced s/porcelain/short/,
so they exist.

>
> [...]
>> --- a/wt-status.c
>> +++ b/wt-status.c
>> @@ -1664,9 +1664,19 @@ static void wt_shortstatus_status(struct string_list_item *it,
>>               color_fprintf(s->fp, color(WT_STATUS_UPDATED, s), "%c", d->index_status);
>>       else
>>               putchar(' ');
>> -     if (d->worktree_status)
>> +     if (d->worktree_status) {
>> +             if (!s->submodule_porcelain1 &&
>
> Isn't this 's->status_format != STATUS_FORMAT_PORCELAIN'?

yes.

>> +                 (d->dirty_submodule || d->new_submodule_commits)) {
>> +                     /* It is a submodule, and we're not in plumbing mode. */
>> +                     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 = '?';
>> +             }
>
> Is this the right place to do this?  Could wt_status_collect_changed_cb set
> worktree_status to the right value in the first place based on whether
> 'status_format == STATUS_FORMAT_SHORT' or would that have undesirable
> knock-on effects elsewhere?
>

yes that would be better. I totally missed status_format as a variable.

>> +     int submodule_porcelain1;
>
> As described above, this shouldn't be needed since status_format
> already has this information.

good point. will omit this variable.

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

[Jonathan Nieder]

Stefan Beller wrote:
> Jonathan Nieder wrote:

>> Is it important to avoid clutter by showing the submodule only once?
>> What would you think of showing whatever subset of those three
>> statuses apply to a given submodule as separate lines instead, to
>> match the information that long-form "git status" shows?
>
> I considered it, but it would break the visual appeal of git status --short ?

I could go either way.  As long as you've thought about it, I'm happy.

>> How should a new untracked file in a submodule of a submodule be
>> shown?
>
> The same way " ?" indicates that (1) there is an untracked file due to
> the question mark and (2) that you need to recurse because it differs from
> "??" for regular untracked files.
>
> The problem here is that we do not know about these nested untracked files,
> because we use --porcelain instead of --short for submodules in
> submodule.c#is_submodule_modified(). I am rewriting that function anyway
> for the "git-describe --dirty" bug, so maybe it's time to switch to porcelain=2
> internally there, which can surface untracked files in nested subs.

Punting to a TODO / separate patch sounds reasonable.  Tests in this
patch describing either the current behavior or the desired behavior
would be helpful, though.

Thanks,
Jonathan