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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 24, 2017, 23:31 UTC
Message-ID: <20170324233156.GL31294@aiede.mtv.corp.google.com>
In-Reply-To: <20170324182902.19280-8-sbeller@google.com>

Stefan Beller wrote:

Show 28 quoted lines

> 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".
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c                 | 16 ++++++++++++++--
>  t/t3600-rm.sh               |  2 +-
>  t/t7506-status-submodule.sh |  2 +-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 467f1de763..ec7e9b1b06 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1078,8 +1078,20 @@ 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;
> +
> +		/* regular unmerged and renamed files */
> +		if (buf.buf[0] == 'u' ||
> +		    buf.buf[0] == '1' ||
> +		    buf.buf[0] == '2') {
> +			if (buf.buf[5] == 'S') {

Can this overflow the buffer? Submodule state is supposed to be 4 characters, so could do

			/*
			 * 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);

			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
				/* untracked file */
				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;

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

Show 8 quoted lines

> +				/* nested submodule handling */
> +				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
> +					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> +				if (buf.buf[8] == 'U')
> +					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> +			} else
> +				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> +		}

I get lost in these cases. What does it mean if we see S..., for example?

As an example, if I am understanding correctly, before this patch, if I have a submodule-in-submodule:

	$ find . -name .git
	.git
	sub/.git
	sub/subsub/.git

and I put a stray file in the sub-sub module:

	$ echo stray-file >sub/subsub/x.o

then status --porcelain=2 tells me that sub is modified:

	$ git status --porcelain=2
	1 .M S.M. [...] sub

What should it say after the patch?  Is the XY field ".." or ".M"?

Some tests covering these cases with --porcelain=2 and a brief mention in documentation may help.

Thanks, Jonathan

diff --git i/submodule.c w/submodule.c
index ec7e9b1b06..aec1b2cdca 100644
--- i/submodule.c
+++ w/submodule.c
@@ -1083,13 +1083,18 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		if (buf.buf[0] == 'u' ||
 		    buf.buf[0] == '1' ||
 		    buf.buf[0] == '2') {
-			if (buf.buf[5] == 'S') {
-				/* nested submodule handling */
-				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
-					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-				if (buf.buf[8] == 'U')
-					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			} else
+			/*
+			 * 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);
+			if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+				/* untracked file */
+				dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			if (memcmp(buf.buf + 5, "S..U", 4))
+				/* other change */
 				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}