Closed Bug 1511181 Opened 5 years ago Closed 5 years ago

Reformat everything to the Google coding style (the actual change)

Categories

(Developer Infrastructure :: Lint and Formatting, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Let's use this bug for the actual change and keep bug 1188202 as a meta bug
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/6f3709b38781
Reformat everything to the Google coding style r=ehsan a=clang-format
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/4a8721265f7e
Quick fix for .hg-format-source that has an empty line. r=ehsan, a=aryx
I landed without any backout \o/
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
we added a tag on the changeset before the big patch:
https://hg.mozilla.org/mozilla-central/rev/2ac59ec6f8de
hg tag PRE_TREEWIDE_CLANG_FORMAT
LIST=$(find .  -maxdepth 1 -type d|grep -v "^.$"|grep -v /obj|grep -v "\.deps"|grep -v .hg|grep -v third_par|grep -v "\./\.")
echo $LIST
for f in $LIST; do
f2=$(echo $f|sed -e "s|\./||g")
cat <<EOF >> .hg-format-source
{"configpaths": [".clang-format", ".clang-format-ignore"], "pattern": "glob:$f2/**", "tool": "clang-format"}
EOF
done

for f in $LIST; do
        echo "$f";

    for i in {1..7}; do
            ./mach clang-format -p $f;
    done
    hg revert $( find $f -iname '*.java') || true
done
hg add .hg-format-source
hg commit -m "Bug 1511181 - Reformat everything to the Google coding style r=ehsan\n# ignore-this-changeset"
Blocks: 1511887
See Also: → 1513900
Congratulations. As of this bug you have made all blame logs utterly useless. :(
(In reply to Mark Straver from comment #9)
> Congratulations. As of this bug you have made all blame logs utterly
> useless. :(

First and foremost, your tone isn't appreciated here.  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 whining".  A more productive way to have asked about this would have been something along the lines of "With this change, is there any guideline available for how to use blame tools across the reformat changeset?".  See the difference there?  :-)

After studying the Etiquette Guidelines, I invite you to read the rest of my comment here where I explain how to deal with blames post this reformat: https://bugzilla.mozilla.org/show_bug.cgi?id=1513900#c4
First and foremost, my expression of frustration finding it impossible to do my research and work on the codebase due to the appearance of wilful destructive actions (also uplifted to ESR) is hardly "whining", and I likewise do not appreciate your tone and your obvious urge to put on the police cap with an immediate covered threat about etiquette "before posting further comments", implying I am somehow purposefully trying to cause a disturbance, which is absolutely not the case.

A more productive way to have responded to this would have been showing some understanding of this frustration and kindly pointing to your comment without requiring to drop what I'm doing to study the guidelines which I am already familiar with -- not like I'm new to Bugzilla, Ehsan.

If you feel there is somehow something that needs further discussion to be resolved to maintain a proper working environment for me on Bugzilla, then please feel free to e-mail me directly.
"wilful destructive actions"?  You've misunderstood the purpose of the uplift of the reformatting change to the ESR branch, and clearly haven't taken the time to read the original announcement which I linked to above.

As explained in <https://groups.google.com/d/topic/mozilla.dev.platform/VCLB1Lz4yXc/discussion>, this uplift to ESR was done in order to ensure that we can continue to backport our security fixes that impact the ESR branch for the duration of the long-term support cycle.  Basically, had we not done this, it would have been a tremendous amount of effort to backport our future fixes for security bugs back to the ESR branch (and we weren't sure if it would be possible to do it without introducing mistakes in the patches), and we can't take such a risk when it comes to security fixes.
You know as well as I do that I wasn't aware of your announcement at the time I voiced my frustration, which is what this refers to, at which time it -did- appear to be a wanton destructive act.

Please understand my frustration from the point of view of a maintainer of a Mozilla fork. Not only does it make it excessively difficult to use blame logs, it additionally causes the same problem you have attempted to avoid by porting this change to ESR. It will not be a tremendous amount of effort going forward to backport future sec fixes which I have been doing every cycle to keep our users safe and I am most definitely not happy about the prospect of having to deal with massively changed formatting and code style. Let's say I've gotten used to the Mozilla Code Style over the close to 10 years I've worked with Mozilla code ;)

I'd love to form a strategy to go forward from this point, perhaps adopting the same code style changes, but Bugzilla is hardly the right place. Please let me know where I should go to get help forming a plan of action.
"It will not be a tremendous" -> "It will now be a tremendous"
If you run clang-format-diff with the Google coding style on your patches, it should simplify the application of the patches.
Another way would to work with upstream to see the patches merged in-tree.
As mentioned in the original announcement (see under "What will happen to my local changes? Will I have to rebase everything?") we have a tool which should be helpful in rebasing any patches you had on the pre-reformatted version of the tree on top of the new tree tip.

Of course, maintaining a separate fork of Mozilla with a different coding style basically amounts to maintaining a 100MB+ patch on top of a giant code repository.  In my humble opinion, that is not a viable path going forward (you are of course welcome to disagree) and as such I really can't think of any workflow that isn't going to be hugely painful if that is a path you choose to take going into the future.  Just in order to give you enough information to make sure you can make a good decision, all of our attempts in this project have merely been focused on ensuring that people can easily port their patches which were based on the tree with the Mozilla C++ coding style to the tree with the Google C++ coding style, not the other way around.  So if you decide to take the opposite direction, you'd basically be on your own forever, and there won't be anyone upstream who would be able to help in case you run into issues.

About the blame logs, as I have already mentioned there are existing solutions which you can use and they will work well out of the box, so I don't think there is anything that would hold you back there.  For example we've already deployed our fixes to searchfox.org (see https://searchfox.org/mozilla-central/source/dom/base/nsINode.h for example, no sign of the reformatting there.)  Searchfox is open source https://github.com/mozsearch/mozsearch so you can deploy it for your own project too.  (Or file a bug to get ESR listed on it too if your code is similar enough to the ESR upstream...)

About personal affinity towards one coding style vs another one, I'm not going to enter a debate about that if you'll excuse me.  The announcement explains why we decided to have a unified coding style, and acknowledges that this is a contentious issue which not every single person will agree on.

If there are any other hiccups that you run into you're welcome to send me an email directly, there is a good chance that we have already thought about and handled the issue in one way or another so I may be able to help.  Good luck!
Thanks for the extra information. I don't think further communication will be needed, considering:

> So if you decide to take the opposite direction, you'd basically be on your own forever, and there won't be anyone upstream who would be able to help in case you run into issues.

To be frank, this help has been completely nonexistent in the past, so nothing much has changed there. We've initially been treated as insignificant, then treated as unwanted and actively been worked against, and even been accused of "destroying code" and "extorting" in this context. There has never been anything but either silence or a response that we should, in so many words, keep following m-c (and not maintain a fork) to have any assistance whatsoever. I do not believe this will change.

As such, it's clear that we have been, are, and will be on our own and I fully agree that a decision to keep a huge code patch like that around is not going to be a sustainable solution for ANY fork.
The only logical choice here will be to put in the extra work for applicable sec patches, and to define our own coding style for future development. Thankfully MozSec and Dan in particular have been helpful even within the limited freedom the sec team has (for obvious reasons).
Thanks for the additional pointers to searchfox which has been specifically fixed to work around this - I'll look into deploying it for our own use if the need arises.
Type: defect → task

Sylvestre, can you please add target milestone and update status flags? I need to know on which release this has landed. Thanks.

Flags: needinfo?(sledru)

I wish I could but the values aren't available anymore.
It landed in 65.

Flags: needinfo?(sledru)

Thanks, this is sufficient!

Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.