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

[GSoC][RESEND][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 1/4] t7401: modernize style

[Denton Liu]

Hi Shourya,

On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:
> 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.

I'm not aware of anywhere in CodingGuidelines that says you can't have
the pipe operator on a single line. I assume you're referring to the
part that reads

	If a command sequence joined with && or || or | spans multiple
	lines, put each command on a separate line and put && and || and
	| operators at the end of each line, rather than the start.

Note that says "If a command sequence [...] spans multiple lines", which
doesn't apply in our case.

> 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

For the reason above, I disagree with this change as-is. However, one
useful thing that you can do here is breaking the pipe up entirely. We
want to avoid is having a git command in the upstream of a pipe. This is
because the return code of a pipe comes from the last command executed
so if the rev-parse fails, its return code is swallowed and we have no
way of knowing.

You could break the pipe up by storing the output of the rev-parse in an
intermediate file and then have cut read from that file.

Re: [PATCH 1/4] t7401: modernize style

[Taylor Blau]

On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote:
> Hi Shourya,
>
> On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:
> > 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.
>
> I'm not aware of anywhere in CodingGuidelines that says you can't have
> the pipe operator on a single line. I assume you're referring to the
> part that reads
>
> 	If a command sequence joined with && or || or | spans multiple
> 	lines, put each command on a separate line and put && and || and
> 	| operators at the end of each line, rather than the start.
>
> Note that says "If a command sequence [...] spans multiple lines", which
> doesn't apply in our case.
>
> > 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" &&

This change is good.

> >  		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
>
> For the reason above, I disagree with this change as-is. However, one
> useful thing that you can do here is breaking the pipe up entirely. We
> want to avoid is having a git command in the upstream of a pipe. This is
> because the return code of a pipe comes from the last command executed
> so if the rev-parse fails, its return code is swallowed and we have no
> way of knowing.
>
> You could break the pipe up by storing the output of the rev-parse in an
> intermediate file and then have cut read from that file.

This is a good suggestion (I was preparing to write an email to say the
same thing, but I'm glad that I checked Denton's response before doing
so). Something like:

	git rev-parse --verify HEAD >out &&
	cut -c1-7 out

would suffice and be in good style.

Thanks,
Taylor

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

[Taylor Blau]

On Wed, Aug 05, 2020 at 11:19:19PM +0530, Shourya Shukla wrote:
> 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

This all looks good, and matches a long-standing convention of having
the expected value as the first argument and the comparison value as the
latter argument.

There's one spot you missed, which could be addressed by folding in this
diff on top:

--- >8 ---

Subject: [PATCH] fixup! t7401: change test_i18ncmp syntax for clarity

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t7401-submodule-summary.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 18fefdb0ba..bd8fc8511a 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -285,12 +285,14 @@ EOF

 test_expect_success '--for-status' "
 	git submodule summary --for-status HEAD^ >actual &&
-	test_i18ncmp actual - <<EOF
-* sm1 $head6...0000000:
+	cat >expected <<-\EOF &&
+	* sm1 $head6...0000000:

-* sm2 0000000...$head7 (2):
-  > Add foo9
+	* sm2 0000000...$head7 (2):
+	  > Add foo9

+	EOF
+	test_i18ncmp expected actual
 EOF
 "

--
2.28.0.rc1.13.ge78abce653

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

[Taylor Blau]

On Wed, Aug 05, 2020 at 11:19:20PM +0530, Shourya Shukla wrote:
> 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.

... serves me right for not reading the whole thread before responding
to the previous patch ;).

On a technical note, I don't think this is different enough from the
previous patch that they couldn't be combined. (A good indicator of this
is that I expected this change to be included in 2/4 and was surprised
that it was a separate step afterwords).

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

I think that this is on the right track, but you can use a '<<-\EOF'
here instead of '<<-EOF' to make the tabulation a little more flexible.

>
>  test_expect_success 'fail when using --files together with --cached' "
> --
> 2.27.0

Thanks,
Taylor

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

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

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

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

