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

[Jonathan Nieder <jrnieder@gmail.com>]

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