{"thread":{"id":"60094","subject":"[PATCH] t9001: fix/unify indentation regarding pipes somewhat","startedAt":"2023-08-09T17:15:42Z","lastAt":"2023-08-13T10:46:52Z","messageCount":6,"participants":["Oswald Buddenhagen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480369","messageId":"20230809171531.2564769-1-oswald.buddenhagen@gmx.de","threadId":"60094","inReplyTo":null,"subject":"[PATCH] t9001: fix/unify indentation regarding pipes somewhat","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-09T17:15:31Z","receivedAt":"2023-08-09T17:15:42Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Always indent the continuation of a pipe, and do not indent the\ncontinuation of a compound statement (involving a pipe). This better\nreflects the precedence (and is thus potentially less misleading about\nthe actual structure of the compound command) and better follows most\nexisting precedents.\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nThere are also many cases of weirdly wrapped commands with the pipe in\nthe middle of the line; I did not touch these.\n\nCc: Junio C Hamano \n---\n t/t9001-send-email.sh | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 48bf0af2ee..c459311a60 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -61,8 +61,8 @@ test_no_confirm () {\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$@ \\\n \t\t$patches >stdout &&\n-\t\t! grep \"Send this email\" stdout &&\n-\t\t>no_confirm_okay\n+\t! grep \"Send this email\" stdout &&\n+\t>no_confirm_okay\n }\n \n # Exit immediately to prevent hang if a no-confirm test fails\n@@ -342,8 +342,8 @@ test_expect_success $PREREQ 'Prompting works' '\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$patches \\\n \t\t2>errors &&\n-\t\tgrep \"^From: A U Thor \\$\" msgtxt1 &&\n-\t\tgrep \"^To: to@example.com\\$\" msgtxt1\n+\tgrep \"^From: A U Thor \\$\" msgtxt1 &&\n+\tgrep \"^To: to@example.com\\$\" msgtxt1\n '\n \n test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '\n@@ -1197,7 +1197,7 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '\n \t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \toutdir/*.patch &&\n \tgrep \"^\t\" msgtxt1 |\n-\tgrep \"=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= \"\n+\t\tgrep \"=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= \"\n '\n \n test_expect_success $PREREQ '--compose adds MIME for utf8 body' '\n@@ -1599,7 +1599,7 @@ test_expect_success $PREREQ 'setup expect' '\n test_expect_success $PREREQ 'ASCII subject is not RFC2047 quoted' '\n \tclean_fake_sendmail &&\n \techo bogus |\n-\tgit send-email --from=author@example.com --to=nobody@example.com \\\n+\t\tgit send-email --from=author@example.com --to=nobody@example.com \\\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\t--8bit-encoding=UTF-8 \\\n \t\t\temail-using-8bit >stdout &&\n@@ -1618,7 +1618,7 @@ test_expect_success $PREREQ 'setup expect' '\n test_expect_success $PREREQ 'asks about and fixes 8bit encodings' '\n \tclean_fake_sendmail &&\n \techo |\n-\tgit send-email --from=author@example.com --to=nobody@example.com \\\n+\t\tgit send-email --from=author@example.com --to=nobody@example.com \\\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\temail-using-8bit >stdout &&\n \tgrep \"do not declare a Content-Transfer-Encoding\" stdout &&\n@@ -1632,7 +1632,7 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding works' '\n \tclean_fake_sendmail &&\n \tgit config sendemail.assume8bitEncoding UTF-8 &&\n \techo bogus |\n-\tgit send-email --from=author@example.com --to=nobody@example.com \\\n+\t\tgit send-email --from=author@example.com --to=nobody@example.com \\\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\temail-using-8bit >stdout &&\n \tgrep -E \"Content|MIME\" msgtxt1 >actual &&\n@@ -1646,19 +1646,19 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding in .git/config overrides --g\n \tmkdir home &&\n \tgit config -f home/.gitconfig sendemail.assume8bitEncoding \"bogus too\" &&\n \techo bogus |\n-\tenv HOME=\"$(pwd)/home\" DEBUG=1 \\\n-\tgit send-email --from=author@example.com --to=nobody@example.com \\\n+\t\tenv HOME=\"$(pwd)/home\" DEBUG=1 \\\n+\t\tgit send-email --from=author@example.com --to=nobody@example.com \\\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\temail-using-8bit >stdout &&\n \tgrep -E \"Content|MIME\" msgtxt1 >actual &&\n \ttest_cmp content-type-decl actual\n '\n \n test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '\n \tclean_fake_sendmail &&\n \tgit config sendemail.assume8bitEncoding \"bogus too\" &&\n \techo bogus |\n-\tgit send-email --from=author@example.com --to=nobody@example.com \\\n+\t\tgit send-email --from=author@example.com --to=nobody@example.com \\\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\t--8bit-encoding=UTF-8 \\\n \t\t\temail-using-8bit >stdout &&\n@@ -1687,7 +1687,7 @@ test_expect_success $PREREQ 'setup expect' '\n test_expect_success $PREREQ '--8bit-encoding also treats subject' '\n \tclean_fake_sendmail &&\n \techo bogus |\n-\tgit send-email --from=author@example.com --to=nobody@example.com \\\n+\t\tgit send-email --from=author@example.com --to=nobody@example.com \\\n \t\t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t\t--8bit-encoding=UTF-8 \\\n \t\t\temail-using-8bit >stdout &&\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480394","messageId":"xmqq350sm21v.fsf@gitster.g","threadId":"60094","inReplyTo":"20230809171531.2564769-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T19:46:36Z","receivedAt":"2023-08-09T19:46:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> @@ -61,8 +61,8 @@ test_no_confirm () {\n> \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> \t\t$@ \\\n> \t\t$patches >stdout &&\n> -\t\t! grep \"Send this email\" stdout &&\n> -\t\t>no_confirm_okay\n> +\t! grep \"Send this email\" stdout &&\n> +\t>no_confirm_okay\n> }\n\nIt is hard to see what is going on here, but ...\n\n> @@ -1197,7 +1197,7 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '\n> \t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n> \toutdir/*.patch &&\n> \tgrep \"^\t\" msgtxt1 |\n> -\tgrep \"=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= \"\n> +\t\tgrep \"=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= \"\n\n... I do not think we want this. A long pipeline should be written\nwithout extra indentation like\n\n\tA |\n\t\tB |\n\t\t\tC\n\nbut more like\n\n\tA |\n\tB |\n\tC\n\nIf we do not have it in the coding guidelines document, perhaps we\nshould add an entry for it.\n\nThanks.\n"},{"id":"480430","messageId":"ZNS7PJ0oyOQLeaZ7@ugly","threadId":"60094","inReplyTo":"xmqq350sm21v.fsf@gitster.g","subject":"Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-10T10:26:04Z","receivedAt":"2023-08-10T10:26:10Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, Aug 09, 2023 at 12:46:36PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>> -\tgrep \"=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= \"\n>> +\t\tgrep \"=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= \"\n>\n>... I do not think we want this. A long pipeline should be written\n>without extra indentation like\n>\n>\tA |\n>\t\tB |\n>\t\t\tC\n>\n... which is not how i would do it.\n\n>but more like\n>\n>\tA |\n>\tB |\n>\tC\n>\ni'd argue that this should be written as\n\n\tA |\n\t\tB |\n\t\tC\n\nlike other continued lines (no trailing backslashes are needed here, but \nit would be ok to add them, and there is in fact a commit that does just \nthat in other places, and one might do the same here in a followup).\n\nregards\n"},{"id":"480462","messageId":"xmqqa5uysoi0.fsf@gitster.g","threadId":"60094","inReplyTo":"ZNS7PJ0oyOQLeaZ7@ugly","subject":"Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T19:09:43Z","receivedAt":"2023-08-10T19:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n>>but more like\n>>\n>>\tA |\n>>\tB |\n>>\tC\n>>\n> i'd argue that this should be written as\n>\n> \tA |\n> \t\tB |\n> \t\tC\n>\n> like other continued lines (no trailing backslashes are needed here,\n> but it would be ok to add them, and there is in fact a commit that\n> does just that in other places, and one might do the same here in a\n> followup).\n\nYou are entitled to your own opinion, and you are welcome to stick\nto it in projects you run. But please refrain from wasting time of\nthis project on something that is subjective preference and has no\nabsolute yardstick to tell which is _right_ or _wrong_. Difference\nbetween the above two falls into \"once it is written in one way, it\nis not worth the patch noise to turn it into the other way\", and I\nalready told you which is the preferred way for new code.\n\nAs to trailing backslashes _after_ pipe, we have preference that is\na bit stronger than \"once it is written, it is not worth fixing\".\nThe shell knows, when you end a line with a vertical bar, you\nhaven't finished your pipeline, and we do want to omit such an\nunnecessary backslash if we accidentally added one. b8403129\n(t/t0015-hash.sh: remove unnecessary '\\' at line end, 2022-02-08)\nis an example of such a style fix, and that is why I said the\npreference is a bit stronger than \"once it is written...\".\n\n\n\n"},{"id":"480633","messageId":"ZNi0ZrjSyE7vpQva@ugly","threadId":"60094","inReplyTo":"xmqqa5uysoi0.fsf@gitster.g","subject":"Re: [PATCH] t9001: fix/unify indentation regarding pipes somewhat","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-13T10:45:58Z","receivedAt":"2023-08-13T10:46:09Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Aug 10, 2023 at 12:09:43PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>\n>>>but more like\n>>>\n>>>\tA |\n>>>\tB |\n>>>\tC\n>>>\n>> i'd argue that this should be written as\n>>\n>> \tA |\n>> \t\tB |\n>> \t\tC\n>>\n>> like other continued lines (no trailing backslashes are needed here,\n>> but it would be ok to add them, and there is in fact a commit that\n>> does just that in other places, and one might do the same here in a\n>> followup).\n>\n>You are entitled to your own opinion, and you are welcome to stick\n>to it in projects you run. But please refrain from wasting time of\n>this project on something that is subjective preference and has no\n>absolute yardstick to tell which is _right_ or _wrong_.\n>\ni think it's a rather uncontroversial statement that the whitespace \nshould visualize the code structure to the greatest degree possible \n(which of course doesn't imply blindly maximizing indentation, as there \nare multiple considerations). so while the details can be bike-shedded \nto death, that doesn't mean that there isn't a trend.\n\ni can totally see why one wouldn't indent the top-level `&&` chains in \nthe test cases (they are really kinda a local `set -e`, and with bash \none could probably actually use that with an ERR trap), but generally \nnot indenting continuations (also of compound statements) is confusing \n(and doing it incorrectly even more so, as you agree).\n\n>Difference\n>between the above two falls into \"once it is written in one way, it\n>is not worth the patch noise to turn it into the other way\",\n>\nthat i'll happily agree with in this case.\n\nregards\n"},{"id":"480634","messageId":"20230813104649.2919412-1-oswald.buddenhagen@gmx.de","threadId":"60094","inReplyTo":"xmqq350sm21v.fsf@gitster.g","subject":"[PATCH v2] t9001: fix indentation in test_no_confirm()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-13T10:46:49Z","receivedAt":"2023-08-13T10:46:52Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"The continuations of the compound command were indented as if they were\ncontinuations of the embedded pipe, which was misleading.\n\nSigned-off-by: Oswald Buddenhagen \n\n---\nv2:\n- reduced to what seems to be welcome.\n fwiw, this is actually v0.\n\nCc: Junio C Hamano \n---\n t/t9001-send-email.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex 48bf0af2ee..542f524f04 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -61,8 +61,8 @@ test_no_confirm () {\n \t\t--smtp-server=\"$(pwd)/fake.sendmail\" \\\n \t\t$@ \\\n \t\t$patches >stdout &&\n-\t\t! grep \"Send this email\" stdout &&\n-\t\t>no_confirm_okay\n+\t! grep \"Send this email\" stdout &&\n+\t>no_confirm_okay\n }\n \n # Exit immediately to prevent hang if a no-confirm test fails\n-- \n2.40.0.152.g15d061e6df\n\n"}]}