What would it take to have "git cl format" run as part of presubmit?

[Colin Blundell]

Hi all,

I would like to get to a point where "git cl format" runs as part of presubmit, eliminating virtually all current overhead that formatting imposes on CL authors and reviewers.

What do people see as the blockers to such a change? I am motivated to enable this change to happen, as I think that it would be a huge productivity win for Chromium development.

Thanks,

Colin

[Marc-Antoine Ruel]

Presubmit checks are idempotent by definition, e.g. they must not have side-effects on the inputs.

That said, "something else" can be used. One option is a .git/pre-commit hook to do it on your behalf on commits.

M-A

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

[Colin Blundell]

Sure, what I mean is "have it run automatically as part of upload" (or on commit if that works out better for technical reasons).

Note that technically speaking "git cl format" is idempotent, i.e., if you run it twice in a row you'll get the same result each time :). It's not side-effect-free of course.

Thanks,

Colin

[Peter Kasting]

On Mon, Apr 28, 2014 at 2:16 AM, Colin Blundell <blun...@chromium.org> wrote:

I would like to get to a point where "git cl format" runs as part of presubmit, eliminating virtually all current overhead that formatting imposes on CL authors and reviewers.

What do people see as the blockers to such a change? I am motivated to enable this change to happen, as I think that it would be a huge productivity win for Chromium development.

This has been discussed repeatedly before (as you seem to know, given your CC list).  I will say the same thing I've said before: before making this change I'd like us to make sure either:

(a) clang-format output matches current prevailing style except perhaps for very esoteric cases, or

(b) We run clang-format on the entire codebase first

In general, no one seems eager to do (b).  (a) is slowly getting better over time, but there are still too many issues today, such as formatting of ternaries (which is controlled by an option, and I think someone is looking at changing the option, finally) or clang-format's seemingly random decisions to wrap call chains like:

...foo()

    .bar()

    .baz()

    .qux();

...which in the last thread someone said was probably due to tripping a "builder pattern" heuristic, but I don't know the details of this heuristic (and every time I've seen this, the code in question has not been doing anything like a "builder pattern", it's just trying to make some calls).

PK

[Dana Jansens]

We check that a patch is formatted at upload time for cc/

https://code.google.com/p/chromium/codesearch#chromium/src/cc/PRESUBMIT.py&l=224

It's the author's job to run the format, but I use a script that does format+git cl upload to make that easy.

[John Abd-El-Malek]

+1 to Peter's comment. It seems that clang-format is close, but not quite there. i.e. in recent weeks we have seen it format code in unnatural ways (i.e. starting a line with OVERRIDE, or the example that Peter gave below). Suddenly making parts, or all, of the code get reformatted by it before bugs like these are fixed seems premature.

[Nico Weber]

On Mon, Apr 28, 2014 at 5:25 PM, John Abd-El-Malek <j...@chromium.org> wrote:

+1 to Peter's comment. It seems that clang-format is close, but not quite there. i.e. in recent weeks we have seen it format code in unnatural ways (i.e. starting a line with OVERRIDE,

This is fixed in the clang-format binary used in chromium.

[John Abd-El-Malek]

On Mon, Apr 28, 2014 at 5:33 PM, Nico Weber <tha...@chromium.org> wrote:

On Mon, Apr 28, 2014 at 5:25 PM, John Abd-El-Malek <j...@chromium.org> wrote:

+1 to Peter's comment. It seems that clang-format is close, but not quite there. i.e. in recent weeks we have seen it format code in unnatural ways (i.e. starting a line with OVERRIDE,

This is fixed in the clang-format binary used in chromium.

Yep, I realize that, but this only happened after Peter/I complained. I don't know how many other bugs like this exist. The problem with forcing the use of clang-format is that if we see other issues, until we get a new binary then code blocks which change in the meantime will get formatted incorrectly. When this is caught, will folks somehow go through old commits and clean it up? Searching for lines beginning with OVERRIDE shows 591 hits. They don't conform to our style guide, and were landed because of clang-format. What are we going to do about that?

Another example from today, where clang-format changed the line spacing so that each block of lines with C++ comments starts 2 spaces after the longest enum value (in that block, not the file as we usually do). Check out all the hysteresis in https://codereview.chromium.org/203993004/diff/80001/chrome/common/chrome_paths.h , which would add extra steps to code annotations, and which would keep getting worse as people edit that file.

[Nico Weber]

On Mon, Apr 28, 2014 at 6:03 PM, John Abd-El-Malek <j...@chromium.org> wrote:
>
>
> On Mon, Apr 28, 2014 at 5:33 PM, Nico Weber <tha...@chromium.org> wrote:
>>
>> On Mon, Apr 28, 2014 at 5:25 PM, John Abd-El-Malek <j...@chromium.org>
>> wrote:
>>>
>>> +1 to Peter's comment. It seems that clang-format is close, but not quite
>>> there. i.e. in recent weeks we have seen it format code in unnatural ways
>>> (i.e. starting a line with OVERRIDE,
>>
>>
>> This is fixed in the clang-format binary used in chromium.
>
>
> Yep, I realize that, but this only happened after Peter/I complained. I
> don't know how many other bugs like this exist. The problem with forcing the
> use of clang-format is that if we see other issues, until we get a new
> binary then code blocks which change in the meantime will get formatted
> incorrectly. When this is caught, will folks somehow go through old commits
> and clean it up? Searching for lines beginning with OVERRIDE shows 591 hits.
> They don't conform to our style guide, and were landed because of
> clang-format.

How do you know they're all due to clang-format?

[Rachel Blum]

On Mon, Apr 28, 2014 at 6:05 PM, Nico Weber <tha...@chromium.org> wrote:

How do you know they're all due to clang-format?

The last patchset is "undo git cl format changes" :)

