Junio C Hamano <gitster@xxxxxxxxx> writes:
> Linus Arver <linusa@xxxxxxxxxx> 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.
