Reformatting 100k Files at Google in 2011

https://laurent.le-brun.eu/blog/the-story-of-reformatting-100k-files-at-google-in-2011

by laurentlb • 2 years ago

230 155 2 years ago

wonger_• 2 years ago

Autoformatting is so nice. Crazy to think that formatters only became popular after `gofmt`.

I also found this related quote from Russ Cox intriguing: "Most people think that we format Go code with gofmt to make code look nicer or to end debates among team members about program layout. But the most important reason for gofmt is that if an algorithm defines how Go source code is formatted, then programs, like goimports or gorename or go fix, can edit the source code more easily, without introducing spurious formatting changes when writing the code back. This helps you maintain code over time."

DangitBobby2• 2 years ago

That doesn't really seem like a good reason. They remove all formatting control with an automated tool so it doesn't matter when (if, big if) an automated tool later rewrites some of your source code formatted in a way you don't control?

Nzen• 2 years ago

Mu. Russ Cox quote indicates that the maintainers of goimports and gorename do not need to handle formatting at all (and hence, benefit). The go team put that wholly in the gofmt team's court. Otherwise, the go team would likely have to handle tickets about how the formatting of goimports and gorename differ over time for edge cases (if only by paying the social capital of ignoring the tickets).

striking3• 2 years ago

It's more common than you think! Tools like https://github.com/facebook/jscodeshift, https://ts-morph.com/, and https://ast-grep.github.io/ are the ones I'm more familiar with (since my day-to-day is TypeScript).

When I led the conversion of a decently-sized codebase from Flow-typed JS to TypeScript, I ensured that a code formatting tool that we were already using on pre-commit and CI called `prettier` was executed after each step. We took a git snapshot of each step of our automated conversion pipeline, and the diff was much clearer with `prettier` in place at each of those steps.

We've since used codemods frequently to make huge changes to the codebase in an automatic, reproducible, iterable way. They're very comfortable, very fun, and (thanks to the use of a formatter on all code) rarely produce incomprehensible diffs.

delichon• 2 years ago

It reduces cognitive load on integrations, both software and wetware. So many brain hertz are wasted on internally bikeshedding format details. Autoformat fixes a hole where the rain gets in and stops my mind from coding.

autarch5• 2 years ago

The Perl world had perltidy (first release in 2002) many years before Go was even a thing. It's funny that Perl, a language notorious for it's "There's More Than One Way To Do It" (TMTOWTDI) philosophy had a tidier so early. Of course, perltidy is _ridiculously_ configurable.

One thing I really love about gofmt is that it has no configuration at all. I think that was a major "innovation" and I'd love to see more languages adopt this approach.

XorNot• 2 years ago

There were also various Python formatters. The problem was the zeitgeist - engineers would tell me that "they didn't want an algorithm messing with their code" as though that was a serious concern, or that wading through endless pull-request changes for syntax was a good use of anyones time.

ravishi• 2 years ago

The thing about configurable vs non-configurable in this case is that when its configurable then people will spend time debating how exactly they should configure it.

rurban• 2 years ago

I now almost always format C and C++ with clang-format -I pure, without any .clang-format overrides.

There's no need for lengthy discussions with co-workers then. Just for bigger projects there's a need for Statement Macro Declarations, but that's arguably a bug/limitation in clang-format

freedomben4• 2 years ago

Perl is a fascinating example because it was so ahead of its time, so innovative, forward-thinking, so wonderful, and so loved, and yet today is looked upon with such disdain. I remember people battling for Perl positions and being so happy to get them! Now you can't even find people that admit to knowing Perl ;-)

Someday I want to study this and really understand what happened.

afavour2• 2 years ago

I’ve wondered before whether the world would be well served by a programming language (or source control system, I suppose) that just stores ASTs in files rather than text code. When users open the file the editor formats to whatever their personal preference is, then saves edits back to the AST.

It really is dumb to be arguing over tabs vs spaces, after all.

rsc9• 2 years ago

At that point you could even use different languages. Maybe you like programs that look like Lisp and I don't. There was a project at Microsoft Research in the late 1990s/early 2000s that did exactly this - storing ASTs in source control instead of code - but the name escapes me at the moment.

turtleyacht• 2 years ago

One (naive) approach I keep thinking on is using ed(1) to write transforms of code, so chunks of Java are later sewn together to create the app.

