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