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

From: Stefan Beller <sbeller@google.com>
Date: Mar 28, 2017, 21:20 UTC
Message-ID: <CAGZ79kYmWW5tp26aCkWeYjyp8KeP2jcL43JGiPO3pHW=wo7uzw@mail.gmail.com>
In-Reply-To: <xmqqpoh2z93q.fsf@gitster.mtv.corp.google.com>

On Mon, Mar 27, 2017 at 2:46 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

> 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

Show 20 quoted lines

>> +                     /* 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.

Show 5 quoted lines

>  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