Upcoming changes to our C++ Coding Style

[Ehsan Akhgari]

(In case the formatting doesn't survive transit, a copy of this note can be
found here:
https://docs.google.com/document/d/1CTaWucldHxEri5BUB1kL4faF4prwPlBa31yHFd-l8uc
.)


TL;DR

To improve developer productivity, we are planning to a) start
automatically formatting all Gecko C++ code with clang-format, and b)
switch to the Google C++ style
<https://google.github.io/styleguide/cppguide.html> and retire the Mozilla
C/C++ coding style
<https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style>.
These changes are being tested and verified with a small part of the
codebase before rolling out to the entire tree.

Background

Mozilla has been moving towards using more automated tools for checking and
rewriting code. For example, eslint <https://eslint.org/> (JS), checkstyle
<https://github.com/checkstyle/checkstyle> (Java), flake8
<http://flake8.pycqa.org/en/latest/> (Python) are now Tier-1 in the CI. We
also deployed C/C++ static analysis as part of the development process.

In parallel, tooling to reformat C & C++ is now used in a wide variety of
projects like Chromium, LLVM and MongoDB. Mozilla has historically had a
coding style but it was never really enforced through tooling. In addition,
the Mozilla style is incomplete and used only in our projects; by contrast
the Google style is very complete, well supported in automated tooling, and
widely used by projects inside and outside of Google such as Chromium,
Tensorflow, and MySQL.

Because of the lack of checks and automation, we now have significant
inconsistencies in the style of our codebase. On top of that, we have been
spending a lot of time talking about and adjusting whitespace. This wastes
human time when writing, reviewing, and discussing code.

Last but not least, being able to automatically format whitespace in our
C/C++ code is helpful for other types of automated rewrites so we can fix
wide-scale problems in our codebase using static analysis tools. Right now
such fixes are difficult or impossible because of inconsistencies in the
Mozilla C++ coding style and lack of support in clang-format and other
tools. Addressing this issue is the first step in unblocking those future
improvements.

Goals

During patch authoring, code reviews, and mailing list discussions, Mozilla
developers have traditionally spent a lot of time on minor issues that
could easily be fixed through automated processes. Our Engineering
Operations and Product Integrity groups have helped to improve developer
productivity in this area through tools such as static analysis
<https://static-analysis.moz.tools/#/stats>, linting via Phabricator
reviewbot <https://phabricator.services.mozilla.com/p/reviewbot/>, and codebase
reformats
<https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format>
using clang-format. We will build on this foundation by applying coding
style checks and automated rewrites throughout the authoring process
(pre-commit, commit, review, landing).

Details

We’re announcing the following changes to our coding style and its
enforcement through our commit policy:


1.

We will check conformance with coding style rules upon commit, review,
and landing so that issues can be easily addressed or fixed through
automation. The preference will be to enforce style issues as early as
possible (before landing) to avoid late surprises.

2.

We will migrate to the Google C++ coding style to encourage more
consistent code and re-use a wider array of existing tools. As part of
this migration, we will retire the existing Mozilla C/C++ Coding Style. The
intention behind retiring the current Mozilla style is to just stop
maintaining it as an active coding style going forward, but the
documentation for this coding style will be kept intact.



1.

