What you need to know after the big reformatting

[Daniel Cheng]

All of Blink is now clang-formatted [1]. Things seem to still work.

- If you haven't done a `git pull` and a `gclient sync` since Monday, please do so before trying to rebase.

- Rebasing across the reformatting commit should Just Work™, assuming you `gclient sync`ed.

- Reverting across the reformatting commit mostly works (both Rietveld and `git revert`). If your patch added a new file, you'll have to manually resolve the conflict (by removing the added file) in the revert commit.

- `git cl format` is enforced by the presubmit.

- The reformatting commit has been added to hyperblame's ignore list, so `git hyper-blame` will skip over it (`git help hyper-blame`).

For the initial reformat, we haven't reflowed comments, because clang-format doesn't do a great job with them. We're looking for volunteers to help distribute the work here: https://docs.google.com/spreadsheets/d/1B8a7O2FP6L67qpkYRrdvEJkgVAUuHZwrKbGZdh7O4KU/edit#gid=0

If you’re looking at the blame view of a file in gitiles, replacing “master” with “1c8e1a7719e9d223cc84e838c9a31a0210f5878b^” in the URL will show you the blame from before the reformatting.

Let us know if you have any questions.

dcheng & thakis

1: https://crrev.com/1c8e1a7719e9d223cc84e838c9a31a0210f5878b

[Elliott Sprehn]

Congrats everyone who helped! Nearly 2 million lines processed in perhaps the largest patch ever. :)

[Kentaro Hara]

Thanks for driving this and make it happen finally! Congrats :D

--

Kentaro Hara, Tokyo, Japan

[Charles Harrison]

Wow! Great work :) Thanks everyone who put time into this.

[Chris Harrelson]

Amazing! Congratulations and thanks to everyone who helped with this effort.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Chris Harrelson]

I just rebased a large patch across the boundary. It worked flawlessly. Double amazing!

[Dimitri Glazkov]

\o/

:DG<

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

[Stefan Zager]

On Fri, Sep 30, 2016 at 7:39 PM, Daniel Cheng <dch...@chromium.org> wrote:

- Rebasing across the reformatting commit should Just Work™, assuming you `gclient sync`ed.

I'm having trouble merging origin/master to my existing branches.  Is merging supposed to work?  Is there any magic incantation?

[Primiano Tucci]

yes, I did try merging and is supposed to work as well.  the merge driver will reformat both sides.
what's happening for you? do you see the message from the clang format merge driver when the merge falls in 3 way merge mode?

[Daniel Cheng]

I talked offline with Stefan earlier, and we're not sure what's going on, but it may not have been related to the reformat: there were some random conflicts in non-Blink files, which shouldn't have been affected at all.

If anyone else experiences anomalous behavior, please let us know though. Thanks!

Daniel

[Peter Kasting]

On Fri, Sep 30, 2016 at 7:39 PM, Daniel Cheng <dch...@chromium.org> wrote:

For the initial reformat, we haven't reflowed comments, because clang-format doesn't do a great job with them. We're looking for volunteers to help distribute the work here: https://docs.google.com/spreadsheets/d/1B8a7O2FP6L67qpkYRrdvEJkgVAUuHZwrKbGZdh7O4KU/edit#gid=0

By "doesn't do a great job", I assume you meant that while clang-format will introduce new linebreaks to keep comment lines to 80 characters, it won't remove pre-existing linebreaks to completely rewrap the paragraph, so this:

// really long line, longer than 80 chars.

// line 2

Turns into this:

// really long line, longer than 80

// chars.

// line 2

Instead of this:

// really long line, longer than 80

// chars. line 2

That was the only problem I saw in the comment reflow patch I just posted up.

Is there a clang-format bug on this?

PK

[Charles Harrison]

I think that's the only case where clang-format doesn't handle comments well.

My guess is the design is intentional, because sometimes non 80 col wrapped linebreaks are there for clarity, or some sort of diagram-in-comment.