Yeah, this is a good thing to do, as a failing test_cmp gives a diff
between the first file to the second file, i.e. a patch that turns
the expected output into what the tests actually produced, so that
the tester can see how the expectation is broken.
>
> 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

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

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

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

There are a handful examples in t5401 and another one in t3700 that
give the "golden master" from the standard input.  When the expected
output is used only once, I do not think it is particularlly bad to
have it this way.  So,... meh?

Unless there is a compelling reason to insist both are files
(that may make it easier to enhance test_cmp in a certain way
you plan to, for example), that is.

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

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 3/4] t7401: ensure uniformity in the '--for-status' test

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>>  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
>>  "
>
> I think that this is on the right track, but you can use a '<<-\EOF'
> here instead of '<<-EOF' to make the tabulation a little more flexible.

You are correct that the patch could have indented the here-doc and
the line with EOF with a tab to make it easier to read.

The leading '-' after '<<' is what controls tabulation, so <<-EOF as
written in the patch is good enough; you do not want to quote it
further, because $head6 wants to be interpolated.

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

[Taylor Blau]

On Wed, Aug 05, 2020 at 03:25:17PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >>  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
> >>  "
> >
> > I think that this is on the right track, but you can use a '<<-\EOF'
> > here instead of '<<-EOF' to make the tabulation a little more flexible.
>
> You are correct that the patch could have indented the here-doc and
> the line with EOF with a tab to make it easier to read.
>
> The leading '-' after '<<' is what controls tabulation, so <<-EOF as
> written in the patch is good enough; you do not want to quote it
> further, because $head6 wants to be interpolated.

Ah, I never actually knew the difference between those two (and
apparently had been too lazy to look it up myself). Thanks for the
clarification.

Thanks,
Taylor

Re: [PATCH 1/4] t7401: modernize style

[Shourya Shukla]

On 05/08 04:49, Taylor Blau wrote:
> On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote:
> > Hi Shourya,
> >
> > On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:
> > > 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.
> >
> > I'm not aware of anywhere in CodingGuidelines that says you can't have
> > the pipe operator on a single line. I assume you're referring to the
> > part that reads
> >
> > 	If a command sequence joined with && or || or | spans multiple
> > 	lines, put each command on a separate line and put && and || and
> > 	| operators at the end of each line, rather than the start.
> >
> > Note that says "If a command sequence [...] spans multiple lines", which
> > doesn't apply in our case.
> >
> > > 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" &&
> 
> This change is good.
> 
> > >  		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
> >
> > For the reason above, I disagree with this change as-is. However, one
> > useful thing that you can do here is breaking the pipe up entirely. We
> > want to avoid is having a git command in the upstream of a pipe. This is
> > because the return code of a pipe comes from the last command executed
> > so if the rev-parse fails, its return code is swallowed and we have no
> > way of knowing.
> >
> > You could break the pipe up by storing the output of the rev-parse in an
> > intermediate file and then have cut read from that file.
> 
> This is a good suggestion (I was preparing to write an email to say the
> same thing, but I'm glad that I checked Denton's response before doing
> so). Something like:
> 
> 	git rev-parse --verify HEAD >out &&
> 	cut -c1-7 out
> 
> would suffice and be in good style.

Sure. I will make the amends.

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

[Shourya Shukla]

On 05/08 02:30, Junio C Hamano wrote:
> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > 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.
> 
> There are a handful examples in t5401 and another one in t3700 that
> give the "golden master" from the standard input.  When the expected
> output is used only once, I do not think it is particularlly bad to
> have it this way.  So,... meh?

I realised what you were trying to say after checking out t5400 and
t3700. I understand that this change may not be immediately needed but I
think it does make reading the diff a bit easier since having a '-' as a
file name does get a bit confusing when reading the output. If a
separeate commit just for this change is a problem then I can squash
this with the previous commit? What do you think?

