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

From: Stefan Beller <sbeller@google.com>
Date: Mar 25, 2017, 00:25 UTC
Message-ID: <CAGZ79kaYSMoMEQ8y+xwLT65suva_DY3ZWKfKSeCPPu2hgtYt-w@mail.gmail.com>
In-Reply-To: <20170324233156.GL31294@aiede.mtv.corp.google.com>

On Fri, Mar 24, 2017 at 4:31 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 19 quoted lines

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

good idea, will use.

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

That means there is a submodule with no new commits, no modified files and no untracked files. ("Why did we even output this?", oh because of it is in either 2 (moved) or because the hashes are different, i.e. we already added the new HEAD of the submodule)

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

ok, will do.