Closed Bug 1513900 Opened 1 year ago Closed 1 year ago

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

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox-esr60 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr60 --- fixed

People

(Reporter: ehsan, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

Like bug 1511181 but for ESR.
Depends on: 1513901
Over to Sylvestre to do the honours tomorrow.  :-)
Assignee: nobody → sledru
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
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.
(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
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.
(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.
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).
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.
(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.
(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
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.
You need to log in before you can comment on or make changes to this bug.