Discussion:
The upcoming C/C++ coding style change
Sylvestre Ledru
2018-11-29 12:38:01 UTC
Permalink
Hello,

As Ehsan announced recently, we are going to reformat Friday 30 November.
In order to mitigate the impact on the uplifts process, we choose this date before the merge day.

In term of execution, we will close the trees Friday around 8am Paris/Berlin time (11pm PST).
We will then merge autoland and inbound into mozilla-central.
We are planning to land the big patch around 12am (3am PST).

The experimentation with dom/media highlighted that we need an efficient way to automatically rebase patches before the merge.
To achieve this, we updated the bootstrap process to include an extension called hg formatsource.
This extension will automatically rebase the local changes to avoid conflicts.
Please note that it is important that the extension is installed before the pulling a revision including the reformatted sources.

More information on:
https://docs.google.com/document/d/13AwAsvKMhH0mflDlfatBqn6LmZHiQih76oxM4zfrPl4/edit
https://bugzilla.mozilla.org/show_bug.cgi?id=1507711

Sylvestre, Ehsan and Andi
Boris Zbarsky
2018-11-29 12:57:55 UTC
Permalink
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Sylvestre,

Will it also update mq patches, or just in-repo commits?

-Boris
Ehsan Akhgari
2018-11-29 16:12:15 UTC
Permalink
Post by Sylvestre Ledru
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Sylvestre,
Will it also update mq patches, or just in-repo commits?
I don't think this extension is aware of mq patches (since they don't
really get rebased), but that being said from my foggy memory of using mq
ages ago, I seem to remember that it used to be possible to use the qfinish
command to convert them into regular hg commits and then later on to use
qimport -r (IIRC) to convert them back into mq patches again. If that is
still a workflow that works, then it should be possible to:

* Pull from a pre-reformat of the tree, run ./mach bootstrap to install
hg-formatsource.
* Do a one-time translation of your mq queue to a set of commits.
* Pull from hg.mozilla.org to pick up the reformat commit.
* Do a one-time translation of your mq queue back to a series of patches.

Cheers,
--
Ehsan
Boris Zbarsky
2018-11-29 16:25:20 UTC
Permalink
Post by Ehsan Akhgari
* Pull from a pre-reformat of the tree, run ./mach bootstrap to install
hg-formatsource.
* Do a one-time translation of your mq queue to a set of commits.
* Pull from hg.mozilla.org to pick up the reformat commit.
* Do a one-time translation of your mq queue back to a series of patches.
So just to be clear, I have a dozen or so queues with hundreds of
patches in them, not all of which will immediately apply due to age. So
this is not a simple operation, unfortunately...

I guess there's no real way to clang-format diffs, so maybe the answer
is manual merging when I actually need those patches.

-Boris
Ehsan Akhgari
2018-11-29 16:40:58 UTC
Permalink
Post by Sylvestre Ledru
Post by Ehsan Akhgari
* Pull from a pre-reformat of the tree, run ./mach bootstrap to
install
Post by Ehsan Akhgari
hg-formatsource.
* Do a one-time translation of your mq queue to a set of commits.
* Pull from hg.mozilla.org to pick up the reformat commit.
* Do a one-time translation of your mq queue back to a series of
patches.
So just to be clear, I have a dozen or so queues with hundreds of
patches in them, not all of which will immediately apply due to age. So
this is not a simple operation, unfortunately...
Yes, that is true sadly. But to be fair here, old mq patches that do not
apply due to age are already beyond saving with any kind of automated
tooling, and they require manual work to get them applied first. :-/
Post by Sylvestre Ledru
I guess there's no real way to clang-format diffs, so maybe the answer
is manual merging when I actually need those patches.
That's not true. clang-format can happily format diffs. When formatting
diffs though it tries to format your changes, not the context around the
diffs, as the use case it has been designed for is for example quickly
reformatting your changes in a pre-commit hook.

But still all is not lost here. When you do decide to do the manual
merging when you needed those patches, you would need to:

* Update your working tree to the parent of the commit that did the
reformat.
* Apply your patch to that tree and reformat the tree.
* Diff the reformat commit and your current working directory. That
would give the reformatted diff.
--
Ehsan
Boris Zbarsky
2018-11-29 17:16:07 UTC
Permalink
Post by Ehsan Akhgari
Yes, that is true sadly. But to be fair here, old mq patches that do not
apply due to age are already beyond saving with any kind of automated
tooling, and they require manual work to get them applied first. :-/
Sure.
Post by Ehsan Akhgari
That's not true. clang-format can happily format diffs. When formatting
diffs though it tries to format your changes, not the context around the
diffs
Ah, OK. That makes sense.
Post by Ehsan Akhgari
* Update your working tree to the parent of the commit that did the
reformat.
This also makes sense.

Have we considered adding an easy-to-remember tag for that commit to
make that easier in the future?

-Boris
Ehsan Akhgari
2018-11-29 19:37:58 UTC
Permalink
Post by Boris Zbarsky
Post by Ehsan Akhgari
Yes, that is true sadly. But to be fair here, old mq patches that do not
apply due to age are already beyond saving with any kind of automated
tooling, and they require manual work to get them applied first. :-/
Sure.
Post by Ehsan Akhgari
That's not true. clang-format can happily format diffs. When formatting
diffs though it tries to format your changes, not the context around the
diffs
Ah, OK. That makes sense.
Post by Ehsan Akhgari
* Update your working tree to the parent of the commit that did the
reformat.
This also makes sense.
Have we considered adding an easy-to-remember tag for that commit to
make that easier in the future?
Yes, please check out https://bugzilla.mozilla.org/show_bug.cgi?id=1508324.
(For git users, its sha1 will be recorded in .git-blame-ignore-revs for the
benefit of the git hyper-blame command[1].)

[1]
https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html
--
Ehsan
Boris Zbarsky
2018-11-29 17:15:56 UTC
Permalink
Post by Ehsan Akhgari
Yes, that is true sadly. But to be fair here, old mq patches that do not
apply due to age are already beyond saving with any kind of automated
tooling, and they require manual work to get them applied first. :-/
Sure.
Post by Ehsan Akhgari
That's not true. clang-format can happily format diffs. When formatting
diffs though it tries to format your changes, not the context around the
diffs
Ah, OK. That makes sense.
Post by Ehsan Akhgari
* Update your working tree to the parent of the commit that did the
reformat.
This also makes sense.

Have we considered adding an easy-to-remember tag for that commit to
make that easier in the future?

-Boris
Honza Bambas
2018-11-29 16:44:31 UTC
Permalink
Post by Ehsan Akhgari
Post by Sylvestre Ledru
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Sylvestre,
Will it also update mq patches, or just in-repo commits?
I don't think this extension is aware of mq patches (since they don't
really get rebased), but that being said from my foggy memory of using mq
ages ago, I seem to remember that it used to be possible to use the qfinish
command to convert them into regular hg commits and then later on to use
qimport -r (IIRC) to convert them back into mq patches again. If that is
* Pull from a pre-reformat of the tree, run ./mach bootstrap to install
hg-formatsource.
* Do a one-time translation of your mq queue to a set of commits.
* Pull from hg.mozilla.org to pick up the reformat commit.
And you forget that a merge will be needed here, because the formatting
changes will likely collide with the code one's patches are touching.

When we were mass-converting from PRUint32 to uint32_t etc, there was a
tool capable of converting your patches based on the pre-formated code
to be apply-able on the formatted code.

This is what we are missing.  So some of us may expect a huge merge pain
w/o something like that.


OTOH, if the changes are only whitespace changes, maybe there is some
way `patch --ignore-whitespace --fuzz N` could apply the patches.  Then
just re-format and your patches are OK.
Post by Ehsan Akhgari
* Do a one-time translation of your mq queue back to a series of patches.
That doesn't make much sense, because the commit history will look
something like (newest to oldest):
- merge of my patches with the formatted changes, having two parents
(formatted code default + my mq tip)
- formatted code `tip` (or `default`)
- my mq committed [ ]
- pre-formated parent
- ...

You can't just recreate your mq from such changeset tree and you also
can't avoid the likely quite complicated merge anyway.

