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

From: Shourya Shukla <shouryashukla.oo@gmail.com>
Date: Aug 6, 2020, 11:27 UTC
Message-ID: <20200806112702.GA15343@konoha>
In-Reply-To: <xmqqy2msn5b2.fsf@gitster.c.googlers.com>

On 05/08 02:36, Junio C Hamano wrote:

Show 12 quoted lines

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

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?

Show 39 quoted lines

> > NEEDSWORK regarding the outdated syntax and working of the test, which
> > may need to be improved to obtain better and desired results.
> 
> Sounds good.
> 
> > While at it, change the word 'test' to 'test script' in the test
> > description to avoid ambiguity.
> 
> Sounds good.  I often search for a pair of phrases to differentiate
> a single tXXXX-name.sh file as a whole and an individual test piece
> in it.  "This test script", especially when written near the
> beginning of the file, is a good way to clearly convey that you want
> to refer to the former.
> 
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> > ---
> >  t/t7401-submodule-summary.sh | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> > index 145914cd5a..2db4cf5cbf 100755
> > --- a/t/t7401-submodule-summary.sh
> > +++ b/t/t7401-submodule-summary.sh
> > @@ -5,8 +5,13 @@
> >  
> >  test_description='Summary support for submodules
> >  
> > -This test tries to verify the sanity of summary subcommand of git submodule.
> > +This test script tries to verify the sanity of summary subcommand of git submodule.
> >  '
> > +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add
> > +# submodules to the superproject. Some submodule subcommands such as init and
> > +# deinit might not work as expected in this script.
> > +
> > +# NEEDSWORK: This test script is old fashioned and may need a big cleanup.
> >  
> >  . ./test-lib.sh