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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 28, 2017, 23:42 UTC
Message-ID: <20170328234217.GU31294@aiede.mtv.corp.google.com>
In-Reply-To: <20170328230938.9887-3-sbeller@google.com>

Stefan Beller wrote:

Show 15 quoted lines

> Suppose I have a superproject 'super', with two submodules 'super/sub'
> and 'super/sub1'. 'super/sub' itself contains a submodule
> 'super/sub/subsub'. Now suppose I run, from within 'super':
>
>     echo hi >sub/subsub/stray-file
>     echo hi >sub1/stray-file
>
> Currently we get would see the following output in git-status:
>
>     git status --short
>      m sub
>      ? sub1
>
> With this patch applied, the untracked file in the nested submodule is
> turned into an untracked file on the 'super' level as well.

s/turned into/displayed as/

Show 12 quoted lines

>     git status --short
>      ? sub
>      ? sub1
>
> This doesn't change the output of 'git status --porcelain=1' for nested
> submodules, because its output is always ' M' for either untracked files
> or local modifications no matter the nesting level of the submodule.
>
> 'git status --porcelain=2' is affected by this change in a nested
> submodule, though. Without this patch it would report the direct submodule
> as modified and having no untracked files. With this patch it would report
> untracked files. Chalk this up as a bug fix.

I think that's reasonable, and the documentation does a good job of describing it.

Does this have any effect on the default mode of 'git status' (without --short or --porcelain)?

[...]

Show 8 quoted lines

> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -187,6 +187,8 @@ Submodules have more state and instead report
>  		m    the submodule has modified content
>  		?    the submodule has untracked files
>  
> +Note that 'm' and '?' are applied recursively, e.g. if a nested submodule
> +in a submodule contains an untracked file, this is reported as '?' as well.

Language nits:

* Can simplify by leaving out "Note that ".
* s/, e\.g\./. For example,/

Should this say a brief word about rationale? The obvious way to describe it would involve "git add --recurse-submodules", which alas doesn't exist yet. But we could say

                                              this is reported as '?' as well,
   since the change cannot be added using "git add -u" from within any of the
   submodules.

[...]

Show 17 quoted lines

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

optional: Can shorten:

			/* T = line type, XY = status, SSSS = submodule state */
			if (buf.len < strlen("T XY SSSS"))
				...

That produces the same code at run time because compilers are able to inline the strlen of a constant. What you already have also seems sensible, though.

[...]

Show 11 quoted lines

> +				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;
> +
> +			if (memcmp(buf.buf + 5, "S..U", 4))
> +				/* other change */
> +				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

sanity check: What does this do for a "2" line indicating a sub-submodule that has been renamed that contains an untracked file? Do we need to rely on some other indication to show this as a change?

Enumerating some more cases, since I keep finding myself getting lost:

 - if the HEAD commit of "sub" changes, we show this as " M sub".
   What should we show if the HEAD commit of "sub/subsub" changes?
   I think this should be " m".

 - if "sub" is renamed, we show this as "R  sub -> newname".
   What should we show if "sub/subsub" is renamed?  It is tempting
   to show this as " m".

 - if "sub" is deleted, we show this as "D  sub". What should we
   show if "sub/subsub" is deleted? I think this is " m".

It keeps getting more complicated, but I think this is making sense.

Thanks and hope that helps, Jonathan