-hb-
Post by Ehsan Akhgari
Cheers,
Ehsan Akhgari
2018-11-29 16:49:42 UTC
Permalink
Post by Ehsan Akhgari
Post by Ehsan Akhgari
Post by Sylvestre Ledru
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Sylvestre,
Will it also update mq patches, or just in-repo commits?
I don't think this extension is aware of mq patches (since they don't
really get rebased), but that being said from my foggy memory of using mq
ages ago, I seem to remember that it used to be possible to use the
qfinish
Post by Ehsan Akhgari
command to convert them into regular hg commits and then later on to use
qimport -r (IIRC) to convert them back into mq patches again. If that is
* Pull from a pre-reformat of the tree, run ./mach bootstrap to
install
Post by Ehsan Akhgari
hg-formatsource.
* Do a one-time translation of your mq queue to a set of commits.
* Pull from hg.mozilla.org to pick up the reformat commit.
And you forget that a merge will be needed here, because the formatting
changes will likely collide with the code one's patches are touching.
No, I didn't. :-) That's exactly what the hg-formatsource extension does
for you, it reformats your local changes on the fly as the rebase is in
progress, so you will get no collisions.
Post by Ehsan Akhgari
When we were mass-converting from PRUint32 to uint32_t etc, there was a
tool capable of converting your patches based on the pre-formated code
to be apply-able on the formatted code.
This is what we are missing. So some of us may expect a huge merge pain
w/o something like that.
No, those are old days and long gone, my friend. We are living in a new
world with better tools these days (for Mercurial users).
Post by Ehsan Akhgari
OTOH, if the changes are only whitespace changes, maybe there is some
way `patch --ignore-whitespace --fuzz N` could apply the patches. Then
just re-format and your patches are OK.
Post by Ehsan Akhgari
* Do a one-time translation of your mq queue back to a series of
patches.
That doesn't make much sense, because the commit history will look
- merge of my patches with the formatted changes, having two parents
(formatted code default + my mq tip)
- formatted code `tip` (or `default`)
- my mq committed [ ]
- pre-formated parent
- ...
You can't just recreate your mq from such changeset tree and you also
can't avoid the likely quite complicated merge anyway.
Again, no merge commits. Please do note that I was suggesting there that
you should use the rebase command, not merge. I think that would be |hg
pull --rebase|.
--
Ehsan
Honza Bambas
2018-11-29 16:58:13 UTC
Permalink
No, I didn't.  :-)  That's exactly what the hg-formatsource extension
does for you, it reformats your local changes on the fly as the rebase
is in progress, so you will get no collisions.

\o/
No, those are old days and long gone, my friend.  We are living in a
new world with better tools these days (for Mercurial users).
\o/ \o/
Again, no merge commits.  Please do note that I was suggesting there
that you should use the rebase command, not merge.  I think that would
be |hg pull --rebase|.
I missed that!  This is awesome, thanks.  Hopefully it will work well :)