We will automatically enforce restrictions on formatting of whitespace
(such as indentation and braces). For stylistic features other than that
(such as naming of functions and #include order), Google C++ style will be
permitted but not initially enforced, and consistency with surrounding code
should take precedence.


These changes have been approved both by Firefox senior engineering
leadership and the Firefox Technical Leadership Module
<https://wiki.mozilla.org/Modules/Firefox_Technical_Leadership>. This work
is being tracked in bug 1188202
<https://bugzilla.mozilla.org/show_bug.cgi?id=1188202>.

When?

On Monday this week, we pushed a patch
<https://bugzilla.mozilla.org/show_bug.cgi?id=1204606> to an integration
branch to rewrite a section of the tree (dom/media) in the Google C++ style
using clang-format. This first step is intended to be a smoke test step,
designed to test the process of doing the rewrite and test the waters in a
small controlled environment with a small team of developers who have
volunteered to help us test this change. We haven’t found any major
surprises yet.

Assuming that everything goes well, on Friday, November 30, we will push a
patch to mozilla-central and the integration branches syncing into it in
order to rewrite the entire tree using clang-format with the Google C++
Coding Style.

We want to do this work right before a merge (when nightly version moves to
beta) to limit the impact on the beta uplift process. Conveniently, this
happens to be the Friday just before the Mozilla All Hands when a lot of
Mozilla developers will be traveling, so we are hoping that would be a less
busy time for landing this change.

To mitigate the impact on the developers who backport fixes to ESR, on
Friday, December 14, we will also reformat the ESR60 code base.

Reformatting Code Locally

To reformat code locally

On a directory or a file specified by <path>:

$ ./mach clang-format -p <path>

On a modified tree (where hg diff or git diff) return a non-empty output:

$ ./mach clang-format

See the documentation for more information
<https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format>
. There are integrations available for most popular code editors allowing
developers to easily format the code they’re working on at the time they’re
making changes to it. The documentation links to several integrations for
the popular editors.

Rationale Behind Current Plan

We understand that switching coding styles can be a contentious choice, and
not one that everyone may be happy about initially. Many people would
agree that having a consistent style across the code base is valuable, but
that’s not where we are now. To a great degree, that has been caused by
lack of good tools to enforce our existing coding style. By contrast,
there is tooling to enforce quite a bit of the Google C++ Style, and this
is why we would like to convert entirely to this coding style -- perhaps
with some exceptions for Mozilla-specific constraints. That is something
that we are going to look at seriously going forward.

We have decided here to start this conversion by converting the whitespace
related portions of our coding style mechanically using clang-format as a
first step. Of course, other strategies for this conversion are also
possible, such as waiting until such a time when we have the tooling ready
for switching wholesale to the Google C++ Style. But we have decided that
by allowing the usage of the Google C++ Style in new areas, we are taking
an incremental step towards that ultimate goal by allowing (not requiring)
people to write code immediately after our switch that is consistent with
our newly adopted coding style and can be verified as such using tools. The
intention here is not that people intermix Google C++ Style and Mozilla
Style code on a per function or even a per file basis but rather that they
be free to do so when starting new, self-contained work, or to convert an
entire module.

Providing Feedback

This post is intended as an announcement, but we do welcome your feedback
on this, both at a high level and at the technical level. For high level
feedback please reach out to tl...@mozilla.com and CC myself. For technical
feedback (e.g. bugs about the conversion process) please file bugs under bug
clang-format <https://bugzilla.mozilla.org/show_bug.cgi?id=1188202>.

Last but not least, coding style is a living organism. This change doesn’t
mean that the coding style will remain forever this way; to the contrary
this will give us the opportunity to easily change our coding style and
apply the change on the code base.

I’d like to extend my gratitude to many who have helped getting us to this
point, including Sylvestre Ledru, Peter Saint-Andre, Andi-Bogdan Postelnicu
and Birunthan Mohanathas.

Best regards,

Ehsan

FAQ:

Can I see what it looks like?

With an up-to-date repository, you can use

$ ./mach clang-format -p <path>

A formatted repository is available on github:
https://github.com/sylvestre/gecko-dev/ to visualize the changes.

Is it going to break something?

Of course, every software has bugs but all the testing that we have done
didn’t show any sign of regression.

You will break the blame on VCS with this change

Yes and no. Of course, just like many tree-wide mass changes in the past
(e.g. the MPL2 header update), this will remain in the log.

Mercurial and Git both support a -w argument to ignore whitespace with
annotate/blame.

In addition, modern versions of Mercurial have `hg annotate --skip
<revset>` which allows you to specify a revset used to select revisions to
skip over when annotating.

Last but not least, we will tag the changeset’s commit message with
“skip-blame” so that Mercurial would automatically ignore the reformat
changeset for blame operations.

Why doing that in a single patch instead of doing that file per file or
module per module?

We are doing that into a single patch for several reasons:

-

We will be done quickly
-

We will have a single revision to ignore in the annotate/blame
-

We can tag this magic revision to run some tasks on it (local reformat
to avoid conflicts)


clang-format doesn’t add missing braces, is that normal?

clang-format only does whitespace changes. It doesn’t add or remove code.

However, this can be done through ./mach static-analysis. For example:

-

./mach static-analysis check --check
google-readability-braces-around-statements --fix js/src/wasm/


This checker is enabled at review phase
<https://static-analysis.moz.tools/#/check/clang-tidy.readability-braces-around-statements>
in Phabricator. Note that the --fix argument here means that the
static-analysis job will automatically rewrite the local tree in order to
add braces around statements anywhere it finds those issues, no need to do
it manually!

Does everything has to be formatted?

Yes, all of the C/C++ code that we maintain in mozilla-central has to be
formatted.

There are cases, like some structure declarations, for which we want to
ignore the formatting. Example:

https://hg.mozilla.org/mozilla-central/rev/7807742373e1

In that case, add:

// clang-format off

code not to be formatted

// clang-format on

My component has some very specific needs with very good reasons, can I
customize the coding style?

No, going forward we are planning to unify all of our code (excluding
third-party code and any potential code owned by us that is developed
outside of mozilla-central) under the same coding style. If there are
individual blocks within your code that require custom formatting for good
reasons, please see the previous question for how to turn clang-format off
for specific sections.

If you see issues about the style change that make you unhappy and you
think affect the code in the areas you work on in negative ways, please
consider spending some time to try to get used to the change. But if after
some time the change still seems inferior compared to before please raise
the issues and we will work to figure out how to address those problems to
the best of our ability.

Can I ignore just a file?

If you have a good reason, yes. For example, in some cases, we have some
tooling which parses the C/C++ code with a very specific format. For this,
just add it into .clang-format-ignore
<https://dxr.mozilla.org/mozilla-central/source/.clang-format-ignore> file
with a comment explaining why it should be ignored.

Please note that examples of what constitutes a good reason for excluding
single files would be external tools needing to parse the file in specific
ways, or cases where clang-format can’t deal with the input (such as bug
1342657 <https://bugzilla.mozilla.org/show_bug.cgi?id=1342657>). Examples
of what does not constitute a good reason would be personal preferences.

What about variable naming convention? What about function/class/etc.
naming conventions?

With this change, we are not aiming to enforce any naming conventions at
this time.

What about third party code?

Third party code will not be touched. See this list:
https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt

This is mirrored here for clang-format to ignore these directories:

https://dxr.mozilla.org/mozilla-central/source/.clang-format-ignore

If you are aware of third-party code developed outside of mozilla-central
that isn’t listed here, please file a bug as soon as possible to get it
added to this list.

Why are we switching to the Google C++ coding style?

The current Mozilla C++ style
<https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style>
is not well supported by tools (in particular clang-format) and it has
been difficult
to <https://reviews.llvm.org/D37979> upstream patches
<https://reviews.llvm.org/D12921#247649> to clang-format to handle the
Mozilla style. By contrast, the Google C++ style used in a number of
projects (parts of NSS
<https://hg.mozilla.org/projects/nss/file/tip/cpputil/.clang-format>, MySQL
<https://dev.mysql.com/doc/dev/mysql-server/latest/PAGE_CODING_GUIDELINES.html>,
Android
<https://github.com/search?q=org%3Aaosp-mirror+BasedOnStyle&type=Code>,
Tensorflow
<https://github.com/tensorflow/tensorflow/blob/master/tensorflow/.clang-format>,
and many others
<https://github.com/search?q=BasedOnStyle%3A+Google&type=Code>) and well
supported in clang-format, linters
<https://raw.githubusercontent.com/google/styleguide/gh-pages/cpplint/cpplint.py>,
and clang-tidy checks
<https://clang.llvm.org/extra/clang-tidy/checks/list.html>. Defining and
maintaining our own C++ coding style probably isn’t the best use of our
time, and switching to the Google style will help us improve the
productivity of our developers, make onboarding easier, and pave the way
for more automation of trivial but time-consuming code fixes.

The feature foo from the coding style is missing in clang-format, what
should I do?

Going forward, our precise coding style will be the result of formatting
our tree with clang-format, so by definition our code, when formatted with
clang-format, will be following our new coding style.

In cases where you have found bugs in how clang-format implements the
Google C++ Style, please report those issues to the LLVM project:
https://bugs.llvm.org/. When the fixes are available in new versions of
clang-format, we’ll pick up the resulting reformatting automatically
through version upgrades.

In case you have feedback on the Google C++ Style Guide itself, please
consider filing an issue/PR in the upstream repository
<https://github.com/google/styleguide>.

How will we be sure we don’t regress?

For a while we’ve been running clang-format linting checks at review time
on Phabricator in shadow mode
<https://static-analysis.moz.tools/#/check/clang-format.lint> (executed but
not published). Over the next few weeks, we will enable verifications at
review phase on on Phabricator.

In parallel of that, we will offer hooks that can be enabled locally to
check for formatting. We’re hoping to enable a hook on hg.mozilla.org to
automatically reject improperly formatted commits soon after that. This
will prevent badly formatted code from being pushed to hg.mozilla.org in
the first place.

What will happen to my local changes? Will I have to rebase everything?

In order to mitigate the impact for pending changes, we developed a
mercurial extension called hg format-source
<https://pypi.python.org/pypi/hg-formatsource> to locally reformat the
local changes before the big patch is pulled. This will fix most of the
conflict issues.

We will share more details later.

For git users, we are working on providing a similar script based on the
one developed by the Mongo project
<https://github.com/mongodb/mongo/blob/8b66399b8536dc3120be5444b81ba51d4b8a95e4/buildscripts/clang_format.py>
.

Playing with clang-format, I noticed changes in the output between
versions, how will we manage that?

Because clang-format evolves, this is true that the output can change from
a version to another. This is why ./mach clang-format downloads an specific
artifact from the taskcluster CI. At this date, we are using clang-format
version 7.

As the Phabricator review bot is using ./mach clang-format to run the
analysis, the same version is used.

The version of the LLVM tools used for this purpose is defined in-tree for
Linux
<https://searchfox.org/mozilla-central/source/build/build-clang/clang-tidy-linux64.json>,
Mac
<https://searchfox.org/mozilla-central/source/build/build-clang/clang-tidy-macosx64.json>
and Windows
<https://searchfox.org/mozilla-central/source/build/build-clang/clang-tidy-win64.json>
.

What will happen to the downstream consumers of the ESR repository after
ESR60 is rewritten with clang-format?

We are aware of the impact of this plan on Firefox derivatives but we think
it is worth it the pain. The goal behind rewriting the ESR branch is to
make it easier to uplift patches from trunk onto the ESR branch, and this
change should make things easier for downstream consumers creating Firefox
derivatives based on the ESR60 codebase.

Such consumers still have the capability to run the following pseudo code
in the derivative project to generate up to date patches if they have local
modifications on top of ESR. In the example below, fx-esr60-reformatted is
the SHA1 hash of the Mercurial changeset that has rewritten the ESR60
repository with clang-format. This pseudo code shows the necessary steps
if the local modifications are applied on top of fx-esr60-reformatted.


-

# Rewrite the locally modified tree
./mach clang-format -p <path of the file touched>
-

# Generate a diff of the local changes against
hg diff -r [fx-esr60-reformatted]

[Jean-Yves Avenard]

Hi

> On 21 Nov 2018, at 3:54 am, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>
>
> You will break the blame on VCS with this change
>
> Yes and no. Of course, just like many tree-wide mass changes in the past
> (e.g. the MPL2 header update), this will remain in the log.
>
> Mercurial and Git both support a -w argument to ignore whitespace with
> annotate/blame.
>
> In addition, modern versions of Mercurial have `hg annotate --skip
> <revset>` which allows you to specify a revset used to select revisions to
> skip over when annotating.
>
> Last but not least, we will tag the changeset’s commit message with
> “skip-blame” so that Mercurial would automatically ignore the reformat
> changeset for blame operations.

I’ve found the Google’s depot_tools hyper-blame particularly useful here.

It takes a .git-blame-ignore-revs file containing the list of commits to ignore.

$ cat .git-blame-ignore-revs
abd6d77c618998827e5ffc3dab12f1a34d6ed03d

That’s with Sylvestre single commit changing dom/media (hg SHA1: 0ceae9db9ec0be18daa1a279511ad305723185d4)

$ git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
$ export PATH=$PATH:$PWD/depot_tools

now git hyper-blame will behave in the same fashion as git blame, but ignore that particular commit.

I’m guessing we could make this .git-blame-ignore-revs part of the tree, assuming though everyone must use git-cinnabar.

Jean-Yves

[Ehsan Akhgari]

On Thu, Nov 22, 2018 at 4:07 AM Jean-Yves Avenard <jyav...@mozilla.com>
wrote:

Indeed. Kats was asking me about how we can possibly support skipping over
these commits in searchfox.org, and I pointed this tool to him as well.
I'm hoping that he would be able to find a way to integrate hyper-blame
with searchfox so that more people can benefit from this. (I am not sure
how frequently people run blame tools locally...)

For Mercurial, "# skip-blame" is just a text token that Mercurial's revset
syntax can parse out of commit messages. The revset syntax also supports
reading external files. Felipe, gps and I talked a bit about adding a
similar .hg-blame-ignore-revs file in the tree which can contain Mercurial
sha1 changeset IDs for the rewrite commits to be skipped over, and people
would be able to use the file through an alias that can be configured in
~/.hgrc (possible to set it up via ./mach bootstrap). Not sure if Felipe's
investigations have lead to results yet.

> I’m guessing we could make this .git-blame-ignore-revs part of the tree,
> assuming though everyone must use git-cinnabar.
>

Indeed we can. I would be happy to review a patch. I think the file can
contain both the cinnabar and the gecko-dev sha1 variants of the rewrite
commit(s), since sha1s that are non-existent in a repo will just be ignored.

Cheers,
--
Ehsan

[Felipe G]

On Thu, Nov 22, 2018 at 3:07 PM Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> Felipe, gps and I talked a bit about adding a similar
> .hg-blame-ignore-revs file in the tree which can contain Mercurial sha1
> changeset IDs for the rewrite commits to be skipped over, and people would
> be able to use the file through an alias that can be configured in ~/.hgrc
> (possible to set it up via ./mach bootstrap). Not sure if Felipe's
> investigations have lead to results yet.
>

Indeed they have. I'm working on this in two separate bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=1508002 is implementing the
same functionality in mercurial from`git hyper-blame`, and
https://bugzilla.mozilla.org/show_bug.cgi?id=1508324 will be used to
generate the list of past changesets

Hope to have these wrapped up soon, and I'll write about them once they're
done. One thing to mention in advance is that gps asked to not use the word
"blame" as the Mercurial project doesn't like the negative connotations
that it brings, so I'm leaning towards `hg smart-annotate` and "#
ignore-changeset" for the string to be included in the commit message. Let
me know if anyone has thoughts about these (or leave comments in the bug)

Felipe

[Botond Ballo]

Hi,

I'd like to ask a couple of clarifications about process here:

* Is clang-format invoked automatically as part of a pre-commit hook,
or something like that?
* If not, should we be invoking it manually before submitting a patch?
* If we submit a patch without having invoked it, will violations be
caught in automation? Will they trigger a backout?

Thanks,
Botond

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Sylvestre Ledru]

Le 05/12/2018 à 00:22, Botond Ballo a écrit :
> Hi, I'd like to ask a couple of clarifications about process here: *
Is clang-format invoked automatically as part of a pre-commit hook, or
something like that?

We have been working on that. There is a pending patch
https://bugzilla.mozilla.org/show_bug.cgi?id=1507007
We will enable it as part of the ./mach bootstrap.

> * If not, should we be invoking it manually before submitting a patch?

Yes, I think this should be done as part of the developer workflows
now.  Each developer’s local development environment is different but
we’re hoping that for many people this actually shouldn’t mean running
./mach clang-format manually.  The recommended setup is to integrate
clang-format with your code editor using one of the integrations
documented here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format#Editor_Integrations

If you have tips to share about editor integrations, we’d appreciate
keeping that page updated!

And note that if you submit patches to Phabicator for review, a review
bot will automatically check your patches for formatting mistakes, and
if it detects them will provide a link to a patch that can be applied
locally to fix the issues.

> * If we submit a patch without having invoked it, will violations be
caught in automation? Will they trigger a backout?

We have been discussing about that. We don’t think we should backout a
C++ patch because of a missing space or line return and for now we
should focus on helping more developers update their local setup to run
clang-format as part of their local development workflow. Developing
local commit hooks, asking people to enable editor integrations and the
Phabricator review bot are efforts in this category.
In the meantime, we will be running a bot weekly to reformat the
mistakes and add the changeset into the ignore lists.
But in the long run this won’t be sustainable, so once we gain
confidence that a good number of developers have successfully integrated
clang-format into their local workflow, we will look into enabling a
Mercurial hook on hg.mozilla.org to reject misformatted code upon push
time.  That will be the ultimate solution to help ensure that our code
will be properly formatted at all times.

Ehsan & Sylvestre

[Botond Ballo]

On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru <sle...@mozilla.com> wrote:
> In the meantime, we will be running a bot weekly to reformat the
> mistakes and add the changeset into the ignore lists.
> But in the long run this won’t be sustainable, so once we gain
> confidence that a good number of developers have successfully integrated
> clang-format into their local workflow, we will look into enabling a
> Mercurial hook on hg.mozilla.org to reject misformatted code upon push
> time. That will be the ultimate solution to help ensure that our code
> will be properly formatted at all times.

Have you considered something like running clang-format automatically
during landing (i.e. as part of what Lando does to the patch)? That
seems like it would achieve the best of both worlds, by not placing
any requirements on what developers do locally, while also ensuring
the tree is always properly formatted without cleanup commits.

Cheers,
Botond

[Gregory Szorc]

I chatted with Sylvestre earlier today. While I don't want to speak for
him, I believe we both generally agree that the formatting should happen
"automagically" as part of the patch review and landing lifecycle, even if
the client doesn't have their machine configured for formatting on save.
This would mean that patches are either:

a) auto-formatted on clients as part of being submitted to Phabricator
b) updated automatically by bots after submission to Phabricator
c) auto-formatted by Lando as part of landing

