Git-friendly CL Descriptions

[Adrienne Walker]

As most folks now use git and git tools in Chromium, please consider making your change descriptions more git-friendly.  Such a message looks something like this:

    Capitalized, short (50 chars or less) summary

    More detailed explanatory text, if necessary.  The blank line

    separating the summary from the body is critical (unless you omit

    the body entirely); tools like rebase can get confused if you run

    the two together.  Ideally, the entire description should also be

    wrapped to 72 characters so that tools like git log can fit the

    entire commit message in a standard-sized terminal window.

    More paragraphs, &c

    R=no...@chromium.org

    BUG=12345

The most important part is the single, short, stand-alone summary line.  Many git tools use that as the short version of the commit message.  As a bonus, having such a summary line will also help the depot_tools/my_activity.py auto-generate better snippits.

Example description taken from http://www.tpope.net/node/106.

-enne

[Robert Sesek]

+1 to the idea, but I think just enforcing 80cols for CL descriptions would be sufficient and Chromiumy.

rsesek / @chromium.org

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Elliot Glaysher (Chromium)]

This probably needs support on codereview.chromium.org. There have been several times where I've written a git friendly commit message, and then edited it online and then it came out all ugly.

[Thiago Farina]

On Wed, Jan 16, 2013 at 5:18 PM, Robert Sesek <rse...@chromium.org> wrote:
> +1 to the idea, but I think just enforcing 80cols for CL descriptions would
> be sufficient and Chromiumy.
>

That would be a huge improvement for our commit messages! It will make
them much more sane and readable!

Some of us using vim/emacs don't suffer that harm.

--
Thiago

[Peter Kasting]

On Wed, Jan 16, 2013 at 11:15 AM, Adrienne Walker <en...@chromium.org> wrote:

    Capitalized, short (50 chars or less) summary

What about a summary line longer than 50 chars?  Will it get auto-ellipsized?

    Ideally, the entire description should also be

    wrapped to 72 characters so that tools like git log can fit the

    entire commit message in a standard-sized terminal window.

If we're going to enforce a line length limit, I'd really prefer we use 80, for consistency with the code.  It'd actually be fairly annoying for me to wrap at 72.  (It's somewhat annoying to wrap at 80 too, but I'll live with that if it makes others' lives better.)

PK 

[Christian Biesinger]

