Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 7, 2020, 14:55 UTC
Message-ID: <CAP8UFD21i6UOwSA8hoCbF9kbRaaQzmN4e0t+pPrF1fW+SSJwww@mail.gmail.com>
In-Reply-To: <xmqqa6z7jx7q.fsf@gitster.c.googlers.com>

On Thu, Aug 6, 2020 at 11:11 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

>
> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>
> > On 05/08 02:36, Junio C Hamano wrote:
> >> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> >>
> >> > Add a WARNING regarding the usage of 'git add' instead of 'git
> >> > submodule add' to add submodules to the superproject.
> >>
> >> Is that a warning worthy thing?  As far as I know, using "git add"
> >> to register a gitlink is perfectly fine and a supported way to start
> >> a new submodule.  It may have to be followed by other steps like
> >> "git config -f .gitmodules" (e.g. when operations that needs to use
> >> the contents recorded in the .gitmodules file are to be tested), but
> >> writing tests using lower-level ingredients for finer grained tests
> >> is not all that unusual, is it?  I dunno.
> >
> > The thing is that 'git submodule {init,deinit}' fail when there is no
> > .gitmodules. I can initiliase the .gitmodules separately using 'git
> > config -f .gitmodules' but I think it will be better to use 'git
> > submodule add' throughout the script rather than worry about it all the
> > time.
>
> On the other hand, we do want to make sure that the workflow using
> lower-level tools continues to work, so that is a balancing act.

Yeah, that's the reason why we suggested to add the new tests in a new test script t7421:

https://lore.kernel.org/git/20200806164102.6707-5-shouryashukla.oo@gmail.com/

Show 8 quoted lines

> > But again, if the warning seems unnecessary, then I can obviously use
> > 'git config' to initilaise the submodules and change this commit. What
> > do you reckon?
>
> If you need "git submodule init" etc. to work in this test, yes, you
> can change the test to either use "git submodule add" instead, or
> "git config -f .gitmodules" in addition.  If you don't, there is no
> need to change anything, no?

In t7421 "git submodule add" is used, so that we can perform the new tests we want there. The purpose of this patch adding a WARNING and a NEEDSWORK was to tell explicitly that this test script (t7401) is not necessarily the best test script for "git submodule summary" tests because it is very old fashioned as it has a lot of boilerplate stuff outside test_expect_{success, failure} and it uses "git add" instead of "git submodule add".

I think we might want to just add the NEEDSWORK in this patch series and add the WARNING, or perhaps just a NOTE, about "git add" vs "git submodule add" in the other patch series when we introduce t7421.