Re: [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2

From: Stefan Beller <sbeller@google.com>
Date: Mar 23, 2017, 18:47 UTC
Message-ID: <CAGZ79kZriqCE4-9bVQTNGXEZsJi-Z+ryp3QRu-MC8Sn1kaGSdg@mail.gmail.com>
In-Reply-To: <xmqqo9wsfrsr.fsf@gitster.mtv.corp.google.com>

On Wed, Mar 22, 2017 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Stefan Beller wrote:
>>
>>> Migrate 'is_submodule_modified' to the new porcelain format of
>>> git-status.
>>>
>>> As the old porcelain only reported ' M' for submodules, no
>>> matter what happened inside the submodule (untracked files,
>>> changes to tracked files or move of HEAD), the new API
>>> properly reports the different scenarios.
>> [...]
>>>  submodule.c | 53 ++++++++++++++++++++++++-----------------------------
>>>  1 file changed, 24 insertions(+), 29 deletions(-)
>>
>> Neat.  Is this something that could be covered in tests, or should I
>> be patient and rely on patch 3/3 for that?

I am not sure how to cover it in tests properly, as we do not expose this function to the outside directly.

This function is used in only one place (diff-lib.c, in match_stat_with_submodule, which itself is used in run_diff_files and get_stat_data), which is deep down in the diff library.

>> I think this would be easier to understand if it were two patches: one
>> that switched to --porcelain=2 with no change in behavior,

I don't think so as it would double the code to review. I'll see if I can present this conversion in an easier way.

> That sounds like a sensible organization.

ok.

Thanks, Stefan