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