This always sounds more difficult on paper than just wrestling dependencies till dawn, upgrading from JDK 11 to JDK 17, for example. So I usually give up the mental exercise there.

Plus, following a file of transforms is mind-bending: someone may follow a method definition with a pattern seek, and then start appending some more code. Context is lost. It would be literate programming only with enough empathy for comments.

Which is all to say, would it be easier to move between Spring versions if the app's commit history were a series of transforms instead of changes to static files?

Suppose a commit establishes a framework version, and then follow a bunch of commits for domain objects, a skeleton controller, and so on. If we could play those decisions forward, but edit the transform instead of the source, would it be easier to dissect which next dependency to manage?

This loops back to ASTs: we would still edit and change files, but the history would be ed(1) macros (or something better, like ASTs). Somehow, it feels like there could be reconciliation between source control and "manipulating a timeline of changes."

Git may already have this, or a simple while loop with some decisions about how far to play the changes, like editing a cassette tape. A list of patches to apply, with pre- and post- hooks for rules scripts.

striking3• 2 years ago

A sufficiently-motivated engineer could do this today by setting a different formatter in their editor than in their pre-commit hook. Then they need only activate the formatter in their editor as they edit, and ensure pre-commit hooks run as they commit.

This saves you the trouble of authoring a separate programming language, or finding a way to preserve all of the niceties of the original syntax and formatting that wouldn't directly translate to an AST (like how many newlines are after a particular stanza or function).

Case in point: Recast (https://github.com/benjamn/recast) is of particular interest with regards to JS/TS in this vein, because it does preserve a lot of the spirit of the source in its conception of the AST. But also last time I used it (couple years ago now) it would explode on any code with an emoji in it. It's genuinely not an easy problem.

shawn_w• 2 years ago

I think that's what InterLisp did.

eru4• 2 years ago

It really is dumb to be arguing over tabs vs spaces, after all.

In an in-house dialect of Haskell I used to work with, we solved this problem by just making tabs a syntax error. Never had any problems.

(I think tabs might be have been allowed inside strings.)

python_pele• 2 years ago

Unison has something similar to what you're talking about: https://www.unison-lang.org/

coolcoder613• 2 years ago

Many BASIC dialects did this.

tengbretson• 2 years ago

Does that mean that a file with a syntax error is not able to be saved?

duped3• 2 years ago

One very nice property of an AST is that it's an IR that is allowed be instable even if the syntax it represents is stabilized. If the AST becomes the source of truth on the file system you lose that property.

On top of that, now you need to write a parser and compiler for your AST file. It's probably very simple and does rudimentary validation, but that defeats the point of the AST - it's a valid, canonical representation of a program by construction.

All in all, it seems like a good idea, and people have done it. But there's also good reasons to be apprehensive.

And at the end of the day, you need to ingest text as input, and you need to do it as fast as possible. There's not a ton of benefit to keeping an AST around on disk and in sync with the text that generated it when you are already able to compute it faster than you can read and deserialize it.

lmm• 2 years ago

No, because there are no good tools for this format. It's a classic "worse is better" problem - a programming language cannot succeed unless its source code format is unix text files.

arp2422• 2 years ago

Well-formatted code is more than just an AST. Even in Go, a language with probably one of the lowest style divergences out there (for better or worse) there are tons of style choices that aren't in an AST.

Blank lines is an obvious one: where do you insert them to "group" sections of a 30 line function? Or are there no blank lines at all?

Line length: just "wrap at columns X" (or never wrap) is not enough, because people can and do wrap at specific locations for specific reasons, because that makes more semantic sense or looks nicer than cramming as much as possible.

omoikane• 2 years ago

Autoformatters got popular was because a lot people don't care about formatting, and those who do care can't win against the auto part of autoformatters.

It works for go because gofmt was there from the start, so even if you are returning a multi-dimension array and elements come out unaligned, that's just accepted as how it is and nobody cares. For other languages, people will have to either accept "not caring" as becoming the norm, or actively fight the autoformatter from steamrolling over their code.

For people who would give more thought to how their code would be read, autoformatters were often more frustrating than "nice".

YZF3• 2 years ago

Interesting enough people still find things to argue about even with gofmt and goimports. The order of imports and maximum line length are two examples. I imagine if those were done by gofmt/goimports then people would argue about other things.

It's why we can't have nice things... Autoformatters do help though.

freedomben4• 2 years ago

Yes precisely this. I worked on a team in C++ that vertically aligned things, and at first it seemed like extra work but once I started seriously reading the code, I had an epiphany of realization that it is an amazing improvement in readability, but also makes it super easy to find certain bugs, and also do block editing in tools like vim. After leaving that team, I found that everywhere I tried to introduce this people just shrugged and rolled an auto-formatter over and told me that it's not worth thinking about.

IMHO autoformatters are awesome until there's something you care about that the implementers didn't care about, then they are horrible. Problem is, the people who put a lot of thought into those decisions are often in the minority and tend to lose the argument.

rowanG0773• 2 years ago

I never thought it only got popular with go. I remember using formatters before go. What go did do is that it shows how good it is if it is ubiquitous.

plorkyeran• 2 years ago

I think mandatory code formatting was popularized by Go? Plenty of code formatting tools had been around for quite a while by the time Go came out, but they were the sort of thing you ran on a codebase once to clean up a mess, not something you checked on each commit. A common sentiment was that it was a good idea but was only viable because Go had it from the beginning, which fortunately turned out to be wrong.

fweimer• 2 years ago

Java IDEs have had optional reformat on safe for a long time.

https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclip...

jakjak123• 2 years ago

They have, but the different IDEs do not format equivalent and are way too easy to reconfigure to personal preference.

vitiral5• 2 years ago

I'm going to take a contrarian view here. Code formatting is amazing in a corporate environment where nobody truly cares about their code -- it's just a means to get a paycheck. It's also great for beginners to a language who are still trying to get a handle of the syntax.

But where you are nearly the sole owner of a small library and you are crafting that library to be beautiful and understandable... there is something pleasurable about structuring concepts so you have each on a single line, or creating similar functions so the concepts are structured by column.

I know not everyone will hold this view and that is fine, but when you are writing your own hobby library in your favorite language for your own purposes I recommend you try it out.

JZL003• 2 years ago

I do more research, one-off ,code and being able to align equals signs or successive lines which do similar things both makes me happy and makes it easier to skim. I think the Linux kernel does it too. Emacs has a way of aligning by common separators (=/, as well as quick regex)

I guess I've never gotten into an actual serious debate with someone over formatting so I don't know what I'm avoiding, but sometimes auto formatted code makes it harder to skim

lokar• 2 years ago

Everywhere I’ve worked, including Google, many (probably most) people care a lot about the code and the format. Often too much. That was the point rsc was making.

neilv2• 2 years ago

Also, sometimes the formatting simply makes reading something messy merely tractable, not aesthetically pleasing.

Once, a product launch depended on me urgently kludging a device driver in Python (long story). And this involved a large hand-maintained mapping table. I wrote it quickly but carefully, and found some formatting that made the table readable enough, without implementing a minilanguage in Python.

But the Black formatter had been rigged to run automatically on commit, so... poof! :)

