Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> When a nested submodule has untracked files, it would be reported as
> "modified submodule" in the superproject, because submodules are not
> parsed correctly in is_submodule_modified as they are bucketed into
> the modified pile as "they are not an untracked file".

I cannot quite parse the above.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c                 | 23 +++++++++++++++++++++--
>  t/t3600-rm.sh               |  2 +-
>  t/t7506-status-submodule.sh |  2 +-
>  3 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index fa21c7bb72..730cc9513a 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
>  		/* regular untracked files */
>  		if (buf.buf[0] == '?')
>  			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> -		else
> -			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> +
> +		if (buf.buf[0] == 'u' ||
> +		    buf.buf[0] == '1' ||
> +		    buf.buf[0] == '2') {
> +			/*
> +			 * T XY SSSS:
> +			 * T = line type, XY = status, SSSS = submodule state
> +			 */
> +			if (buf.len < 1 + 1 + 2 + 1 + 4)
> +				die("BUG: invalid status --porcelain=2 line %s",
> +				    buf.buf);
> +
> +			/* regular unmerged and renamed files */
> +			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
> +				/* nested untracked file */
> +				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;

OK, we have untracked one.

> +			if (memcmp(buf.buf + 5, "S..U", 4))
> +				/* other change */
> +				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

And for other cases like C at buf.buf[6] or M at buf.buf[7],
i.e. where the submodule not just has untracked files but has real
changes, we say it is truly MODIFIED here.

If there are changes to paths that is not a submodule but a tracked
file in the submodule in question would have N at buf.buf[5] and is
also caught with the same "not S..U so that's MODIFIED" logic.

OK.

Shouldn't this done as part of 4/7 where is_submodule_modified()
starts reading from the porcelain v2 output?  4/7 does adjust for
the change from double question mark (porcelain v1) to a single one
for untracked, but at the same time it needs to prepare for these
'u' (unmerged), '1' (normal modification) and '2' (mods with rename)
to appear in the output, no?

IOW, with 4/7 and 7/7 done as separate steps, isn't the system
broken between these steps?