threads / patch / 53922

patch, 4 partst7401: modernize style

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

## tl;dr

5 messages between Jul 26, 2020 and Jul 26, 2020. Diffs are folded; open one to read it.

replies: 4people: 1as markdown or json

Shourya Shukla· Jul 26, 2020, 14:25 UTC · lore

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

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
Shourya Shukla· Jul 26, 2020, 14:25 UTC · re: Shourya Shukla · lore

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(-)
Show changes to t/t7401-submodule-summary.sh +5 −3
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
Shourya Shukla· Jul 26, 2020, 14:25 UTC · re: Shourya Shukla · lore

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

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(-)
Show changes to t/t7401-submodule-summary.sh +4 −4
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
Shourya Shukla· Jul 26, 2020, 14:25 UTC · re: Shourya Shukla · lore

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

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(-)
Show changes to t/t7401-submodule-summary.sh +2 −1
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
Shourya Shukla· Jul 26, 2020, 14:25 UTC · re: Shourya Shukla · lore

[PATCH 4/4] t7401: add a WARNING and 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.

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(-)
Show changes to t/t7401-submodule-summary.sh +6 −1
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

← back to recent threads