tczMUFlmoNk• 2 years ago

Black supports ignoring sections with a `# fmt: off` directive, and a hand-formatted constant table is a common use for that:

https://black.readthedocs.io/en/stable/usage_and_configurati...

Other formatters have similar functionality; e.g.:

- /* prettier-ignore */: https://prettier.io/docs/en/ignore.html#javascript

- #[rustfmt::skip]: https://github.com/rust-lang/rustfmt?tab=readme-ov-file#tips

markrages• 2 years ago

Yes! Code is for people to read, not computers. Beautiful code uses the full expressiveness that the language allows.

JyB3• 2 years ago

And people have, by definition, a much easier time reading code when it’s always consistent across codebases. Thanks for making the point.

striking3• 2 years ago

I think you're doing yourself a disservice by having to play with the formatting itself instead of being able to express what you mean with the tools the language already provides you.

As soon as anyone else looks at your code or wants to participate in the development process, what is meaningful to you about the arrangement simply won't translate into their view; what is the use of a programming language if not to use its existing definitions and concepts to communicate the concepts of programming?

You can use fancy syntax tricks, but domain-specific languages are a much better way to handle the same problem. You can express things with them that other humans can understand while still retaining access to your existing formatting tools.

vitiral5• 2 years ago

I personally dislike ad-hoc DSLs that aren't composed of the language's existing syntax.

Considering I get pleasure from formatting the code beautifully I'm not sure how it's even possible to be doing _myself_ a disservice. It's like saying I'm doing a disservice learning guitar instead of listening to Coldplay on my speakers.

