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

From: Junio C Hamano <gitster@pobox.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
	kaartic.sivaraam@gmail.com, johannes.schindelin@gmx.de,
	liu.denton@gmail.com, Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK
Date: Wed, 05 Aug 2020 14:36:33 -0700	[thread overview]
Message-ID: <xmqqy2msn5b2.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200805174921.16000-5-shouryashukla.oo@gmail.com> (Shourya Shukla's message of "Wed, 5 Aug 2020 23:19:21 +0530")

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.

> 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