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

[Stefan Beller <sbeller@google.com>]

On Mon, Mar 27, 2017 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I tried to describe the example Jonathan gave in his reply in a shorter form.
I'll
>> +                     /* 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.

ok, thanks for checking.

>
> Shouldn't this done as part of 4/7 where is_submodule_modified()
> starts reading from the porcelain v2 output?

I did that in an earlier version of the series. However the change from
porcelain=1 to 2 should not be observable by the end user.

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

No, up to patch 5/7 we only had refactors, no user visible changes
intended. And until then we had "has untracked files" and "everything
else". The nice part of the conversion was to cover the "everything else"
part easily.

This patch transforms it into "has untracked files or submodule reports
untracked files (possibly nested)" and "everything else", but the former
is more complicated to compute.

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

No, see Jonathans answer.

We could argue, that 6/7 and 7/7 done as separate steps is broken,
because since 6/7 we promise to report untracked files inside
submodules as " ?", but we do not report them properly for nested
submodules.

Suppose I have a superproject 'parent', with two submodules 'parent/sub'
and 'parent/sub1'. 'parent/sub' itself contains a submodule 'parent/sub/subsub'.
Now suppose I run, from within 'parent':

    echo hi >sub/subsub/stray-file
    echo hi >sub1/stray-file

with patches 1..5:
    git status --short
     M sub
     M sub1

patch 6:
    git status --short
     m sub
     ? sub1

with patch 7:
    git status --short
     ? sub
     ? sub1

The documentation update in patch 6 is unclear what to expect from
nested submodules, so that documentation is technically not wrong,
but maybe misleading.

I want to resend patch 7 with a documentation update as well as tests.

Thanks,
Stefan