[PATCH] t9001: fix/unify indentation regarding pipes somewhat

[PATCH] t9001: fix/unify indentation regarding pipes somewhat

[Oswald Buddenhagen]

Always indent the continuation of a pipe, and do not indent the
continuation of a compound statement (involving a pipe). This better
reflects the precedence (and is thus potentially less misleading about
the actual structure of the compound command) and better follows most
existing precedents.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
There are also many cases of weirdly wrapped commands with the pipe in
the middle of the line; I did not touch these.

Cc: Junio C Hamano <gitster@pobox.com>
---
 t/t9001-send-email.sh | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 48bf0af2ee..c459311a60 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -61,8 +61,8 @@ test_no_confirm () {
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$@ \
 		$patches >stdout &&
-		! grep "Send this email" stdout &&
-		>no_confirm_okay
+	! grep "Send this email" stdout &&
+	>no_confirm_okay
 }
 
 # Exit immediately to prevent hang if a no-confirm test fails
@@ -342,8 +342,8 @@ test_expect_success $PREREQ 'Prompting works' '
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$patches \
 		2>errors &&
-		grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
-		grep "^To: to@example.com\$" msgtxt1
+	grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
+	grep "^To: to@example.com\$" msgtxt1
 '
 
 test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
@@ -1197,7 +1197,7 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '
 	--smtp-server="$(pwd)/fake.sendmail" \
 	outdir/*.patch &&
 	grep "^	" msgtxt1 |
-	grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"
+		grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"
 '
 
 test_expect_success $PREREQ '--compose adds MIME for utf8 body' '
@@ -1599,7 +1599,7 @@ test_expect_success $PREREQ 'setup expect' '
 test_expect_success $PREREQ 'ASCII subject is not RFC2047 quoted' '
 	clean_fake_sendmail &&
 	echo bogus |
-	git send-email --from=author@example.com --to=nobody@example.com \
+		git send-email --from=author@example.com --to=nobody@example.com \
 			--smtp-server="$(pwd)/fake.sendmail" \
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
@@ -1618,7 +1618,7 @@ test_expect_success $PREREQ 'setup expect' '
 test_expect_success $PREREQ 'asks about and fixes 8bit encodings' '
 	clean_fake_sendmail &&
 	echo |
-	git send-email --from=author@example.com --to=nobody@example.com \
+		git send-email --from=author@example.com --to=nobody@example.com \
 			--smtp-server="$(pwd)/fake.sendmail" \
 			email-using-8bit >stdout &&
 	grep "do not declare a Content-Transfer-Encoding" stdout &&
@@ -1632,7 +1632,7 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding works' '
 	clean_fake_sendmail &&
 	git config sendemail.assume8bitEncoding UTF-8 &&
 	echo bogus |
-	git send-email --from=author@example.com --to=nobody@example.com \
+		git send-email --from=author@example.com --to=nobody@example.com \
 			--smtp-server="$(pwd)/fake.sendmail" \
 			email-using-8bit >stdout &&
 	grep -E "Content|MIME" msgtxt1 >actual &&
@@ -1646,19 +1646,19 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding in .git/config overrides --g
 	mkdir home &&
 	git config -f home/.gitconfig sendemail.assume8bitEncoding "bogus too" &&
 	echo bogus |
-	env HOME="$(pwd)/home" DEBUG=1 \
-	git send-email --from=author@example.com --to=nobody@example.com \
+		env HOME="$(pwd)/home" DEBUG=1 \
+		git send-email --from=author@example.com --to=nobody@example.com \
 			--smtp-server="$(pwd)/fake.sendmail" \
 			email-using-8bit >stdout &&
 	grep -E "Content|MIME" msgtxt1 >actual &&
 	test_cmp content-type-decl actual
 '
 
 test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '
 	clean_fake_sendmail &&
 	git config sendemail.assume8bitEncoding "bogus too" &&
 	echo bogus |
-	git send-email --from=author@example.com --to=nobody@example.com \
+		git send-email --from=author@example.com --to=nobody@example.com \
 			--smtp-server="$(pwd)/fake.sendmail" \
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
@@ -1687,7 +1687,7 @@ test_expect_success $PREREQ 'setup expect' '
 test_expect_success $PREREQ '--8bit-encoding also treats subject' '
 	clean_fake_sendmail &&
 	echo bogus |
-	git send-email --from=author@example.com --to=nobody@example.com \
+		git send-email --from=author@example.com --to=nobody@example.com \
 			--smtp-server="$(pwd)/fake.sendmail" \
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> @@ -61,8 +61,8 @@ test_no_confirm () {
>  		--smtp-server="$(pwd)/fake.sendmail" \
>  		$@ \
>  		$patches >stdout &&
> -		! grep "Send this email" stdout &&
> -		>no_confirm_okay
> +	! grep "Send this email" stdout &&
> +	>no_confirm_okay
>  }

It is hard to see what is going on here, but ...

> @@ -1197,7 +1197,7 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '
>  	--smtp-server="$(pwd)/fake.sendmail" \
>  	outdir/*.patch &&
>  	grep "^	" msgtxt1 |
> -	grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"
> +		grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"

... I do not think we want this.  A long pipeline should be written
without extra indentation like

	A |
		B |
			C

but more like

	A |
	B |
	C

If we do not have it in the coding guidelines document, perhaps we
should add an entry for it.

Thanks.

Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat

[Oswald Buddenhagen]

On Wed, Aug 09, 2023 at 12:46:36PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> -	grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"
>> +		grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"
>
>... I do not think we want this.  A long pipeline should be written
>without extra indentation like
>
>	A |
>		B |
>			C
>
... which is not how i would do it.

>but more like
>
>	A |
>	B |
>	C
>
i'd argue that this should be written as

	A |
		B |
		C

like other continued lines (no trailing backslashes are needed here, but 
it would be ok to add them, and there is in fact a commit that does just 
that in other places, and one might do the same here in a followup).

regards

Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

>>but more like
>>
>>	A |
>>	B |
>>	C
>>
> i'd argue that this should be written as
>
> 	A |
> 		B |
> 		C
>
> like other continued lines (no trailing backslashes are needed here,
> but it would be ok to add them, and there is in fact a commit that
> does just that in other places, and one might do the same here in a
> followup).

You are entitled to your own opinion, and you are welcome to stick
to it in projects you run.  But please refrain from wasting time of
this project on something that is subjective preference and has no
absolute yardstick to tell which is _right_ or _wrong_.  Difference
between the above two falls into "once it is written in one way, it
is not worth the patch noise to turn it into the other way", and I
already told you which is the preferred way for new code.

As to trailing backslashes _after_ pipe, we have preference that is
a bit stronger than "once it is written, it is not worth fixing".
The shell knows, when you end a line with a vertical bar, you
haven't finished your pipeline, and we do want to omit such an
unnecessary backslash if we accidentally added one.  b8403129
(t/t0015-hash.sh: remove unnecessary '\' at line end, 2022-02-08)
is an example of such a style fix, and that is why I said the
preference is a bit stronger than "once it is written...".

Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat

[Oswald Buddenhagen]

On Thu, Aug 10, 2023 at 12:09:43PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>>>but more like
>>>
>>>	A |
>>>	B |
>>>	C
>>>
>> i'd argue that this should be written as
>>
>> 	A |
>> 		B |
>> 		C
>>
>> like other continued lines (no trailing backslashes are needed here,
>> but it would be ok to add them, and there is in fact a commit that
>> does just that in other places, and one might do the same here in a
>> followup).
>
>You are entitled to your own opinion, and you are welcome to stick
>to it in projects you run.  But please refrain from wasting time of
>this project on something that is subjective preference and has no
>absolute yardstick to tell which is _right_ or _wrong_.
>
i think it's a rather uncontroversial statement that the whitespace 
should visualize the code structure to the greatest degree possible 
(which of course doesn't imply blindly maximizing indentation, as there 
are multiple considerations). so while the details can be bike-shedded 
to death, that doesn't mean that there isn't a trend.

i can totally see why one wouldn't indent the top-level `&&` chains in 
the test cases (they are really kinda a local `set -e`, and with bash 
one could probably actually use that with an ERR trap), but generally 
not indenting continuations (also of compound statements) is confusing 
(and doing it incorrectly even more so, as you agree).

>Difference
>between the above two falls into "once it is written in one way, it
>is not worth the patch noise to turn it into the other way",
>
that i'll happily agree with in this case.

regards

[PATCH v2] t9001: fix indentation in test_no_confirm()

[Oswald Buddenhagen]

The continuations of the compound command were indented as if they were
continuations of the embedded pipe, which was misleading.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
v2:
- reduced to what seems to be welcome.
  fwiw, this is actually v0.

Cc: Junio C Hamano <gitster@pobox.com>
---
 t/t9001-send-email.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 48bf0af2ee..542f524f04 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -61,8 +61,8 @@ test_no_confirm () {
 		--smtp-server="$(pwd)/fake.sendmail" \
 		$@ \
 		$patches >stdout &&
-		! grep "Send this email" stdout &&
-		>no_confirm_okay
+	! grep "Send this email" stdout &&
+	>no_confirm_okay
 }
 
 # Exit immediately to prevent hang if a no-confirm test fails
-- 
2.40.0.152.g15d061e6df