As far as others are concerned... I agree somewhat, but I don't think it's by any means proven. I think we are always better off then cobbled together code where formatting hasn't been thought about. By well-designed hand-crafted formatting can be expressive in it's own way IMO

thfuran2• 2 years ago

You'd write a DSL to somehow clarify formatting a matrix literal instead of just putting some spaces as needed to get the rows and columns to line up cleanly?

rowanG0773• 2 years ago

I mean you can do whatever you want in a hobby project you work on solo.

I do want to say that I have the opposite view. People who use formatters want their code to be consistent and go the extra mile it ensure it does. It's like manual testing vs automated testing to me. Sure with manual testing you can test many more corner cases as they come up as an intelligent person is in the loop. But there will be mistakes made, tests forgotten etc. Just like there will always be inconsistencies when you manually format the code.

vitiral5• 2 years ago

Im not sure how it's even remotely related to testing. I do agree that it depends on the person, just like every medium depends on both the artist creating the work and the patrons viewing it.

Many people who use formatters (myself included) just want consistent code and don't want to bicker with others about it. When it is soley owned by me and I'm doing it for fun, these reasons fall away for me.

erik_seaberg• 2 years ago

This. Automatically doing it well (clarifying the intent of the code) is an AI-complete problem. That's not an excuse for automatically doing it poorly and forcing everyone else to live with it.

paulddraper2• 2 years ago

I don't find the mental burden of formatting code to have a positive ROI.

I want to write syntacially-valid code, without worrying about the visual presentation of it. (I want a good presentation, but I don't want to put forth the effort to create it.)

fragmede4• 2 years ago

Yeah that's why I fight with how I feel about python so much. In other languages I can just shove the curly bracket in the right place and fix the whitespace later, but Python has whitespace as significant, so I'm forced to fix it/get it right. But that means it's right when I dig a "temporary" script up years later.

flymasterv2• 2 years ago

I don’t understand why they had to format 100k files. You enforce the format with a presubmit and let the code get formatted in the next change.

I have long felt that Google’s strength has always been making a bad architectural choice and then executing on it flawlessly. So many systems are designed in ways that require incredible technical execution to make them workable, and they do it.

oceanstone• 2 years ago

Ex: Angular

valicord2• 2 years ago

Because then every commit from now on has a ton of formatting changes that make it harder to see what was actually changed.

flymasterv2• 2 years ago

And this is a flaw of Perforce: in a Git/Mercurial system, the presubmit can stack the changes into two commits. In P4, one CL has to contain both changes.

And Google uses P4(ish) because monorepo, so they build further abstractions over P4 to enable git and hg in user space which erases most of the potential benefits of either which is also all really, really good software, but it’s all effort necessitated by monorepo. CitC is a work of art, but it is also something necessitated by a stack of other choices that forced their hand into inventing something miraculous to keep hacking around a previous limitation that nobody else has.

rsc9• 2 years ago

If you do it that way, then what should be a 1-line BUILD file change turns into something that changes every line. It distracts from the actual purpose of the future change. Many directories aren't touched for long periods of time. A few months from now someone tries to make a 1-line change and is unpleasantly surprised they have to deal with tons of seemingly spurious formatting changes. Not good.

Putting the time of submitting the changes on a small team (mostly me, with approvals from Rob and help from Laurent) was absolutely the right tradeoff. It avoided the "unfunded mandate" and tech debt of making everyone else deal with it.

Update: I found the FAQ we wrote back then. It was very short. These were the last two questions:

Q: Who will update all the existing BUILD files?

A: We will. There are nearly 200,000 of them, and we’ll take care of that. We’re sending CLs out now. If you want to do it yourself, that’s fine: see go/buildifiernow for a tool that can help.

Q: You’re creating a lot more work for me.

A: We are creating significant amounts of work for ourselves, including reformatting all 193,000 BUILD files in google3. For the rest of the engineers in the company, we intend to make the transition as smooth as possible, with integration in Eclipse, Emacs, and Vim, as well as tools like Rosie and GenJsDeps. It is an explicit goal not to create significant work for other engineers. If, as we roll this out, you find that we’ve created noticeable work in your workflow, please let us know so that we can address that.

diegocg• 2 years ago

Make the one line change be a commit, then the reformatting be another one, review only the first one. It shouldn't be a problem with a proper review system.

y2mango3• 2 years ago

Foremost: formatting these BUILD files was the correct decision as rsc already explained.

Then, there are also other formatters that support "incremental formatting", meaning it only formats lines that are changed in your commit.

Disclaimer: I authored https://github.com/google/pyink and replaced Google Python's YAPF formatter with this Black fork and also implemented the "incremental formatting" feature in Pyink and upstreamed to Black.

When we were rolling out the formatter change, we chose to NOT format the Python files mainly because 1) not all teams at Google enforce Python formatting at presubmit time; 2) the formatter supports "incremental formatting" to minimize the diffs introduced by the formatter.