Lando rewrites/rebases commits as part of landing, so commit hashes already
change. So if auto-formatting magically occurs and "just works" as part of
the review/landing process, there should be little to no developer
inconvenience compared to what happens today. i.e. developers don't need to
do anything special to have their patches land with proper formatting.

[Andrew Halberstadt]

I think we should implement a) and do the formatting prior to submission.
This prevents us from wasting reviewer time on format issues, and also
makes sure that "what you see in phab, is what gets landed".

[Mike Hommey]

On Fri, Dec 07, 2018 at 03:08:51PM -0500, Andrew Halberstadt wrote:
> I think we should implement a) and do the formatting prior to submission.
> This prevents us from wasting reviewer time on format issues, and also
> makes sure that "what you see in phab, is what gets landed".

Also, that would make it clear if weird formatting would entirely
be due to clang-format (in which case, we should probably file bugs),
and not a person's doing.

Mike

[Ehsan Akhgari]

Previously I had shied away from ideas similar to (a) or (b) since I wasn't
sure if that would be what developers would expect and accept, given that
it would cause the change the reviewer sees to no longer match their local
change, which could cause hicups e.g. when trying to find the code that a
reviewer has commented on locally, or when rebasing on top of a newer
version of mozilla-central after Lando has landed your patch (which may
cause conflicts with your local patch due to formatting differences).

