* [PATCH] t9001: fix/unify indentation regarding pipes somewhat
@ 2023-08-09 17:15 Oswald Buddenhagen
2023-08-09 19:46 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat
2023-08-09 17:15 [PATCH] t9001: fix/unify indentation regarding pipes somewhat Oswald Buddenhagen
@ 2023-08-09 19:46 ` Junio C Hamano
2023-08-10 10:26 ` Oswald Buddenhagen
2023-08-13 10:46 ` [PATCH v2] t9001: fix indentation in test_no_confirm() Oswald Buddenhagen
0 siblings, 2 replies; 6+ messages in thread
From: Junio C Hamano @ 2023-08-09 19:46 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat
2023-08-09 19:46 ` Junio C Hamano
@ 2023-08-10 10:26 ` Oswald Buddenhagen
2023-08-10 19:09 ` Junio C Hamano
2023-08-13 10:46 ` [PATCH v2] t9001: fix indentation in test_no_confirm() Oswald Buddenhagen
1 sibling, 1 reply; 6+ messages in thread
From: Oswald Buddenhagen @ 2023-08-10 10:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat
2023-08-10 10:26 ` Oswald Buddenhagen
@ 2023-08-10 19:09 ` Junio C Hamano
2023-08-13 10:45 ` Oswald Buddenhagen
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2023-08-10 19:09 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
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...".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat
2023-08-10 19:09 ` Junio C Hamano
@ 2023-08-13 10:45 ` Oswald Buddenhagen
0 siblings, 0 replies; 6+ messages in thread
From: Oswald Buddenhagen @ 2023-08-13 10:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] t9001: fix indentation in test_no_confirm()
2023-08-09 19:46 ` Junio C Hamano
2023-08-10 10:26 ` Oswald Buddenhagen
@ 2023-08-13 10:46 ` Oswald Buddenhagen
1 sibling, 0 replies; 6+ messages in thread
From: Oswald Buddenhagen @ 2023-08-13 10:46 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-13 10:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-09 17:15 [PATCH] t9001: fix/unify indentation regarding pipes somewhat Oswald Buddenhagen
2023-08-09 19:46 ` Junio C Hamano
2023-08-10 10:26 ` Oswald Buddenhagen
2023-08-10 19:09 ` Junio C Hamano
2023-08-13 10:45 ` Oswald Buddenhagen
2023-08-13 10:46 ` [PATCH v2] t9001: fix indentation in test_no_confirm() Oswald Buddenhagen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).