{"thread":{"id":"53989","subject":"[PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","startedAt":"2020-08-05T17:52:09Z","lastAt":"2020-08-14T17:06:45Z","messageCount":33,"participants":["Shourya Shukla","Denton Liu","Taylor Blau","Junio C Hamano","Christian Couder","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"402930","messageId":"20200805174921.16000-4-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200805174921.16000-1-shouryashukla.oo@gmail.com","subject":"[PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-05T17:49:20Z","receivedAt":"2020-08-05T17:52:09Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The '--for-status' test got its expected output from stdin. This is\ninconsistent with the other tests in the test script which get their\nexpected output from a file named 'expected'.\n\nSo, change the syntax of the '--for-status' test for uniformity.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 18fefdb0ba..145914cd5a 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -285,13 +285,14 @@ EOF\n \n test_expect_success '--for-status' \"\n \tgit submodule summary --for-status HEAD^ >actual &&\n-\ttest_i18ncmp actual - <expected <<-EOF &&\n * sm1 $head6...0000000:\n \n * sm2 0000000...$head7 (2):\n > Add foo9\n \n EOF\n+\ttest_i18ncmp expected actual\n \"\n \n test_expect_success 'fail when using --files together with --cached' \"\n-- \n2.27.0\n\n"},{"id":"402931","messageId":"20200805174921.16000-5-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200805174921.16000-1-shouryashukla.oo@gmail.com","subject":"[PATCH 4/4] t7401: add a WARNING and a NEEDSWORK","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-05T17:49:21Z","receivedAt":"2020-08-05T17:52:13Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Add a WARNING regarding the usage of 'git add' instead of 'git\nsubmodule add' to add submodules to the superproject. Also add a\nNEEDSWORK regarding the outdated syntax and working of the test, which\nmay need to be improved to obtain better and desired results.\n\nWhile at it, change the word 'test' to 'test script' in the test\ndescription to avoid ambiguity.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 145914cd5a..2db4cf5cbf 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -5,8 +5,13 @@\n \n test_description='Summary support for submodules\n \n-This test tries to verify the sanity of summary subcommand of git submodule.\n+This test script tries to verify the sanity of summary subcommand of git submodule.\n '\n+# WARNING: This test script uses 'git add' instead of 'git submodule add' to add\n+# submodules to the superproject. Some submodule subcommands such as init and\n+# deinit might not work as expected in this script.\n+\n+# NEEDSWORK: This test script is old fashioned and may need a big cleanup.\n \n . ./test-lib.sh\n \n-- \n2.27.0\n\n"},{"id":"402932","messageId":"20200805174921.16000-3-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200805174921.16000-1-shouryashukla.oo@gmail.com","subject":"[PATCH 2/4] t7401: change test_i18ncmp syntax for clarity","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-05T17:49:19Z","receivedAt":"2020-08-05T17:52:40Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n'test_i18ncmp expected actual' to align it with the convention followed\nby other tests in the test script.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 4439fb7c17..18fefdb0ba 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -183,7 +183,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' \"\n < Add foo5\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n@@ -193,7 +193,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n > Add foo5\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n rm -rf sm1 &&\n@@ -204,7 +204,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' \"\n * sm1 $head4(submodule)->$head5(blob):\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n rm -f sm1 &&\n@@ -217,7 +217,7 @@ test_expect_success 'nonexistent commit' \"\n Warn: sm1 doesn't contain commit $head4_full\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n commit_file\n-- \n2.27.0\n\n"},{"id":"402933","messageId":"20200805174921.16000-2-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200805174921.16000-1-shouryashukla.oo@gmail.com","subject":"[PATCH 1/4] t7401: modernize style","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-05T17:49:18Z","receivedAt":"2020-08-05T17:53:35Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The tests in 't7401-submodule-summary.sh' were written a long time ago\nand have some violations with respect to our CodingGuidelines such as\nincorrect spacing in usages of the redirection operator and absence\nof line feed between statements in case of the '|' (pipe) operator.\nUpdate it to match the CodingGuidelines.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 9bc841d085..4439fb7c17 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -16,12 +16,13 @@ add_file () {\n \towd=$(pwd)\n \tcd \"$sm\"\n \tfor name; do\n-\t\techo \"$name\" > \"$name\" &&\n+\t\techo \"$name\" >\"$name\" &&\n \t\tgit add \"$name\" &&\n \t\ttest_tick &&\n \t\tgit commit -m \"Add $name\"\n \tdone >/dev/null\n-\tgit rev-parse --verify HEAD | cut -c1-7\n+\tgit rev-parse --verify HEAD |\n+\tcut -c1-7\n \tcd \"$owd\"\n }\n commit_file () {\n@@ -125,7 +126,8 @@ commit_file sm1 &&\n head3=$(\n \tcd sm1 &&\n \tgit reset --hard HEAD~2 >/dev/null &&\n-\tgit rev-parse --verify HEAD | cut -c1-7\n+\tgit rev-parse --verify HEAD |\n+\tcut -c1-7\n )\n \n test_expect_success 'modified submodule(backward)' \"\n-- \n2.27.0\n\n"},{"id":"402934","messageId":"20200805174921.16000-1-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":null,"subject":"[GSoC][RESEND][PATCH 0/4] t7401: modernize, cleanup and warn","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-05T17:49:17Z","receivedAt":"2020-08-05T17:54:09Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings,\n\nThe current phase of my GSoC project involves porting\n'git submodule summary' from shell to C. While doing so, I, along with\nmy mentors Christian and Kaartic noticed some discrepancies in the test\nscript when trying to add a couple of tests to it. Though the test works\nperfectly for my C port of 'summary', there were some unexpected behaviours\nwhen trying to some tests to it. This patch series addresses these\nissues in the test script by modernizing it, cleaning it up and warning\nabout some other issues.\n\nChiefly about patch 4/4 (t7401: add a WARNING and a NEEDSWORK),\nwhen trying to write a test for verifying the summary output of\ndeinitialized submodule, doing a 'git submodule deinit ' did not\nbear any fruit since the submodule never really got deinitialized. \nThe deinit documentation states that:\n\n\tUnregister the given submodules, i.e. remove the whole\n\tsubmodule.$name section from .git/config together with\n\ttheir work tree.\n\nSomething which was not actually happening in the test. It appeared\nthat the reason for the deinit issue is that the test script uses\n'git add' to add submodules instead of the command 'git submodule add'.\n\nThis behaviour also prompted the need to design a new test script to\nhave a testing of some niche cases such as those stated before, but\nthis is something that will be covered in the patch series responsible\nfor porting the 'summary' subcommand to C.\n\nComments and reviews are appreciated.\n\nThanks,\nShourya Shukla\n\nShourya Shukla (4):\n t7401: modernize style\n t7401: change test_i18ncmp syntax for clarity\n t7401: ensure uniformity in the '--for-status' test\n t7401: add a WARNING and a NEEDSWORK\n\n t/t7401-submodule-summary.sh | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\n-- \n2.27.0\n\n"},{"id":"402942","messageId":"20200805193755.GA2650@generichostname","threadId":"53989","inReplyTo":"20200805174921.16000-2-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 1/4] t7401: modernize style","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-08-05T19:37:55Z","receivedAt":"2020-08-05T19:45:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Shourya,\n\nOn Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:\n> The tests in 't7401-submodule-summary.sh' were written a long time ago\n> and have some violations with respect to our CodingGuidelines such as\n> incorrect spacing in usages of the redirection operator and absence\n> of line feed between statements in case of the '|' (pipe) operator.\n\nI'm not aware of anywhere in CodingGuidelines that says you can't have\nthe pipe operator on a single line. I assume you're referring to the\npart that reads\n\n\tIf a command sequence joined with && or || or | spans multiple\n\tlines, put each command on a separate line and put && and || and\n\t| operators at the end of each line, rather than the start.\n\nNote that says \"If a command sequence [...] spans multiple lines\", which\ndoesn't apply in our case.\n\n> Update it to match the CodingGuidelines.\n> \n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 9bc841d085..4439fb7c17 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -16,12 +16,13 @@ add_file () {\n> \towd=$(pwd)\n> \tcd \"$sm\"\n> \tfor name; do\n> -\t\techo \"$name\" > \"$name\" &&\n> +\t\techo \"$name\" >\"$name\" &&\n> \t\tgit add \"$name\" &&\n> \t\ttest_tick &&\n> \t\tgit commit -m \"Add $name\"\n> \tdone >/dev/null\n> -\tgit rev-parse --verify HEAD | cut -c1-7\n> +\tgit rev-parse --verify HEAD |\n> +\tcut -c1-7\n\nFor the reason above, I disagree with this change as-is. However, one\nuseful thing that you can do here is breaking the pipe up entirely. We\nwant to avoid is having a git command in the upstream of a pipe. This is\nbecause the return code of a pipe comes from the last command executed\nso if the rev-parse fails, its return code is swallowed and we have no\nway of knowing.\n\nYou could break the pipe up by storing the output of the rev-parse in an\nintermediate file and then have cut read from that file.\n"},{"id":"402963","messageId":"20200805204924.GF9546@syl.lan","threadId":"53989","inReplyTo":"20200805193755.GA2650@generichostname","subject":"Re: [PATCH 1/4] t7401: modernize style","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T20:49:24Z","receivedAt":"2020-08-05T20:49:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote:\n> Hi Shourya,\n>\n> On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:\n> > The tests in 't7401-submodule-summary.sh' were written a long time ago\n> > and have some violations with respect to our CodingGuidelines such as\n> > incorrect spacing in usages of the redirection operator and absence\n> > of line feed between statements in case of the '|' (pipe) operator.\n>\n> I'm not aware of anywhere in CodingGuidelines that says you can't have\n> the pipe operator on a single line. I assume you're referring to the\n> part that reads\n>\n> \tIf a command sequence joined with && or || or | spans multiple\n> \tlines, put each command on a separate line and put && and || and\n> \t| operators at the end of each line, rather than the start.\n>\n> Note that says \"If a command sequence [...] spans multiple lines\", which\n> doesn't apply in our case.\n>\n> > Update it to match the CodingGuidelines.\n> >\n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Signed-off-by: Shourya Shukla \n> > ---\n> > t/t7401-submodule-summary.sh | 8 +++++---\n> > 1 file changed, 5 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> > index 9bc841d085..4439fb7c17 100755\n> > --- a/t/t7401-submodule-summary.sh\n> > +++ b/t/t7401-submodule-summary.sh\n> > @@ -16,12 +16,13 @@ add_file () {\n> > \towd=$(pwd)\n> > \tcd \"$sm\"\n> > \tfor name; do\n> > -\t\techo \"$name\" > \"$name\" &&\n> > +\t\techo \"$name\" >\"$name\" &&\n\nThis change is good.\n\n> > \t\tgit add \"$name\" &&\n> > \t\ttest_tick &&\n> > \t\tgit commit -m \"Add $name\"\n> > \tdone >/dev/null\n> > -\tgit rev-parse --verify HEAD | cut -c1-7\n> > +\tgit rev-parse --verify HEAD |\n> > +\tcut -c1-7\n>\n> For the reason above, I disagree with this change as-is. However, one\n> useful thing that you can do here is breaking the pipe up entirely. We\n> want to avoid is having a git command in the upstream of a pipe. This is\n> because the return code of a pipe comes from the last command executed\n> so if the rev-parse fails, its return code is swallowed and we have no\n> way of knowing.\n>\n> You could break the pipe up by storing the output of the rev-parse in an\n> intermediate file and then have cut read from that file.\n\nThis is a good suggestion (I was preparing to write an email to say the\nsame thing, but I'm glad that I checked Denton's response before doing\nso). Something like:\n\n\tgit rev-parse --verify HEAD >out &&\n\tcut -c1-7 out\n\nwould suffice and be in good style.\n\nThanks,\nTaylor\n"},{"id":"402965","messageId":"20200805205836.GG9546@syl.lan","threadId":"53989","inReplyTo":"20200805174921.16000-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 2/4] t7401: change test_i18ncmp syntax for clarity","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T20:58:36Z","receivedAt":"2020-08-05T20:58:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 05, 2020 at 11:19:19PM +0530, Shourya Shukla wrote:\n> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n> 'test_i18ncmp expected actual' to align it with the convention followed\n> by other tests in the test script.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 4439fb7c17..18fefdb0ba 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -183,7 +183,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' \"\n> < Add foo5\n>\n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n>\n> test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n> @@ -193,7 +193,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n> > Add foo5\n>\n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n>\n> rm -rf sm1 &&\n> @@ -204,7 +204,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' \"\n> * sm1 $head4(submodule)->$head5(blob):\n>\n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n>\n> rm -f sm1 &&\n> @@ -217,7 +217,7 @@ test_expect_success 'nonexistent commit' \"\n> Warn: sm1 doesn't contain commit $head4_full\n>\n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n>\n> commit_file\n> --\n> 2.27.0\n\nThis all looks good, and matches a long-standing convention of having\nthe expected value as the first argument and the comparison value as the\nlatter argument.\n\nThere's one spot you missed, which could be addressed by folding in this\ndiff on top:\n\n--- >8 ---\n\nSubject: [PATCH] fixup! t7401: change test_i18ncmp syntax for clarity\n\nSigned-off-by: Taylor Blau \n---\n t/t7401-submodule-summary.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 18fefdb0ba..bd8fc8511a 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -285,12 +285,14 @@ EOF\n\n test_expect_success '--for-status' \"\n \tgit submodule summary --for-status HEAD^ >actual &&\n-\ttest_i18ncmp actual - <expected <<-\\EOF &&\n+\t* sm1 $head6...0000000:\n\n-* sm2 0000000...$head7 (2):\n- > Add foo9\n+\t* sm2 0000000...$head7 (2):\n+\t > Add foo9\n\n+\tEOF\n+\ttest_i18ncmp expected actual\n EOF\n \"\n\n--\n2.28.0.rc1.13.ge78abce653\n"},{"id":"402966","messageId":"20200805210104.GH9546@syl.lan","threadId":"53989","inReplyTo":"20200805174921.16000-4-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T21:01:04Z","receivedAt":"2020-08-05T21:01:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 05, 2020 at 11:19:20PM +0530, Shourya Shukla wrote:\n> The '--for-status' test got its expected output from stdin. This is\n> inconsistent with the other tests in the test script which get their\n> expected output from a file named 'expected'.\n>\n> So, change the syntax of the '--for-status' test for uniformity.\n\n... serves me right for not reading the whole thread before responding\nto the previous patch ;).\n\nOn a technical note, I don't think this is different enough from the\nprevious patch that they couldn't be combined. (A good indicator of this\nis that I expected this change to be included in 2/4 and was surprised\nthat it was a separate step afterwords).\n\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 18fefdb0ba..145914cd5a 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -285,13 +285,14 @@ EOF\n>\n> test_expect_success '--for-status' \"\n> \tgit submodule summary --for-status HEAD^ >actual &&\n> -\ttest_i18ncmp actual - < +\tcat >expected <<-EOF &&\n> * sm1 $head6...0000000:\n>\n> * sm2 0000000...$head7 (2):\n> > Add foo9\n>\n> EOF\n> +\ttest_i18ncmp expected actual\n> \"\n\nI think that this is on the right track, but you can use a '<<-\\EOF'\nhere instead of '<<-EOF' to make the tabulation a little more flexible.\n\n>\n> test_expect_success 'fail when using --files together with --cached' \"\n> --\n> 2.27.0\n\nThanks,\nTaylor\n"},{"id":"402968","messageId":"20200805210408.GI9546@syl.lan","threadId":"53989","inReplyTo":"20200805174921.16000-5-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T21:04:08Z","receivedAt":"2020-08-05T21:04:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 05, 2020 at 11:19:21PM +0530, Shourya Shukla wrote:\n> Add a WARNING regarding the usage of 'git add' instead of 'git\n> submodule add' to add submodules to the superproject. Also add a\n> NEEDSWORK regarding the outdated syntax and working of the test, which\n> may need to be improved to obtain better and desired results.\n>\n> While at it, change the word 'test' to 'test script' in the test\n> description to avoid ambiguity.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 145914cd5a..2db4cf5cbf 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -5,8 +5,13 @@\n>\n> test_description='Summary support for submodules\n>\n> -This test tries to verify the sanity of summary subcommand of git submodule.\n> +This test script tries to verify the sanity of summary subcommand of git submodule.\n> '\n> +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add\n> +# submodules to the superproject. Some submodule subcommands such as init and\n> +# deinit might not work as expected in this script.\n> +\n> +# NEEDSWORK: This test script is old fashioned and may need a big cleanup.\n\nIt would be worth saying why, especially if you're re-rolling anyway.\nEven something as simple as: \"there are lots of commands taking place\noutside of a 'test_expect_{success,failure}' block, which is no longer\nin good-style\".\n\nI also wouldn't be upset to see some of those fixed up in this series,\nbut I realize that may be a bigger endeavor than you are willing to take\non for now.\n\n>\n> . ./test-lib.sh\n>\n> --\n> 2.27.0\n\nThanks,\nTaylor\n"},{"id":"402973","messageId":"xmqq7ducokgq.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200805174921.16000-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 2/4] t7401: change test_i18ncmp syntax for clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T21:23:49Z","receivedAt":"2020-08-05T21:23:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n> 'test_i18ncmp expected actual' to align it with the convention followed\n> by other tests in the test script.\n\nYeah, this is a good thing to do, as a failing test_cmp gives a diff\nbetween the first file to the second file, i.e. a patch that turns\nthe expected output into what the tests actually produced, so that\nthe tester can see how the expectation is broken.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 4439fb7c17..18fefdb0ba 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -183,7 +183,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' \"\n> < Add foo5\n> \n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n> \n> test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n> @@ -193,7 +193,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n> > Add foo5\n> \n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n> \n> rm -rf sm1 &&\n> @@ -204,7 +204,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' \"\n> * sm1 $head4(submodule)->$head5(blob):\n> \n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n> \n> rm -f sm1 &&\n> @@ -217,7 +217,7 @@ test_expect_success 'nonexistent commit' \"\n> Warn: sm1 doesn't contain commit $head4_full\n> \n> EOF\n> -\ttest_i18ncmp actual expected\n> +\ttest_i18ncmp expected actual\n> \"\n> \n> commit_file\n"},{"id":"402975","messageId":"xmqq3650ok53.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200805174921.16000-4-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T21:30:48Z","receivedAt":"2020-08-05T21:30:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> The '--for-status' test got its expected output from stdin. This is\n> inconsistent with the other tests in the test script which get their\n> expected output from a file named 'expected'.\n>\n> So, change the syntax of the '--for-status' test for uniformity.\n\nThere are a handful examples in t5401 and another one in t3700 that\ngive the \"golden master\" from the standard input. When the expected\noutput is used only once, I do not think it is particularlly bad to\nhave it this way. So,... meh?\n\nUnless there is a compelling reason to insist both are files\n(that may make it easier to enhance test_cmp in a certain way\nyou plan to, for example), that is.\n\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 18fefdb0ba..145914cd5a 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -285,13 +285,14 @@ EOF\n> \n> test_expect_success '--for-status' \"\n> \tgit submodule summary --for-status HEAD^ >actual &&\n> -\ttest_i18ncmp actual - < +\tcat >expected <<-EOF &&\n> * sm1 $head6...0000000:\n> \n> * sm2 0000000...$head7 (2):\n> > Add foo9\n> \n> EOF\n> +\ttest_i18ncmp expected actual\n> \"\n> \n> test_expect_success 'fail when using --files together with --cached' \"\n"},{"id":"402976","messageId":"xmqqy2msn5b2.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200805174921.16000-5-shouryashukla.oo@gmail.com","subject":"Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T21:36:33Z","receivedAt":"2020-08-05T21:36:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> Add a WARNING regarding the usage of 'git add' instead of 'git\n> submodule add' to add submodules to the superproject.\n\nIs that a warning worthy thing? As far as I know, using \"git add\"\nto register a gitlink is perfectly fine and a supported way to start\na new submodule. It may have to be followed by other steps like\n\"git config -f .gitmodules\" (e.g. when operations that needs to use\nthe contents recorded in the .gitmodules file are to be tested), but\nwriting tests using lower-level ingredients for finer grained tests\nis not all that unusual, is it? I dunno.\n\n> NEEDSWORK regarding the outdated syntax and working of the test, which\n> may need to be improved to obtain better and desired results.\n\nSounds good.\n\n> While at it, change the word 'test' to 'test script' in the test\n> description to avoid ambiguity.\n\nSounds good. I often search for a pair of phrases to differentiate\na single tXXXX-name.sh file as a whole and an individual test piece\nin it. \"This test script\", especially when written near the\nbeginning of the file, is a good way to clearly convey that you want\nto refer to the former.\n\n> Mentored-by: Christian Couder \n> Mentored-by: Kaartic Sivaraam \n> Signed-off-by: Shourya Shukla \n> ---\n> t/t7401-submodule-summary.sh | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> index 145914cd5a..2db4cf5cbf 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -5,8 +5,13 @@\n> \n> test_description='Summary support for submodules\n> \n> -This test tries to verify the sanity of summary subcommand of git submodule.\n> +This test script tries to verify the sanity of summary subcommand of git submodule.\n> '\n> +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add\n> +# submodules to the superproject. Some submodule subcommands such as init and\n> +# deinit might not work as expected in this script.\n> +\n> +# NEEDSWORK: This test script is old fashioned and may need a big cleanup.\n> \n> . ./test-lib.sh\n"},{"id":"402996","messageId":"xmqqd044n31u.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200805210104.GH9546@syl.lan","subject":"Re: [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T22:25:17Z","receivedAt":"2020-08-05T22:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> test_expect_success '--for-status' \"\n>> \tgit submodule summary --for-status HEAD^ >actual &&\n>> -\ttest_i18ncmp actual - <> +\tcat >expected <<-EOF &&\n>> * sm1 $head6...0000000:\n>>\n>> * sm2 0000000...$head7 (2):\n>> > Add foo9\n>>\n>> EOF\n>> +\ttest_i18ncmp expected actual\n>> \"\n>\n> I think that this is on the right track, but you can use a '<<-\\EOF'\n> here instead of '<<-EOF' to make the tabulation a little more flexible.\n\nYou are correct that the patch could have indented the here-doc and\nthe line with EOF with a tab to make it easier to read.\n\nThe leading '-' after '<<' is what controls tabulation, so <<-EOF as\nwritten in the patch is good enough; you do not want to quote it\nfurther, because $head6 wants to be interpolated.\n\n\n"},{"id":"402998","messageId":"20200805222618.GB30420@syl.lan","threadId":"53989","inReplyTo":"xmqqd044n31u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-05T22:26:18Z","receivedAt":"2020-08-05T22:26:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Aug 05, 2020 at 03:25:17PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> >> test_expect_success '--for-status' \"\n> >> \tgit submodule summary --for-status HEAD^ >actual &&\n> >> -\ttest_i18ncmp actual - < >> +\tcat >expected <<-EOF &&\n> >> * sm1 $head6...0000000:\n> >>\n> >> * sm2 0000000...$head7 (2):\n> >> > Add foo9\n> >>\n> >> EOF\n> >> +\ttest_i18ncmp expected actual\n> >> \"\n> >\n> > I think that this is on the right track, but you can use a '<<-\\EOF'\n> > here instead of '<<-EOF' to make the tabulation a little more flexible.\n>\n> You are correct that the patch could have indented the here-doc and\n> the line with EOF with a tab to make it easier to read.\n>\n> The leading '-' after '<<' is what controls tabulation, so <<-EOF as\n> written in the patch is good enough; you do not want to quote it\n> further, because $head6 wants to be interpolated.\n\nAh, I never actually knew the difference between those two (and\napparently had been too lazy to look it up myself). Thanks for the\nclarification.\n\nThanks,\nTaylor\n"},{"id":"403031","messageId":"20200806084553.GA13942@konoha","threadId":"53989","inReplyTo":"20200805204924.GF9546@syl.lan","subject":"Re: [PATCH 1/4] t7401: modernize style","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T08:45:53Z","receivedAt":"2020-08-06T11:13:16Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 05/08 04:49, Taylor Blau wrote:\n> On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote:\n> > Hi Shourya,\n> >\n> > On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:\n> > > The tests in 't7401-submodule-summary.sh' were written a long time ago\n> > > and have some violations with respect to our CodingGuidelines such as\n> > > incorrect spacing in usages of the redirection operator and absence\n> > > of line feed between statements in case of the '|' (pipe) operator.\n> >\n> > I'm not aware of anywhere in CodingGuidelines that says you can't have\n> > the pipe operator on a single line. I assume you're referring to the\n> > part that reads\n> >\n> > \tIf a command sequence joined with && or || or | spans multiple\n> > \tlines, put each command on a separate line and put && and || and\n> > \t| operators at the end of each line, rather than the start.\n> >\n> > Note that says \"If a command sequence [...] spans multiple lines\", which\n> > doesn't apply in our case.\n> >\n> > > Update it to match the CodingGuidelines.\n> > >\n> > > Mentored-by: Christian Couder \n> > > Mentored-by: Kaartic Sivaraam \n> > > Signed-off-by: Shourya Shukla \n> > > ---\n> > > t/t7401-submodule-summary.sh | 8 +++++---\n> > > 1 file changed, 5 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> > > index 9bc841d085..4439fb7c17 100755\n> > > --- a/t/t7401-submodule-summary.sh\n> > > +++ b/t/t7401-submodule-summary.sh\n> > > @@ -16,12 +16,13 @@ add_file () {\n> > > \towd=$(pwd)\n> > > \tcd \"$sm\"\n> > > \tfor name; do\n> > > -\t\techo \"$name\" > \"$name\" &&\n> > > +\t\techo \"$name\" >\"$name\" &&\n> \n> This change is good.\n> \n> > > \t\tgit add \"$name\" &&\n> > > \t\ttest_tick &&\n> > > \t\tgit commit -m \"Add $name\"\n> > > \tdone >/dev/null\n> > > -\tgit rev-parse --verify HEAD | cut -c1-7\n> > > +\tgit rev-parse --verify HEAD |\n> > > +\tcut -c1-7\n> >\n> > For the reason above, I disagree with this change as-is. However, one\n> > useful thing that you can do here is breaking the pipe up entirely. We\n> > want to avoid is having a git command in the upstream of a pipe. This is\n> > because the return code of a pipe comes from the last command executed\n> > so if the rev-parse fails, its return code is swallowed and we have no\n> > way of knowing.\n> >\n> > You could break the pipe up by storing the output of the rev-parse in an\n> > intermediate file and then have cut read from that file.\n> \n> This is a good suggestion (I was preparing to write an email to say the\n> same thing, but I'm glad that I checked Denton's response before doing\n> so). Something like:\n> \n> \tgit rev-parse --verify HEAD >out &&\n> \tcut -c1-7 out\n> \n> would suffice and be in good style.\n\nSure. I will make the amends.\n\n"},{"id":"403032","messageId":"20200806085043.GB13942@konoha","threadId":"53989","inReplyTo":"xmqq3650ok53.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-06T08:50:43Z","receivedAt":"2020-08-06T11:13:16Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 05/08 02:30, Junio C Hamano wrote:\n> Shourya Shukla writes:\n> \n> > The '--for-status' test got its expected output from stdin. This is\n> > inconsistent with the other tests in the test script which get their\n> > expected output from a file named 'expected'.\n> >\n> > So, change the syntax of the '--for-status' test for uniformity.\n> \n> There are a handful examples in t5401 and another one in t3700 that\n> give the \"golden master\" from the standard input. When the expected\n> output is used only once, I do not think it is particularlly bad to\n> have it this way. So,... meh?\n\nI realised what you were trying to say after checking out t5400 and\nt3700. I understand that this change may not be immediately needed but I\nthink it does make reading the diff a bit easier since having a '-' as a\nfile name does get a bit confusing when reading the output. If a\nsepareate commit just for this change is a problem then I can squash\nthis with the previous commit? What do you think?\n\n> Unless there is a compelling reason to insist both are files\n> (that may make it easier to enhance test_cmp in a certain way\n> you plan to, for example), that is.\n> \n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Signed-off-by: Shourya Shukla \n> > ---\n> > t/t7401-submodule-summary.sh | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> > index 18fefdb0ba..145914cd5a 100755\n> > --- a/t/t7401-submodule-summary.sh\n> > +++ b/t/t7401-submodule-summary.sh\n> > @@ -285,13 +285,14 @@ EOF\n> > \n> > test_expect_success '--for-status' \"\n> > \tgit submodule summary --for-status HEAD^ >actual &&\n> > -\ttest_i18ncmp actual - < > +\tcat >expected <<-EOF &&\n> > * sm1 $head6...0000000:\n> > \n> > * sm2 0000000...$head7 (2):\n> > > Add foo9\n> > \n> > EOF\n> > +\ttest_i18ncmp expected actual\n> > \"\n> > \n> > test_expect_success 'fail when using --files together with --cached' \"\n"},{"id":"403056","messageId":"xmqqimdvlml9.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200806085043.GB13942@konoha","subject":"Re: [PATCH 3/4] t7401: ensure uniformity in the '--for-status' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-06T17:18:26Z","receivedAt":"2020-08-06T17:19:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> On 05/08 02:30, Junio C Hamano wrote:\n>> Shourya Shukla writes:\n>> \n>> > The '--for-status' test got its expected output from stdin. This is\n>> > inconsistent with the other tests in the test script which get their\n>> > expected output from a file named 'expected'.\n>> >\n>> > So, change the syntax of the '--for-status' test for uniformity.\n>> \n>> There are a handful examples in t5401 and another one in t3700 that\n>> give the \"golden master\" from the standard input. When the expected\n>> output is used only once, I do not think it is particularlly bad to\n>> have it this way. So,... meh?\n>\n> I realised what you were trying to say after checking out t5400 and\n> t3700. I understand that this change may not be immediately needed but I\n> think it does make reading the diff a bit easier since having a '-' as a\n> file name does get a bit confusing when reading the output. \n\nIf so, perhaps justifying the change based on that, not on\n\"consistency\", would be a good idea.\n\n Side note: But would that mean you'd find it \"confusing\" to\n read output from 3700 and 5400? Would \"test writers should\n get used to it\" be a workable alternative solution?\n\nSince \"test_cmp expect actual\" and \"test_cmp - actual < Shourya Shukla writes:\n> \n> > Add a WARNING regarding the usage of 'git add' instead of 'git\n> > submodule add' to add submodules to the superproject.\n> \n> Is that a warning worthy thing? As far as I know, using \"git add\"\n> to register a gitlink is perfectly fine and a supported way to start\n> a new submodule. It may have to be followed by other steps like\n> \"git config -f .gitmodules\" (e.g. when operations that needs to use\n> the contents recorded in the .gitmodules file are to be tested), but\n> writing tests using lower-level ingredients for finer grained tests\n> is not all that unusual, is it? I dunno.\n\nThe thing is that 'git submodule {init,deinit}' fail when there is no\n.gitmodules. I can initiliase the .gitmodules separately using 'git\nconfig -f .gitmodules' but I think it will be better to use 'git\nsubmodule add' throughout the script rather than worry about it all the\ntime.\n\nBut again, if the warning seems unnecessary, then I can obviously use\n'git config' to initilaise the submodules and change this commit. What\ndo you reckon?\n\n> > NEEDSWORK regarding the outdated syntax and working of the test, which\n> > may need to be improved to obtain better and desired results.\n> \n> Sounds good.\n> \n> > While at it, change the word 'test' to 'test script' in the test\n> > description to avoid ambiguity.\n> \n> Sounds good. I often search for a pair of phrases to differentiate\n> a single tXXXX-name.sh file as a whole and an individual test piece\n> in it. \"This test script\", especially when written near the\n> beginning of the file, is a good way to clearly convey that you want\n> to refer to the former.\n> \n> > Mentored-by: Christian Couder \n> > Mentored-by: Kaartic Sivaraam \n> > Signed-off-by: Shourya Shukla \n> > ---\n> > t/t7401-submodule-summary.sh | 7 ++++++-\n> > 1 file changed, 6 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\n> > index 145914cd5a..2db4cf5cbf 100755\n> > --- a/t/t7401-submodule-summary.sh\n> > +++ b/t/t7401-submodule-summary.sh\n> > @@ -5,8 +5,13 @@\n> > \n> > test_description='Summary support for submodules\n> > \n> > -This test tries to verify the sanity of summary subcommand of git submodule.\n> > +This test script tries to verify the sanity of summary subcommand of git submodule.\n> > '\n> > +# WARNING: This test script uses 'git add' instead of 'git submodule add' to add\n> > +# submodules to the superproject. Some submodule subcommands such as init and\n> > +# deinit might not work as expected in this script.\n> > +\n> > +# NEEDSWORK: This test script is old fashioned and may need a big cleanup.\n> > \n> > . ./test-lib.sh\n"},{"id":"403099","messageId":"xmqqa6z7jx7q.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200806112702.GA15343@konoha","subject":"Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-06T21:11:53Z","receivedAt":"2020-08-06T21:11:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> On 05/08 02:36, Junio C Hamano wrote:\n>> Shourya Shukla writes:\n>> \n>> > Add a WARNING regarding the usage of 'git add' instead of 'git\n>> > submodule add' to add submodules to the superproject.\n>> \n>> Is that a warning worthy thing? As far as I know, using \"git add\"\n>> to register a gitlink is perfectly fine and a supported way to start\n>> a new submodule. It may have to be followed by other steps like\n>> \"git config -f .gitmodules\" (e.g. when operations that needs to use\n>> the contents recorded in the .gitmodules file are to be tested), but\n>> writing tests using lower-level ingredients for finer grained tests\n>> is not all that unusual, is it? I dunno.\n>\n> The thing is that 'git submodule {init,deinit}' fail when there is no\n> .gitmodules. I can initiliase the .gitmodules separately using 'git\n> config -f .gitmodules' but I think it will be better to use 'git\n> submodule add' throughout the script rather than worry about it all the\n> time.\n\nOn the other hand, we do want to make sure that the workflow using\nlower-level tools continues to work, so that is a balancing act.\n\n> But again, if the warning seems unnecessary, then I can obviously use\n> 'git config' to initilaise the submodules and change this commit. What\n> do you reckon?\n\nIf you need \"git submodule init\" etc. to work in this test, yes, you\ncan change the test to either use \"git submodule add\" instead, or\n\"git config -f .gitmodules\" in addition. If you don't, there is no\nneed to change anything, no?\n"},{"id":"403131","messageId":"CAP8UFD21i6UOwSA8hoCbF9kbRaaQzmN4e0t+pPrF1fW+SSJwww@mail.gmail.com","threadId":"53989","inReplyTo":"xmqqa6z7jx7q.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] t7401: add a WARNING and a NEEDSWORK","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-08-07T14:55:11Z","receivedAt":"2020-08-07T14:55:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Aug 6, 2020 at 11:11 PM Junio C Hamano wrote:\n>\n> Shourya Shukla writes:\n>\n> > On 05/08 02:36, Junio C Hamano wrote:\n> >> Shourya Shukla writes:\n> >>\n> >> > Add a WARNING regarding the usage of 'git add' instead of 'git\n> >> > submodule add' to add submodules to the superproject.\n> >>\n> >> Is that a warning worthy thing? As far as I know, using \"git add\"\n> >> to register a gitlink is perfectly fine and a supported way to start\n> >> a new submodule. It may have to be followed by other steps like\n> >> \"git config -f .gitmodules\" (e.g. when operations that needs to use\n> >> the contents recorded in the .gitmodules file are to be tested), but\n> >> writing tests using lower-level ingredients for finer grained tests\n> >> is not all that unusual, is it? I dunno.\n> >\n> > The thing is that 'git submodule {init,deinit}' fail when there is no\n> > .gitmodules. I can initiliase the .gitmodules separately using 'git\n> > config -f .gitmodules' but I think it will be better to use 'git\n> > submodule add' throughout the script rather than worry about it all the\n> > time.\n>\n> On the other hand, we do want to make sure that the workflow using\n> lower-level tools continues to work, so that is a balancing act.\n\nYeah, that's the reason why we suggested to add the new tests in a new\ntest script t7421:\n\nhttps://lore.kernel.org/git/20200806164102.6707-5-shouryashukla.oo@gmail.com/\n\n> > But again, if the warning seems unnecessary, then I can obviously use\n> > 'git config' to initilaise the submodules and change this commit. What\n> > do you reckon?\n>\n> If you need \"git submodule init\" etc. to work in this test, yes, you\n> can change the test to either use \"git submodule add\" instead, or\n> \"git config -f .gitmodules\" in addition. If you don't, there is no\n> need to change anything, no?\n\nIn t7421 \"git submodule add\" is used, so that we can perform the new\ntests we want there. The purpose of this patch adding a WARNING and a\nNEEDSWORK was to tell explicitly that this test script (t7401) is not\nnecessarily the best test script for \"git submodule summary\" tests\nbecause it is very old fashioned as it has a lot of boilerplate stuff\noutside test_expect_{success, failure} and it uses \"git add\" instead\nof \"git submodule add\".\n\nI think we might want to just add the NEEDSWORK in this patch series\nand add the WARNING, or perhaps just a NOTE, about \"git add\" vs \"git\nsubmodule add\" in the other patch series when we introduce t7421.\n"},{"id":"403488","messageId":"20200812192737.13971-1-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200805174921.16000-1-shouryashukla.oo@gmail.com","subject":"[GSoC][PATCH v2 0/4] t7401: modernize, cleanup and more","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:27:33Z","receivedAt":"2020-08-12T19:27:54Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Greetings,\n\nThis is the v2 of the previously posted patch series titled\n't7401: modernize, cleanup and warn':\nhttps://lore.kernel.org/git/20200805174921.16000-1-shouryashukla.oo@gmail.com/\n\nAfter suggestions from Denton, Taylor and Junio I made some changes:\n\n-> In 2939804509 (t7401: modernize style, 2020-07-23), after suggestions\n from Denton and Taylor, I redirected the output of the two\n 'rev-parse' commands to a file and then read from it instead of using\n a pipe '|' operator.\n\n-> Drop the commit ab28283b67 (t7401: ensure uniformity in the\n '--for-status' test, 2020-07-10) and instead combine it with\n 00c6289d5e (t7401: change test_i18ncmp syntax for clarity, 2020-07-10).\n This was suggested by Junio and Taylor.\n\n-> Introduce the commit f0b87ddaf6 (t7401: change indentation for\n enhanced readability, 2020-08-11) which improves the indentation of\n the tests in t7401. This was suggested by Junio and Taylor.\n\n-> Remove the WARNING from the commit 0bdb0bd72c (t7401: add a WARNING\n and a NEEDSWORK, 2020-07-23) and instead limit it to a more improved\n NEEDSWORK thus leading to the commit a743c28d71 (t7401: add a\n NEEDSWORK, 2020-07-23). This was suggested by Taylor.\n\nFeedback and reviews are appreciated. I am tagging along a range-diff\nbetween the v1 and v2 for ease of review.\n\nRegards,\nShourya Shukla\n\n-----\nrange-diff:\n\n1: 31ae4038f1 ! 1: 2939804509 t7401: modernize style\n @@ Commit message\n t7401: modernize style\n\n The tests in 't7401-submodule-summary.sh' were written a long time ago\n - and have some violations with respect to our CodingGuidelines such as\n - incorrect spacing in usages of the redirection operator and absence\n - of line feed between statements in case of the '|' (pipe) operator.\n - Update it to match the CodingGuidelines.\n + and has a violation with respect to our CodingGuidelines which is,\n + incorrect spacing in usages of the redirection operator.\n + Using a Git command in the upstream of a pipe might result in us\n + losing its exit code. So, convert such usages so that they write to\n + a file and read from them.\n\n Mentored-by: Christian Couder \n Mentored-by: Kaartic Sivaraam \n + Helped-by: Denton Liu \n + Helped-by: Taylor Blau \n Signed-off-by: Shourya Shukla \n\n ## t/t7401-submodule-summary.sh ##\n @@ t/t7401-submodule-summary.sh: add_file () {\n git commit -m \"Add $name\"\n git commit -m \"Add $name\"\n done >/dev/null\n - git rev-parse --verify HEAD | cut -c1-7\n -+ git rev-parse --verify HEAD |\n -+ cut -c1-7\n ++ git rev-parse --verify HEAD >out &&\n ++ cut -c1-7 out\n cd \"$owd\"\n }\n commit_file () {\n @@ t/t7401-submodule-summary.sh: commit_file sm1 &&\n cd sm1 &&\n git reset --hard HEAD~2 >/dev/null &&\n - git rev-parse --verify HEAD | cut -c1-7\n -+ git rev-parse --verify HEAD |\n -+ cut -c1-7\n ++ git rev-parse --verify HEAD >out &&\n ++ cut -c1-7 out\n )\n\n test_expect_success 'modified submodule(backward)' \"\n2: a3160d1ecc ! 2: 00c6289d5e t7401: change test_i18ncmp syntax for clarity\n @@ t/t7401-submodule-summary.sh: test_expect_success 'nonexistent commit' \"\n \"\n\n commit_file\n +@@ t/t7401-submodule-summary.sh: EOF\n +\n + test_expect_success '--for-status' \"\n + git submodule summary --for-status HEAD^ >actual &&\n +- test_i18ncmp actual - < 3: f0b87ddaf6 t7401: change indentation for enhanced readability\n4: 0bdb0bd72c ! 4: a743c28d71 t7401: add a WARNING and a NEEDSWORK\n @@ Metadata\n Author: Shourya Shukla \n\n ## Commit message ##\n - t7401: add a WARNING and a NEEDSWORK\n + t7401: add a NEEDSWORK\n\n - Add a WARNING regarding the usage of 'git add' instead of 'git\n - submodule add' to add submodules to the superproject. Also add a\n - NEEDSWORK regarding the outdated syntax and working of the test, which\n - may need to be improved to obtain better and desired results.\n + Add a NEEDSWORK regarding the outdated syntax and working of the test,\n + which may need to be improved to obtain better and desired results.\n\n While at it, change the word 'test' to 'test script' in the test\n description to avoid ambiguity.\n\n Mentored-by: Christian Couder \n Mentored-by: Kaartic Sivaraam \n + Helped-by: Taylor Blau \n Signed-off-by: Shourya Shukla \n\n ## t/t7401-submodule-summary.sh ##\n @@ t/t7401-submodule-summary.sh\n -This test tries to verify the sanity of summary subcommand of git submodule.\n +This test script tries to verify the sanity of summary subcommand of git submodule.\n '\n -+# WARNING: This test script uses 'git add' instead of 'git submodule add' to add\n -+# submodules to the superproject. Some submodule subcommands such as init and\n -+# deinit might not work as expected in this script.\n -+\n -+# NEEDSWORK: This test script is old fashioned and may need a big cleanup.\n ++# NEEDSWORK: This test script is old fashioned and may need a big cleanup since\n ++# there are lots of commands taking place outside of 'test_expect_success'\n ++# block, which is no longer in good-style.\n\n . ./test-lib.sh\n\n-----\n\nShourya Shukla (4):\n t7401: modernize style\n t7401: change test_i18ncmp syntax for clarity\n t7401: change indentation for enhanced readability\n t7401: add a NEEDSWORK\n\n t/t7401-submodule-summary.sh | 151 ++++++++++++++++++-----------------\n 1 file changed, 78 insertions(+), 73 deletions(-)\n\n-- \n2.28.0\n\n"},{"id":"403489","messageId":"20200812192737.13971-2-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200812192737.13971-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 1/4] t7401: modernize style","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:27:34Z","receivedAt":"2020-08-12T19:27:58Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"The tests in 't7401-submodule-summary.sh' were written a long time ago\nand has a violation with respect to our CodingGuidelines which is,\nincorrect spacing in usages of the redirection operator.\nUsing a Git command in the upstream of a pipe might result in us\nlosing its exit code. So, convert such usages so that they write to\na file and read from them.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Denton Liu \nHelped-by: Taylor Blau \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 9bc841d085..8ee78bcb69 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -16,12 +16,13 @@ add_file () {\n \towd=$(pwd)\n \tcd \"$sm\"\n \tfor name; do\n-\t\techo \"$name\" > \"$name\" &&\n+\t\techo \"$name\" >\"$name\" &&\n \t\tgit add \"$name\" &&\n \t\ttest_tick &&\n \t\tgit commit -m \"Add $name\"\n \tdone >/dev/null\n-\tgit rev-parse --verify HEAD | cut -c1-7\n+\tgit rev-parse --verify HEAD >out &&\n+\tcut -c1-7 out\n \tcd \"$owd\"\n }\n commit_file () {\n@@ -125,7 +126,8 @@ commit_file sm1 &&\n head3=$(\n \tcd sm1 &&\n \tgit reset --hard HEAD~2 >/dev/null &&\n-\tgit rev-parse --verify HEAD | cut -c1-7\n+\tgit rev-parse --verify HEAD >out &&\n+\tcut -c1-7 out\n )\n \n test_expect_success 'modified submodule(backward)' \"\n-- \n2.28.0\n\n"},{"id":"403490","messageId":"20200812192737.13971-3-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200812192737.13971-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 2/4] t7401: change test_i18ncmp syntax for clarity","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:27:35Z","receivedAt":"2020-08-12T19:28:02Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n'test_i18ncmp expected actual' to align it with the convention followed\nby other tests in the test script.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 8ee78bcb69..53943c9cc9 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -183,7 +183,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' \"\n < Add foo5\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n@@ -193,7 +193,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n > Add foo5\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n rm -rf sm1 &&\n@@ -204,7 +204,7 @@ test_expect_success 'typechanged submodule(submodule->blob)' \"\n * sm1 $head4(submodule)->$head5(blob):\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n rm -f sm1 &&\n@@ -217,7 +217,7 @@ test_expect_success 'nonexistent commit' \"\n Warn: sm1 doesn't contain commit $head4_full\n \n EOF\n-\ttest_i18ncmp actual expected\n+\ttest_i18ncmp expected actual\n \"\n \n commit_file\n@@ -285,7 +285,7 @@ EOF\n \n test_expect_success '--for-status' \"\n \tgit submodule summary --for-status HEAD^ >actual &&\n-\ttest_i18ncmp actual - <\nMentored-by: Kaartic Sivaraam \nHelped-by: Junio C Hamano \nHelped-by: Taylor Blau \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 128 +++++++++++++++++------------------\n 1 file changed, 64 insertions(+), 64 deletions(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex 53943c9cc9..dd0e88fc6a 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -39,10 +39,10 @@ test_expect_success 'added submodule' \"\n \tgit add sm1 &&\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 0000000...$head1 (2):\n- > Add foo2\n+\t* sm1 0000000...$head1 (2):\n+\t > Add foo2\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -53,10 +53,10 @@ test_expect_success 'added submodule (subdirectory)' \"\n \t\tgit submodule summary >../actual\n \t) &&\n \tcat >expected <<-EOF &&\n-* ../sm1 0000000...$head1 (2):\n- > Add foo2\n+\t* ../sm1 0000000...$head1 (2):\n+\t > Add foo2\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -74,10 +74,10 @@ test_expect_success 'added submodule (subdirectory with explicit path)' \"\n \t\tgit submodule summary ../sm1 >../actual\n \t) &&\n \tcat >expected <<-EOF &&\n-* ../sm1 0000000...$head1 (2):\n- > Add foo2\n+\t* ../sm1 0000000...$head1 (2):\n+\t > Add foo2\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -87,20 +87,20 @@ head2=$(add_file sm1 foo3)\n test_expect_success 'modified submodule(forward)' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head1...$head2 (1):\n- > Add foo3\n+\t* sm1 $head1...$head2 (1):\n+\t > Add foo3\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n test_expect_success 'modified submodule(forward), --files' \"\n \tgit submodule summary --files >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head1...$head2 (1):\n- > Add foo3\n+\t* sm1 $head1...$head2 (1):\n+\t > Add foo3\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -111,10 +111,10 @@ test_expect_success 'no ignore=all setting has any effect' \"\n \tgit config diff.ignoreSubmodules all &&\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head1...$head2 (1):\n- > Add foo3\n+\t* sm1 $head1...$head2 (1):\n+\t > Add foo3\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual &&\n \tgit config --unset diff.ignoreSubmodules &&\n \tgit config --remove-section submodule.sm1 &&\n@@ -133,11 +133,11 @@ head3=$(\n test_expect_success 'modified submodule(backward)' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head2...$head3 (2):\n- < Add foo3\n- < Add foo2\n+\t* sm1 $head2...$head3 (2):\n+\t < Add foo3\n+\t < Add foo2\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -146,25 +146,25 @@ head4_full=$(GIT_DIR=sm1/.git git rev-parse --verify HEAD)\n test_expect_success 'modified submodule(backward and forward)' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head2...$head4 (4):\n- > Add foo5\n- > Add foo4\n- < Add foo3\n- < Add foo2\n+\t* sm1 $head2...$head4 (4):\n+\t > Add foo5\n+\t > Add foo4\n+\t < Add foo3\n+\t < Add foo2\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n test_expect_success '--summary-limit' \"\n \tgit submodule summary -n 3 >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head2...$head4 (4):\n- > Add foo5\n- > Add foo4\n- < Add foo3\n+\t* sm1 $head2...$head4 (4):\n+\t > Add foo5\n+\t > Add foo4\n+\t < Add foo3\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -179,20 +179,20 @@ mv sm1-bak sm1\n test_expect_success 'typechanged submodule(submodule->blob), --cached' \"\n \tgit submodule summary --cached >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head4(submodule)->$head5(blob) (3):\n- < Add foo5\n+\t* sm1 $head4(submodule)->$head5(blob) (3):\n+\t < Add foo5\n \n-EOF\n+\tEOF\n \ttest_i18ncmp expected actual\n \"\n \n test_expect_success 'typechanged submodule(submodule->blob), --files' \"\n \tgit submodule summary --files >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head5(blob)->$head4(submodule) (3):\n- > Add foo5\n+\t* sm1 $head5(blob)->$head4(submodule) (3):\n+\t > Add foo5\n \n-EOF\n+\tEOF\n \ttest_i18ncmp expected actual\n \"\n \n@@ -201,9 +201,9 @@ git checkout-index sm1\n test_expect_success 'typechanged submodule(submodule->blob)' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head4(submodule)->$head5(blob):\n+\t* sm1 $head4(submodule)->$head5(blob):\n \n-EOF\n+\tEOF\n \ttest_i18ncmp expected actual\n \"\n \n@@ -213,10 +213,10 @@ head6=$(add_file sm1 foo6 foo7)\n test_expect_success 'nonexistent commit' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head4...$head6:\n- Warn: sm1 doesn't contain commit $head4_full\n+\t* sm1 $head4...$head6:\n+\t Warn: sm1 doesn't contain commit $head4_full\n \n-EOF\n+\tEOF\n \ttest_i18ncmp expected actual\n \"\n \n@@ -224,10 +224,10 @@ commit_file\n test_expect_success 'typechanged submodule(blob->submodule)' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head5(blob)->$head6(submodule) (2):\n- > Add foo7\n+\t* sm1 $head5(blob)->$head6(submodule) (2):\n+\t > Add foo7\n \n-EOF\n+\tEOF\n \ttest_i18ncmp expected actual\n \"\n \n@@ -236,9 +236,9 @@ rm -rf sm1\n test_expect_success 'deleted submodule' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head6...0000000:\n+\t* sm1 $head6...0000000:\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -251,22 +251,22 @@ test_expect_success 'create second submodule' '\n test_expect_success 'multiple submodules' \"\n \tgit submodule summary >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head6...0000000:\n+\t* sm1 $head6...0000000:\n \n-* sm2 0000000...$head7 (2):\n- > Add foo9\n+\t* sm2 0000000...$head7 (2):\n+\t > Add foo9\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n test_expect_success 'path filter' \"\n \tgit submodule summary sm2 >actual &&\n \tcat >expected <<-EOF &&\n-* sm2 0000000...$head7 (2):\n- > Add foo9\n+\t* sm2 0000000...$head7 (2):\n+\t > Add foo9\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n@@ -274,24 +274,24 @@ commit_file sm2\n test_expect_success 'given commit' \"\n \tgit submodule summary HEAD^ >actual &&\n \tcat >expected <<-EOF &&\n-* sm1 $head6...0000000:\n+\t* sm1 $head6...0000000:\n \n-* sm2 0000000...$head7 (2):\n- > Add foo9\n+\t* sm2 0000000...$head7 (2):\n+\t > Add foo9\n \n-EOF\n+\tEOF\n \ttest_cmp expected actual\n \"\n \n test_expect_success '--for-status' \"\n \tgit submodule summary --for-status HEAD^ >actual &&\n \ttest_i18ncmp - actual <<-EOF\n-* sm1 $head6...0000000:\n+\t* sm1 $head6...0000000:\n \n-* sm2 0000000...$head7 (2):\n- > Add foo9\n+\t* sm2 0000000...$head7 (2):\n+\t > Add foo9\n \n-EOF\n+\tEOF\n \"\n \n test_expect_success 'fail when using --files together with --cached' \"\n-- \n2.28.0\n\n"},{"id":"403492","messageId":"20200812192737.13971-5-shouryashukla.oo@gmail.com","threadId":"53989","inReplyTo":"20200812192737.13971-1-shouryashukla.oo@gmail.com","subject":"[PATCH v2 4/4] t7401: add a NEEDSWORK","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-12T19:27:37Z","receivedAt":"2020-08-12T19:28:09Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Add a NEEDSWORK regarding the outdated syntax and working of the test,\nwhich may need to be improved to obtain better and desired results.\n\nWhile at it, change the word 'test' to 'test script' in the test\ndescription to avoid ambiguity.\n\nMentored-by: Christian Couder \nMentored-by: Kaartic Sivaraam \nHelped-by: Taylor Blau \nSigned-off-by: Shourya Shukla \n---\n t/t7401-submodule-summary.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh\nindex dd0e88fc6a..8f5e4515d3 100755\n--- a/t/t7401-submodule-summary.sh\n+++ b/t/t7401-submodule-summary.sh\n@@ -5,8 +5,11 @@\n \n test_description='Summary support for submodules\n \n-This test tries to verify the sanity of summary subcommand of git submodule.\n+This test script tries to verify the sanity of summary subcommand of git submodule.\n '\n+# NEEDSWORK: This test script is old fashioned and may need a big cleanup since\n+# there are lots of commands taking place outside of 'test_expect_success'\n+# block, which is no longer in good-style.\n \n . ./test-lib.sh\n \n-- \n2.28.0\n\n"},{"id":"403507","messageId":"xmqq1rkb60ql.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"20200812192737.13971-3-shouryashukla.oo@gmail.com","subject":"Re: [PATCH v2 2/4] t7401: change test_i18ncmp syntax for clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T20:57:38Z","receivedAt":"2020-08-12T20:57:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shourya Shukla writes:\n\n> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n> 'test_i18ncmp expected actual' to align it with the convention followed\n> by other tests in the test script.\n> ...\n> @@ -285,7 +285,7 @@ EOF\n> \n> test_expect_success '--for-status' \"\n> \tgit submodule summary --for-status HEAD^ >actual &&\n> -\ttest_i18ncmp actual - < +\ttest_i18ncmp - actual <<-EOF\n> * sm1 $head6...0000000:\n> \n> * sm2 0000000...$head7 (2):\n\nThis one does more than what the proposed log message explains, but\nit does not do enough at the same time. \n\nIf \"actual vs expected order\" is what this commit wants to fix, then\n\"< writes:\n\n> Shourya Shukla writes:\n>\n>> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n>> 'test_i18ncmp expected actual' to align it with the convention followed\n>> by other tests in the test script.\n>> ...\n>> @@ -285,7 +285,7 @@ EOF\n>> \n>> test_expect_success '--for-status' \"\n>> \tgit submodule summary --for-status HEAD^ >actual &&\n>> -\ttest_i18ncmp actual - <> +\ttest_i18ncmp - actual <<-EOF\n>> * sm1 $head6...0000000:\n>> \n>> * sm2 0000000...$head7 (2):\n>\n> This one does more than what the proposed log message explains, but\n> it does not do enough at the same time. \n>\n> If \"actual vs expected order\" is what this commit wants to fix, then\n> \"<\n> Personally, I think it is a good idea to update the end-of-heredoc\n> marker to \"<<-EOF\", ...\n\nIt seems that the theme of your [3/4] is exactly about fixing the\n\"end-of-heredoc marker is prefixed with dash, but the heredoc is not\nindented for readability\", so perhaps you'd want to stop this step\nat turning the line to\n\n>> -\ttest_i18ncmp actual - <> +\ttest_i18ncmp - actual <\n> [...]\n>\n> Using a Git command in the upstream of a pipe might result in us\n> losing its exit code. So, convert such usages so that they write to\n> a file and read from them.\n> \n\nWhile that is a good enough reason to avoid using pipes in places where\nwe look for the exit code of a command like within test_expect_success,\nI'm not sure if that reason holds for the places that the patch\nchanges.\n\n> [...]\n>\n> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-\n> summary.sh\n> index 9bc841d085..8ee78bcb69 100755\n> --- a/t/t7401-submodule-summary.sh\n> +++ b/t/t7401-submodule-summary.sh\n> @@ -16,12 +16,13 @@ add_file () {\n> \towd=$(pwd)\n> \tcd \"$sm\"\n> \tfor name; do\n> -\t\techo \"$name\" > \"$name\" &&\n> +\t\techo \"$name\" >\"$name\" &&\n> \t\tgit add \"$name\" &&\n> \t\ttest_tick &&\n> \t\tgit commit -m \"Add $name\"\n> \tdone >/dev/null\n> -\tgit rev-parse --verify HEAD | cut -c1-7\n> +\tgit rev-parse --verify HEAD >out &&\n> +\tcut -c1-7 out\n\nIn any case, I believe we can avoid the 'cut' altogether in both places\nby doing something like this instead:\n\n git rev-parse --short=7 HEAD\n\nMy quick check shows the test script is happy with this change.\n\n> \tcd \"$owd\"\n> }\n> commit_file () {\n> @@ -125,7 +126,8 @@ commit_file sm1 &&\n> head3=$(\n> \tcd sm1 &&\n> \tgit reset --hard HEAD~2 >/dev/null &&\n> -\tgit rev-parse --verify HEAD | cut -c1-7\n> +\tgit rev-parse --verify HEAD >out &&\n> +\tcut -c1-7 out\n> )\n> \n> test_expect_success 'modified submodule(backward)' \"\n\n-- \nSivaraam\n\n\n"},{"id":"403566","messageId":"xmqqd03u4hpl.fsf@gitster.c.googlers.com","threadId":"53989","inReplyTo":"c70f7bb5d1289f5318da7b99e41cf5828a451174.camel@gmail.com","subject":"Re: [PATCH v2 1/4] t7401: modernize style","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-13T16:46:14Z","receivedAt":"2020-08-13T16:46:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n>> @@ -16,12 +16,13 @@ add_file () {\n>> \towd=$(pwd)\n>> \tcd \"$sm\"\n>> \tfor name; do\n>> -\t\techo \"$name\" > \"$name\" &&\n>> +\t\techo \"$name\" >\"$name\" &&\n>> \t\tgit add \"$name\" &&\n>> \t\ttest_tick &&\n>> \t\tgit commit -m \"Add $name\"\n>> \tdone >/dev/null\n>> -\tgit rev-parse --verify HEAD | cut -c1-7\n>> +\tgit rev-parse --verify HEAD >out &&\n>> +\tcut -c1-7 out\n>\n> In any case, I believe we can avoid the 'cut' altogether in both places\n> by doing something like this instead:\n>\n> git rev-parse --short=7 HEAD\n\nAh, I missed the fact that this was a helper function and most of\nthe error status is discarded anyway. For example, we still run the\nrev-parse even after the for loop fails.\n\nIf the focus of this test script were to ensure that rev-parse works\ncorrectly, being careful to catch its exit status might have had a\ngood value, but for that, all the other operations that happen in\nthis helper function (including the \"what happens when the loop body\nfails for $name that is not at the end of the argument list?\") must\nalso be checked for their exit status in the first place.\n\nSince that is not done, and since testing rev-parse should not have\nto be part of the job for submodule test, the net effect of the\nabove change has quite dubious value---it clobbered a file 'out'\nthat may be used by the caller.\n\nDoing \"cd\" without introducing a subshell is a bit harder to fix, as\ntest_tick relies on the global counter in the topmost process. It\ncan be done, but I do not think it is worth doing here. Most of the\nusers of this helper function call it in var=$(add_file ...)\nsubshell anyway (so test_tick is incrementing the timestamp\nindependently for each caller and discarding the resulting\ntimestamp). As a NEEDSWORK comment added in the series says, this\nscript may need a bit more work.\n\nI agree with you that the split of \"rev-parse | cut -c1-7\" into two\nstatements and clobbering 'out' is a bad change---that part should\nbe reverted. The style change on 'echo \"$name\" >\"$name\"' line is\nOK, though.\n\nThanks.\n\n> My quick check shows the test script is happy with this change.\n>\n>> \tcd \"$owd\"\n>> }\n>> commit_file () {\n>> @@ -125,7 +126,8 @@ commit_file sm1 &&\n>> head3=$(\n>> \tcd sm1 &&\n>> \tgit reset --hard HEAD~2 >/dev/null &&\n>> -\tgit rev-parse --verify HEAD | cut -c1-7\n>> +\tgit rev-parse --verify HEAD >out &&\n>> +\tcut -c1-7 out\n>> )\n>> \n>> test_expect_success 'modified submodule(backward)' \"\n"},{"id":"403666","messageId":"20200814144126.GA137865@konoha","threadId":"53989","inReplyTo":"xmqqd03u4hpl.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/4] t7401: modernize style","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-14T14:41:26Z","receivedAt":"2020-08-14T14:41:36Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 13/08 09:46, Junio C Hamano wrote:\n> Kaartic Sivaraam writes:\n> \n> >> @@ -16,12 +16,13 @@ add_file () {\n> >> \towd=$(pwd)\n> >> \tcd \"$sm\"\n> >> \tfor name; do\n> >> -\t\techo \"$name\" > \"$name\" &&\n> >> +\t\techo \"$name\" >\"$name\" &&\n> >> \t\tgit add \"$name\" &&\n> >> \t\ttest_tick &&\n> >> \t\tgit commit -m \"Add $name\"\n> >> \tdone >/dev/null\n> >> -\tgit rev-parse --verify HEAD | cut -c1-7\n> >> +\tgit rev-parse --verify HEAD >out &&\n> >> +\tcut -c1-7 out\n> >\n> > In any case, I believe we can avoid the 'cut' altogether in both places\n> > by doing something like this instead:\n> >\n> > git rev-parse --short=7 HEAD\n> \n> Ah, I missed the fact that this was a helper function and most of\n> the error status is discarded anyway. For example, we still run the\n> rev-parse even after the for loop fails.\n> \n> If the focus of this test script were to ensure that rev-parse works\n> correctly, being careful to catch its exit status might have had a\n> good value, but for that, all the other operations that happen in\n> this helper function (including the \"what happens when the loop body\n> fails for $name that is not at the end of the argument list?\") must\n> also be checked for their exit status in the first place.\n> \n> Since that is not done, and since testing rev-parse should not have\n> to be part of the job for submodule test, the net effect of the\n> above change has quite dubious value---it clobbered a file 'out'\n> that may be used by the caller.\n> \n> Doing \"cd\" without introducing a subshell is a bit harder to fix, as\n> test_tick relies on the global counter in the topmost process. It\n> can be done, but I do not think it is worth doing here. Most of the\n> users of this helper function call it in var=$(add_file ...)\n> subshell anyway (so test_tick is incrementing the timestamp\n> independently for each caller and discarding the resulting\n> timestamp). As a NEEDSWORK comment added in the series says, this\n> script may need a bit more work.\n> \n> I agree with you that the split of \"rev-parse | cut -c1-7\" into two\n> statements and clobbering 'out' is a bad change---that part should\n> be reverted. The style change on 'echo \"$name\" >\"$name\"' line is\n> OK, though.\n> \n> Thanks.\n\nUnderstood. I will revert the change. Though, what Kaartic suggested, to\ndo a '--short=7', that will be okay to keep right? Something like:\n\n git rev-parse --short=7 HEAD\n\nThis way we will not need a 'cut'. This change goes as a separate commit\nobviously.\n\n"},{"id":"403669","messageId":"20200814145741.GA147891@konoha","threadId":"53989","inReplyTo":"xmqqwo234lya.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/4] t7401: change test_i18ncmp syntax for clarity","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-14T14:57:41Z","receivedAt":"2020-08-14T14:57:51Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 12/08 02:02, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Shourya Shukla writes:\n> >\n> >> Change the test_i18ncmp syntax from 'test_i18ncmp actual expected' to\n> >> 'test_i18ncmp expected actual' to align it with the convention followed\n> >> by other tests in the test script.\n> >> ...\n> >> @@ -285,7 +285,7 @@ EOF\n> >> \n> >> test_expect_success '--for-status' \"\n> >> \tgit submodule summary --for-status HEAD^ >actual &&\n> >> -\ttest_i18ncmp actual - < >> +\ttest_i18ncmp - actual <<-EOF\n> >> * sm1 $head6...0000000:\n> >> \n> >> * sm2 0000000...$head7 (2):\n> >\n> > This one does more than what the proposed log message explains, but\n> > it does not do enough at the same time. \n> >\n> > If \"actual vs expected order\" is what this commit wants to fix, then\n> > \"< >\n> > Personally, I think it is a good idea to update the end-of-heredoc\n> > marker to \"<<-EOF\", ...\n> \n> It seems that the theme of your [3/4] is exactly about fixing the\n> \"end-of-heredoc marker is prefixed with dash, but the heredoc is not\n> indented for readability\", so perhaps you'd want to stop this step\n> at turning the line to\n> \n> >> -\ttest_i18ncmp actual - < >> +\ttest_i18ncmp - actual < \n> i.e. \"compare expected vs actual in this order\", and then in the\n> next patch [3/4], edit the line further to\n> \n> \ttest_i18ncmp - actual <<-EOF\n> \n> *and* indent the here-doc at the same time?\n\nOhh okay okay. I understand what you are saying. I wanted to fix the\nheredoc markers and indent the tests for better readibility but actually\nI fixed the heredoc marker in [2/4]. Therefore, the change in [2/4]\nshould in fact be:\n\n -\ttest_i18ncmp actual - < writes:\n\n> Understood. I will revert the change. Though, what Kaartic suggested, to\n> do a '--short=7', that will be okay to keep right?\n\nSure, that is a strict improvement to lose an unneeded process, as\nlong as we know HEAD is guaranteed to be unique with 7 hexdigits\n(otherwise \"cut\" to strictly 7 and \"rev-parse --short=7 HEAD\" would\nproduce different results.\n\nThanks.\n"}]}