| Message ID | 20240316035612.752910-1-brianmlyles@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | docs: correct trailer `key_value_separator` description | expand |
Brian Lyles <brianmlyles@gmail.com> writes: > The description for `key_value_separator` incorrectly states that this > separator is inserted between trailer lines, which appears likely to > have been incorrectly copied from `separator` when this option was > added. > > Update the description to correctly indicate that it is a separator that > appears between the key and the value of each trailer. > > Signed-off-by: Brian Lyles <brianmlyles@gmail.com> > --- > Documentation/pretty-formats.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt > index d38b4ab566..4839c2843c 100644 > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -329,9 +329,9 @@ multiple times, the last occurrence wins. > `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. > ** 'keyonly[=<bool>]': only show the key part of the trailer. > ** 'valueonly[=<bool>]': only show the value part of the trailer. > -** 'key_value_separator=<sep>': specify a separator inserted between Nit: This line was modified to have " each" at the end. If you did that on the next line, then this diff could have been a touch smaller. > - trailer lines. When this option is not given each trailer key-value > - pair is separated by ": ". Otherwise it shares the same semantics > +** 'key_value_separator=<sep>': specify a separator inserted between each > + trailer's key and value. When this option is not given each trailer > + key-value pair is separated by ": ". Otherwise it shares the same semantics > as 'separator=<sep>' above. LGTM. It's probably not worth re-rolling, but a small suggestion I have is to simplify the language a bit to reduce repetition, like so: ** 'key_value_separator=<sep>': specify the separator between the key and value of each trailer. Defaults to ": ". Otherwise it shares the same semantics as 'separator=<sep>' above. Thanks.
Hi Linus On Sat, Mar 16, 2024 at 1:53 AM Linus Arver <linusa@google.com> wrote: > Nit: This line was modified to have " each" at the end. If you did that > on the next line, then this diff could have been a touch smaller. Sure -- it looks like this was a result of applying a different hard-wrap width than the previous author, perhaps? I thought it more prudent to wrap to a consistent length than to be overly concerned about the diff given that it was already a fairly trivial patch. That said, I'm not seeing a recommended wrap width for doc files documented anywhere either. Is there a documented guideline to follow here, both in terms of preferred wrap width as well as when it might be appropriate to stray from it for reasons such as this? > It's probably not worth re-rolling, but a small suggestion I have is to > simplify the language a bit to reduce repetition, like so: > > ** 'key_value_separator=<sep>': specify the separator between > the key and value of each trailer. Defaults to ": ". Otherwise it > shares the same semantics as 'separator=<sep>' above. > I do prefer the simplified language. I had initially aimed to simply correct the inaccuracy, but I think that it probably *is* worth a quick re-roll to make this simplification. I will send that out shortly.
"Brian Lyles" <brianmlyles@gmail.com> writes: > Hi Linus > > On Sat, Mar 16, 2024 at 1:53 AM Linus Arver <linusa@google.com> wrote: > >> Nit: This line was modified to have " each" at the end. If you did that >> on the next line, then this diff could have been a touch smaller. > > [...] Is there a documented guideline to follow here, both in > terms of preferred wrap width as well as when it might be appropriate to > stray from it for reasons such as this? WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The text files aren't really meant for end-user consumption (that's what the manpage and HTML formats are for), so I think it's OK if the line lengths are roughly in the same ballpark (no need to worry too much about exact lengths). When I contributed some patches to the docs last year, I was advised to minimize diffs where appropriate, to make it easier for reviewers. In this case it didn't matter too much (the patch being so small), but I thought it was still worth mentioning. /shrug
Linus Arver <linusa@google.com> writes: > WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The Your patches will be reviewed on the mailing list. If you keep your line length to somewhere around ~70, the line will still fit within the 80-ish terminal width after a few rounds of review exchanges, with ">> " prefixed. That reasoning is mostly about the proposed commit log messages, but the same would apply to things like AsciiDoc sources. It is true that we do not write it down. Perhaps something like this is in order? diff --git i/Documentation/SubmittingPatches w/Documentation/SubmittingPatches index e734a3f0f1..68e9ad71a1 100644 --- i/Documentation/SubmittingPatches +++ w/Documentation/SubmittingPatches @@ -280,6 +280,14 @@ or, on an older version of Git without support for --pretty=reference: git show -s --date=short --pretty='format:%h (%s, %ad)' <commit> .... +[[line-wrap]] + +Just like we limit the patch subject to 50 chars or so, the lines in +the proposed log message should be around 70 chars to make sure that +it still can be shown on 80-column terminal without line wrapping +after a handful of review exchanges add "> " prefix to them. + + [[sign-off]] === Certify your work by adding your `Signed-off-by` trailer > text files aren't really meant for end-user consumption (that's what the > manpage and HTML formats are for), so I think it's OK if the line > lengths are roughly in the same ballpark (no need to worry too much > about exact lengths). Yes, too. And it is one way to reduce patch noise and nicer to reviewers, when used moderately (i.e. removing a word and making a line to occupy only 50 columns when ajacent ones are 70 columns may still be better than reflowing. Leaving only a single word on such a line may not be reasonable and tucking the word after or before one of these ajacent 70-column lines would work better in such a case). Thanks.
On Mon, Mar 18, 2024, at 17:02, Junio C Hamano wrote: > Linus Arver <linusa@google.com> writes: > >> WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The > > Your patches will be reviewed on the mailing list. If you keep your > line length to somewhere around ~70, the line will still fit within > the 80-ish terminal width after a few rounds of review exchanges, > with ">> " prefixed. That reasoning is mostly about the proposed > commit log messages, but the same would apply to things like > AsciiDoc sources. > > It is true that we do not write it down. Perhaps something like > this is in order? > > diff --git i/Documentation/SubmittingPatches > w/Documentation/SubmittingPatches > index e734a3f0f1..68e9ad71a1 100644 > --- i/Documentation/SubmittingPatches > +++ w/Documentation/SubmittingPatches > @@ -280,6 +280,14 @@ or, on an older version of Git without support for > --pretty=reference: > git show -s --date=short --pretty='format:%h (%s, %ad)' <commit> > .... > > +[[line-wrap]] > + > +Just like we limit the patch subject to 50 chars or so, the lines in > +the proposed log message should be around 70 chars to make sure that > +it still can be shown on 80-column terminal without line wrapping > +after a handful of review exchanges add "> " prefix to them. > + > + There’s also `.editorconfig` which says that it should be 72 characters. My Magit respects it but NeoVim doesn’t seem to. Maybe worth mentioning since you might not need to configure it yourself for this project, depending on your commit message editor. > [[sign-off]] > === Certify your work by adding your `Signed-off-by` trailer > > >> text files aren't really meant for end-user consumption (that's what the >> manpage and HTML formats are for), so I think it's OK if the line >> lengths are roughly in the same ballpark (no need to worry too much >> about exact lengths). > > Yes, too. And it is one way to reduce patch noise and nicer to > reviewers, when used moderately (i.e. removing a word and making a > line to occupy only 50 columns when ajacent ones are 70 columns may > still be better than reflowing. Leaving only a single word on such > a line may not be reasonable and tucking the word after or before > one of these ajacent 70-column lines would work better in such a > case). > > Thanks. My interpretation of this is 1. Commit messages are flowed/reflowed to 72 columns 2. Code is reflowed to 80 columns (enforced by tools like clang-format) • See `.clang-format` and `.editorconfig` (kept in synch.) 3. Source documentation (AsciiDoc) is reflowed to 72 opportunistically; not every time (in order to avoid diff noise) but when it feels like it makes sense Maybe SubmittingPatches should mention that last point? If my interpretation is correct.
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > My interpretation of this is > > 1. Commit messages are flowed/reflowed to 72 columns > 2. Code is reflowed to 80 columns (enforced by tools like clang-format) > • See `.clang-format` and `.editorconfig` (kept in synch.) > 3. Source documentation (AsciiDoc) is reflowed to 72 opportunistically; > not every time (in order to avoid diff noise) but when it feels like it > makes sense > > Maybe SubmittingPatches should mention that last point? If my > interpretation is correct. I do not know about #2. I've seen cases where a patch trying to stick to the hard 80-column limit is hurting readability a lot. I think the moral of the story is that code should never be reflowed mechanically without thinking---rather developers, when they see the need to go way too deep in indentation levels, should learn to take it a sign that they need to first refactor their code, e.g. with smaller helper functions with meaningful names.
Junio C Hamano <gitster@pobox.com> writes: > Linus Arver <linusa@google.com> writes: > >> WRT line lengths, probably 80-ish columns is the (unwritten?) rule. The > > Your patches will be reviewed on the mailing list. If you keep your > line length to somewhere around ~70, the line will still fit within > the 80-ish terminal width after a few rounds of review exchanges, > with ">> " prefixed. That reasoning is mostly about the proposed > commit log messages, but the same would apply to things like > AsciiDoc sources. Agreed. > It is true that we do not write it down. Perhaps something like > this is in order? > > diff --git i/Documentation/SubmittingPatches w/Documentation/SubmittingPatches > index e734a3f0f1..68e9ad71a1 100644 > --- i/Documentation/SubmittingPatches > +++ w/Documentation/SubmittingPatches > @@ -280,6 +280,14 @@ or, on an older version of Git without support for --pretty=reference: > git show -s --date=short --pretty='format:%h (%s, %ad)' <commit> > .... > > +[[line-wrap]] > + > +Just like we limit the patch subject to 50 chars or so, the lines in > +the proposed log message should be around 70 chars to make sure that > +it still can be shown on 80-column terminal without line wrapping > +after a handful of review exchanges add "> " prefix to them. > + I would tweak it slightly like this: [[line-lengths]] Just like we limit the patch subject to 50 chars or so, the lines in the proposed log message should be around 70 chars. This helps avoid line wrapping on 80-column terminal displays, even after after a handful of review exchanges add "> " prefixes to them. > [[sign-off]] > === Certify your work by adding your `Signed-off-by` trailer > > >> text files aren't really meant for end-user consumption (that's what the >> manpage and HTML formats are for), so I think it's OK if the line >> lengths are roughly in the same ballpark (no need to worry too much >> about exact lengths). > > Yes, too. And it is one way to reduce patch noise and nicer to > reviewers, when used moderately (i.e. removing a word and making a > line to occupy only 50 columns when ajacent ones are 70 columns may > still be better than reflowing. Leaving only a single word on such > a line may not be reasonable and tucking the word after or before > one of these ajacent 70-column lines would work better in such a > case). Agreed. Thank you for kindly putting into concrete examples what I was too lazy to write out in my earlier response to Brian. :) Speaking of reducing patch noise, perhaps it deserves a callout in SubmittingPatches, something like this (first bullet point)? [[optimize-for-reviewers]] To help speed up the review process (and to incentivize would-be reviewers), avoid introducing unnecessary noise in your patch series. The following are some things to avoid: . Avoid _reflowing_ (i.e., adjusting where lines start and end in a paragraph) around chunks of prose such as in documentation or comments, for relatively minor changes. For example, given a paragraph with lines about 70 characters long and where your patch wants to change the content of one line, consider changing only that one line (and leaving the surrounding lines as is) --- even if doing so would make that one line go under or over 70 characters. This makes the patch (now just a one-line diff) easier to read, versus a reflowed version where N lines are modified. . Avoid _extraneous changes_ (however small) in your patch that are not called out in the commit log message. Reviewers read your log message first, then read the diffs; if there are things in the diff that do not line up with your log message, it will surprise reviewers. . Avoid _breaking tests_ in your series, even if you fix them up later. Consider flipping the broken tests to expect to fail temporarily, and then changing them back to their original state. Making sure that all tests pass (at every patch in your series) helps to keep the history clean, which can potentially help things like git-bisect later on. . Avoid having _too many patches_ in one series. Aim for a maximum of 5-10 patches in your series. If your series requires additional patches, consider breaking it up into multiple series (where each series achieves one major objective). Wait for reviews of the first series to be accepted before sending up the next series. I took the liberty of documenting some additional "what not to do" lessons I learned from reviewers from my time on the list so far. I assume the "reflowing" thing happens more frequently than the other bullet points, so I put it first.
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index d38b4ab566..4839c2843c 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -329,9 +329,9 @@ multiple times, the last occurrence wins. `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. ** 'keyonly[=<bool>]': only show the key part of the trailer. ** 'valueonly[=<bool>]': only show the value part of the trailer. -** 'key_value_separator=<sep>': specify a separator inserted between - trailer lines. When this option is not given each trailer key-value - pair is separated by ": ". Otherwise it shares the same semantics +** 'key_value_separator=<sep>': specify a separator inserted between each + trailer's key and value. When this option is not given each trailer + key-value pair is separated by ": ". Otherwise it shares the same semantics as 'separator=<sep>' above. NOTE: Some placeholders may depend on other options given to the
The description for `key_value_separator` incorrectly states that this separator is inserted between trailer lines, which appears likely to have been incorrectly copied from `separator` when this option was added. Update the description to correctly indicate that it is a separator that appears between the key and the value of each trailer. Signed-off-by: Brian Lyles <brianmlyles@gmail.com> --- Documentation/pretty-formats.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)