However, for a massive update like this, it would probably be better to remove this "feature" and just fix up the (smaller) number of cases that really need the existing line breaks.

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

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

[Nico Weber]

There's half a CL for reflowing in following lines here: https://reviews.llvm.org/D4345

However, you don't always want to reflow with the next line. Say for example you have

// paragraph 1 bla bla

// bla bla.

// paragraph 2 bla bla

// bla useful text.

If the first line of this comment gets longer, you want to reflow it into the second line, but you don't want to reflow that to the third line. Not to speak of numbered lists, ascii art, etc in comments. This is a fairly hard problem, which is why it's not done yet and why people have to format their comments manually.

 

PK

[Peter Kasting]

Yup.  I think in general reflowing continuously as long as all the lines in question were >80 chars to start with, and then stopping at the ends of any lines which weren't, probably gets you most of the way there, but it would still go wrong in certain cases.  And that would only aim to solve the cases I saw locally which were about reflowing existing landed comments, while the review you linked tries to tackle the harder case of "I had a legal comment and then added something in the middle".  I could imagine doing something crude with checking for trailing sentence breaks here but I dunno if clang-format wants to depend on ICU to detect those :)

Anyway, I mostly wanted to be sure (a) I wasn't missing possible bugs and (b) someone was at least trying to look at this.

PK

[Philip Jägenstedt]

The comment reflows will introduce conflicts that aren't handled automatically, right? I was trying to rebase a long branch in dom/ (fullscreen) and think the conflicts are due to comment changes.

If this is correct, maybe it's best to wait with widespread comment reformatting for 1-2 weeks?

[Nico Weber]

Yes, the comment reflow CLs are just regular manual changes. Were the conflicts difficult to resolve, or were there unusually many conflicts? Was this different from, say, the OwnPtr->unique_ptr change? I had expected that the comment changes should be fairly easy to merge manually, and that conflicts on them should be relatively rare (reflowing all comments touches a bit under 30k lines, while the main clang-formatting CL touched 900k lines, so comments are about 3% of the code).

Depending on what the specific problem is, we might be able to do something about it (for example, ask people who have bigger pending changes in some directories to sign up to reflow comments in that dir, so that they can do that in a way that interacts well with their pending change, or rate-limit changes in busy directories, or ...). I can't think of a scenario where waiting 1-2 weeks changes much except delaying merge conflicts by 1-2 weeks though (?)

(I'm guessing this is https://codereview.chromium.org/2202493002/ 

[Aleks Totic]

On Fri, Sep 30, 2016 at 7:39 PM, Daniel Cheng <dch...@chromium.org> wrote:

> - The reformatting commit has been added to hyperblame's ignore list, so
> `git hyper-blame` will skip over it (`git help hyper-blame`).

Any chance of incorporating hyper-blame into codesearch? git blame is
now useless.

Aleks

[Chris Harrelson]

Not useless. You can do a blame in codesearch, then edit the URL to add a tilde (~) at the end of the commit hash. 

Aleks

[Elliott Sprehn]

It's pretty painful though as the code continues to move around having to jump over the mass reformat will be annoying. Getting gitiles to support hyper blame should block the next step in the mega reformat.

[Chris Harrelson]

On Mon, Oct 3, 2016 at 4:42 PM, Elliott Sprehn <esp...@chromium.org> wrote:

It's pretty painful though as the code continues to move around having to jump over the mass reformat will be annoying. Getting gitiles to support hyper blame should block the next step in the mega reformat.

Agreed. Just pointing out a workaround. Should have phrased a bit better..

[Nico Weber]

As mentioned in the OP, s/master/1c8e1a7719e9^/ in the URL will get you the pre-reformat blame. To get this in one click, open chrome's bookmark bar, right-click, "Add Page…", then enter "reblame" for "Name" and "javascript:document.location = String(document.location).replace('/master/', '/1c8e1a7719e9^/')" for "URL:". (The JS wizzes out there can probably golf that down more.)

Even without this, I learned that there's a pretty fast 2-click method to get at that. Say you're at e.g. https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebKit/Source/core/layout/LayoutHTMLCanvas.cpp

1. Click "[blame]" on a "Blink Reformat" line, e.g. line 41

2. On the page that opens, click "blame^" on the same line

(It'd be nice if there was a "^" that did the same in step 1.)

I don't disagree that hyperblame integration for gitiles is a nice-to-have, but it's pretty easy to get the blame output you want still, even in the web interface, imho.

[Elliott Sprehn]

This isn't about master though, but about the code continuing to get moved around post reformat, and it's only going to get worse once we rename every file to foo_bar.cc instead of FooBar.cpp. I think Gitiles integration is not just nice to have, but a requirement of that last step. :)

- E

[Nico Weber]

No argument that hyperblame integration in gitiles is going to become more useful over time :-) I'm just saying that blame right after the big reformat isn't as bad as Aleks suggests.

