Minimizing negative impacts of mass reformat

[Peter Kasting]

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

[Leszek Swirski]

On Tuesday, September 10, 2024 at 2:04:14 AM UTC+2 Peter Kasting wrote:

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.

Can this be automated (a roller bot that adds all CLs with that prefix to .git-blame-ignore-revs)?

• 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.
  

How does code search implement the blame layer, I assume it must either shell out to git or use something like libgit? Could we simply (haha simple things in Google) change the gitconfig equivalent in code search?

• 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!
  

 Without wanting to block anything, I want to add my concern around regressing the blame layer in code search -- I do a lot of git archaeology and it's already painful enough, so I want to make sure that affecting the code search blame layer is given its fair "con" weight against the "pro"s of enabling mass reformats.

[Calder Kitagawa]

+agrieve@ to cc in case there is more to add.

As a point of reference, we did a mass reformat for Chromium Java files last year due to switching formatting tools (internal discussion here). Admittedly, the Java codebase is much smaller and we felt compelled to mass reformat as the deltas between the output of the format tools was much larger. However, in general I think people have been happy with the change as compared to receiving CLs with large, hard to find, format-only deltas.

We had similar concerns about git blame functionality. As someone on the stability rotation, codesearch blame has been good enough especially as you can jump back to before the reformat CLs if needed; however, mileage may vary and sometimes it is still a bit frustrating to need to jump over cleanup CLs even almost a year on.

As for our process, Andrew executed the reformat over a weekend after consulting with Chrome Java engineers via mailing lists. Due to low weekend commit traffic, realtime impact was fairly minimal, but obviously there were still many merge conflicts to WIP CLs come Monday.

[Aaron Leventhal]

Like Calder, I also do a lot of code archeology. Some a11y decisions go back to WebKit, where files and symbols often had different names. Not all engineers have the skills and patience to do it and skip that step, thus not learning from the past.

We really need to make code archeology easier, not harder. The right tool should help you skip through formatting and rename changes. Personally, I would prioritize this way higher over reformatting the code. But maybe that's because I don't see the tangible benefit for our users or developer velocity, and I hate how difficult it makes merges and code archaeology. If we're going to do it, it should create more benefit than pain.

--
--
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/77c1915e-b274-44f0-89bd-5b52ac47b819n%40chromium.org.

[Peter Kasting]

On Tue, Sep 10, 2024, 2:28 AM Leszek Swirski <les...@chromium.org> wrote:

On Tuesday, September 10, 2024 at 2:04:14 AM UTC+2 Peter Kasting wrote:

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.

Can this be automated (a roller bot that adds all CLs with that prefix to .git-blame-ignore-revs)?

It's possible. I don't know whether it's a desirable effect. The majority of these I've seen are one-offs and not necessarily the sort of things you want to hide in blame, but some are. I suspect on balance we shouldn't do this automatically, but I wonder if there's a way to make it easier to have it happen automatically where people do want (e.g. some other tag on CLs that makes this happen).

• 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.
  

How does code search implement the blame layer, I assume it must either shell out to git or use something like libgit?

I believe it uses git -on-borg, which is a black box to me but I assume is hand-written custom stuff and thus not guaranteed to have any of git's functionality.

Could we simply (haha simple things in Google) change the gitconfig equivalent in code search?

I don't know. If so this is technically easy and just a matter of finding things. But I don't know where to look either. Having someone figure this out is welcome :)

• 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!
  

 Without wanting to block anything, I want to add my concern around regressing the blame layer in code search -- I do a lot of git archaeology and it's already painful enough, so I want to make sure that affecting the code search blame layer is given its fair "con" weight against the "pro"s of enabling mass reformats.

Agree, and this is part of why forward motion here has paused for a couple years while we pursued alternate avenues.

Right now the GoB team is working on making blame and history much faster, which I think would ameliorate pain -- if you have to hop repeatedly but each hop is fast, that's far less bad. So I agreed with their prioritizing that work over this. And I also agree with you that status quo isn't good and this is a real con. 

PK

[Peter Kasting]

On Tue, Sep 10, 2024, 7:24 AM Aaron Leventhal <aleve...@chromium.org> wrote:

Like Calder, I also do a lot of code archeology. Some a11y decisions go back to WebKit, where files and symbols often had different names. Not all engineers have the skills and patience to do it and skip that step, thus not learning from the past.

This is also my experience, though I don't know that making blame-trawling take fewer steps will ultimately solve the problem -- I suspect some folks just don't realize the benefits of doing this systematically and don't think to do it. (Of course, if when they do doing it is hard, then that's bad.)

Without it actually solving your problem, I will note that we can always manually add more CLs -- even old CLs -- to 

.git-blame-ignore-revs; it's human-writable. So if you do run across "useless" cleanup CLs while you're trawling, and you have time to add them here, that will skip them in the future. At least locally. 

PK

We really need to make code archeology easier, not harder. The right tool should help you skip through formatting and rename changes. Personally, I would prioritize this way higher over reformatting the code. But maybe that's because I don't see the tangible benefit for our users or developer velocity, and I hate how difficult it makes merges and code archaeology. If we're going to do it, it should create more benefit than pain.

There are some improvements I'd love here, but I have much more ability to modify Chromium source than code search, so my choice is less "one type of work or the other" than "do this in chrome, or just don't do anything".

I think sesse's per-token tool is fascinating here, but I imagine integrating with that would be quite challenging. 

PK

[Leszek Swirski]

On Tue, Sep 10, 2024 at 4:27 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Sep 10, 2024, 2:28 AM Leszek Swirski <les...@chromium.org> wrote:

Can this be automated (a roller bot that adds all CLs with that prefix to .git-blame-ignore-revs)?

It's possible. I don't know whether it's a desirable effect. The majority of these I've seen are one-offs and not necessarily the sort of things you want to hide in blame, but some are. I suspect on balance we shouldn't do this automatically, but I wonder if there's a way to make it easier to have it happen automatically where people do want (e.g. some other tag on CLs that makes this happen).

Maybe the bot could auto send ignore-rev CLs to authors of cleanup CLs, and it's their choice whether to accept it or not?

How does code search implement the blame layer, I assume it must either shell out to git or use something like libgit?

I believe it uses git -on-borg, which is a black box to me but I assume is hand-written custom stuff and thus not guaranteed to have any of git's functionality.

Could we simply (haha simple things in Google) change the gitconfig equivalent in code search?

I don't know. If so this is technically easy and just a matter of finding things. But I don't know where to look either. Having someone figure this out is welcome :)

With a bit of digging, I think it uses jgit? https://gerrit.googlesource.com/gitiles/+/refs/heads/master/java/com/google/gitiles/blame/cache/BlameCacheImpl.java

Which I think doesn't support ignore-revs :( https://bugs.eclipse.org/bugs/show_bug.cgi?id=559671

Right now the GoB team is working on making blame and history much faster, which I think would ameliorate pain -- if you have to hop repeatedly but each hop is fast, that's far less bad. So I agreed with their prioritizing that work over this. And I also agree with you that status quo isn't good and this is a real con

Can we push this onto them as a good (next) high priority, if this is important to us? Not sure how much influence we have on their priorities. 

[Peter Kasting]

On Tue, Sep 10, 2024 at 7:59 AM Leszek Swirski <les...@chromium.org> wrote:

On Tue, Sep 10, 2024 at 4:27 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Sep 10, 2024, 2:28 AM Leszek Swirski <les...@chromium.org> wrote:

Can this be automated (a roller bot that adds all CLs with that prefix to .git-blame-ignore-revs)?

It's possible. I don't know whether it's a desirable effect. The majority of these I've seen are one-offs and not necessarily the sort of things you want to hide in blame, but some are. I suspect on balance we shouldn't do this automatically, but I wonder if there's a way to make it easier to have it happen automatically where people do want (e.g. some other tag on CLs that makes this happen).

Maybe the bot could auto send ignore-rev CLs to authors of cleanup CLs, and it's their choice whether to accept it or not?

If messaged properly, that seems possibly reasonable. I don't know much about the autoroller system and whether it could be repurposed for something like this; I can ask.

To be clear, this is orthogonal to the particular formatting change being discussed on this thread, so I'll pursue in parallel.

How does code search implement the blame layer, I assume it must either shell out to git or use something like libgit?