Do we think these types of issues aren't something we should be concerned
about?

Thanks,
Ehsan

On Fri, Dec 7, 2018 at 3:09 PM Andrew Halberstadt <ah...@mozilla.com> wrote:

> I think we should implement a) and do the formatting prior to submission.
> This prevents us from wasting reviewer time on format issues, and also
> makes sure that "what you see in phab, is what gets landed".
>

--
Ehsan

[Kartikaya Gupta]

Personally I would prefer if we rewrote the commits locally to be
formatted prior to submitting it into Phabricator. That way everything
stays in sync. Also usually clang-formatting a patch is the last thing
I want to do before submission anyway. And doing it now is kind of
annoying if you have a multi-patch stack because by default it only
operates on uncommitted changes so formatting a patch that's not the
topmost means doing some patch juggling/rebasing.

[Ehsan Akhgari]

On Fri, Dec 14, 2018 at 7:20 AM Kartikaya Gupta <kgu...@mozilla.com> wrote:

> Personally I would prefer if we rewrote the commits locally to be
> formatted prior to submitting it into Phabricator. That way everything
> stays in sync. Also usually clang-formatting a patch is the last thing
> I want to do before submission anyway. And doing it now is kind of
> annoying if you have a multi-patch stack because by default it only
> operates on uncommitted changes so formatting a patch that's not the
> topmost means doing some patch juggling/rebasing.
>

Right. I think that is the thinking behind the plan of enabling local
commit hooks (provided in bug 1507007) if I understand your point
correctly. That will ensure that anything that is committed will be
formatted properly, and as such it should ensure that code will be
formatted before being submitted for review, as well as making sure that
scenarios such as rebasing will also not mess up the formatting of the
commits in your local queue.

--
Ehsan

[Kartikaya Gupta]

Local commit hooks are even better, thanks! Are there instructions
somewhere on how to set up the hooks and make sure they run properly?

[Ehsan Akhgari]

On Mon, Dec 17, 2018 at 9:33 AM Kartikaya Gupta <kgu...@mozilla.com> wrote:

> Local commit hooks are even better, thanks! Are there instructions
> somewhere on how to set up the hooks and make sure they run properly?
>

We've yet to document these hooks but enabling them is very similar to
enabling the lint hooks which are documented here: <
https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook>.
The only difference is that you should use hooks_clang_format.py as the
file name.

--
Ehsan