[John Abd-El-Malek]

On Mon, Apr 28, 2014 at 6:24 PM, Rachel Blum <gr...@chromium.org> wrote:

On Mon, Apr 28, 2014 at 6:05 PM, Nico Weber <tha...@chromium.org> wrote:

How do you know they're all due to clang-format?

The formatting examples I gave were never* in the code base until people started using clang-format.

The last patchset is "undo git cl format changes" :)

That is because I asked the developer to undo clang-format's changes.

 

*or in extremely limited instances where the author & reviewer weren't familiar with the style guide and/or convention.

[Nico Weber]

On Mon, Apr 28, 2014 at 8:55 PM, John Abd-El-Malek <j...@chromium.org> wrote:
>
>
>
> On Mon, Apr 28, 2014 at 6:24 PM, Rachel Blum <gr...@chromium.org> wrote:
>>
>>
>> On Mon, Apr 28, 2014 at 6:05 PM, Nico Weber <tha...@chromium.org> wrote:
>>>
>>> How do you know they're all due to clang-format?
>
>
> The formatting examples I gave were never* in the code base until people
> started using clang-format.

That's a good criterion :-) I checked out r190002 (which is from Mar
23 2013, which predates clang-format being usable and well-known), and
`git gs " OVERRIDE" | wc -l` returns 302 there. On today's trunk
it returns 816, up 170%.

For reference, `git gs "[^ ] OVERRIDE" | wc -l` (which should be
unrelated to clang-format) went from 37397 to 49350, up 32%, so it
looks like clang-format caused a good fraction of these. "never*" is
too strong though.

[Daniel Cheng]

Do we typically ask developers go back and fix their style mistakes? We check in incorrect formatting all the time. I'm not feeling very creative, but a quick search turns up things like 6 space indents for wrapped lines, unnecessary double spaces, and an incorrectly indented private: label. In most of these cases, wouldn't we just let the next developer that touches these lines fix them?

