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

On 05/08 04:49, Taylor Blau wrote:

On Wed, Aug 05, 2020 at 03:37:55PM -0400, Denton Liu wrote:

quoted

Hi Shourya,

On Wed, Aug 05, 2020 at 11:19:18PM +0530, Shourya Shukla wrote:

quoted

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.

quoted

Update it to match the CodingGuidelines.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Kaartic Sivaraam [off-list ref]
Signed-off-by: Shourya Shukla <redacted>
---
 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.

quoted

quoted

 		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.