Thanks!
-hb-
--
Ehsan
Ehsan Akhgari
2018-11-29 17:09:26 UTC
Permalink
This short guide should be helpful for Mercurial users:
https://docs.google.com/document/d/13AwAsvKMhH0mflDlfatBqn6LmZHiQih76oxM4zfrPl4
Post by Ehsan Akhgari
Post by Ehsan Akhgari
Post by Ehsan Akhgari
Post by Sylvestre Ledru
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Sylvestre,
Will it also update mq patches, or just in-repo commits?
I don't think this extension is aware of mq patches (since they don't
really get rebased), but that being said from my foggy memory of using
mq
Post by Ehsan Akhgari
ages ago, I seem to remember that it used to be possible to use the
qfinish
Post by Ehsan Akhgari
command to convert them into regular hg commits and then later on to use
qimport -r (IIRC) to convert them back into mq patches again. If that
is
Post by Ehsan Akhgari
* Pull from a pre-reformat of the tree, run ./mach bootstrap to
install
Post by Ehsan Akhgari
hg-formatsource.
* Do a one-time translation of your mq queue to a set of commits.
* Pull from hg.mozilla.org to pick up the reformat commit.
And you forget that a merge will be needed here, because the formatting
changes will likely collide with the code one's patches are touching.
No, I didn't. :-) That's exactly what the hg-formatsource extension does
for you, it reformats your local changes on the fly as the rebase is in
progress, so you will get no collisions.
Post by Ehsan Akhgari
When we were mass-converting from PRUint32 to uint32_t etc, there was a
tool capable of converting your patches based on the pre-formated code
to be apply-able on the formatted code.
This is what we are missing. So some of us may expect a huge merge pain
w/o something like that.
No, those are old days and long gone, my friend. We are living in a new
world with better tools these days (for Mercurial users).
Post by Ehsan Akhgari
OTOH, if the changes are only whitespace changes, maybe there is some
way `patch --ignore-whitespace --fuzz N` could apply the patches. Then
just re-format and your patches are OK.
Post by Ehsan Akhgari
* Do a one-time translation of your mq queue back to a series of
patches.
That doesn't make much sense, because the commit history will look
- merge of my patches with the formatted changes, having two parents
(formatted code default + my mq tip)
- formatted code `tip` (or `default`)
- my mq committed [ ]
- pre-formated parent
- ...
You can't just recreate your mq from such changeset tree and you also
can't avoid the likely quite complicated merge anyway.
Again, no merge commits. Please do note that I was suggesting there that
you should use the rebase command, not merge. I think that would be |hg
pull --rebase|.
--
Ehsan
--
Ehsan
Andrew McCreight
2018-11-29 16:57:18 UTC
Permalink
Post by Honza Bambas
OTOH, if the changes are only whitespace changes, maybe there is some
way `patch --ignore-whitespace --fuzz N` could apply the patches. Then
just re-format and your patches are OK.
The changes that clang-format makes include things like moving a { to a
different line, which I believe ignore-whitespace doesn't recognize as a
whitespace only change, because it is across multiple lines, and things
like reflowing comment blocks to reduce their width to 80 characters, which
of course will also not be counted as whitespace only.

Andrew
Post by Honza Bambas
Post by Ehsan Akhgari
* Do a one-time translation of your mq queue back to a series of
patches.
That doesn't make much sense, because the commit history will look
- merge of my patches with the formatted changes, having two parents
(formatted code default + my mq tip)
- formatted code `tip` (or `default`)
- my mq committed [ ]
- pre-formated parent
- ...
You can't just recreate your mq from such changeset tree and you also
can't avoid the likely quite complicated merge anyway.
-hb-
Post by Ehsan Akhgari
Cheers,
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform
Emilio Cobos Álvarez
2018-11-29 14:43:03 UTC
Permalink
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Is there a way to do the same for cinnabar users?

-- Emilio
Ehsan Akhgari
2018-11-30 01:06:55 UTC
Permalink
Post by Emilio Cobos Álvarez
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Is there a way to do the same for cinnabar users?
Yes! Sorry for the delay...

Please check out this script:
https://github.com/ehsan/clang-format-reformat-branch. This does something
similar to the format-source extension for Mercurial but done as a one-time
tool, borrowing from the tool that the MongoDB project developed for the
same use case. It takes a local branch based on a clone of mozilla-central
that doesn't yet have the reformat commit and rebases it on top of the
reformat commit, reformatting your local modifications in the process. I
hope it proves to be helpful for the git users out there!

The repository has full instructions on how to use it, but please let me
know if you have any questions or hit any issues.

Thanks,
--
Ehsan
Emilio Cobos Álvarez
2018-11-30 03:47:22 UTC
Permalink
Post by Ehsan Akhgari
Post by Emilio Cobos Álvarez
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Is there a way to do the same for cinnabar users?
Yes! Sorry for the delay...
NP!
Post by Ehsan Akhgari
https://github.com/ehsan/clang-format-reformat-branch. This does something
similar to the format-source extension for Mercurial but done as a one-time
tool, borrowing from the tool that the MongoDB project developed for the
same use case. It takes a local branch based on a clone of mozilla-central
that doesn't yet have the reformat commit and rebases it on top of the
reformat commit, reformatting your local modifications in the process. I
hope it proves to be helpful for the git users out there!
Nice! I haven't tried it yet (actually was going to report back when I
found the reply).