> Unless there is a compelling reason to insist both are files
> (that may make it easier to enhance test_cmp in a certain way
> you plan to, for example), that is.
> 
> > 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' "

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 3/4] t7401: ensure uniformity in the '--for-status' test

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> On 05/08 02:30, Junio C Hamano wrote:
>> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>> 
>> > 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.
>> 
>> There are a handful examples in t5401 and another one in t3700 that
>> give the "golden master" from the standard input.  When the expected
>> output is used only once, I do not think it is particularlly bad to
>> have it this way.  So,... meh?
>
> I realised what you were trying to say after checking out t5400 and
> t3700. I understand that this change may not be immediately needed but I
> think it does make reading the diff a bit easier since having a '-' as a
> file name does get a bit confusing when reading the output. 

If so, perhaps justifying the change based on that, not on
"consistency", would be a good idea.

        Side note: But would that mean you'd find it "confusing" to
        read output from 3700 and 5400?  Would "test writers should
        get used to it" be a workable alternative solution?

Since "test_cmp expect actual" and "test_cmp - actual <<HERE" are
_both_ valid forms that are useful for different situations, I do
not see a compelling reason to insist on one form is consistently
used and ban the use of the other.

So...

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

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

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

[Shourya Shukla]

Greetings,

This is the v2 of the previously posted patch series titled
't7401: modernize, cleanup and warn':
https://lore.kernel.org/git/20200805174921.16000-1-shouryashukla.oo@gmail.com/

After suggestions from Denton, Taylor and Junio I made some changes:

-> In 2939804509 (t7401: modernize style, 2020-07-23), after suggestions
   from Denton and Taylor, I redirected the output of the two
   'rev-parse' commands to a file and then read from it instead of using
   a pipe '|' operator.

-> Drop the commit ab28283b67 (t7401: ensure uniformity in the
   '--for-status' test, 2020-07-10) and instead combine it with
   00c6289d5e (t7401: change test_i18ncmp syntax for clarity, 2020-07-10).
   This was suggested by Junio and Taylor.

-> Introduce the commit f0b87ddaf6 (t7401: change indentation for
   enhanced readability, 2020-08-11) which improves the indentation of
   the tests in t7401. This was suggested by Junio and Taylor.

-> Remove the WARNING from the commit 0bdb0bd72c (t7401: add a WARNING
   and a NEEDSWORK, 2020-07-23) and instead limit it to a more improved
   NEEDSWORK thus leading to the commit a743c28d71 (t7401: add a
   NEEDSWORK, 2020-07-23). This was suggested by Taylor.

Feedback and reviews are appreciated. I am tagging along a range-diff
between the v1 and v2 for ease of review.

Regards,
Shourya Shukla

-----
range-diff:

1:  31ae4038f1 ! 1:  2939804509 t7401: modernize style
    @@ Commit message
         t7401: modernize style

         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.
    +    and has a violation with respect to our CodingGuidelines which is,
    +    incorrect spacing in usages of the redirection operator.
    +    Using a Git command in the upstream of a pipe might result in us
    +    losing its exit code. So, convert such usages so that they write to
    +    a file and read from them.

         Mentored-by: Christian Couder <chriscool@tuxfamily.org>
         Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
    +    Helped-by: Denton Liu <liu.denton@gmail.com>
    +    Helped-by: Taylor Blau <me@ttaylorr.com>
         Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>

      ## t/t7401-submodule-summary.sh ##
    @@ t/t7401-submodule-summary.sh: add_file () {
                git commit -m "Add $name"
                git commit -m "Add $name"
        done >/dev/null
     -  git rev-parse --verify HEAD | cut -c1-7
    -+  git rev-parse --verify HEAD |
    -+  cut -c1-7
    ++  git rev-parse --verify HEAD >out &&
    ++  cut -c1-7 out
        cd "$owd"
      }
      commit_file () {
    @@ t/t7401-submodule-summary.sh: commit_file sm1 &&
        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
    ++  git rev-parse --verify HEAD >out &&
    ++  cut -c1-7 out
      )

      test_expect_success 'modified submodule(backward)' "
