Clang-format migration: Investigate branch migration plan & document it #60283

New Issue

Keir Mierle · 2019-01-07T17:41:47+01:00

Keir Mierle commented

2019-01-07 17:41:47 +01:00

Before applying the big-bang migration, it's critical to investigate how hard it will be to merge branches that started before the big bang change.

This task:

Figure out the best approach for branches-- should they apply make format themselves? Does that work? Or ignore it?
Document this approach either in the wiki or the blog post.

Before applying the big-bang migration, it's critical to investigate how hard it will be to merge branches that started before the big bang change. This task: - Figure out the best approach for branches-- should they apply `make format` themselves? Does that work? Or ignore it? - Document this approach either in the wiki or the blog post.

Keir Mierle commented

2019-01-07 17:41:47 +01:00

Added subscribers: @Keir, @dr.sybren, @Stefan_Werner, @LazyDodo, @fclem, @Sergey, @dfelinto, @brecht, @ideasman42, @mont29

Bastien Montagne commented

2019-02-04 11:28:57 +01:00

In #53211#610474, @mont29 wrote:
Also, just tried merging this into asset-engine branch, looks like -Xours is mandatory… Should not be an issue with following process though, I think:

Merge temp-clang-format into asset-engine (about 6 conflicting files, essentially due to recent pre-clang-format edits in master).

Apply clang-format on temp-clang-format and commit.

Apply clang-format on asset-engine and commit.

Merge clang-format into asset-engine using git merge -Xignore-all-space -Xours temp-clang-format command.

For the final merge, regular merge is catastrophic ( ALL edited files in asset-engine are conflicting, over 30). Using only -Xignore-all-space option, it’s a tad better (although I did not check the actual result), but am still getting 23 files conflicting, even the mere addition of a single function in creator.c is enough to create an issue!

After discussion, it would be nice to avoid applying clang-format on whole codebase in every branch. So I tried using -s ours instead of -Xours, following that process:

Merge temp-clang-format into asset-engine.
Apply clang-format on temp-clang-format and commit.
Merge clang-format into asset-engine using git merge -Xignore-all-space -s ours temp-clang-format command.
Apply clang-format on asset-engine and commit.

That does not really work well, it basically applies none of clang-format changes, i.e. after step 4 all files get modified, not only the branch-modified ones.

So instead, I reversed the step 3 and 4 of the first attempt, think this gives best results (smooth merge with much smaller commit when applying clang-format on the branch):

Merge temp-clang-format into asset-engine (about 6 conflicting files, essentially due to recent pre-clang-format edits in master).
Apply clang-format on temp-clang-format and commit.
Merge clang-format into asset-engine using git merge -Xignore-all-space -Xours temp-clang-format command.
Apply clang-format on asset-engine and commit.

> In #53211#610474, @mont29 wrote: > Also, just tried merging this into asset-engine branch, looks like `-Xours` is mandatory… Should not be an issue with following process though, I think: > > # Merge `temp-clang-format` into `asset-engine` (about 6 conflicting files, essentially due to recent pre-clang-format edits in master). > # Apply clang-format on `temp-clang-format` and commit. > # Apply clang-format on `asset-engine` and commit. > # Merge `clang-format` into `asset-engine` using `git merge -Xignore-all-space -Xours temp-clang-format` command. > > For the final merge, regular merge is catastrophic ( **ALL** edited files in `asset-engine` are conflicting, over 30). Using only `-Xignore-all-space` option, it’s a tad better (although I did not check the actual result), but am still getting 23 files conflicting, even the mere addition of a single function in `creator.c` is enough to create an issue! After discussion, it would be nice to avoid applying clang-format on whole codebase in every branch. So I tried using `-s ours` instead of `-Xours`, following that process: - Merge `temp-clang-format` into `asset-engine`. - Apply clang-format on `temp-clang-format` and commit. - Merge `clang-format` into `asset-engine` using `git merge -Xignore-all-space -s ours temp-clang-format` command. - Apply clang-format on `asset-engine` and commit. That does not really work well, it basically applies none of clang-format changes, i.e. after step 4 all files get modified, not only the branch-modified ones. So instead, I reversed the step 3 and 4 of the first attempt, think this gives best results (smooth merge with much smaller commit when applying clang-format on the branch): - Merge `temp-clang-format` into `asset-engine` (about 6 conflicting files, essentially due to recent pre-clang-format edits in master). - Apply clang-format on `temp-clang-format` and commit. - Merge `clang-format` into `asset-engine` using `git merge -Xignore-all-space -Xours temp-clang-format` command. - Apply clang-format on `asset-engine` and commit.

Brecht Van Lommel commented

2019-02-05 14:18:23 +01:00

Yes, I think it can be summarized as this once the formatting is done in master:

git merge <clang-format-commit>^1
... merge any conflicts and commit...

git merge -Xignore-all-space -Xours <clang-format-commit>
make format
git commit

git merge master
... merge any conflicts and commit...

Yes, I think it can be summarized as this once the formatting is done in master: ``` git merge <clang-format-commit>^1 ... merge any conflicts and commit... git merge -Xignore-all-space -Xours <clang-format-commit> make format git commit git merge master ... merge any conflicts and commit... ```

Brecht Van Lommel commented

2019-03-21 19:29:38 +01:00

I've added a script to rebase and merge master into branches: blender/blender-dev-tools@a64a4237d8
https://wiki.blender.org/wiki/Tools/ClangFormat#Migrating_Branches_and_Patches

Ended up being more complicated than I hoped, wouldn't mind if someone else can verify the logic is sane.

I've added a script to rebase and merge master into branches: blender/blender-dev-tools@a64a4237d8 https://wiki.blender.org/wiki/Tools/ClangFormat#Migrating_Branches_and_Patches Ended up being more complicated than I hoped, wouldn't mind if someone else can verify the logic is sane.

Brecht Van Lommel commented

2019-04-18 13:24:24 +02:00

Changed status from 'Open' to: 'Resolved'

Brecht Van Lommel closed this issue

2019-04-18 13:24:24 +02:00

Brecht Van Lommel self-assigned this 2019-04-18 13:24:24 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Clang-format migration: Investigate branch migration plan & document it #60283

Merge temp-clang-format into asset-engine (about 6 conflicting files, essentially due to recent pre-clang-format edits in master).

Apply clang-format on temp-clang-format and commit.

Apply clang-format on asset-engine and commit.

Merge clang-format into asset-engine using git merge -Xignore-all-space -Xours temp-clang-format command.

Merge `temp-clang-format` into `asset-engine` (about 6 conflicting files, essentially due to recent pre-clang-format edits in master).

Apply clang-format on `temp-clang-format` and commit.

Apply clang-format on `asset-engine` and commit.

Merge `clang-format` into `asset-engine` using `git merge -Xignore-all-space -Xours temp-clang-format` command.