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

[Junio C Hamano]

Jonathan Nieder  writes:

> Stefan Beller wrote:
>
>> This bug fix also affects the default output (non-short, non-porcelain)
>> of git-status, which is not tested here.
>
> Do you have an example?  (In just the commit message would be fine, in
> tests would be even better.)
>
>> Signed-off-by: Stefan Beller 
>> ---
>>  Documentation/git-status.txt |  2 ++
>>  submodule.c  | 21 +++--
>>  t/t3600-rm.sh|  2 +-
>>  t/t7506-status-submodule.sh  |  4 ++--
>>  4 files changed, 24 insertions(+), 5 deletions(-)
>
> Reviewed-by: Jonathan Nieder 
>
> but I would be a lot more comfortable after looking at the change to
> "git status" output.  (E.g. a test demonstrating it can happen in a
> followup change if that's simpler.)
>
> Thanks for your patient work on this.

Thank you both.  I re-read the whole thing and it feels to me that
the topic is now ready for 'next'.

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

[Jonathan Nieder]

Stefan Beller wrote:

> This bug fix also affects the default output (non-short, non-porcelain)
> of git-status, which is not tested here.

Do you have an example?  (In just the commit message would be fine, in
tests would be even better.)

> Signed-off-by: Stefan Beller 
> ---
>  Documentation/git-status.txt |  2 ++
>  submodule.c  | 21 +++--
>  t/t3600-rm.sh|  2 +-
>  t/t7506-status-submodule.sh  |  4 ++--
>  4 files changed, 24 insertions(+), 5 deletions(-)

Reviewed-by: Jonathan Nieder 

but I would be a lot more comfortable after looking at the change to
"git status" output.  (E.g. a test demonstrating it can happen in a
followup change if that's simpler.)

Thanks for your patient work on this.

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

[Stefan Beller]

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
displayed as an untracked file on the 'super' level as well.

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.

This bug fix also affects the default output (non-short, non-porcelain)
of git-status, which is not tested here.

Signed-off-by: Stefan Beller 
---
 Documentation/git-status.txt |  2 ++
 submodule.c  | 21 +++--
 t/t3600-rm.sh|  2 +-
 t/t7506-status-submodule.sh  |  4 ++--
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 67f1a910f3..d70abc6afe 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -189,6 +189,8 @@ Submodules have more state and instead report
 since modified content or untracked files in a submodule cannot be added
 via `git add` in the superproject to prepare a commit.
 
+'m' and '?' are applied recursively. For example if a nested submodule
+in a submodule contains an untracked file, this is reported as '?' as well.
 
 If -b is used the short-format status is preceded by a line
 
diff --git a/submodule.c b/submodule.c
index fa21c7bb72..3da65100e3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1078,8 +1078,25 @@ 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 = line type, XY = status,  = submodule state */
+   if (buf.len < strlen("T XY "))
+   die("BUG: invalid status --porcelain=2 line %s",
+   buf.buf);
+
+   if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+   /* nested untracked file */
+   dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+   if (buf.buf[0] == 'u' ||
+   buf.buf[0] == '2' ||
+   memcmp(buf.buf + 5, "S..U", 4))
+   /* other change */
+   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+   }
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested untracked fi
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified_inside actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 1fa2ff2909..055c90736e 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -356,7 +356,7 @@ test_expect_success 'status with untracked file in nested 
submodule (porcelain=2
git -C super status --porcelain=2 >output &&
sanitize_output output &&
diff output - <<-\EOF
-   1 .M S.M. 16 16 16 HASH HASH sub1
+   1 .M S..U 16 16 16 HASH HASH sub1
1 .M S..U 16 16 16 HASH HASH sub2
1 .M S..U 16 16 16 HASH HASH sub3
EOF
@@ -365,7 +365,7 @@ test_expect_success 'status with untracked file in nested 
submodule (porcelain=2
 test_expect_success 'status with untracked file in nested submodule (short)' '
git -C super status --short >output &&
diff output - <<-\EOF
-m sub1
+? sub1
 ? sub2
   

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

[Stefan Beller]

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

Oh. :(

In case of 'u' and '2' we need to set DIRTY_SUBMODULE_MODIFIED
additionally. will fix in a reroll.

>
> 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".

All these cases are ' m', which I agree with, as it is a "modification
that cannot be git-add'ed in the superproject".

We might be inclined to later come up with  ' d' for a deleted nested
submodule, but I do not think it is worth the effort.

Thanks,
Stefan

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

[Jonathan Nieder]

Stefan Beller wrote:

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

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

[...]
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -187,6 +187,8 @@ Submodules have more state and instead report
>   mthe 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.

[...]
> --- 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 :
> +  * T = line type, XY = status,  = submodule state
> +  */
> + if (buf.len < 1 + 1 + 2 + 1 + 4)

optional: Can shorten:

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

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.

[...]
> + 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

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

[Stefan Beller]

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.

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.

Signed-off-by: Stefan Beller 
---
 Documentation/git-status.txt |  2 ++
 submodule.c  | 23 +--
 t/t3600-rm.sh|  2 +-
 t/t7506-status-submodule.sh  |  2 +-
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 01b457c322..452c6eb875 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -187,6 +187,8 @@ Submodules have more state and instead report
mthe 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.
 
 If -b is used the short-format status is preceded by a line
 
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 :
+* T = line type, XY = status,  = 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;
+
+   if (memcmp(buf.buf + 5, "S..U", 4))
+   /* other change */
+   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+   }
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested untracked fi
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified_inside actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index fd057751df..4d6d8f6817 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -346,7 +346,7 @@ test_expect_success 'status with untracked file in nested 
submodule (porcelain=2
git -C super status --porcelain=2 >output &&
awk -f suppress_hashes.awk output >output2 &&
diff output2 - <<-\EOF
-   1 .M S.M. 16 16 16 HASH HASH sub1
+   1 .M S..U 16 16 16 HASH HASH sub1
1 .M S..U 16 16 16 HASH HASH sub2
EOF
 '
-- 
2.12.1.438.g67623a8358