Fwd: [chromium-dev] Minimizing negative impacts of mass reformat

[Rick Byers]

Just raising visibility here. I'm interested to hear from any embedders / downstream Chromium folks what the impact of a C++ mass-reformat (to match our style guide of always using braces for conditionals) would be for you and what mitigations might help reduce any merge or rebase pain. 

Rick

---------- Forwarded message ---------
From: Peter Kasting <pkas...@chromium.org>
Date: Mon, Sep 9, 2024 at 8:03 PM
Subject: [chromium-dev] Minimizing negative impacts of mass reformat
To: Chromium-dev <chromi...@chromium.org>
Cc: cxx <c...@chromium.org>

TLDR: We want to mass-format the codebase, and we want to minimize negative impact on you. The reformat won't appear in local `git blame` by default, but it will appear in code search blame. If you have blocking issues or requests that would improve things, please share them. Otherwise we will likely do this within Q4 2024.

Background: In late 2022 crrev.com/1082610 changed Chromium's clang-format defaults to require braces on all conditionals. There have also been multiple smaller tweaks either to our settings or to clang-format's behavior since the last mass-format, which was many years ago.

Resulting problems:

• Tools are set to "only reformat modified lines", which leads to decreased consistency within files
• Lack of consistency not only hampers readability, it has led to confusion about what the expected style is
• Even with the above "only modified lines" setting, CLs often make noticeable formatting tweaks
• These add noise to code reviews
• They also hinder blame-trawling
• At least Visual Studio can still add opening braces to modified conditions without also adding matching closing braces, leading to cryptic compile errors (hopefully not also bugs)

Proposal: Mass-reformat the codebase one time. Use the "[cleanup]" prefix and "cleanup" hashtag on all CLs. Add all CLs to .git-blame-ignore-revs.

Other solutions instead of/in addition to a one-time reformat have been considered (periodic formats, continual formats, format whole files when touched) but none have gained consensus across discussions in both cxx@ and on Slack. Initially even a one-time format lacked consensus, but that has changed over the past months as the trickle of formatting changes has not slowed.

Concerns: 

• Formatting changes make blame noisier.
  
• Since crrev.com/c/5838110, CLs in .git-blame-ignore-revs (as these will be) will be ignored by default for `git blame` in Chromium. They will also be ignored by the older (and less-known) `git hyper-blame`.
• Unfortunately code search blame will still show them.
• b/167275014 (internal-only) proposes that git respect these by default. I have an outstanding email thread with the git maintainer at Google about this, but it's not looking promising.
• Mid-last year, the code search folks suggested addressing this at the git layer instead of in code search. Given staffing changes this year, it seems even less likely there will be any code search changes to improve blame in the foreseeable future.
• The "cleanup" hashtag makes internal codesearch display CLs less prominently, but the public codesearch lacks this support, and is unlikely to gain it soon (see above). Nevertheless, adding it to these CLs seems potentially beneficial for the future.
• sesse@ built a per-token (not per-line) blame prototype at blametest.sesse.net that could be interesting here, but that has even less chance of actual integration than the above (since it would be much more complex).
• We don't want to block refactorings on this issue since it hinders improving the codebase, but if there are other solutions, or people who want to contribute engineering effort to unblocking the above, we welcome them!
• LSCs like this result in local merge conflicts.
• Hopefully since this is a format-only change, resolving these will be fast and straightforward, and we don't need additional tooling support.
• Meta-level concern: There is no formal process to decide whether, when, and how to do changes like this.
• Technically, little prevents someone just doing this on their own initiative, but that seems inconsiderate.
• lsc-owners@ can unilaterally approve LSC proposals, and might be the closest thing that exists.
• Past queries to ATLs about this have been met with "maybe poll chromium-dev@", which feels prone to derailing and stop energy.
• Most discussions have happened on cxx@, which keeps them more focused, but also feels self-appointed and gatekeepy.

Feedback welcome.

PK

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFA4H1M%3DhS-v_n-5yYgJB_FRb2V9SKx7i1f5xvE7Cxybog%40mail.gmail.com.

[José Dapena Paz]

Hi,

Mass reformats have impact, increasing the chances of conflicts, and requiring downstream patches to adapt to the new standard (if it is new).

It is usually not a terrible thing, as the generated conflicts are usually trivially fixed, and a one time thing. In any case, it is another reason to try to keep the conflicting delta low.

I mean, at least in my experience this is quite far from being the most complex thing. Other types of change are far more problematic (rearchitecturing, refactors, ...).

[Sébastien Marchand]

Thanks for including us in this discussion Rick! 

On Fri, Sep 20, 2024 at 10:06 AM José Dapena Paz <jda...@igalia.com> wrote:

Hi,

Mass reformats have impact, increasing the chances of conflicts, and requiring downstream patches to adapt to the new standard (if it is new).

It is usually not a terrible thing, as the generated conflicts are usually trivially fixed, and a one time thing. In any case, it is another reason to try to keep the conflicting delta low.

I mean, at least in my experience this is quite far from being the most complex thing. Other types of change are far more problematic (rearchitecturing, refactors, ...).

+1, the downstream impact will be a one time thing with a relatively low engineering cost (as the conflicts will be trivial).

I'm more concerned by the other issues that have been discussed in the thread and some related ones:

- Impact on code archeology - As embedders who radically change the product, we sometimes need to do some archeology to understand why things are designed/implemented in a specific way.

- Impact on the Github clone of the Chromium repo - I often need to rely on the Github clone of chromium.git to do some archeology as the history pane in CodeSearch sometimes doesn't work (it times out). If the Github UI doesn't handle the blame ignore list then it'll make this slightly more inconvenient.

- Impact on tools - VSCode is getting better at showing the git blame directly in the IDE, it's super convenient to just hover over a line of code and click on a link to be sent to the CL that modified it. I don't know if VSCode supports the git blame ignore list.

 

--
You received this message because you are subscribed to the Google Groups "Chromium Embedders" group.
To unsubscribe from this group and stop receiving emails from it, send an email to embedder-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/embedder-dev/28ae5851e01958684ab6605d65b3d92fa61e13e8.camel%40igalia.com.

[Peter Kasting]

On Fri, Sep 20, 2024, 7:35 AM Sébastien Marchand <s...@thebrowser.company> wrote:

- Impact on the Github clone of the Chromium repo - I often need to rely on the Github clone of chromium.git to do some archeology as the history pane in CodeSearch sometimes doesn't work (it times out). If the Github UI doesn't handle the blame ignore list then it'll make this slightly more inconvenient.

GitHub already respects .git-blame-ignore-revs. (Additionally, if the GoB folks do indeed make blame fast and reliable, it sounds like this will be moot.)

PK

[Zach Murphy]

Agree with the others about the overall low impact and the specific archaeology concerns. Two other questions:

1. Are you planning on doing any sort of code-chill to get these CLs in?

2. Going forward, will tools continue to only format modified lines or will that change as part of this work?

Also, if there is additional tooling needed, please keep this thread updated.