As for the specific example you linked, I'm not sure there would be much more than that one-time churn. From what I can see, it tried to align most things, but even lines that are only slightly longer didn't get realigned (DIR_MANAGED_USERS_DEFAULT_APPS). But even this is something we could disable if we wanted (see AlignTrailingComments on http://clang.llvm.org/docs/ClangFormatStyleOptions.html).

Daniel

[Colin Blundell]

I am aware that there have been previous discussions on this topic. The question that I'm interested in is: Is there some way to set a measure on when clang-format would be good enough for us to start using it as our enforcer of style and to start working toward that goal? I would guess that it's never going to perfectly match what every developer desires as there are areas of ambiguity in the style guide. For me personally, the win from not having to spend energy on style issues as either an author or reviewer would be so large that I don't care all that much about most of the ways that "git cl format" formats code differently from how I personally would choose to. Some of them, however, are more glaring, like the seemingly-arbitrary decisions on when a chain of method calls gets formatted in "builder-style". Is there any process that we could set in place for getting clang-format to be good enough that we could adopt it as our style arbiter? Can we even set a definition on what "good enough" means? I'm sure that at whatever point we would enable clang-format as the enforcer of Chromium style, we would still experience issues with how clang-format formats code from time to time. If the metric is "we have to be sure we won't experience such issues", how would we ever get to the point of enabling clang-format in an automated fashion?

I agree that there's a lack of enthusiasm for running clang-format over the entire codebase, largely because of the blame churn that it would induce I believe.

Thanks,

Colin

[David Michael]

FWIW, I've been slowly making all Pepper code go through clang-format:

https://code.google.com/p/chromium/issues/detail?id=345417

When I'm done, I'll turn on a presubmit check for those directories.

Let me relate a bit of my experience, in case it helps the discussion...

I've run in to a few annoyances along the way with what clang-format generates. The good news is, when I followed the instructions and filed bugs using this link, thakis@, nick@, djasper@ and others have been really responsive and helpful. All of the bugs I've filed have had a good outcome; sometimes I just learned something about the style guide.

• Sometimes clang-format still isn't perfect, but we can fix it.
• In other cases, (In Pepper, anyway) we're just going to adapt, because clang-format's style is at least as good as what we were doing before.

Given my experience, I don't feel ready to recommend this for all of Chromium yet. I also don't think we need to develop some metric to decide when to switch. I think it's probably going to be a sea change where eventually it will be obvious that it's the right thing to do for all of Chromium. (A bit like how the SVN->git transition has gone over time).

For now, if you're interested in using clang-format, I suggest you champion using it in the directories you own or work on. When you have problems, file bugs. "Think globally, act locally" ;-)

If a few people do that, then eventually the downsides of using clang-format everywhere will be obviously outweighed by the benefits.

-Dave

[John Abd-El-Malek]

We shouldn't be using the presence of incorrectly formatted code as a precedent :)

There's a difference between having 1% off a pattern of code formatted incorrectly because of developer/reviewer oversight vs having 100% forced to be incorrect because of a bug in a tool.

On Mon, Apr 28, 2014 at 11:36 PM, Daniel Cheng <dch...@chromium.org> wrote:

Do we typically ask developers go back and fix their style mistakes? We check in incorrect formatting all the time. I'm not feeling very creative, but a quick search turns up things like 6 space indents for wrapped lines, unnecessary double spaces, and an incorrectly indented private: label. In most of these cases, wouldn't we just let the next developer that touches these lines fix them?

As for the specific example you linked, I'm not sure there would be much more than that one-time churn. From what I can see, it tried to align most things, but even lines that are only slightly longer didn't get realigned (DIR_MANAGED_USERS_DEFAULT_APPS). But even this is something we could disable if we wanted (see AlignTrailingComments on http://clang.llvm.org/docs/ClangFormatStyleOptions.html).

What the tool did is that for each ifdef block in that file, it aligned the comments 2 spaces after the longest enum value. This means that in each block, as the longest enum name changes, all the values in that block will have their whitespace changed. 

[Adrienne Walker]

The story in cc/ has been identical to that of Pepper.

Folks have been super responsive in responding to bug reports, and
kind in teaching me about incorrect assumptions I made about the style
guide. I think we've had one serious bug (from reveman@, about a
"while(...);" statement where lint and clang-format disagreed), but
presubmit formatting in cc has gone quietly and with few complaints.
If anything, reviews are easier, because I don't have to think about
glaring formatting errors and can more easily review the more
important parts of patches.

I think clang-format is good enough for all of Chrome at this point.

[Peter Kasting]

On Tue, Apr 29, 2014 at 9:01 AM, David Michael <dmic...@chromium.org> wrote:

I've run in to a few annoyances along the way with what clang-format generates. The good news is, when I followed the instructions and filed bugs using this link, thakis@, nick@, djasper@ and others have been really responsive and helpful. All of the bugs I've filed have had a good outcome; sometimes I just learned something about the style guide.

I think this is good advice.  I want to publicly +1 that the clang-format folks are responsive and helpful.  I imagine they get the idea that people like me, and maybe John, hold only irritation and hostility toward them, but they do a good job, and I'm sorry that the message from some quarters sounds so negative.  The goal is noble, and we're getting there.

I think the best way for people who want clang-format used more broadly to proceed is to begin running it over the code they own and filing bugs for anything that seems inconsistent with existing Chromium style, even changes they would personally be OK with.  This ensures we raise every issue we see, and can make an informed decision on each one about whether we want to change or WontFix in response.  Once this happens in enough places, broader use of the tool is likely to Just Happen.

PK 

[Nick Carter]

On the question of measuring whether clang-format is "good enough": I've been doing this when I roll the binary, by keeping statistics that score how well clang-format is doing relative to the prevailing style, on the past months worth of commits. I've only been running them for two months, but the stats can be seen in the commit log for the clang-format binaries: https://src.chromium.org/viewvc/chrome/trunk/src/third_party/clang_format/bin/linux/clang-format.sha1?view=log

The first metric listed in the reports is the most interesting one for this discussion:

== How well did the old
== version of clang-format do?

47.49% commits were completely clang-format "clean" according to the
old version. This metric is an indicator of either clang-format
matching the prevailing style (which is more likely to happen with
smaller CLs), or people using clang-format as part of their workflow.

Between 78.11% and 86.58% of all committed lines (depending on how you
count -- the lower bound is very conservative) were clang-format
"clean" according to the old version. This metric is an indicator of
the deviations between prevailing style and the auto-formatting of
clang-format.

[Colin Blundell]

Thanks for the feedback, all. I will take David's advice.

Best,

Colin

[Vincent Scheib]

apps and extensions directories also use the cl format presubmit reminder; I've heard only good things from the team.

[Balazs Kelemen]

As a compromise, can we use it as a style checker, i.e. running right after upload and report results to retveild (like WebKit's style bot)?

Br,
Balazs

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

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[David Michael]

On Mon, May 5, 2014 at 11:10 PM, Balazs Kelemen <b.ke...@samsung.com> wrote:

As a compromise, can we use it as a style checker, i.e. running right after upload and report results to retveild (like WebKit's style bot)?

The problem with that is that clang-format isn't a style checker/linter. It reformats the code to one way that is (in the absence of bugs :) ) consistent with the style guide. Most code that hasn't been run through clang-format will not match that particular format, even if it is acceptable style. If it was just an FYI, so people could start seeing what it would do (and possibly filing bugs if it gets it wrong), that might be useful. But it should not at this stage be treated like a linter.