(I think renaming .cc to .cpp isn't visible in blame output at all, but eg changing m_foo to foo_ is, as are many other mechanical changes.)

[Matt Giuca]

Here's the bug for porting hyperblame to gitiles:

https://code.google.com/p/gitiles/issues/detail?id=100

If anyone wants to work on it, go for it and let me know so we can discuss the approach. I agree it would be super useful!

[Philip Jägenstedt]

Yep, you guessed the right CL. At first I didn't know if the conflicts were due to a bug in the merge driver or a real conflict. Once I was sure it was because of the comment reflow it was easy to resolve, blindly throwing away one side and reflowing comments again.

I didn't have OwnPtr->unique_ptr conflicts, so I can't compare. I now think postponing comment reflowing isn't necessary.

[Nico Weber]

On Fri, Sep 30, 2016 at 10:39 PM, Daniel Cheng <dch...@chromium.org> wrote:

All of Blink is now clang-formatted [1]. Things seem to still work.

- If you haven't done a `git pull` and a `gclient sync` since Monday, please do so before trying to rebase.

- Rebasing across the reformatting commit should Just Work™, assuming you `gclient sync`ed.

- Reverting across the reformatting commit mostly works (both Rietveld and `git revert`). If your patch added a new file, you'll have to manually resolve the conflict (by removing the added file) in the revert commit.

- `git cl format` is enforced by the presubmit.

- The reformatting commit has been added to hyperblame's ignore list, so `git hyper-blame` will skip over it (`git help hyper-blame`).

For the initial reformat, we haven't reflowed comments, because clang-format doesn't do a great job with them. We're looking for volunteers to help distribute the work here: https://docs.google.com/spreadsheets/d/1B8a7O2FP6L67qpkYRrdvEJkgVAUuHZwrKbGZdh7O4KU/edit#gid=0

Of 165 directories, only 13 are left. Thanks to everyone who helped so far! The remaining available directories are under core/css and core/editing. If some more people pitch in, we might be able to complete this today.

[Nico Weber]

On Wed, Oct 5, 2016 at 6:08 PM, Nico Weber <tha...@chromium.org> wrote:

On Fri, Sep 30, 2016 at 10:39 PM, Daniel Cheng <dch...@chromium.org> wrote:

All of Blink is now clang-formatted [1]. Things seem to still work.

- If you haven't done a `git pull` and a `gclient sync` since Monday, please do so before trying to rebase.

- Rebasing across the reformatting commit should Just Work™, assuming you `gclient sync`ed.

- Reverting across the reformatting commit mostly works (both Rietveld and `git revert`). If your patch added a new file, you'll have to manually resolve the conflict (by removing the added file) in the revert commit.

- `git cl format` is enforced by the presubmit.

- The reformatting commit has been added to hyperblame's ignore list, so `git hyper-blame` will skip over it (`git help hyper-blame`).

For the initial reformat, we haven't reflowed comments, because clang-format doesn't do a great job with them. We're looking for volunteers to help distribute the work here: https://docs.google.com/spreadsheets/d/1B8a7O2FP6L67qpkYRrdvEJkgVAUuHZwrKbGZdh7O4KU/edit#gid=0

Of 165 directories, only 13 are left. Thanks to everyone who helped so far! The remaining available directories are under core/css and core/editing. If some more people pitch in, we might be able to complete this today.

A belated follow-up here: As of last week Friday (7 days ago), all comments in blink have been reformatted and all of blink (including comments) is now clang-formatted. Blink now uses Chromium's top-level .clang-format file without any changes.

A huge shoutout to the blink community who helped format over 35k lines of comments in less than 5 working days. Some stats: 18 authors touched 2663 files, with 35k lines of comments touched, in 97 commits. The detailed breakdown (ordered by percentage of total lines touched):

54.0%     tha...@chromium.org 19029 lines 1678 files

19.8%        e...@chromium.org  6977 lines  229 files

 5.7%     dch...@chromium.org  2027 lines  105 files

 5.2% cshar...@chromium.org  1846 lines   98 files

 5.1%      tk...@chromium.org  1792 lines  152 files

 3.7%   pkas...@chromium.org  1311 lines  116 files

 2.3%      me...@chromium.org   828 lines   93 files

 0.9%     gui...@chromium.org   308 lines   36 files

 0.7% cbies...@chromium.org   250 lines    3 files

 0.7%      su...@chromium.org   236 lines   60 files

 0.7%          ru...@opera.com   260 lines   25 files

 0.5%       xl...@chromium.org   193 lines   21 files

 0.2%    nhi...@chromium.org    84 lines   18 files

 0.1%     mca...@chromium.org    33 lines    6 files

 0.1%      pe...@chromium.org    41 lines   13 files

 0.1%       ha...@chromium.org    28 lines    7 files

 0.0%   skyo...@chromium.org     9 lines    1 files

 0.0%   jrum...@chromium.org     4 lines    2 files

Over 40% of changes were done by people not-dcheng not-me, so thanks to everyone who chimed in -- and a special thanks to eae who took on core/layout.

[Ojan Vafai]

\o/

[Anton Vayvod]

Is reformatting of long one-line indented blocks supposed to work? Will the following:

if (condition)

  very very very very long statement longer than 80 chars;

become this:

if (condition) {

  very very very very

      long statement longer

          than 80 chars;

}

I just found plenty of examples that became multiline but don't have {} added. For instance, this one.

Is anyone working on fixing this? Do we leave them as is / reformat when we touch the files?

[Charles Harrison]

I've just been fixing these when I touch the affected files.

[Daniel Cheng]

It's a known issue: https://llvm.org/bugs/show_bug.cgi?id=26215. It's always bugged me that clang-format can't do this automatically, but it's non-trivial to implement. I do plan on working on this at some point; however, if anyone want to volunteer for this, I'd be happy to pass along the implementation strategy that I discussed with djasper@.

For now, I prefer to fix them as check-webkit-style warns about them (e.g. when those lines are modified as a natural part of another CL). If it makes another refactoring harder though (e.g. a bulk move of files), it might be worth it to proactively fix it.

Daniel

[Anton Vayvod]

Are there any other issues like these?

[Daniel Cheng]

Not that I'm aware of.

Daniel

[Victor Costan]

Do we have anything similar planned for layout tests?

Apologies if this was answered somewhere and I didn't catch it!

[Nico Weber]

You mean reformatting the layout test js source files? If so: I wasn't planning on doing that soon, but clang-format can in theory format .js files as well.

[Emil A Eklund]

On Tue, Oct 25, 2016 at 7:12 AM, Nico Weber <tha...@chromium.org> wrote:
> You mean reformatting the layout test js source files? If so: I wasn't
> planning on doing that soon, but clang-format can in theory format .js files
> as well.

Traditionally we haven't enforced the style guide for layout tests and
they tend to vary quite a bit depending on source, type, and the
engineer that created them.