There are of course less ideal cases where even incremental formatting has to touch not-changed-lines, such as a large Python dictionary/list/set literal that spans across dozens or even hundreds of lines. It's a tradeoff in the end.

rsc9• 2 years ago

My notes say it was 193k at the start. The final dashboard when we stopped said "216,626 / 216,890 = 99.8%; 264 to go".

The other correction I would make is that this post does not mention Nilton Volpato, who had written an earlier Buildifier and graciously accepted replacing his implementation with a new one and then taking over ownership for that new implementation as well. (Eventually ownership moved to Laurent's team.)

It looks like it was just under 2,000 commits. We did pretty extensive testing, by having Blaze load a BUILD file and its transitive closure and then dump that parsed form back out to a binary format. Any automated commit had to preserve that parsed-and-dumped binary format bit for bit. The slowest part of the testing was waiting for Blaze to do all the loads.

Every day I would prepare and test as many files as I could, break them into CLs (think PRs), mail Rob a shell script he could run to approve them all, and go to bed. Then I'd get up early in the morning (5am ET) to submit the changes, because there were various cached indexes that got updated when BUILD files got submitted, and it seemed better to send them when not many people would be working.

That scheme worked until a system did fall over and someone got paged, and then after that I agreed to only submit the large changes during business hours. :-)

laurentlb4• 2 years ago

Thanks for the precisions! I've added some updates at the bottom of the post.

haburka• 2 years ago

You didn’t have Rosie to automatically split up your changes and send them out yet?? That must have been rough. LSCs are way easier now

ammar2• 2 years ago

Yeah I'm surprised by that as well. As far as I remember, Rosie started out in 2010 and people were using in 2012. Maybe the clustering/splitting didn't support this use-case or it wasn't well-known enough?

rsc9• 2 years ago

(Rosie is a code janitor program at Google that takes a change that affects files all across the tree and automatically cuts it into individual changes that can be mailed out to the individual teams as CLs/PRs for their approval.)

Rosie existed but very much wanted to break up the CL into independent per-directory CLs, and since I was editing one file in every directory in the entire tree, that would have been 200,000 independent CLs. I broke the list up by top-level directory or sub-directory and hit 100+ directories at a time.

Rosie also really wants to run each affected directory's tests, and I did not, because at scale flaky tests and such would be a significant source of false positives. The bit-for-bit check on the internal parsed representation of the meaning of the BUILD file proved that the changes were no-ops. That was better than any tests of the code in the directory.

I was already automating everything else, including deciding which files to change, reverting edits in files that were concurrently modified (they got swept into the next attempt), and the testing. Running a shell command to actually make the CLs was not difficult. And it generating the approval script trivial too.

Rosie is great but it wasn't the right tool for this job.

troad2• 2 years ago

The use of light grey text on a darker grey background strains my eyes and makes this unnecessarily unpleasant to read. I'd respectfully suggest increasing the contrast dramatically.

I keep a quick little scriptlet in my bookmark bar for cases like this:

  javascript:(function(){ $('head').append('<style>*{color:#101010 !important; background:#f0f0f0 !important;}</style>'); }());

(A ten second hack job; suggested improvements from front-end friends are welcome.)

laurentlb4• 2 years ago

Thanks, I've increased the contrast (not as much as your suggestion, but I hope it's better).

Dagger2• 2 years ago

I've been using this:

    javascript:(function() { for (var n of document.querySelectorAll('a, p, li, div')) { n.style.color = (n.nodeName == 'A' ? 'LinkText' : 'CanvasText'); n.style.backgroundColor = 'Canvas'; n.style.font = '500 16px/1.4em sans-serif'; }})();

