threads / patch / 60094

patcht9001: fix/unify indentation regarding pipes somewhat

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

## tl;dr

6 messages between Aug 9, 2023 and Aug 13, 2023. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Oswald Buddenhagen· Aug 9, 2023, 17:15 UTC · lore

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(-)
Show changes to t/t9001-send-email.sh +12 −12
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
Junio C Hamano· Aug 9, 2023, 19:46 UTC · re: Oswald Buddenhagen · lore

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

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 9 quoted lines
> @@ -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 ...
Show 6 quoted lines
> @@ -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.
Oswald Buddenhagen· Aug 10, 2023, 10:26 UTC · re: Junio C Hamano · lore

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

On Wed, Aug 09, 2023 at 12:46:36PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
>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.
Show 6 quoted lines
>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
Junio C Hamano· Aug 10, 2023, 19:09 UTC · re: Oswald Buddenhagen · lore

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

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
Show 16 quoted lines
>>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...".

Oswald Buddenhagen· Aug 13, 2023, 10:45 UTC · re: Junio C Hamano · lore

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

On Thu, Aug 10, 2023 at 12:09:43PM -0700, Junio C Hamano wrote:
Show 24 quoted lines
>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
Oswald Buddenhagen· Aug 13, 2023, 10:46 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to t/t9001-send-email.sh +2 −2
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

← back to recent threads