| Message ID | 20230809171531.2564769-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t9001: fix/unify indentation regarding pipes somewhat | expand |
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.
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
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...".
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
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 &&
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(-)