On Wed, Jan 16, 2013 at 6:07 PM, Peter Kasting <pkas...@chromium.org> wrote:
>> Ideally, the entire description should also be
>> wrapped to 72 characters so that tools like git log can fit the
>> entire commit message in a standard-sized terminal window.
>
>
> If we're going to enforce a line length limit, I'd really prefer we use 80,
> for consistency with the code. It'd actually be fairly annoying for me to
> wrap at 72. (It's somewhat annoying to wrap at 80 too, but I'll live with
> that if it makes others' lives better.)

Wrapping at 80 doesn't work, because git log indents your commit
message by a few spaces, and a git revert by a few more, so to stay
under 80 for the total you need 72.

(dammit, I keep forgetting to change my from address)

-christian

[Peter Kasting]

Then to be perfectly honest I'd rather not enforce a line length limit.

PK 

[Scott Hess]

On Wed, Jan 16, 2013 at 6:11 PM, Christian Biesinger <cbies...@chromium.org> wrote:

Personally, I think that boat has sailed, hit an iceberg, burst into flames, and sank to the bottom of the ocean, then the ocean silted up and a new continent was formed.  I'd rate having a line-length discussion right up there with a copyright notice discussion for enjoyability and likelihood of influencing anything.

-scott

[Ryan Hamilton]

+1.  Well said, as usual. 

[Stuart Morgan]

2013/1/17 Peter Kasting <pkas...@google.com>:

> Then to be perfectly honest I'd rather not enforce a line length limit.

The original email didn't appear to call for an enforced limit, it
just pointed out that if people want to make life easier for everyone
using git (which is a large and growing portion of the team), they
could do so by following this guideline.

I expect that anyone not currently using git (and who works with
80-col command-line windows), will have a renewed appreciation for
this PSA the first time they do. I agree we don't need a centithread
about a rule; I would just encourage anyone who encounters
hard-to-read commit messages from git log and other git tools to think
back to this thread and remember that they can choose going forward to
be part of the solution instead of part of the problem.

(As for 80 columns, if wrapping your commit messages at 80 is livable
and 72 isn't, please do consider 80 instead of not wrapping at all.
Missing some letters at the end of the line occasionally is still
orders of magnitude better than having an 'infinite' horizontal
scroll.)

-Stuart

[Greg Thompson]

This sounds like a shortcoming of git that should be fixed in git rather than in the bajillion developers using git.

 

(dammit, I keep forgetting to change my from address)

-christian

[Torne (Richard Coles)]

On 17 Jan 2013 07:07, "Greg Thompson" <g...@chromium.org> wrote:
>
> On Wed, Jan 16, 2013 at 9:11 PM, Christian Biesinger <cbies...@chromium.org> wrote:
>>
>> On Wed, Jan 16, 2013 at 6:07 PM, Peter Kasting <pkas...@chromium.org> wrote:
>> >>     Ideally, the entire description should also be
>> >>     wrapped to 72 characters so that tools like git log can fit the
>> >>     entire commit message in a standard-sized terminal window.
>> >
>> >
>> > If we're going to enforce a line length limit, I'd really prefer we use 80,
>> > for consistency with the code.  It'd actually be fairly annoying for me to
>> > wrap at 72.  (It's somewhat annoying to wrap at 80 too, but I'll live with
>> > that if it makes others' lives better.)
>>
>> Wrapping at 80 doesn't work, because git log indents your commit
>> message by a few spaces, and a git revert by a few more, so to stay
>> under 80 for the total you need 72.
>
>
> This sounds like a shortcoming of git that should be fixed in git rather than in the bajillion developers using git.

Git intentionally doesn't wrap commit messages in order to avoid damaging formatting in cases where long lines were intentional/significant (e.g. source/output samples). Also, git commits are designed to be transmittable via plaintext email, which also has similar wrapping preferences in many mail clients (or mail client users).

You can disagree about the value of these reasons, but git does what it does intentionally and is not going to change.

[Peter Kasting]

Thanks for the clarity (and also to others in this thread who added it).

I will try to keep y'all in mind when writing my future messages!

PK

[Vincent Scheib]

Re: Line wrapping: Modify your local tool options to wrap lines by default. I happen to use less as my git pager most of the time (though tig is awesome too, and I use it when I'm really looking). For less, you can set your LESS variable and or muck with how git sets it in config core.pager.

[Vadim Bendebury]

May i suggest that each change description contains a section describing how the change was tested.

This is very useful for many reasons, in particular:

  - helps the reviewer understand if the change was adequately tested

  - helps the SQA personnel to retest the change

cheers,

/vb

[Nico Weber]

On Thu, Jan 17, 2013 at 5:02 PM, Vadim Bendebury <vbe...@google.com> wrote:
> May i suggest that each change description contains a section describing how
> the change was tested.
>
> This is very useful for many reasons, in particular:
>
> - helps the reviewer understand if the change was adequately tested
> - helps the SQA personnel to retest the change

We used to have mandatory TEST= lines for a while, it didn't work out.
Too many people just left them empty or wrote "TEST=unit tests".

> cheers,
> /vb
>
> On Wednesday, January 16, 2013 11:15:21 AM UTC-8, Adrienne Walker wrote:
>>
>> As most folks now use git and git tools in Chromium, please consider
>> making your change descriptions more git-friendly. Such a message looks
>> something like this:
>>
>> Capitalized, short (50 chars or less) summary
>>
>> More detailed explanatory text, if necessary. The blank line
>> separating the summary from the body is critical (unless you omit
>> the body entirely); tools like rebase can get confused if you run
>> the two together. Ideally, the entire description should also be
>> wrapped to 72 characters so that tools like git log can fit the
>> entire commit message in a standard-sized terminal window.
>>
>> More paragraphs, &c
>>
>> R=no...@chromium.org
>> BUG=12345
>>
>> The most important part is the single, short, stand-alone summary line.
>> Many git tools use that as the short version of the commit message. As a
>> bonus, having such a summary line will also help the
>> depot_tools/my_activity.py auto-generate better snippits.
>>
>> Example description taken from http://www.tpope.net/node/106.
>>
>> -enne
>

[Dana Jansens]

On Thu, Jan 17, 2013 at 8:35 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Jan 17, 2013 at 5:02 PM, Vadim Bendebury <vbe...@google.com> wrote:
> May i suggest that each change description contains a section describing how
> the change was tested.
>
> This is very useful for many reasons, in particular:
>
>   - helps the reviewer understand if the change was adequately tested
>   - helps the SQA personnel to retest the change

We used to have mandatory TEST= lines for a while, it didn't work out.
Too many people just left them empty or wrote "TEST=unit tests".

The TEST= format doesn't work well for unit tests. I usually include something like:

Tests:

UnitTest.Test1

UnitTest.Test2

MoreTests.Follow

Which I guess I inherited from WebKit days. Maybe we could presubmit warn for changes that don't have some mention of tests, but in a more flexible format than TEST=. Or encourage reviewers to ask "which tests cover this?" explicitly.

[Tommi]

On that subject and hoping not to derail the thread too much, I've noticed lately that some CLs have "TESTED=<what I did to test that my code works>" and not "TEST=<what QA should do to verify my code>".

Which is preferred?  Is "TESTED" something we started using recently or am I just out of the loop?

[Tommi]

On that subject and hoping not to derail the thread too much, I've noticed lately that some CLs have "TESTED=<what I did to test that my code works>" and not "TEST=<what QA should do to verify my code>".

Which is preferred?  Is "TESTED" something we added recently?

[Victor Khimenko]

On Fri, Jan 18, 2013 at 5:35 AM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Jan 17, 2013 at 5:02 PM, Vadim Bendebury <vbe...@google.com> wrote:
> May i suggest that each change description contains a section describing how
> the change was tested.
>
> This is very useful for many reasons, in particular:
>
>   - helps the reviewer understand if the change was adequately tested
>   - helps the SQA personnel to retest the change

We used to have mandatory TEST= lines for a while, it didn't work out.
Too many people just left them empty or wrote "TEST=unit tests".

That's kind of understandable: most CLs I ever write have NOTHING to test. Why? Because they have no new functionality. They move things around, add or remove variables, etc. Sure, at some point I *do* commit change which is externally-visible testable change (otherwise my work is kind of pointless) but these are rare. For most CLs I only need green bots and nothing else.

P.S. And of course when I try to combine all these changes to have a meaningful TEST= line reviewers [rightfully] insist that preparatory refactorings should be committed separately.

[Vadim Bendebury (вб)]

On Fri, Jan 18, 2013 at 7:47 AM, Victor Khimenko <kh...@chromium.org> wrote:
>
> On Fri, Jan 18, 2013 at 5:35 AM, Nico Weber <tha...@chromium.org> wrote:
>>
>> On Thu, Jan 17, 2013 at 5:02 PM, Vadim Bendebury <vbe...@google.com>
>> wrote:
>> > May i suggest that each change description contains a section describing
>> > how
>> > the change was tested.
>> >
>> > This is very useful for many reasons, in particular:
>> >
>> > - helps the reviewer understand if the change was adequately tested
>> > - helps the SQA personnel to retest the change
>>
>> We used to have mandatory TEST= lines for a while, it didn't work out.
>> Too many people just left them empty or wrote "TEST=unit tests".
>>
> That's kind of understandable: most CLs I ever write have NOTHING to test.
> Why? Because they have no new functionality. They move things around, add or
> remove variables, etc. Sure, at some point I *do* commit change which is
> externally-visible testable change (otherwise my work is kind of pointless)
> but these are rare. For most CLs I only need green bots and nothing else.
>

This seems odd to me: if you change some code without introducing new
functionality - the code still does something and your change
conceivably could affect that. Shouldn't the test in this case be the
same, as when the code was originally submitted, demonstrating that
the code still works?

cheers,
/v

[Victor Khimenko]

On Fri, Jan 18, 2013 at 8:35 PM, Vadim Bendebury (вб) <vbe...@google.com> wrote:

On Fri, Jan 18, 2013 at 7:47 AM, Victor Khimenko <kh...@chromium.org> wrote:
>
> On Fri, Jan 18, 2013 at 5:35 AM, Nico Weber <tha...@chromium.org> wrote:
>>
>> On Thu, Jan 17, 2013 at 5:02 PM, Vadim Bendebury <vbe...@google.com>
>> wrote:
>> > May i suggest that each change description contains a section describing
>> > how
>> > the change was tested.
>> >
>> > This is very useful for many reasons, in particular:
>> >
>> >   - helps the reviewer understand if the change was adequately tested
>> >   - helps the SQA personnel to retest the change
>>
>> We used to have mandatory TEST= lines for a while, it didn't work out.
>> Too many people just left them empty or wrote "TEST=unit tests".
>>
> That's kind of understandable: most CLs I ever write have NOTHING to test.
> Why? Because they have no new functionality. They move things around, add or
> remove variables, etc. Sure, at some point I *do* commit change which is
> externally-visible testable change (otherwise my work is kind of pointless)
> but these are rare. For most CLs I only need green bots and nothing else.
>

This seems odd to me: if you change some code without introducing new
functionality - the code still does something and your change
conceivably could affect that. Shouldn't the test in this case be the
same, as when the code was originally submitted, demonstrating that
the code still works?

Sure - but since I don't introduce anything new I don't need any new tests and thus I don't care which particular existing test checks that this particular piece of code is unbroken. If bots are green then I assume everything is good.

[Peter Kasting]

Because we've had so many long threads about testing, TEST=, etc. in the past, I think it'd be nice to keep this thread about the original topic, namely git-friendly change descriptions.

If people feel like there is something lacking about how we communicate change testability, perhaps those people could start a new thread about that.

PK

[Daniel Murphy]

Can we put this in the style guide?

[Daniel Murphy]

Whoops, by that I mean, can we have some sort of style-guide writing around the commit message - maybe at least the # of characters to wrap to?

(didn't mean to quote the TEST= part)

[Fabrice de Gans-Riberi]

On that note, it would be very nice if gerrit showed you a column limit when editing the commit message in the gerrit UI.

Fabrice

On Mon, Apr 1, 2019 at 1:23 PM 'Daniel Murphy' via Chromium-dev <chromi...@chromium.org> wrote:

Whoops, by that I mean, can we have some sort of style-guide writing around the commit message - maybe at least the # of characters to wrap to?

(didn't mean to quote the TEST= part)

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/f2c04b40-4065-4ea2-b961-471801ee735a%40chromium.org.

[Sylvain Defresne]

In the document at https://chromium.googlesource.com/chromium/src/+/master/docs/contributing.md#uploading-a-change-for-review, the sample commit message says that lines should be wrapped at 72 characters. It also says that the message should have a one line summary.

From my point of view, this is already documented (though I guess people may just glance over this as it is formatted as an example and not in a list of bullet points).

-- Sylvain

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJ0M83UG%2BP-rfn7YXCOL1%3D1qMEacuXZa%3D6zbnqFmGNKYBV_LkQ%40mail.gmail.com.