git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Mar 16, 2017, 22:19 UTC
Message-ID
<20170316221907.GA9135@aiede.mtv.corp.google.com>
In-Reply-To
<20170316204644.11616-1-sbeller@google.com>
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.
Show 11 quoted lines
> 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@google.com>
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?

[...]
Show 7 quoted lines
> --- 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/
Show 6 quoted lines
> @@ -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
[...]
Show 9 quoted lines
> --- 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'?
[...]
Show 9 quoted lines
> +		    (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?

[...]
Show 7 quoted lines
> --- 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

Previous: Stefan BellerNext: Stefan Beller
Message 5 of 7 in “short status: improve reporting for submodule changes”
  1. short status: improve reporting for submodule changesStefan Beller, Mar 16, 2017
  2. Junio C HamanoMar 16, 2017
  3. Stefan BellerMar 16, 2017
  4. short status: improve reporting for submodule changesStefan Beller, Mar 16, 2017
  5. Jonathan NiederMar 16, 2017
  6. Stefan BellerMar 16, 2017
  7. Jonathan NiederMar 16, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.