[PATCH] docs: correct trailer `key_value_separator` description

[PATCH] docs: correct trailer `key_value_separator` description

[Brian Lyles]

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
-   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
-- 
2.43.0

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Linus Arver]

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.

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Brian Lyles]

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.

-- 
Thank you,
Brian Lyles

[PATCH v2 1/2] docs: correct trailer `key_value_separator` description

[Brian Lyles]

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>
---
Changes since v1:
- Minor wording tweak
- Minor wrapping tweak

 Documentation/pretty-formats.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d38b4ab566..e1788cb07a 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -330,8 +330,8 @@ multiple times, the last occurrence wins.
 ** '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
+   the key and value of each trailer. 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
-- 
2.43.2

[PATCH v2 2/2] docs: adjust trailer `separator` and `key_value_separator` language

[Brian Lyles]

The language describing the trailer separator and key-value separator
default value is overly complicated.

Indicate the default with simpler "Defaults to ..." language.

Suggested-by: Linus Arver <linusa@google.com>
Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
---
This commit is new in v2 per Linus' suggestion[1].

[1]: https://lore.kernel.org/git/owly1q8a4qhh.fsf@fine.c.googlers.com/

 Documentation/pretty-formats.txt | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e1788cb07a..8ee940b6a4 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -316,9 +316,8 @@ multiple times, the last occurrence wins.
    `Reviewed-by`.
 ** 'only[=<bool>]': select whether non-trailer lines from the trailer
    block should be included.
-** 'separator=<sep>': specify a separator inserted between trailer
-   lines. When this option is not given each trailer line is
-   terminated with a line feed character. The string <sep> may contain
+** 'separator=<sep>': specify the separator inserted between trailer
+   lines. Defaults to a line feed character. The string <sep> may contain
    the literal formatting codes described above. To use comma as
    separator one must use `%x2C` as it would otherwise be parsed as
    next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
@@ -329,10 +328,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
-   the key and value of each trailer. When this option is not given each trailer
-   key-value pair is separated by ": ". Otherwise it shares the same semantics
-   as 'separator=<sep>' above.
+** 'key_value_separator=<sep>': specify the separator inserted between
+   the key and value of each trailer. Defaults to ": ". Otherwise it
+   shares the same semantics as 'separator=<sep>' above.

 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.43.2

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Linus Arver]

"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

Re: [PATCH v2 2/2] docs: adjust trailer `separator` and `key_value_separator` language

[Linus Arver]

Brian Lyles <brianmlyles@gmail.com> writes:

This v2 LGTM. Thanks!

> The language describing the trailer separator and key-value separator
> default value is overly complicated.
>
> Indicate the default with simpler "Defaults to ..." language.
>
> Suggested-by: Linus Arver <linusa@google.com>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
> This commit is new in v2 per Linus' suggestion[1].
>
> [1]: https://lore.kernel.org/git/owly1q8a4qhh.fsf@fine.c.googlers.com/
>
>  Documentation/pretty-formats.txt | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index e1788cb07a..8ee940b6a4 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -316,9 +316,8 @@ multiple times, the last occurrence wins.
>     `Reviewed-by`.
>  ** 'only[=<bool>]': select whether non-trailer lines from the trailer
>     block should be included.
> -** 'separator=<sep>': specify a separator inserted between trailer
> -   lines. When this option is not given each trailer line is
> -   terminated with a line feed character. The string <sep> may contain
> +** 'separator=<sep>': specify the separator inserted between trailer
> +   lines. Defaults to a line feed character. The string <sep> may contain
>     the literal formatting codes described above. To use comma as
>     separator one must use `%x2C` as it would otherwise be parsed as
>     next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
> @@ -329,10 +328,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
> -   the key and value of each trailer. When this option is not given each trailer
> -   key-value pair is separated by ": ". Otherwise it shares the same semantics
> -   as 'separator=<sep>' above.
> +** 'key_value_separator=<sep>': specify the separator inserted between
> +   the key and value of each trailer. Defaults to ": ". Otherwise it
> +   shares the same semantics as 'separator=<sep>' above.
>
>  NOTE: Some placeholders may depend on other options given to the
>  revision traversal engine. For example, the `%g*` reflog options will
> -- 
> 2.43.2

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Junio C Hamano]

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.

Re: [PATCH v2 1/2] docs: correct trailer `key_value_separator` description

[Junio C Hamano]

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>
> ---
> Changes since v1:
> - Minor wording tweak
> - Minor wrapping tweak
>
>  Documentation/pretty-formats.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index d38b4ab566..e1788cb07a 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -330,8 +330,8 @@ multiple times, the last occurrence wins.
>  ** '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
> +   the key and value of each trailer. When this option is not given each trailer
> +   key-value pair is separated by ": ". Otherwise it shares the same semantics
>     as 'separator=<sep>' above.

I was tempted to insert a comma before "each trailer key-value pair"
while queuing this, but the missing comma is shared with other
entries of the same list, so I'd queue it as-is.

Thanks.

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Kristoffer Haugsbakk]

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.

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Junio C Hamano]

"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.

Re: [PATCH] docs: correct trailer `key_value_separator` description

[Linus Arver]

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.