2:  a3160d1ecc ! 2:  00c6289d5e t7401: change test_i18ncmp syntax for clarity
    @@ t/t7401-submodule-summary.sh: test_expect_success 'nonexistent commit' "
      "

      commit_file
    +@@ t/t7401-submodule-summary.sh: EOF
    +
    + test_expect_success '--for-status' "
    +   git submodule summary --for-status HEAD^ >actual &&
    +-  test_i18ncmp actual - <<EOF
    ++  test_i18ncmp - actual <<-EOF
    + * sm1 $head6...0000000:
    +
    + * sm2 0000000...$head7 (2):
3:  ab28283b67 < -:  ---------- t7401: ensure uniformity in the '--for-status' test
-:  ---------- > 3:  f0b87ddaf6 t7401: change indentation for enhanced readability
4:  0bdb0bd72c ! 4:  a743c28d71 t7401: add a WARNING and a NEEDSWORK
    @@ Metadata
     Author: Shourya Shukla <shouryashukla.oo@gmail.com>

      ## Commit message ##
    -    t7401: add a WARNING and a NEEDSWORK
    +    t7401: add a NEEDSWORK

    -    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.
    +    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>
    +    Helped-by: Taylor Blau <me@ttaylorr.com>
         Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>

      ## t/t7401-submodule-summary.sh ##
    @@ t/t7401-submodule-summary.sh
     -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.
    ++# NEEDSWORK: This test script is old fashioned and may need a big cleanup since
    ++# there are lots of commands taking place outside of 'test_expect_success'
    ++# block, which is no longer in good-style.

      . ./test-lib.sh

-----

Shourya Shukla (4):
  t7401: modernize style
  t7401: change test_i18ncmp syntax for clarity
  t7401: change indentation for enhanced readability
  t7401: add a NEEDSWORK

 t/t7401-submodule-summary.sh | 151 ++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 73 deletions(-)

-- 
2.28.0

[PATCH v2 1/4] t7401: modernize style

[Shourya Shukla]

The tests in 't7401-submodule-summary.sh' were written a long time ago
and has a violation with respect to our CodingGuidelines which is,
incorrect spacing in usages of the redirection operator.
Using a Git command in the upstream of a pipe might result in us
losing its exit code. So, convert such usages so that they write to
a file and read from them.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Denton Liu <liu.denton@gmail.com>
Helped-by: Taylor Blau <me@ttaylorr.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..8ee78bcb69 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 >out &&
+	cut -c1-7 out
 	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 >out &&
+	cut -c1-7 out
 )
 
 test_expect_success 'modified submodule(backward)' "
-- 
2.28.0

[PATCH v2 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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 8ee78bcb69..53943c9cc9 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
@@ -285,7 +285,7 @@ EOF
 
 test_expect_success '--for-status' "
 	git submodule summary --for-status HEAD^ >actual &&
-	test_i18ncmp actual - <<EOF
+	test_i18ncmp - actual <<-EOF
 * sm1 $head6...0000000:
 
 * sm2 0000000...$head7 (2):
-- 
2.28.0

[PATCH v2 3/4] t7401: change indentation for enhanced readability

[Shourya Shukla]

Change the indentation of expected outputs for enhanced readability of
the tests.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Taylor Blau <me@taylorr.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t7401-submodule-summary.sh | 128 +++++++++++++++++------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 53943c9cc9..dd0e88fc6a 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -39,10 +39,10 @@ test_expect_success 'added submodule' "
 	git add sm1 &&
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 0000000...$head1 (2):
-  > Add foo2
+	* sm1 0000000...$head1 (2):
+	  > Add foo2
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -53,10 +53,10 @@ test_expect_success 'added submodule (subdirectory)' "
 		git submodule summary >../actual
 	) &&
 	cat >expected <<-EOF &&
-* ../sm1 0000000...$head1 (2):
-  > Add foo2
+	* ../sm1 0000000...$head1 (2):
+	  > Add foo2
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -74,10 +74,10 @@ test_expect_success 'added submodule (subdirectory with explicit path)' "
 		git submodule summary ../sm1 >../actual
 	) &&
 	cat >expected <<-EOF &&
