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@xxxxxxxxx>
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