It uses system colors and thus, if your browser supports them, should adapt to dark mode automatically. Using .style has the advantage that sites can't override the style themselves using .style. (You'd think looping over all these elements would be slow, but it's not.) This version also works on sites that aren't using jQuery, although it wouldn't be hard to use `var s = document.head.appendChild(document.createElement("style")); s.innerText = "...";` for that.

I was surprised at how helpful forcing the font face and spacing is. There's a lot of sites out there with bad-looking fonts or huge line spacing on top of unreadably-light gray.

I added the background color part based on your version. Thanks for prompting me to try that; the way my bookmarklet didn't work on black backgrounds was occasionally a problem. I also added a bit to force link colors, since neither of our versions handled those well.

Perhaps the next step is a "multistage" bookmarklet that applies more rules the more times you click on it, so the more forceful rules (like background color, which often messes up other parts of the site design) can be optional.

troad2• 2 years ago

Thank you, this is great! I hadn't considered changing the font for greater readability, but it's an obvious improvement. Good job with the links, too, that was a great idea. I've added 'blockquote' to your tag list, since I find that often gets a background shade of its own (e.g. code snippets), but otherwise I've saved it as is.

I tend to prefer interfaces in dark mode and content in light mode, so I'll see how I feel about the conditional logic there, I may eventually wind up going back to hardcoding some colours.

Perhaps the next step is a "multistage" bookmarklet that applies more rules the more times you click on it, so the more forceful rules (like background color, which often messes up other parts of the site design) can be optional.

That's a really neat idea. I can imagine it stretching from slight readability tweaks all the way to a pseudo-reader mode. It would definitely be a bit more of an undertaking than either of our quick snippets, though.

bsmith0• 2 years ago

I'd be curious what role "global approvers" at Google/Google's scale typically have/how many are there/what's the process?

dmazzoni• 2 years ago

I was at Google until a few years ago.

The purpose of global approvers was exactly things like this. If you want to do a mechanical change to an insanely huge number of files, they can potentially approve it.

In my experience, global approvers were used extremely rarely, only in cases like this where the transformation was purely mechanical and it was possible to verify that there were no logic changes.

Most of the time rather than global approvers, you were encouraged to use a system that would automatically split your change into a bunch of smaller CLs (PRs), automatically send those to owners of each module, then automatically merge the changes if approved. It would even nag owners daily to please review. If you had trouble getting approval for some files you could escalate to owners of a parent directory, but it'd rarely be necessary to go all the way up to global approvers.

Basically if there was even the slightest chance that your change could break something, it's always safer to ask individual code owners to approve the change.

tylerhou• 2 years ago

Even if you get global approval it is still good to split CLs to avoid e.g. merge conflicts.

y2mango3• 2 years ago

I was one of the global approvers (and also on the Python team until my role at Google was eliminated recently). I no longer have access to the stats, but from my memory, there are currently 50+ global approvers depending on how many are still considered active.

Typically, code authors would create a proposal by filling out a doc template. It's usually light weight and also accompanied with examples or full set of the pending code changes. Then 1-3 of us will review and LGTM the proposal. As part of the review, we also determine whether the changes should be sent to local code owners, or "globally approved" by one of us. The default option is to use "global approval", unless the changes need local code owner's knowledge during the code review. Said in another way, when sent to local code owners, their role is not gate keeping the changes, but to provide necessary local knowledge where we as global approvers don't have.

Refactoring changes, such as formatting or API migrations, shouldn't bother local code owners because 1) it would just be a waste of their time to review and approve; 2) in practice, we find a central code reviewer for the same large set of code changes is more likely to catch bugs (with review automation tooling) than local reviewers.

We consider ourselves as facilitators rather than approvers or gatekeepers of the code changes. Our goal is to make these changes done more efficiently and save engineering time when possible.

If you like stats: over the past 5 years, I have reviewed ~300 such proposals and ~40K changelists (equivalent to PRs). One changelist/PR typically contains 10s to 100s of files depending on the nature of the change. When I was most active, I was about ~5th-ish when ranking the number of changes we were approving. There are many global approvers who have approved more than 100K changelists, which is a milestone we celebrate with a cake. Too bad I didn't have the chance to have my cake.

mudkipdev• 2 years ago

Just curious, what kind of global changes do you usually make? And what is the process of becoming an approver?

yegle• 2 years ago