-* ../sm1 0000000...$head1 (2):
-  > Add foo2
+	* ../sm1 0000000...$head1 (2):
+	  > Add foo2
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -87,20 +87,20 @@ head2=$(add_file sm1 foo3)
 test_expect_success 'modified submodule(forward)' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head1...$head2 (1):
-  > Add foo3
+	* sm1 $head1...$head2 (1):
+	  > Add foo3
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
 test_expect_success 'modified submodule(forward), --files' "
 	git submodule summary --files >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head1...$head2 (1):
-  > Add foo3
+	* sm1 $head1...$head2 (1):
+	  > Add foo3
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -111,10 +111,10 @@ test_expect_success 'no ignore=all setting has any effect' "
 	git config diff.ignoreSubmodules all &&
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head1...$head2 (1):
-  > Add foo3
+	* sm1 $head1...$head2 (1):
+	  > Add foo3
 
-EOF
+	EOF
 	test_cmp expected actual &&
 	git config --unset diff.ignoreSubmodules &&
 	git config --remove-section submodule.sm1 &&
@@ -133,11 +133,11 @@ head3=$(
 test_expect_success 'modified submodule(backward)' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head2...$head3 (2):
-  < Add foo3
-  < Add foo2
+	* sm1 $head2...$head3 (2):
+	  < Add foo3
+	  < Add foo2
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -146,25 +146,25 @@ head4_full=$(GIT_DIR=sm1/.git git rev-parse --verify HEAD)
 test_expect_success 'modified submodule(backward and forward)' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head2...$head4 (4):
-  > Add foo5
-  > Add foo4
-  < Add foo3
-  < Add foo2
+	* sm1 $head2...$head4 (4):
+	  > Add foo5
+	  > Add foo4
+	  < Add foo3
+	  < Add foo2
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
 test_expect_success '--summary-limit' "
 	git submodule summary -n 3 >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head2...$head4 (4):
-  > Add foo5
-  > Add foo4
-  < Add foo3
+	* sm1 $head2...$head4 (4):
+	  > Add foo5
+	  > Add foo4
+	  < Add foo3
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -179,20 +179,20 @@ mv sm1-bak sm1
 test_expect_success 'typechanged submodule(submodule->blob), --cached' "
 	git submodule summary --cached >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head4(submodule)->$head5(blob) (3):
-  < Add foo5
+	* sm1 $head4(submodule)->$head5(blob) (3):
+	  < Add foo5
 
-EOF
+	EOF
 	test_i18ncmp expected actual
 "
 
 test_expect_success 'typechanged submodule(submodule->blob), --files' "
 	git submodule summary --files >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head5(blob)->$head4(submodule) (3):
-  > Add foo5
+	* sm1 $head5(blob)->$head4(submodule) (3):
+	  > Add foo5
 
-EOF
+	EOF
 	test_i18ncmp expected actual
 "
 
@@ -201,9 +201,9 @@ git checkout-index sm1
 test_expect_success 'typechanged submodule(submodule->blob)' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head4(submodule)->$head5(blob):
+	* sm1 $head4(submodule)->$head5(blob):
 
-EOF
+	EOF
 	test_i18ncmp expected actual
 "
 
@@ -213,10 +213,10 @@ head6=$(add_file sm1 foo6 foo7)
 test_expect_success 'nonexistent commit' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head4...$head6:
-  Warn: sm1 doesn't contain commit $head4_full
+	* sm1 $head4...$head6:
+	  Warn: sm1 doesn't contain commit $head4_full
 
-EOF
+	EOF
 	test_i18ncmp expected actual
 "
 
@@ -224,10 +224,10 @@ commit_file
 test_expect_success 'typechanged submodule(blob->submodule)' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head5(blob)->$head6(submodule) (2):
-  > Add foo7
+	* sm1 $head5(blob)->$head6(submodule) (2):
+	  > Add foo7
 
-EOF
+	EOF
 	test_i18ncmp expected actual
 "
 
@@ -236,9 +236,9 @@ rm -rf sm1
 test_expect_success 'deleted submodule' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head6...0000000:
+	* sm1 $head6...0000000:
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -251,22 +251,22 @@ test_expect_success 'create second submodule' '
 test_expect_success 'multiple submodules' "
 	git submodule summary >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head6...0000000:
+	* sm1 $head6...0000000:
 
-* sm2 0000000...$head7 (2):
-  > Add foo9
+	* sm2 0000000...$head7 (2):
+	  > Add foo9
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
 test_expect_success 'path filter' "
 	git submodule summary sm2 >actual &&
 	cat >expected <<-EOF &&
-* sm2 0000000...$head7 (2):
-  > Add foo9
+	* sm2 0000000...$head7 (2):
+	  > Add foo9
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
@@ -274,24 +274,24 @@ commit_file sm2
 test_expect_success 'given commit' "
 	git submodule summary HEAD^ >actual &&
 	cat >expected <<-EOF &&
-* sm1 $head6...0000000:
+	* sm1 $head6...0000000:
 
-* sm2 0000000...$head7 (2):
-  > Add foo9
+	* sm2 0000000...$head7 (2):
+	  > Add foo9
 
-EOF
+	EOF
 	test_cmp expected actual
 "
 
 test_expect_success '--for-status' "
 	git submodule summary --for-status HEAD^ >actual &&
 	test_i18ncmp - actual <<-EOF
-* sm1 $head6...0000000:
+	* sm1 $head6...0000000:
 
-* sm2 0000000...$head7 (2):
-  > Add foo9
+	* sm2 0000000...$head7 (2):
+	  > Add foo9
 
-EOF
+	EOF
 "
 
 test_expect_success 'fail when using --files together with --cached' "
-- 
2.28.0

[PATCH v2 4/4] t7401: add a NEEDSWORK

[Shourya Shukla]

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>
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
 t/t7401-submodule-summary.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index dd0e88fc6a..8f5e4515d3 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -5,8 +5,11 @@
 
 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.
 '
+# NEEDSWORK: This test script is old fashioned and may need a big cleanup since
+# there are lots of commands taking place outside of 'test_expect_success'
+# block, which is no longer in good-style.
 
 . ./test-lib.sh
 
-- 
2.28.0

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

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> 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.
> ...
> @@ -285,7 +285,7 @@ EOF
>  
>  test_expect_success '--for-status' "
>  	git submodule summary --for-status HEAD^ >actual &&
> -	test_i18ncmp actual - <<EOF
> +	test_i18ncmp - actual <<-EOF
>  * sm1 $head6...0000000:
>  
>  * sm2 0000000...$head7 (2):

This one does more than what the proposed log message explains, but
it does not do enough at the same time.  

If "actual vs expected order" is what this commit wants to fix, then
"<<EOF" vs "<<-EOF" is outside the scope of it.

Personally, I think it is a good idea to update the end-of-heredoc
marker to "<<-EOF", but the patch makes its readers wonder if the
author of the patch understands why it is a good idea to begin with,
because the end-of-heredoc marker is the only thing the patch
changes, and it does not change the indentation of heredoc itself.

The whole point of using "<<-EOF" instead of "<<EOF" is so that the
result becomes easier to read with indentation, e.g.

	test_i18ncmp - actual <<-EOF
	* sm1 $head6...0000000:

	* sm2 0000000...$head7 (2):
	...
	EOF

Compared to the original:

	test_i18ncmp - actual <<EOF
* sm1 $head6...0000000:

* sm2 0000000...$head7 (2):
...
EOF

it would be easier to read.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>
>> 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.
>> ...
>> @@ -285,7 +285,7 @@ EOF
>>  
>>  test_expect_success '--for-status' "
>>  	git submodule summary --for-status HEAD^ >actual &&
>> -	test_i18ncmp actual - <<EOF
>> +	test_i18ncmp - actual <<-EOF
>>  * sm1 $head6...0000000:
>>  
>>  * sm2 0000000...$head7 (2):
>
> This one does more than what the proposed log message explains, but
> it does not do enough at the same time.  
>
> If "actual vs expected order" is what this commit wants to fix, then
> "<<EOF" vs "<<-EOF" is outside the scope of it.
>
> Personally, I think it is a good idea to update the end-of-heredoc
> marker to "<<-EOF", ...

It seems that the theme of your [3/4] is exactly about fixing the
"end-of-heredoc marker is prefixed with dash, but the heredoc is not
indented for readability", so perhaps you'd want to stop this step
at turning the line to

>> -	test_i18ncmp actual - <<EOF
>> +	test_i18ncmp - actual <<EOF

i.e. "compare expected vs actual in this order", and then in the
next patch [3/4], edit the line further to

	test_i18ncmp - actual <<-EOF

*and* indent the here-doc at the same time?

Re: [PATCH v2 1/4] t7401: modernize style

[Kaartic Sivaraam]

On Thu, 2020-08-13 at 00:57 +0530, Shourya Shukla wrote:
>
> [...]
>
> Using a Git command in the upstream of a pipe might result in us
> losing its exit code. So, convert such usages so that they write to
> a file and read from them.
> 

While that is a good enough reason to avoid using pipes in places where
we look for the exit code of a command like within test_expect_success,
I'm not sure if that reason holds for the places that the patch
changes.

> [...]
>
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-
> summary.sh
> index 9bc841d085..8ee78bcb69 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 >out &&
> +	cut -c1-7 out

In any case, I believe we can avoid the 'cut' altogether in both places
by doing something like this instead:

   git rev-parse --short=7 HEAD

My quick check shows the test script is happy with this change.

>  	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 >out &&
> +	cut -c1-7 out
>  )
>  
>  test_expect_success 'modified submodule(backward)' "

