Re: [PATCH 3/3] short status: improve reporting for submodule changes
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Mar 23, 2017, 01:06 UTC
- Message-ID
- <20170323010605.GJ26108@aiede.mtv.corp.google.com>
- In-Reply-To
- <20170323004329.15892-4-sbeller@google.com>
Stefan Beller wrote:
Show 5 quoted lines
> 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.
[...]
Show 15 quoted lines
> --- 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.)
Show 6 quoted lines
> + 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