I hacked up something today as well while looking into this. It's not
really sophisticated, and you need to tweak the git repo config, so your
script probably works best for most people.

Just in case it's useful for somebody, while looking into a way to do
this (I basically followed[1]), I wrote a little merge driver which
seems to work fine (with a caveat, see below). I just uploaded it here:

https://github.com/emilio/clang-format-merge

The nice thing is that I can forget about this and all the regular
commands (cherry-pick / rebase / am...) will "just work", which is nice
for me since I don't need to think on all the patches I need to rebase
before-hand.

The caveat is that right now it needs a patch:

https://phabricator.services.mozilla.com/D13505

Because the current './mach clang-format -a' used for 'hg formatsource'
makes assumptions about the path it's formatting, and git loves paths
like .merge_file_lwMX2O. So if it doesn't make it in it'll be quite
useless :-)
Post by Ehsan Akhgari
The repository has full instructions on how to use it, but please let me
know if you have any questions or hit any issues.
Thanks, I'll give that a shot for some of my checkouts and report back :)

-- Emilio

[1]: https://bugs.chromium.org/p/chromium/issues/detail?id=574611
Ehsan Akhgari
2018-11-30 03:59:43 UTC
Permalink
Post by Dave Townsend
Post by Ehsan Akhgari
Post by Emilio Cobos Álvarez
Post by Sylvestre Ledru
This extension will automatically rebase the local changes to avoid conflicts.
Is there a way to do the same for cinnabar users?
Yes! Sorry for the delay...
NP!
Post by Ehsan Akhgari
https://github.com/ehsan/clang-format-reformat-branch. This does
something
Post by Ehsan Akhgari
similar to the format-source extension for Mercurial but done as a
one-time
Post by Ehsan Akhgari
tool, borrowing from the tool that the MongoDB project developed for the
same use case. It takes a local branch based on a clone of
mozilla-central
Post by Ehsan Akhgari
that doesn't yet have the reformat commit and rebases it on top of the
reformat commit, reformatting your local modifications in the process. I
hope it proves to be helpful for the git users out there!
Nice! I haven't tried it yet (actually was going to report back when I
found the reply).
I hacked up something today as well while looking into this. It's not
really sophisticated, and you need to tweak the git repo config, so your
script probably works best for most people.
Just in case it's useful for somebody, while looking into a way to do
this (I basically followed[1]), I wrote a little merge driver which
https://github.com/emilio/clang-format-merge
This actually looks quite decent, and a cleaner approach. It also taught
me about merge drivers. :-) I suspect this would work equally well for
everyone (but I haven't tested it myself, just based on reading the source.)

Thanks for making it happen.
--
Ehsan
Dave Townsend
2018-11-29 16:25:50 UTC
Permalink
Post by Sylvestre Ledru
The experimentation with dom/media highlighted that we need an efficient
way to automatically rebase patches before the merge.
To achieve this, we updated the bootstrap process to include an extension
called hg formatsource.
This extension will automatically rebase the local changes to avoid conflicts.
Please note that it is important that the extension is installed before
the pulling a revision including the reformatted sources.
https://docs.google.com/document/d/13AwAsvKMhH0mflDlfatBqn6LmZHiQih76oxM4zfrPl4/edit
https://bugzilla.mozilla.org/show_bug.cgi?id=1507711
This is great, I was wondering how badly the changes would mess up my
in-progress patches. However I've read the doc and it isn't entirely clear
to me what I actually need to do. It talks about needing to patch the local
version-control-tools until some bugs are fixed, some of which are and some
aren't currently. Do I just need to run './mach bootstrap' or is there
something more?
Steve Fink
2018-11-29 17:41:19 UTC
Permalink
Post by Sylvestre Ledru
The experimentation with dom/media highlighted that we need an
efficient way to automatically rebase patches before the merge.
To achieve this, we updated the bootstrap process to include an
extension called hg formatsource.
This extension will automatically rebase the local changes to avoid conflicts.
Please note that it is important that the extension is installed
before the pulling a revision including the reformatted sources.
I attempted to experiment with this, and ran into some issues and
confusion, which I wrote down in the form of
https://bugzilla.mozilla.org/show_bug.cgi?id=1511093
Loading...