There are 3 tools that that makes maintaining BUILD files enjoyable: buildifier, buildozer and build_cleaner (internal only unfortunately).

lopkeny12ko• 2 years ago

What is build_cleaner?

randomifcpfan• 2 years ago

A tool for updating bazel build target dependencies. It inspects build files and source code, then adds/removes dependencies from build targets as needed. It requires using global include paths in C/C++ sources. It is not perfect, but it is pretty nice!

rapfaria• 2 years ago

After reformatting, did git blame always pointed to those commits and the most recent author, or were they added to --ignore-rev?

Arainach• 2 years ago

Google doesn't use Git internally, and its code search and source control tools expose the concept of "show blame before this change", so in practice reformats like this aren't troublesome with regard to blame.

montag2• 2 years ago

This is true but I think it would still be nice for VCS to have a first class concept of "peek through" changes (whitespace, formatting, etc.) for the purpose of blame.

tantalor2• 2 years ago

Blame can be configured to ignore cleanups like this.

mckn1ght• 2 years ago

This has always been something I use to argue against just reformatting code in isolation. TIL git provides a way to ignore that. Thanks!

rsc9• 2 years ago

Not git, but yes, it did mess up the blame lines. Luckily most people don't care about the attribution on BUILD files. But I did get the occasional email for years afterward about "hey looks like you added this build target, can you explain it?" or just an automated tool deciding I was the last person who edited the file so I was a good reviewer for a future change. Those pings were a fun tour of directories I'd never heard of (but did edit). Been a few years since I got one of those.

ragall• 2 years ago

The real lesson here should be that source code shouldn't be stored in a text format, but in a well-defined strict binary format that stores the parse tree directly, which completely eliminates the need for formatters.

mmastrac• 2 years ago

A well-defined, unambiguous formatting standard for a text file is philosophically identically to a well-defined strict binary format. :)

YZF3• 2 years ago

I agree with this sentiment. It also provides an opportunity to store other metadata with the code. It lets different people choose their formatting preference when rendering back to human readable code.

The source code is text thing seems like a "legacy" concept.

At the very least I think this is an interesting thing to explore. Maybe it leads nowhere...

jerf2• 2 years ago

The term "bikeshedding" comes up a lot on HN, when people spend a lot of time spinning wheels in endless debates about the things that are easy to debate while indefinitely deferring the tough conversations, but I think an underappreciated aspect of this meme is that the unimportant conversations quite frequently really are just that; unimportant, to everybody involved. Often (but not always) nobody actually cares. Caring is not necessarily represented by the amount of time or text spewed forth on a topic. A lot of things other than passion or importance factor into how much verbiage comes through and it is a mistake to overinterpret mere volume.

Unfortunately, the flip side of this coin is harder to deal with; sometimes truly important issues or things that people do deeply care about have disproportionately too little verbiage. Finding those can be very difficult.

fragmede4• 2 years ago

Caring is not necessarily represented by the amount of time or text spewed forth on a topic.

The meta says that it is. there are only 81600 seconds most days, and you get to choose them how you want, so choose how you spend them wisely. if that's arguing over tabs or spaces, then that's your choice.

rsc9• 2 years ago

This One Trick Will Add 80 Minutes To Your Day

jerf2• 2 years ago

The meta would say that if people were optimally spending their time on things that matter to them. They don't. If they did bikeshedding wouldn't exist. It obviously does.

This is basically the same claim that economics can treat humans as perfectly rational actors perfectly rationally pursuing their perfectly rational goals. It is not a good model of humanity.

aiuto2• 2 years ago

I want to start by saying that I do not want to diminish or disparage the work that Russ, Rob, Laurent, and others have done. It has made the Google code base better. That is an inarguable fact. Nor do I want to pick on buildifier or gofmt or any other tool as a singleton problem. I'll talk about buildifier because that is what I personally fight with. Others may have different demons. (YDMV - "your daemon may vary". I'm taking the authorship of that one now, in case it ever takes off).

But back to the point... formatting rules without firm, incredibly strict enforcement ends up being a tax on the janitors - the people who clean the code base and do large scale changes. That makes me sad. These are the people who care a great deal about code health, and their work is hindered by the lint checks that we have imposed.

Let me give an example.

