Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Glen Choo <chooglen@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, Atharva Raykar <raykar.ath@gmail.com>,
	Christian Couder <christian.couder@gmail.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Jonathan Nieder <jrn@google.com>,
	Kaartic Sivaraam <kaartic.sivaraam@gmail.com>,
	pc44800@gmail.com, Shourya Shukla <periperidip@gmail.com>
Subject: Re: [PATCH v7 00/20] submodule: convert the rest of 'update' to C
Date: Tue, 15 Feb 2022 17:34:38 +0800	[thread overview]
Message-ID: <kl6lzgms5v80.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <220214.86h791xsza.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Feb 15 2022, Glen Choo wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> This seems to heavily conflict with "clone, submodule: pass partial
>>>> clone filters to submodules, 2022-02-04" by Josh Steadmon
>>>> <690d2316ad518ea4551821b2b3aa652996858475.1644034886.git.steadmon@google.com>
>>>> in both builtins/submodule--helper.c and git-submodule.sh.
>>>>
>>>> It also removes the code that "submodule: record superproject gitdir
>>>> during 'update', 2022-02-03" by Emily Shaffer
>>>> <20220203215914.683922-5-emilyshaffer@google.com>, so what the other
>>>> topic ends up adding to the shell script needs to eventually be
>>>> redone in the C code.
>>>>
>>>> I think "superproject aware" topic would see a reroll due to a
>>>> slight redesign.  I am not sure how solid the design of the
>>>> "pass down partial clone filter" topic is at this moment.
>>
>> Hm, I haven't looked at where the conflicts are yet, but I'll get to it
>> as I'm reviewing the rest of the feedback.
>
> To save you some time, my v4 CL summarizes the semantic conflict between
> the two:
> https://lore.kernel.org/git/cover-v4-0.7-00000000000-20220127T143552Z-avarab@gmail.com/
>
> I.e. Atharva Raykar had working C code to do what an older version of
> that superproject config series was doing, but the semantics changed in
> a later version. It needs some new usage of path.c (or similar) API
> adjusted, but I didn't (and still don't) have time to look into it.

Ah, thanks for the reminder. That should help.

>>> I can merge this to seen minus the above two topics and get it
>>> compile, but it also seems to have some interaction with 961b130d
>>> (branch: add --recurse-submodules option for branch creation,
>>> 2022-01-28) and makes the t3207, tests added by that other topic,
>>> fail X-<.
>>
>> Oof, that's embarrassing of me, let me take a look at that. There's a
>> nontrivial chance that the "branch --recurse-submodules" tests caught an
>> actual regression.
>
> FWIW one thing I did as an extra sanity check was to run the whole test
> suite with --tee with/without this series (or rather, my earlier
> version), and diffing the full test-results output (which you'll need to
> save in-between the two runs, and IIRC hack t/Makefile to stop removing
> it on successful runs).
>
> There's a lot of differences in output due to general issues in the test
> suite output not being reproducable (writing timestamps etc.), but I
> could not find any issues with the "git submodule" output being
> different, of course we may not have tests, or it may be piped to
> /dev/null....
>
> But I've found it to be a helpful additional validation technique for
> this series & others where I'm not as confident in the test coverage.

Thanks for the tip! I hadn't considered trying this, but this makes a
lot of sense. I can see this being even more useful for this series
since it's supposed to be a faithful conversion of sh->c.