1513900 - Reformat everything on the ESR branch to the Google coding style (the actual change)

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[(no longer active)]

Like bug 1511181 but for ESR.

Comment 1

[(no longer active)]

Over to Sylvestre to do the honours tomorrow.  :-)

Comment 2

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/4bcb64fd8fa1

Comment 3

[Matt A. Tobin [:nsITobin]]

So was the intention of making sure that not only are all blame logs are useless but any and all code on the ESR branch is almost completely useless from a diff standpoint to those who are diverging?

This is selfish at best and destructive at worse. So when is the announcement that Mozilla Corporation was acquired by Alphabet? I want to mark my calendar.

Comment 4

[(no longer active)]

(In reply to Matt A. Tobin [:BinOC] from comment #3)
> So was the intention of making sure that not only are all blame logs are
> useless but any and all code on the ESR branch is almost completely useless
> from a diff standpoint to those who are diverging?
> 
> This is selfish at best and destructive at worse. So when is the
> announcement that Mozilla Corporation was acquired by Alphabet? I want to
> mark my calendar.

First and foremost, your tone isn't appreciated at all.  I do invite you to study https://bugzilla.mozilla.org/page.cgi?id=etiquette.html before posting further comments on Bugzilla, specifically paying attention to "No abusing people", as well as "No obligation".  I invite you to dial down your aggressive tone quite a bit before making any further comments on Bugzilla.

Once you've done that, you're welcome to take some time to study the announcement that was made about this change: https://groups.google.com/d/topic/mozilla.dev.platform/VCLB1Lz4yXc/discussion.  Specifically with regards to the issue with blame logs, if you use hg checkout the smart-annotate command added in bug 1508002, or if you use git checkout git-hyper-blame <https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/master/git_hyper_blame.py> and the integration work we did to support it in searchfox.org (bug 1507923).

I'll ignore the rest of your comments where you're crossing the line into being disrespectful and making snarky comments about the non-profit foundation that is supporting this open source project, please refrain from doing so in the future.  In my experience, when people choose to be aggressive and abusive, most developers just treat their comments as spam and ignore them.  I'm sure you didn't mean to be ignored, and the way to make that happen is to treat people the same way you would have liked to be treated yourself.

With respect,
Ehsan

Comment 6

[Jorg K (CEST = GMT+2)]

https://groups.google.com/forum/#!topic/mozilla.dev.platform/VCLB1Lz4yXc/discussion
To mitigate the impact on the developers who backport fixes to ESR, on Friday, December 14, we will also reformat the ESR60 code base.

===

I maintain the THUNDERBIRD_60_VERBRANCH on mozilla-esr60. On that branch I've backed out various patches and I've backported some patches. I've just tried to merge the reformatting into our branch and I get 10 files with a total of 28 merge conflicts I need to resolve manually. That's of course in code I'm unfamiliar with, so if I get it wrong, Thunderbird will malfunction or crash. I could just start to cry :-( - Any constructive suggestions to resolve this situation?

I hope that statement is in compliance with the Bugzilla etiquette.

Comment 7

[Matt A. Tobin [:nsITobin]]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #6)
> I maintain the THUNDERBIRD_60_VERBRANCH on mozilla-esr60. On that branch
> I've backed out various patches and I've backported some patches. I've just
> tried to merge the reformatting into our branch and I get 10 files with a
> total of 28 merge conflicts I need to resolve manually. That's of course in
> code I'm unfamiliar with, so if I get it wrong, Thunderbird will malfunction
> or crash. I could just start to cry :-( - Any constructive suggestions to
> resolve this situation?
> 
> I hope that statement is in compliance with the Bugzilla etiquette.

A random sampling shows only code format changes just making diff a nightmare.. But there doesn't seem to be any functional changes with this applied. I'd say it is very likely that you can just merge using yours and conform the files later on to Google/Clang style.

Comment 8

[Jorg K (CEST = GMT+2)]

Thanks for the comment, I'll have to take another look at the conflicts.

My thinking was to scrap the branch, start afresh and apply the 23 (backout) patches again that I have applied during the course of Thunderbird 60 ESR, many/most of which will just apply since they are not in C++ code. Those which don't apply I'd try to reformat using "format-source" (bug 1511594) which appears to be working now.

Re. your comment #5: On one hand I can understand the reformatting of ESR to facilitate backports, on the other hand, backports could have just reformatted the file in question as I've done here to backport formatted code to the unformatted branch:
https://hg.mozilla.org/releases/mozilla-esr60/pushloghtml/?changeset=aafc47a21cd3, changeset 5d79ca5eac09.

It's hard to judge which way causes less work in total since I don't know how much (quote) "outside mainstream mozilla-based development" is using mozilla-esr60. Obviously Thunderbird falls into this category. For own Mozilla internal use, this was the right thing to do, which you could call "selfish" as you did (no offense intended).

Comment 9

[Matt A. Tobin [:nsITobin]]

No, I get it. At the risk of being falsely accused of violating the etiquette guidelines again. It is unprecedented for such a major change to an established ESR branch (even if it isn't functional) that makes it difficult for everyone.

Just to add a slight ease for copypasta code reasons that likely won't matter anyway since mozilla-central constant refactoring makes clean patches difficult at best.

However, it is now sets a precedent that massive changes can happen on an ESR branch functionally or not and that is disappointing.

But I must be clear here, I am not calling for any action here. Hopefully, this comment won't be makred as spam like my last on the subject.

I do hope you get past this issue on your branch, Jorg, and I hope I was at least slightly helpful.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #8)
> Those
> which don't apply I'd try to reformat using "format-source" (bug 1511594)
> which appears to be working now.
You could also try to reformat your patch to the new CS using clang-format-diff.
This might just work.

> Re. your comment #5: On one hand I can understand the reformatting of ESR to
> facilitate backports, on the other hand, backports could have just
> reformatted the file in question as I've done here to backport formatted
> code to the unformatted branch:
As a lot of Firefox contributors are working on backport to esr, this would not have scaled.
With that work, we tried to make trivial for Firefox contributors for the rest of the ES60 cycle.

> For own
> Mozilla internal use, this was the right thing to do, which you could call
> "selfish" as you did (no offense intended).
Yeah, we knew that it could be a pain for downstream projects but there wasn't any perfect solution.

Comment 11

[(no longer active)]

(In reply to Jorg K (GMT+1) (urgent reviews and bustage fix only, Dec 22nd to Jan 1st) from comment #8)
> Thanks for the comment, I'll have to take another look at the conflicts.
> 
> My thinking was to scrap the branch, start afresh and apply the 23 (backout)
> patches again that I have applied during the course of Thunderbird 60 ESR,
> many/most of which will just apply since they are not in C++ code. Those
> which don't apply I'd try to reformat using "format-source" (bug 1511594)
> which appears to be working now.

format-source should work well out of the box, it will make rebases across the reformat commit succeed transparently.

If you maintain a patch set using mq however, that wouldn't work, and you would need a more sophisticated approach as described here: https://groups.google.com/d/msg/mozilla.dev.platform/bQg9f4D2OIE/dOluQNK3CAAJ

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks for the comments. In the end I just resolved all the merge conflicts manually keeping comment #7 in mind, that is, mostly using the branch content. I did it twice to minimise human error:

https://hg.mozilla.org/releases/mozilla-esr60/rev/3786ba0b991adca503c4fc9f21d00bb1e9ca6165
(I suggest not to click that since FF (still) doesn't handle large changesets well, see bug 1235321.)

Success try build here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00eb8ec8bea32530744da7110715ed29c42079e0
so at least it compiled.

This is the build:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr60&revision=7bd4edb13f5ea0c5936dbc224f1c178df8328eb7
I'll check for test failures when done.