I'm trying to eliminate a constraint in the build system. It's a "small" large change - only O(30K) instances. (Yeah, Google scale is different). I have an incredible wealth of tools available to me to automate the process. For the benefit of the Googlers, I can identify Blaze targets to change, use buildozer to fix them, and ship off CLs to review. But the changes I want to make are often ones which should be reviewed by the code owners, and not globally approved. So possibly O(10K) individuals might be involved in reviews.

Let's explore the problem. First, shouts to y2mango for bringing up incremental formatters. This should be the default for all tools. And another to flymasterv for raising the question of "why not just format as each person touches a BUILD file". Here's the situation.

1. buildozer is really good at rewriting BUILD files syntactically correctly. 2. It has an unfortunate side effect of not being incremental. It calls buildifier to rewrite the entire file. 3. We update the formatting rules to make them stricter over time. That means that a "correct" BUILD file on January 1, might require changes on March 1. 4. Buildifier findings are advisory, rather than mandatory. 5. No team is staffed with repeating the monumental work this post started with.

The reality on the ground is that little touched BUILD files become stale, and would require a formatting update over time. It is actually worse than that, because many teams take the path of ignoring buildifier warnings and committing their working code anyway. Without continual BUILD file reformatting there is a lot of stale floating around. [Root cause: We could fix this by promoting people for doing that repeat work. But we don't. We promote for the initial sprint.]

And then a janitor comes along.

I use bulldozer to fix a problem. It reformats an ancient BUILD file completely (not incremental). I send it to the code owner. They see changes far beyond my 2 line fix. They reject it, or ask for a change to only the two lines that actually mattered. Sure. I can hand build the change once or twice. But not for a few hundred, or thousands files. So.... I have to hack up an incremental format. Or, it turns out that users are very happy if I don't bother with formatting at all, and just change single lines. It's not that any individual is right or wrong. It is that they all have a choice and a preference and Google created a policy that allowed individual teams to have a choice of strict compliance or not. That is the failure.

If you are going to have a policy about code formatting: - make it hard mandatory for everything except a "break glass" situation - if the policy can evolve, staff a team with enforcing it globally

The fact that Google, as a company, does not reward this behavior does not take away from any individual's accomplishments. This post may sound grumpy to an outsider, but I am constantly amazed at the tools I have available to fix things on an enormous scale. The friction is usually only where we have good intentions, without the policy teeth to enforce alignment with the intentions. That's a management problem, not a technical one.

repsilat• 2 years ago

For this sort of change I think the best strategy is "two passes".

Auto-format those O(30k) files and get global approval. Then, separately, make your two-line semantic change and seek approval from local owners.

aiuto2• 2 years ago

That is missing the point about the misdirection of costs. Your suggestion forces the people doing meaningful semantic changes, into involuntary servants to the goal of cleaning the stylistic problems. It's fine to have a policy that costs a little to each of the owners of their own code. It's a tax for the overall good. It becomes a problem if the cost of compliance is shifted to "the next person who looks at it." That encourages people to not look at it.

MBlume• 2 years ago

Seems like the solution here is to set up buildifier so that it can send out robot CLs on an ongoing basis.

spc476• 2 years ago

formatting rules without firm, incredibly strict enforcement ends up being a tax on the janitors

Wait! I thought Google only promoted and rewarded people who do new development, not maintenance. What gives?

freedomben4• 2 years ago

I think that is what GP was saying:

[Root cause: We could fix this by promoting people for doing that repeat work. But we don't. We promote for the initial sprint.]

rsc9• 2 years ago

I can't speak to what has happened with Buildifier, but in general you are right. It has to be a hard rule that if you change the format rules, you have to reformat everything to match.

If that means it's too hard to change the format rules, then don't change the format rules. And if you don't reformat, then it has to be a clear rule known to everyone (or written down somewhere you can point to) that incidental formatting changes are acceptable and not something you are allowed to push back on.

I can speak to Go and gofmt, and there we are VERY reluctant to change formatting rules. It does happen for the odd corner case once in a while, but nothing that would cause "changes far beyond my 2-line fix".

vasco• 2 years ago

Sounds like a quick way to get Golang installed on every Google developer machine and drive adoption.

rsc9• 2 years ago

No, because Go compiles to binaries, like C/C++ but unlike Java or Python. Only the buildifier binary needed to be installed on everyone's machine.

(I think Go was already installed on every Google developer machine at that point anyway.)

Reformatting 100k Files at Google in 2011

Related Stories