I believe it uses git -on-borg, which is a black box to me but I assume is hand-written custom stuff and thus not guaranteed to have any of git's functionality.

Could we simply (haha simple things in Google) change the gitconfig equivalent in code search?

I don't know. If so this is technically easy and just a matter of finding things. But I don't know where to look either. Having someone figure this out is welcome :)

With a bit of digging, I think it uses jgit? https://gerrit.googlesource.com/gitiles/+/refs/heads/master/java/com/google/gitiles/blame/cache/BlameCacheImpl.java

Which I think doesn't support ignore-revs :( https://bugs.eclipse.org/bugs/show_bug.cgi?id=559671

Heh. Anyone want to go implement? :D

Right now the GoB team is working on making blame and history much faster, which I think would ameliorate pain -- if you have to hop repeatedly but each hop is fast, that's far less bad. So I agreed with their prioritizing that work over this. And I also agree with you that status quo isn't good and this is a real con

Can we push this onto them as a good (next) high priority, if this is important to us? Not sure how much influence we have on their priorities.

We've surfaced this, both in terms of filing (several) relevant bugs and (via the Chrome infra folks, thank you!) in listing it as a desire in the prioritization list. I don't know how much influence that has (I suspect little), so I wouldn't be very hopeful, but we have indeed done what's possible.

PK

[Leszek Swirski]

On Tue, Sep 10, 2024 at 5:37 PM Peter Kasting <pkas...@chromium.org> wrote:

We've surfaced this, both in terms of filing (several) relevant bugs and (via the Chrome infra folks, thank you!) in listing it as a desire in the prioritization list. I don't know how much influence that has (I suspect little), so I wouldn't be very hopeful, but we have indeed done what's possible.

Since this seems to be a reasonably common issue, and comes up as a blocker pretty often, maybe it's something where it's worth applying the new culture principle of "escalate quickly"?

[Peter Kasting]

On Tue, Sep 10, 2024 at 8:36 AM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Sep 10, 2024 at 7:59 AM Leszek Swirski <les...@chromium.org> wrote:

On Tue, Sep 10, 2024 at 4:27 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Sep 10, 2024, 2:28 AM Leszek Swirski <les...@chromium.org> wrote:

Can this be automated (a roller bot that adds all CLs with that prefix to .git-blame-ignore-revs)?

It's possible. I don't know whether it's a desirable effect. The majority of these I've seen are one-offs and not necessarily the sort of things you want to hide in blame, but some are. I suspect on balance we shouldn't do this automatically, but I wonder if there's a way to make it easier to have it happen automatically where people do want (e.g. some other tag on CLs that makes this happen).

Maybe the bot could auto send ignore-rev CLs to authors of cleanup CLs, and it's their choice whether to accept it or not?

If messaged properly, that seems possibly reasonable. I don't know much about the autoroller system and whether it could be repurposed for something like this; I can ask.

https://issues.chromium.org/365712496, for those who want to follow along.

PK 

[Peter Kasting]

To me there's not much that can be done here: the owning team has very little headcount, and right now they're spending it on things that both the chrome-infra folks and I think would be even more beneficial to our team than this. So I don't know what more we could ask, beyond wanting that team to be given more headcount. If that's a thing escalation can plausibly help with, I'm all for it.

PK 

[Leszek Swirski]

Fair - I'm very keen on surfacing to leadership every case where understaffing is hurting engineers, but it likely won't immediately solve this issue.

[Peter Kasting]

Resurfacing this in case anyone else has further thoughts and recommendations.

It sounds to me like if folks want code search to be able to skip over these and other mechanical CLs, the biggest contribution would be to implement blame.ignoreRevsFile support in JGit. https://bugs.eclipse.org/bugs/show_bug.cgi?id=559671 is an open bug, although I believe jgit has migrated to GitHub issues (https://github.com/eclipse-jgit/jgit/issues) where I don't see a bug.

To be clear, while I would love to see the above happen, I don't think it's a blocker.

PK

[Peter Kasting]

Correct, this change did not affect V8, and there is no proposal to mass-format V8.

PK

On Mon, Sep 16, 2024, 7:08 AM Matthias Liedtke <mlie...@google.com> wrote:

Just to clarify:

> Background: In late 2022 crrev.com/1082610 changed Chromium's clang-format defaults to require braces on all conditionals.

This only applies to repositories which use Chromium's clang-format defaults, correct?
So V8 wouldn't be affected by this at all as it's based on the google style in its .clang-format?

--

--
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/64e2d3d4-90e5-4753-8767-5413026b12e9n%40chromium.org.

[Michael Lippautz]

On Mon, Sep 16, 2024 at 4:13 PM Peter Kasting <pkas...@chromium.org> wrote:

Correct, this change did not affect V8, and there is no proposal to mass-format V8.

As someone who works across codebases (Blink & V8) and relies on git blame a lot this will negatively affect my workflows. Blink especially requires a ton of code archeology and I already stumble across mass refactorings in codesearch quite a lot.

I do a lot of code reviews and have never come across any issues with brace style that were mentioned in the initial message. Even if there were nits here and there, I cannot recall a single larger discussion/problem -- people tend to just apply the new style as part of their actual other work and move on. 

 

PK

On Mon, Sep 16, 2024, 7:08 AM Matthias Liedtke <mlie...@google.com> wrote:

Just to clarify:
> Background: In late 2022 crrev.com/1082610 changed Chromium's clang-format defaults to require braces on all conditionals.

This only applies to repositories which use Chromium's clang-format defaults, correct?
So V8 wouldn't be affected by this at all as it's based on the google style in its .clang-format?

On Mon, Sep 16, 2024 at 4:00 PM Peter Kasting <pkas...@chromium.org> wrote:

Resurfacing this in case anyone else has further thoughts and recommendations.

It sounds to me like if folks want code search to be able to skip over these and other mechanical CLs, the biggest contribution would be to implement blame.ignoreRevsFile support in JGit. https://bugs.eclipse.org/bugs/show_bug.cgi?id=559671 is an open bug, although I believe jgit has migrated to GitHub issues (https://github.com/eclipse-jgit/jgit/issues) where I don't see a bug.

To be clear, while I would love to see the above happen, I don't think it's a blocker.

I am a bit confused why none of this should be a blocker. I agree that it's unlikely to be worked on but that shouldn't be a waiver for ignoring it.

The effects of refactorings on git blame are real and I have seen very little agreement that the refactoring is solving a huge problem.

-Michael

[Matthias Liedtke]

For clarification:

> Background: In late 2022 crrev.com/1082610 changed Chromium's clang-format defaults to require braces on all conditionals.

This only applies to repositories which use Chromium's clang-format defaults (or only chromium/src), correct?

So V8 wouldn't be affected by this at all as it's based on the google style in its .clang-format?

[Peter Kasting]

Fine-grained responses below, but I want to first echo back what I hear as the underlying concern here, since I don't want your point to get lost in details.

Working in Chromium is already painful in a lot of ways. It seems to be getting slowly more so all the time -- more checks and processes and bits stopping you, an endless cycle of tooling changes, slower and flakier websites, and a drumbeat of "do more with less" and "we don't have headcount for that". It's enough to leave you on the constant edge of frustration and despair, especially when there's no way to push against it. Multiply that when you're expected to work across multiple repositories, with different style rules, partially owned by people in another timezone.

In such a world, we're not just discussing a reformatting LSC -- we're discussing the general pattern of things being painful, and no one being aware of and concerned about the pain they're inflicting.

If that's how you're feeling, I hear you. It's how I'm feeling too. Every change that is sold as making something better seems to make it better for other people, and worse for me. The things that get sacrificed in endless layoffs are the things I care about, while things I don't find valuable still thrive. I'm told to escalate, but my escalations either get no response or get placating words that mean nothing. I feel powerless.

Maybe I'm projecting, and if so, I'm sorry for mistaking your intent. If not, then I still don't know that I can make things much better. I just want you to at least feel heard, and understood, and agreed with.

I'll copy a phrase from below (that I've also bolded there) to summarize my take on why I'm supporting this change despite sympathizing with you: the claim here is that, even for people who suffer the problems of code search blame, this is less-bad than status quo. That doesn't mean it's monotonically good.

Further responses inline.

On Mon, Sep 16, 2024 at 8:13 AM Michael Lippautz <mlip...@chromium.org> wrote:

As someone who works across codebases (Blink & V8) and relies on git blame a lot this will negatively affect my workflows.

(Clarity: `git blame` will auto-skip these changes once we add them to .git-blame-ignore-revs. Only code search is affected. I assume that's what you meant, though.)

 

Blink especially requires a ton of code archeology and I already stumble across mass refactorings in codesearch quite a lot.

Yes. I find both command-line blame and code search blame frustrating for a variety of reasons:

• They're sloooooow. I could forgive most other faults if they were nigh-instantaneous.
• Code search blame doesn't even work half the time.
• It's shockingly inconvenient to "skip past this CL and keep looking". That should be like... one top-level click!
• It's waaaay harder than it should be to continue tracing across a file move/rename.

Now, admittedly that last one isn't relevant here, since we wouldn't be doing any file moves or renames. But the rest are. I think just about every engineer on the team is frustrated with this, so you're by no means in a minority.

That said, this also means the concerns you're sharing aren't new or surprising; they're the fundamental ones we've been concerned with while debating this over the last several years. They're also behind multiple bugs and email threads with several different groups, trying to get more attention to them.

I am pleased that supposedly the "slow blame/history" issue is the team's current priority. That makes me cautiously hopeful that this pain might become a lot less.

I do a lot of code reviews and have never come across any issues with brace style that were mentioned in the initial message. Even if there were nits here and there, I cannot recall a single larger discussion/problem -- people tend to just apply the new style as part of their actual other work and move on. 

As one of the style arbiters, I do see questions about this (and related issues) come up periodically. They are often not surfaced on reviews directly; a common technique is to ask out of band so as not to block. I also mentor newer engineers who struggle with reviewers asking them to go beyond the style guide, or to revert formatting changes, or whatever. A lot of these are not issues an LSC like this will address, but just as with auto-formatting in general, the more it simply eliminates ambiguity and debate, the better.

On Mon, Sep 16, 2024 at 4:00 PM Peter Kasting <pkas...@chromium.org> wrote:

It sounds to me like if folks want code search to be able to skip over these and other mechanical CLs, the biggest contribution would be to implement blame.ignoreRevsFile support in JGit. https://bugs.eclipse.org/bugs/show_bug.cgi?id=559671 is an open bug, although I believe jgit has migrated to GitHub issues (https://github.com/eclipse-jgit/jgit/issues) where I don't see a bug.

To be clear, while I would love to see the above happen, I don't think it's a blocker.

I am a bit confused why none of this should be a blocker. I agree that it's unlikely to be worked on but that shouldn't be a waiver for ignoring it.

For two years, it was a blocker. Two things changed recently:

1. Primarily, we got feedback that even some folks initially opposed to a mass-format were now reluctantly supportive of it, because a single CL is easier to skip past than having formatting changes mixed into every CL for years. Thus the claim here is that, even for people who suffer the problems of code search blame, this is less-bad than status quo. And if code search ever becomes able to properly ignore these, then at that point the LSC will retroactively become much better (in fact, the earlier we did it, the better it would have been).
2. Secondarily, we were able to change `gclient sync` such that at least `git blame` locally ignores these changes, so people have some kind of workaround/alternative.

Given these, it seemed like we perceived the cost of doing a mass format as having decreased and the cost of not doing it as continually increasing, so changing this from "blocker" to "non-blocker" made sense.

I have seen very little agreement that the refactoring is solving a huge problem.

I don't think it's "huge", I think it's a lot of small papercuts spread across a large surface. Which is similar to the nature of reformatting LSCs affecting blame trawling: it's a periodic annoyance, whose salience differs for different people.

As for why you haven't seen significant agreement, that's partly because the discussions on this have largely not taken place on chromium-dev@, but on Slack and on cxx@. (Neither of those is a closed forum, BTW, so you're welcome to join if this sort of discussion is something you want to be involved in.) I think that's probably for the best on net, but see also the concerns I mentioned in the initial mail in this thread about there being no formal process for such changes, and all the decision routes being problematic.

I hope this helps make the process and thinking behind this one more clear. Definitely interested in any concrete suggested improvements you have -- we've already gotten at least one good feature request bug out of this thread.

PK

[Rachel Blum]

Hi everybody!

In the spirit of "Decide quickly or Escalate", here's a two-part proposal:

Part 1: I think we're all agreed that blame as it is is a very painful tool. So, let's escalate that. I'll work w/ ATLs & mchristoff to see if we can get any (or all) of the following things addressed going forward:

• Blame should work reliably

• Blame should not take 20+s per invocation

• Blame should honor .git-blame-ignore-revs.

Part 2a: This CL runs into the territory of LSC's "What is it not good for?" It's a low-ROI cosmetic change. Code cleanup is valuable, and we want LSCs that clean up our code - but the pain they cause should always outweigh the cost. Landing it right now would cost more than it gains, so we shouldn't.

Part 2b: If we don't want to just shelve it, I'd suggest thinking about doing this in a low-impact way - maybe over either turkey day or the winter holidays, with a heads up so folks know a disruptive change will be landing. The question if we want to allow cosmetic changes in a low impact way at all is a question of general principles - and so, it's a good one to raise with lsc-owners or ATLs specifically. I think the discussion here helped clarify pros and cons and can inform that decision.

 - Rachel

[Daniel Cheng]

One particular blame issue that hasn't been captured here is the fact that blame does not work well across renames. That isn't a problem unique to this particular LSC, but if we're trying to fix blame, this is actually one of the biggest pain points for me when doing code archaeology. I can live with extra clicks (and latency) to say "view blame prior to this change", but when that randomly breaks because someone moves a file, it becomes a much bigger hassle.

https://buganizer.corp.google.com/issues/172085397 is the bug in question. tl;dr blame knows about file history from before the rename and can often include it in the blame, but "view blame prior to this change" doesn't actually follow the rename, so you just get a "file did not exist at this revision".

Daniel

--

--
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/a7d3fe60-012d-47fc-a6eb-3bc9a8adb836n%40chromium.org.

[Fergal Daly]

On Tue, 17 Sept 2024 at 08:39, Peter Kasting <pkas...@chromium.org> wrote:

Fine-grained responses below, but I want to first echo back what I hear as the underlying concern here, since I don't want your point to get lost in details.

Working in Chromium is already painful in a lot of ways. It seems to be getting slowly more so all the time -- more checks and processes and bits stopping you, an endless cycle of tooling changes, slower and flakier websites, and a drumbeat of "do more with less" and "we don't have headcount for that". It's enough to leave you on the constant edge of frustration and despair, especially when there's no way to push against it. Multiply that when you're expected to work across multiple repositories, with different style rules, partially owned by people in another timezone.

In such a world, we're not just discussing a reformatting LSC -- we're discussing the general pattern of things being painful, and no one being aware of and concerned about the pain they're inflicting.

If that's how you're feeling, I hear you. It's how I'm feeling too. Every change that is sold as making something better seems to make it better for other people, and worse for me. The things that get sacrificed in endless layoffs are the things I care about, while things I don't find valuable still thrive. I'm told to escalate, but my escalations either get no response or get placating words that mean nothing. I feel powerless.

Maybe I'm projecting, and if so, I'm sorry for mistaking your intent. If not, then I still don't know that I can make things much better. I just want you to at least feel heard, and understood, and agreed with.

I'll copy a phrase from below (that I've also bolded there) to summarize my take on why I'm supporting this change despite sympathizing with you: the claim here is that, even for people who suffer the problems of code search blame, this is less-bad than status quo. That doesn't mean it's monotonically good.

Further responses inline.

On Mon, Sep 16, 2024 at 8:13 AM Michael Lippautz <mlip...@chromium.org> wrote:

As someone who works across codebases (Blink & V8) and relies on git blame a lot this will negatively affect my workflows.

(Clarity: `git blame` will auto-skip these changes once we add them to .git-blame-ignore-revs. Only code search is affected. I assume that's what you meant, though.)

 

Blink especially requires a ton of code archeology and I already stumble across mass refactorings in codesearch quite a lot.

Yes. I find both command-line blame and code search blame frustrating for a variety of reasons:

• They're sloooooow. I could forgive most other faults if they were nigh-instantaneous.
• Code search blame doesn't even work half the time.
• It's shockingly inconvenient to "skip past this CL and keep looking". That should be like... one top-level click!

So many people mentioning this in the thread. Please +1 this issue http://b/305682616. It seems like such an easy thing to fix. Who is our code-search liaison? Can we push this?

F

 

• It's waaaay harder than it should be to continue tracing across a file move/rename.

Now, admittedly that last one isn't relevant here, since we wouldn't be doing any file moves or renames. But the rest are. I think just about every engineer on the team is frustrated with this, so you're by no means in a minority.

That said, this also means the concerns you're sharing aren't new or surprising; they're the fundamental ones we've been concerned with while debating this over the last several years. They're also behind multiple bugs and email threads with several different groups, trying to get more attention to them.

I am pleased that supposedly the "slow blame/history" issue is the team's current priority. That makes me cautiously hopeful that this pain might become a lot less.

I do a lot of code reviews and have never come across any issues with brace style that were mentioned in the initial message. Even if there were nits here and there, I cannot recall a single larger discussion/problem -- people tend to just apply the new style as part of their actual other work and move on. 

As one of the style arbiters, I do see questions about this (and related issues) come up periodically. They are often not surfaced on reviews directly; a common technique is to ask out of band so as not to block. I also mentor newer engineers who struggle with reviewers asking them to go beyond the style guide, or to revert formatting changes, or whatever. A lot of these are not issues an LSC like this will address, but just as with auto-formatting in general, the more it simply eliminates ambiguity and debate, the better.

On Mon, Sep 16, 2024 at 4:00 PM Peter Kasting <pkas...@chromium.org> wrote:

It sounds to me like if folks want code search to be able to skip over these and other mechanical CLs, the biggest contribution would be to implement blame.ignoreRevsFile support in JGit. https://bugs.eclipse.org/bugs/show_bug.cgi?id=559671 is an open bug, although I believe jgit has migrated to GitHub issues (https://github.com/eclipse-jgit/jgit/issues) where I don't see a bug.

To be clear, while I would love to see the above happen, I don't think it's a blocker.

I am a bit confused why none of this should be a blocker. I agree that it's unlikely to be worked on but that shouldn't be a waiver for ignoring it.

For two years, it was a blocker. Two things changed recently:

1. Primarily, we got feedback that even some folks initially opposed to a mass-format were now reluctantly supportive of it, because a single CL is easier to skip past than having formatting changes mixed into every CL for years. Thus the claim here is that, even for people who suffer the problems of code search blame, this is less-bad than status quo. And if code search ever becomes able to properly ignore these, then at that point the LSC will retroactively become much better (in fact, the earlier we did it, the better it would have been).
2. Secondarily, we were able to change `gclient sync` such that at least `git blame` locally ignores these changes, so people have some kind of workaround/alternative.

Given these, it seemed like we perceived the cost of doing a mass format as having decreased and the cost of not doing it as continually increasing, so changing this from "blocker" to "non-blocker" made sense.

I have seen very little agreement that the refactoring is solving a huge problem.

I don't think it's "huge", I think it's a lot of small papercuts spread across a large surface. Which is similar to the nature of reformatting LSCs affecting blame trawling: it's a periodic annoyance, whose salience differs for different people.

As for why you haven't seen significant agreement, that's partly because the discussions on this have largely not taken place on chromium-dev@, but on Slack and on cxx@. (Neither of those is a closed forum, BTW, so you're welcome to join if this sort of discussion is something you want to be involved in.) I think that's probably for the best on net, but see also the concerns I mentioned in the initial mail in this thread about there being no formal process for such changes, and all the decision routes being problematic.

I hope this helps make the process and thinking behind this one more clear. Definitely interested in any concrete suggested improvements you have -- we've already gotten at least one good feature request bug out of this thread.

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/CAAHOzFDQ%3DbPXOLTGHmpkSOTvsFTecK68gSGPw6NDaz5GrPh6%2Bg%40mail.gmail.com.

[Peter Kasting]

Final update for this thread:

• Thanks to Rachel for offering to escalate the general issue of code search blame pain. This is longstanding, and any fixes we can do will help everyone.
• I will write a doc with some process questions for LSC owners/ATLs around decision-making authority, notification/implementation processes, and escalation paths for cases like this.
• We'll make sure the above issues are resolved, and any change we land here is in accordance with the decisions there.

Thanks folks,

PK