{"thread":{"id":"61130","subject":"[PATCH] docs: correct trailer `key_value_separator` description","startedAt":"2024-03-16T03:56:35Z","lastAt":"2024-03-19T07:21:27Z","messageCount":12,"participants":["Brian Lyles","Linus Arver","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"490757","messageId":"20240316035612.752910-1-brianmlyles@gmail.com","threadId":"61130","inReplyTo":null,"subject":"[PATCH] docs: correct trailer `key_value_separator` description","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-16T03:55:42Z","receivedAt":"2024-03-16T03:56:35Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"The description for `key_value_separator` incorrectly states that this\nseparator is inserted between trailer lines, which appears likely to\nhave been incorrectly copied from `separator` when this option was\nadded.\n\nUpdate the description to correctly indicate that it is a separator that\nappears between the key and the value of each trailer.\n\nSigned-off-by: Brian Lyles \n---\n Documentation/pretty-formats.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d38b4ab566..4839c2843c 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -329,9 +329,9 @@ multiple times, the last occurrence wins.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n ** 'keyonly[=]': only show the key part of the trailer.\n ** 'valueonly[=]': only show the value part of the trailer.\n-** 'key_value_separator=': specify a separator inserted between\n- trailer lines. When this option is not given each trailer key-value\n- pair is separated by \": \". Otherwise it shares the same semantics\n+** 'key_value_separator=': specify a separator inserted between each\n+ trailer's key and value. When this option is not given each trailer\n+ key-value pair is separated by \": \". Otherwise it shares the same semantics\n as 'separator=' above.\n \n NOTE: Some placeholders may depend on other options given to the\n-- \n2.43.0\n\n"},{"id":"490773","messageId":"owly1q8a4qhh.fsf@fine.c.googlers.com","threadId":"61130","inReplyTo":"20240316035612.752910-1-brianmlyles@gmail.com","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-03-16T06:53:30Z","receivedAt":"2024-03-16T06:53:32Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Brian Lyles writes:\n\n> The description for `key_value_separator` incorrectly states that this\n> separator is inserted between trailer lines, which appears likely to\n> have been incorrectly copied from `separator` when this option was\n> added.\n>\n> Update the description to correctly indicate that it is a separator that\n> appears between the key and the value of each trailer.\n>\n> Signed-off-by: Brian Lyles \n> ---\n> Documentation/pretty-formats.txt | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index d38b4ab566..4839c2843c 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -329,9 +329,9 @@ multiple times, the last occurrence wins.\n> `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n> ** 'keyonly[=]': only show the key part of the trailer.\n> ** 'valueonly[=]': only show the value part of the trailer.\n> -** 'key_value_separator=': specify a separator inserted between\n\nNit: This line was modified to have \" each\" at the end. If you did that\non the next line, then this diff could have been a touch smaller.\n\n> - trailer lines. When this option is not given each trailer key-value\n> - pair is separated by \": \". Otherwise it shares the same semantics\n> +** 'key_value_separator=': specify a separator inserted between each\n> + trailer's key and value. When this option is not given each trailer\n> + key-value pair is separated by \": \". Otherwise it shares the same semantics\n> as 'separator=' above.\n\nLGTM.\n\nIt's probably not worth re-rolling, but a small suggestion I have is to\nsimplify the language a bit to reduce repetition, like so:\n\n ** 'key_value_separator=': specify the separator between\n the key and value of each trailer. Defaults to \": \". Otherwise it\n shares the same semantics as 'separator=' above.\n\nThanks.\n"},{"id":"490831","messageId":"17bdc28ea2b88503.70b1dd9aae081c6e.203dcd72f6563036@zivdesk","threadId":"61130","inReplyTo":"owly1q8a4qhh.fsf@fine.c.googlers.com","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-18T04:49:11Z","receivedAt":"2024-03-18T04:49:13Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"Hi Linus\n\nOn Sat, Mar 16, 2024 at 1:53 AM Linus Arver wrote:\n\n> Nit: This line was modified to have \" each\" at the end. If you did that\n> on the next line, then this diff could have been a touch smaller.\n\nSure -- it looks like this was a result of applying a different\nhard-wrap width than the previous author, perhaps? I thought it more\nprudent to wrap to a consistent length than to be overly concerned about\nthe diff given that it was already a fairly trivial patch. That said,\nI'm not seeing a recommended wrap width for doc files documented\nanywhere either. Is there a documented guideline to follow here, both in\nterms of preferred wrap width as well as when it might be appropriate to\nstray from it for reasons such as this?\n\n> It's probably not worth re-rolling, but a small suggestion I have is to\n> simplify the language a bit to reduce repetition, like so:\n> \n> ** 'key_value_separator=': specify the separator between\n> the key and value of each trailer. Defaults to \": \". Otherwise it\n> shares the same semantics as 'separator=' above.\n> \n\nI do prefer the simplified language. I had initially aimed to simply\ncorrect the inaccuracy, but I think that it probably *is* worth a quick\nre-roll to make this simplification. I will send that out shortly.\n\n-- \nThank you,\nBrian Lyles\n"},{"id":"490832","messageId":"20240318053848.185201-1-brianmlyles@gmail.com","threadId":"61130","inReplyTo":"20240316035612.752910-1-brianmlyles@gmail.com","subject":"[PATCH v2 1/2] docs: correct trailer `key_value_separator` description","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-18T05:38:01Z","receivedAt":"2024-03-18T05:39:57Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"The description for `key_value_separator` incorrectly states that this\nseparator is inserted between trailer lines, which appears likely to\nhave been incorrectly copied from `separator` when this option was\nadded.\n\nUpdate the description to correctly indicate that it is a separator that\nappears between the key and the value of each trailer.\n\nSigned-off-by: Brian Lyles \n---\nChanges since v1:\n- Minor wording tweak\n- Minor wrapping tweak\n\n Documentation/pretty-formats.txt | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d38b4ab566..e1788cb07a 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -330,8 +330,8 @@ multiple times, the last occurrence wins.\n ** 'keyonly[=]': only show the key part of the trailer.\n ** 'valueonly[=]': only show the value part of the trailer.\n ** 'key_value_separator=': specify a separator inserted between\n- trailer lines. When this option is not given each trailer key-value\n- pair is separated by \": \". Otherwise it shares the same semantics\n+ the key and value of each trailer. When this option is not given each trailer\n+ key-value pair is separated by \": \". Otherwise it shares the same semantics\n as 'separator=' above.\n \n NOTE: Some placeholders may depend on other options given to the\n-- \n2.43.2\n\n"},{"id":"490833","messageId":"20240318053848.185201-2-brianmlyles@gmail.com","threadId":"61130","inReplyTo":"20240316035612.752910-1-brianmlyles@gmail.com","subject":"[PATCH v2 2/2] docs: adjust trailer `separator` and `key_value_separator` language","fromName":"Brian Lyles","fromEmail":"brianmlyles@gmail.com","sentAt":"2024-03-18T05:38:02Z","receivedAt":"2024-03-18T05:39:58Z","isPatch":true,"sender":{"key":"brianmlyles@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1123282?v=4"},"body":"The language describing the trailer separator and key-value separator\ndefault value is overly complicated.\n\nIndicate the default with simpler \"Defaults to ...\" language.\n\nSuggested-by: Linus Arver \nSigned-off-by: Brian Lyles \n---\nThis commit is new in v2 per Linus' suggestion[1].\n\n[1]: https://lore.kernel.org/git/owly1q8a4qhh.fsf@fine.c.googlers.com/\n\n Documentation/pretty-formats.txt | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex e1788cb07a..8ee940b6a4 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -316,9 +316,8 @@ multiple times, the last occurrence wins.\n `Reviewed-by`.\n ** 'only[=]': select whether non-trailer lines from the trailer\n block should be included.\n-** 'separator=': specify a separator inserted between trailer\n- lines. When this option is not given each trailer line is\n- terminated with a line feed character. The string may contain\n+** 'separator=': specify the separator inserted between trailer\n+ lines. Defaults to a line feed character. The string may contain\n the literal formatting codes described above. To use comma as\n separator one must use `%x2C` as it would otherwise be parsed as\n next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`\n@@ -329,10 +328,9 @@ multiple times, the last occurrence wins.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n ** 'keyonly[=]': only show the key part of the trailer.\n ** 'valueonly[=]': only show the value part of the trailer.\n-** 'key_value_separator=': specify a separator inserted between\n- the key and value of each trailer. When this option is not given each trailer\n- key-value pair is separated by \": \". Otherwise it shares the same semantics\n- as 'separator=' above.\n+** 'key_value_separator=': specify the separator inserted between\n+ the key and value of each trailer. Defaults to \": \". Otherwise it\n+ shares the same semantics as 'separator=' above.\n\n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n2.43.2\n\n"},{"id":"490834","messageId":"owlyv85k2gts.fsf@fine.c.googlers.com","threadId":"61130","inReplyTo":"17bdc28ea2b88503.70b1dd9aae081c6e.203dcd72f6563036@zivdesk","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-03-18T06:29:35Z","receivedAt":"2024-03-18T06:29:38Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"\"Brian Lyles\" writes:\n\n> Hi Linus\n>\n> On Sat, Mar 16, 2024 at 1:53 AM Linus Arver wrote:\n>\n>> Nit: This line was modified to have \" each\" at the end. If you did that\n>> on the next line, then this diff could have been a touch smaller.\n>\n> [...] Is there a documented guideline to follow here, both in\n> terms of preferred wrap width as well as when it might be appropriate to\n> stray from it for reasons such as this?\n\nWRT line lengths, probably 80-ish columns is the (unwritten?) rule. The\ntext files aren't really meant for end-user consumption (that's what the\nmanpage and HTML formats are for), so I think it's OK if the line\nlengths are roughly in the same ballpark (no need to worry too much\nabout exact lengths).\n\nWhen I contributed some patches to the docs last year, I was advised to\nminimize diffs where appropriate, to make it easier for reviewers. In\nthis case it didn't matter too much (the patch being so small), but I\nthought it was still worth mentioning. /shrug\n"},{"id":"490835","messageId":"owlyr0g82g7g.fsf@fine.c.googlers.com","threadId":"61130","inReplyTo":"20240318053848.185201-2-brianmlyles@gmail.com","subject":"Re: [PATCH v2 2/2] docs: adjust trailer `separator` and `key_value_separator` language","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-03-18T06:42:59Z","receivedAt":"2024-03-18T06:43:01Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Brian Lyles writes:\n\nThis v2 LGTM. Thanks!\n\n> The language describing the trailer separator and key-value separator\n> default value is overly complicated.\n>\n> Indicate the default with simpler \"Defaults to ...\" language.\n>\n> Suggested-by: Linus Arver \n> Signed-off-by: Brian Lyles \n> ---\n> This commit is new in v2 per Linus' suggestion[1].\n>\n> [1]: https://lore.kernel.org/git/owly1q8a4qhh.fsf@fine.c.googlers.com/\n>\n> Documentation/pretty-formats.txt | 12 +++++-------\n> 1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index e1788cb07a..8ee940b6a4 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -316,9 +316,8 @@ multiple times, the last occurrence wins.\n> `Reviewed-by`.\n> ** 'only[=]': select whether non-trailer lines from the trailer\n> block should be included.\n> -** 'separator=': specify a separator inserted between trailer\n> - lines. When this option is not given each trailer line is\n> - terminated with a line feed character. The string may contain\n> +** 'separator=': specify the separator inserted between trailer\n> + lines. Defaults to a line feed character. The string may contain\n> the literal formatting codes described above. To use comma as\n> separator one must use `%x2C` as it would otherwise be parsed as\n> next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`\n> @@ -329,10 +328,9 @@ multiple times, the last occurrence wins.\n> `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n> ** 'keyonly[=]': only show the key part of the trailer.\n> ** 'valueonly[=]': only show the value part of the trailer.\n> -** 'key_value_separator=': specify a separator inserted between\n> - the key and value of each trailer. When this option is not given each trailer\n> - key-value pair is separated by \": \". Otherwise it shares the same semantics\n> - as 'separator=' above.\n> +** 'key_value_separator=': specify the separator inserted between\n> + the key and value of each trailer. Defaults to \": \". Otherwise it\n> + shares the same semantics as 'separator=' above.\n>\n> NOTE: Some placeholders may depend on other options given to the\n> revision traversal engine. For example, the `%g*` reflog options will\n> -- \n> 2.43.2\n"},{"id":"490879","messageId":"xmqqh6h3jzp1.fsf@gitster.g","threadId":"61130","inReplyTo":"owlyv85k2gts.fsf@fine.c.googlers.com","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T16:02:18Z","receivedAt":"2024-03-18T16:02:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Arver writes:\n\n> WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The\n\nYour patches will be reviewed on the mailing list. If you keep your\nline length to somewhere around ~70, the line will still fit within\nthe 80-ish terminal width after a few rounds of review exchanges,\nwith \">> \" prefixed. That reasoning is mostly about the proposed\ncommit log messages, but the same would apply to things like\nAsciiDoc sources.\n\nIt is true that we do not write it down. Perhaps something like\nthis is in order?\n\ndiff --git i/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex e734a3f0f1..68e9ad71a1 100644\n--- i/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -280,6 +280,14 @@ or, on an older version of Git without support for --pretty=reference:\n \tgit show -s --date=short --pretty='format:%h (%s, %ad)' \n ....\n \n+[[line-wrap]]\n+\n+Just like we limit the patch subject to 50 chars or so, the lines in\n+the proposed log message should be around 70 chars to make sure that\n+it still can be shown on 80-column terminal without line wrapping\n+after a handful of review exchanges add \"> \" prefix to them.\n+\n+\n [[sign-off]]\n === Certify your work by adding your `Signed-off-by` trailer\n \n\n> text files aren't really meant for end-user consumption (that's what the\n> manpage and HTML formats are for), so I think it's OK if the line\n> lengths are roughly in the same ballpark (no need to worry too much\n> about exact lengths).\n\nYes, too. And it is one way to reduce patch noise and nicer to\nreviewers, when used moderately (i.e. removing a word and making a\nline to occupy only 50 columns when ajacent ones are 70 columns may\nstill be better than reflowing. Leaving only a single word on such\na line may not be reasonable and tucking the word after or before\none of these ajacent 70-column lines would work better in such a\ncase).\n\nThanks.\n"},{"id":"490882","messageId":"xmqq1q87jy70.fsf@gitster.g","threadId":"61130","inReplyTo":"20240318053848.185201-1-brianmlyles@gmail.com","subject":"Re: [PATCH v2 1/2] docs: correct trailer `key_value_separator` description","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T16:34:43Z","receivedAt":"2024-03-18T16:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Lyles writes:\n\n> The description for `key_value_separator` incorrectly states that this\n> separator is inserted between trailer lines, which appears likely to\n> have been incorrectly copied from `separator` when this option was\n> added.\n>\n> Update the description to correctly indicate that it is a separator that\n> appears between the key and the value of each trailer.\n>\n> Signed-off-by: Brian Lyles \n> ---\n> Changes since v1:\n> - Minor wording tweak\n> - Minor wrapping tweak\n>\n> Documentation/pretty-formats.txt | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index d38b4ab566..e1788cb07a 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -330,8 +330,8 @@ multiple times, the last occurrence wins.\n> ** 'keyonly[=]': only show the key part of the trailer.\n> ** 'valueonly[=]': only show the value part of the trailer.\n> ** 'key_value_separator=': specify a separator inserted between\n> - trailer lines. When this option is not given each trailer key-value\n> - pair is separated by \": \". Otherwise it shares the same semantics\n> + the key and value of each trailer. When this option is not given each trailer\n> + key-value pair is separated by \": \". Otherwise it shares the same semantics\n> as 'separator=' above.\n\nI was tempted to insert a comma before \"each trailer key-value pair\"\nwhile queuing this, but the missing comma is shared with other\nentries of the same list, so I'd queue it as-is.\n\nThanks.\n\n"},{"id":"490892","messageId":"f6a16989-cbcb-4558-ae3b-350437fda7c2@app.fastmail.com","threadId":"61130","inReplyTo":"xmqqh6h3jzp1.fsf@gitster.g","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-18T18:15:57Z","receivedAt":"2024-03-18T18:16:19Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Mar 18, 2024, at 17:02, Junio C Hamano wrote:\n> Linus Arver writes:\n>\n>> WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The\n>\n> Your patches will be reviewed on the mailing list. If you keep your\n> line length to somewhere around ~70, the line will still fit within\n> the 80-ish terminal width after a few rounds of review exchanges,\n> with \">> \" prefixed. That reasoning is mostly about the proposed\n> commit log messages, but the same would apply to things like\n> AsciiDoc sources.\n>\n> It is true that we do not write it down. Perhaps something like\n> this is in order?\n>\n> diff --git i/Documentation/SubmittingPatches\n> w/Documentation/SubmittingPatches\n> index e734a3f0f1..68e9ad71a1 100644\n> --- i/Documentation/SubmittingPatches\n> +++ w/Documentation/SubmittingPatches\n> @@ -280,6 +280,14 @@ or, on an older version of Git without support for\n> --pretty=reference:\n> \tgit show -s --date=short --pretty='format:%h (%s, %ad)' \n> ....\n>\n> +[[line-wrap]]\n> +\n> +Just like we limit the patch subject to 50 chars or so, the lines in\n> +the proposed log message should be around 70 chars to make sure that\n> +it still can be shown on 80-column terminal without line wrapping\n> +after a handful of review exchanges add \"> \" prefix to them.\n> +\n> +\n\nThere’s also `.editorconfig` which says that it should be 72\ncharacters. My Magit respects it but NeoVim doesn’t seem to. Maybe worth\nmentioning since you might not need to configure it yourself for this\nproject, depending on your commit message editor.\n\n> [[sign-off]]\n> === Certify your work by adding your `Signed-off-by` trailer\n>\n>\n>> text files aren't really meant for end-user consumption (that's what the\n>> manpage and HTML formats are for), so I think it's OK if the line\n>> lengths are roughly in the same ballpark (no need to worry too much\n>> about exact lengths).\n>\n> Yes, too. And it is one way to reduce patch noise and nicer to\n> reviewers, when used moderately (i.e. removing a word and making a\n> line to occupy only 50 columns when ajacent ones are 70 columns may\n> still be better than reflowing. Leaving only a single word on such\n> a line may not be reasonable and tucking the word after or before\n> one of these ajacent 70-column lines would work better in such a\n> case).\n>\n> Thanks.\n\nMy interpretation of this is\n\n1. Commit messages are flowed/reflowed to 72 columns\n2. Code is reflowed to 80 columns (enforced by tools like clang-format)\n • See `.clang-format` and `.editorconfig` (kept in synch.)\n3. Source documentation (AsciiDoc) is reflowed to 72 opportunistically;\n not every time (in order to avoid diff noise) but when it feels like it\n makes sense\n\nMaybe SubmittingPatches should mention that last point? If my\ninterpretation is correct.\n"},{"id":"490893","messageId":"xmqq5xxjgxp4.fsf@gitster.g","threadId":"61130","inReplyTo":"f6a16989-cbcb-4558-ae3b-350437fda7c2@app.fastmail.com","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T19:13:43Z","receivedAt":"2024-03-18T19:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> My interpretation of this is\n>\n> 1. Commit messages are flowed/reflowed to 72 columns\n> 2. Code is reflowed to 80 columns (enforced by tools like clang-format)\n> • See `.clang-format` and `.editorconfig` (kept in synch.)\n> 3. Source documentation (AsciiDoc) is reflowed to 72 opportunistically;\n> not every time (in order to avoid diff noise) but when it feels like it\n> makes sense\n>\n> Maybe SubmittingPatches should mention that last point? If my\n> interpretation is correct.\n\nI do not know about #2. I've seen cases where a patch trying to\nstick to the hard 80-column limit is hurting readability a lot. I\nthink the moral of the story is that code should never be reflowed\nmechanically without thinking---rather developers, when they see the\nneed to go way too deep in indentation levels, should learn to take\nit a sign that they need to first refactor their code, e.g. with\nsmaller helper functions with meaningful names.\n\n\n"},{"id":"490919","messageId":"owlyle6e3cwa.fsf@fine.c.googlers.com","threadId":"61130","inReplyTo":"xmqqh6h3jzp1.fsf@gitster.g","subject":"Re: [PATCH] docs: correct trailer `key_value_separator` description","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-03-19T07:21:25Z","receivedAt":"2024-03-19T07:21:27Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Junio C Hamano writes:\n\n> Linus Arver writes:\n>\n>> WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The\n>\n> Your patches will be reviewed on the mailing list. If you keep your\n> line length to somewhere around ~70, the line will still fit within\n> the 80-ish terminal width after a few rounds of review exchanges,\n> with \">> \" prefixed. That reasoning is mostly about the proposed\n> commit log messages, but the same would apply to things like\n> AsciiDoc sources.\n\nAgreed.\n\n> It is true that we do not write it down. Perhaps something like\n> this is in order?\n>\n> diff --git i/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\n> index e734a3f0f1..68e9ad71a1 100644\n> --- i/Documentation/SubmittingPatches\n> +++ w/Documentation/SubmittingPatches\n> @@ -280,6 +280,14 @@ or, on an older version of Git without support for --pretty=reference:\n> \tgit show -s --date=short --pretty='format:%h (%s, %ad)' \n> ....\n>\n> +[[line-wrap]]\n> +\n> +Just like we limit the patch subject to 50 chars or so, the lines in\n> +the proposed log message should be around 70 chars to make sure that\n> +it still can be shown on 80-column terminal without line wrapping\n> +after a handful of review exchanges add \"> \" prefix to them.\n> +\n\nI would tweak it slightly like this:\n\n [[line-lengths]]\n\n Just like we limit the patch subject to 50 chars or so, the lines in\n the proposed log message should be around 70 chars. This helps avoid\n line wrapping on 80-column terminal displays, even after after a\n handful of review exchanges add \"> \" prefixes to them.\n\n> [[sign-off]]\n> === Certify your work by adding your `Signed-off-by` trailer\n>\n>\n>> text files aren't really meant for end-user consumption (that's what the\n>> manpage and HTML formats are for), so I think it's OK if the line\n>> lengths are roughly in the same ballpark (no need to worry too much\n>> about exact lengths).\n>\n> Yes, too. And it is one way to reduce patch noise and nicer to\n> reviewers, when used moderately (i.e. removing a word and making a\n> line to occupy only 50 columns when ajacent ones are 70 columns may\n> still be better than reflowing. Leaving only a single word on such\n> a line may not be reasonable and tucking the word after or before\n> one of these ajacent 70-column lines would work better in such a\n> case).\n\nAgreed. Thank you for kindly putting into concrete examples what I was\ntoo lazy to write out in my earlier response to Brian. :)\n\nSpeaking of reducing patch noise, perhaps it deserves a callout in\nSubmittingPatches, something like this (first bullet point)?\n\n [[optimize-for-reviewers]]\n\n To help speed up the review process (and to incentivize would-be\n reviewers), avoid introducing unnecessary noise in your patch\n series. The following are some things to avoid:\n\n . Avoid _reflowing_ (i.e., adjusting where lines start and end in a\n paragraph) around chunks of prose such as in documentation or\n comments, for relatively minor changes. For example, given a\n paragraph with lines about 70 characters long and where your patch\n wants to change the content of one line, consider changing only\n that one line (and leaving the surrounding lines as is) --- even\n if doing so would make that one line go under or over 70\n characters. This makes the patch (now just a one-line diff) easier\n to read, versus a reflowed version where N lines are modified.\n\n . Avoid _extraneous changes_ (however small) in your patch that are\n not called out in the commit log message. Reviewers read your log\n message first, then read the diffs; if there are things in the\n diff that do not line up with your log message, it will surprise\n reviewers.\n\n . Avoid _breaking tests_ in your series, even if you fix them up\n later. Consider flipping the broken tests to expect to fail\n temporarily, and then changing them back to their original state.\n Making sure that all tests pass (at every patch in your series)\n helps to keep the history clean, which can potentially help things\n like git-bisect later on.\n\n . Avoid having _too many patches_ in one series. Aim for a maximum\n of 5-10 patches in your series. If your series requires additional\n patches, consider breaking it up into multiple series (where each\n series achieves one major objective). Wait for reviews of the\n first series to be accepted before sending up the next series.\n\nI took the liberty of documenting some additional \"what not to do\"\nlessons I learned from reviewers from my time on the list so far. I\nassume the \"reflowing\" thing happens more frequently than the other\nbullet points, so I put it first.\n"}]}