[GSoC][PATCH 0/4] t7401: modernize, cleanup and warn

[GSoC][PATCH 0/4] t7401: modernize, cleanup and warn

[Shourya Shukla]

Greetings,

The current phase of my GSoC project involves porting
'git submodule summary' from shell to C. While doing so, I, along with
my mentors Christian and Kaartic noticed some discrepancies in the test
script when trying to add a couple of tests to it. Though the test works
perfectly for my C port of 'summary', there were some unexpected behaviours
when trying to some tests to it. This patch series addresses these
issues in the test script by modernizing it, cleaning it up and warning
about some other issues.

Chiefly about patch 4/4 (t7401: add a WARNING and a NEEDSWORK),
when trying to write a test for verifying the summary output of
deinitialized submodule, doing a 'git submodule deinit <path>' did not
bear any fruit since the submodule never really got deinitialized. 
The deinit documentation states that:

	Unregister the given submodules, i.e. remove the whole
	submodule.$name section from .git/config together with
	their work tree.

Something which was not actually happening in the test. It appeared
that the reason for the deinit issue is that the test script uses
'git add' to add submodules instead of the command 'git submodule add'.

This behaviour also prompted the need to design a new test script to
have a testing of some niche cases such as those stated before, but
this is something that will be covered in the patch series responsible
for porting the 'summary' subcommand to C.

Comments and reviews are appreciated.

Thanks,
Shourya Shukla

Shourya Shukla (4):
  t7401: modernize style
  t7401: change test_i18ncmp syntax for clarity
  t7401: ensure uniformity in the '--for-status' test
  t7401: add a WARNING and a NEEDSWORK

 t/t7401-submodule-summary.sh | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

-- 
2.27.0

[PATCH 1/4] t7401: modernize style

[Shourya Shukla]

The tests in 't7401-submodule-summary.sh' were written a long time ago
and have some violations with respect to our CodingGuidelines such as
incorrect spacing in usages of the redirection operator and absence
of line feed between statements in case of the '|' (pipe) operator.
Update it to match the CodingGuidelines.

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 9bc841d085..4439fb7c17 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -16,12 +16,13 @@ add_file () {
 	owd=$(pwd)
 	cd "$sm"
 	for name; do
-		echo "$name" > "$name" &&
+		echo "$name" >"$name" &&
 		git add "$name" &&
 		test_tick &&
 		git commit -m "Add $name"
 	done >/dev/null
-	git rev-parse --verify HEAD | cut -c1-7
+	git rev-parse --verify HEAD |
+	cut -c1-7
 	cd "$owd"
 }
 commit_file () {
@@ -125,7 +126,8 @@ commit_file sm1 &&
 head3=$(
 	cd sm1 &&
 	git reset --hard HEAD~2 >/dev/null &&
-	git rev-parse --verify HEAD | cut -c1-7
+	git rev-parse --verify HEAD |
+	cut -c1-7
 )
 
 test_expect_success 'modified submodule(backward)' "
-- 
2.27.0

[PATCH 2/4] t7401: change test_i18ncmp syntax for clarity

[Shourya Shukla]

Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to
'test_i18ncmp expected actual' to align it with the convention followed
by other tests in the test script.

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 4439fb7c17..18fefdb0ba 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -183,7 +183,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' "
   < Add foo5
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 test_expect_success 'typechanged submodule(submodule->blob), --files' "
@@ -193,7 +193,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --files' "
   > Add foo5
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 rm -rf sm1 &&
@@ -204,7 +204,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' "
 * sm1 $head4(submodule)->$head5(blob):
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 rm -f sm1 &&
@@ -217,7 +217,7 @@ test_expect_success 'nonexistent commit' "
   Warn: sm1 doesn't contain commit $head4_full
 
 EOF
-	test_i18ncmp actual expected
+	test_i18ncmp expected actual
 "
 
 commit_file
-- 
2.27.0

[PATCH 3/4] t7401: ensure uniformity in the '--for-status' test

[Shourya Shukla]

The '--for-status' test got its expected output from stdin. This is
inconsistent with the other tests in the test script which get their
expected output from a file named 'expected'.

So, change the syntax of the '--for-status' test for uniformity.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 18fefdb0ba..145914cd5a 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -285,13 +285,14 @@ EOF
 
 test_expect_success '--for-status' "
 	git submodule summary --for-status HEAD^ >actual &&
-	test_i18ncmp actual - <<EOF
+	cat >expected <<-EOF &&
 * sm1 $head6...0000000:
 
 * sm2 0000000...$head7 (2):
   > Add foo9
 
 EOF
+	test_i18ncmp expected actual
 "
 
 test_expect_success 'fail when using --files together with --cached' "
-- 
2.27.0

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

[Shourya Shukla]

Add a WARNING regarding the usage of 'git add' instead of 'git
submodule add' to add submodules to the superproject. Also add a
NEEDSWORK regarding the outdated syntax and working of the test, which
may need to be improved to obtain better and desired results.

While at it, change the word 'test' to 'test script' in the test
description to avoid ambiguity.

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

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

[Christian Couder]

On Thu, Aug 6, 2020 at 11:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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/

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

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

[Junio C Hamano]

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.

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

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

[Shourya Shukla]

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.

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?

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

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

[Junio C Hamano]

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

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

[Taylor Blau]

On Wed, Aug 05, 2020 at 11:19:21PM +0530, Shourya Shukla wrote:
> Add a WARNING regarding the usage of 'git add' instead of 'git
> submodule add' to add submodules to the superproject. Also add a
> NEEDSWORK regarding the outdated syntax and working of the test, which
> may need to be improved to obtain better and desired results.
>
> While at it, change the word 'test' to 'test script' in the test
> description to avoid ambiguity.
>
> 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.

It would be worth saying why, especially if you're re-rolling anyway.
Even something as simple as: "there are lots of commands taking place
outside of a 'test_expect_{success,failure}' block, which is no longer
in good-style".

I also wouldn't be upset to see some of those fixed up in this series,
but I realize that may be a bigger endeavor than you are willing to take
on for now.

>
>  . ./test-lib.sh
>
> --
> 2.27.0

Thanks,
Taylor

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

[Shourya Shukla]

Add a WARNING regarding the usage of 'git add' instead of 'git
submodule add' to add submodules to the superproject. Also add a
NEEDSWORK regarding the outdated syntax and working of the test, which
may need to be improved to obtain better and desired results.

While at it, change the word 'test' to 'test script' in the test
description to avoid ambiguity.

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