-- 
Sivaraam

Re: [PATCH v2 1/4] t7401: modernize style

[Junio C Hamano]

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

>> @@ -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 >out &&
>> +	cut -c1-7 out
>
> In any case, I believe we can avoid the 'cut' altogether in both places
> by doing something like this instead:
>
>    git rev-parse --short=7 HEAD

Ah, I missed the fact that this was a helper function and most of
the error status is discarded anyway.  For example, we still run the
rev-parse even after the for loop fails.

If the focus of this test script were to ensure that rev-parse works
correctly, being careful to catch its exit status might have had a
good value, but for that, all the other operations that happen in
this helper function (including the "what happens when the loop body
fails for $name that is not at the end of the argument list?") must
also be checked for their exit status in the first place.

Since that is not done, and since testing rev-parse should not have
to be part of the job for submodule test, the net effect of the
above change has quite dubious value---it clobbered a file 'out'
that may be used by the caller.

Doing "cd" without introducing a subshell is a bit harder to fix, as
test_tick relies on the global counter in the topmost process.  It
can be done, but I do not think it is worth doing here.  Most of the
users of this helper function call it in var=$(add_file ...)
subshell anyway (so test_tick is incrementing the timestamp
independently for each caller and discarding the resulting
timestamp).  As a NEEDSWORK comment added in the series says, this
script may need a bit more work.

I agree with you that the split of "rev-parse | cut -c1-7" into two
statements and clobbering 'out' is a bad change---that part should
be reverted.  The style change on 'echo "$name" >"$name"' line is
OK, though.

Thanks.

> My quick check shows the test script is happy with this change.
>
>>  	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 >out &&
>> +	cut -c1-7 out
>>  )
>>  
>>  test_expect_success 'modified submodule(backward)' "

Re: [PATCH v2 1/4] t7401: modernize style

[Shourya Shukla]

On 13/08 09:46, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> >> @@ -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 >out &&
> >> +	cut -c1-7 out
> >
> > In any case, I believe we can avoid the 'cut' altogether in both places
> > by doing something like this instead:
> >
> >    git rev-parse --short=7 HEAD
> 
> Ah, I missed the fact that this was a helper function and most of
> the error status is discarded anyway.  For example, we still run the
> rev-parse even after the for loop fails.
> 
> If the focus of this test script were to ensure that rev-parse works
> correctly, being careful to catch its exit status might have had a
> good value, but for that, all the other operations that happen in
> this helper function (including the "what happens when the loop body
> fails for $name that is not at the end of the argument list?") must
> also be checked for their exit status in the first place.
> 
> Since that is not done, and since testing rev-parse should not have
> to be part of the job for submodule test, the net effect of the
> above change has quite dubious value---it clobbered a file 'out'
> that may be used by the caller.
> 
> Doing "cd" without introducing a subshell is a bit harder to fix, as
> test_tick relies on the global counter in the topmost process.  It
> can be done, but I do not think it is worth doing here.  Most of the
> users of this helper function call it in var=$(add_file ...)
> subshell anyway (so test_tick is incrementing the timestamp
> independently for each caller and discarding the resulting
> timestamp).  As a NEEDSWORK comment added in the series says, this
> script may need a bit more work.
> 
> I agree with you that the split of "rev-parse | cut -c1-7" into two
> statements and clobbering 'out' is a bad change---that part should
> be reverted.  The style change on 'echo "$name" >"$name"' line is
> OK, though.
> 
> Thanks.

Understood. I will revert the change. Though, what Kaartic suggested, to
do a '--short=7', that will be okay to keep right? Something like:

    git rev-parse --short=7 HEAD

This way we will not need a 'cut'. This change goes as a separate commit
obviously.

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

[Shourya Shukla]

On 12/08 02:02, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> >
> >> 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.
> >> ...
> >> @@ -285,7 +285,7 @@ EOF
> >>  
> >>  test_expect_success '--for-status' "
> >>  	git submodule summary --for-status HEAD^ >actual &&
> >> -	test_i18ncmp actual - <<EOF
> >> +	test_i18ncmp - actual <<-EOF
> >>  * sm1 $head6...0000000:
> >>  
> >>  * sm2 0000000...$head7 (2):
> >
> > This one does more than what the proposed log message explains, but
> > it does not do enough at the same time.  
> >
> > If "actual vs expected order" is what this commit wants to fix, then
> > "<<EOF" vs "<<-EOF" is outside the scope of it.
> >
> > Personally, I think it is a good idea to update the end-of-heredoc
> > marker to "<<-EOF", ...
> 
> It seems that the theme of your [3/4] is exactly about fixing the
> "end-of-heredoc marker is prefixed with dash, but the heredoc is not
> indented for readability", so perhaps you'd want to stop this step
> at turning the line to
> 
> >> -	test_i18ncmp actual - <<EOF
> >> +	test_i18ncmp - actual <<EOF
> 
> i.e. "compare expected vs actual in this order", and then in the
> next patch [3/4], edit the line further to
> 
> 	test_i18ncmp - actual <<-EOF
> 
> *and* indent the here-doc at the same time?

Ohh okay okay. I understand what you are saying. I wanted to fix the
heredoc markers and indent the tests for better readibility but actually
I fixed the heredoc marker in [2/4]. Therefore, the change in [2/4]
should in fact be:

    -	test_i18ncmp actual - <<EOF
    +	test_i18ncmp - actual <<EOF

And in this patch [3/4], it should become:

    -	test_i18ncmp - actual <<EOF
    +	test_i18ncmp - actual <<-EOF

As well as the indentation fixes I did in the patch already. Now I
understand the exact use and significance of the heredoc. Thank you.

Re: [PATCH v2 1/4] t7401: modernize style

[Junio C Hamano]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> Understood. I will revert the change. Though, what Kaartic suggested, to
> do a '--short=7', that will be okay to keep right?

Sure, that is a strict improvement to lose an unneeded process, as
long as we know HEAD is guaranteed to be unique with 7 hexdigits
(otherwise "cut" to strictly 7 and "rev-parse --short=7 HEAD" would
produce different results.

Thanks.

[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