4 ms·

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 fe

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 2y ago
Ex: Angular
valicord 2y ago
Because then every commit from now on has a ton of formatting changes that make it harder to see what was actually changed.
- flymasterv 2y 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.
  - valicord 2y ago
    >the presubmit can stack the changes into two commits That seems like way more complexity than just doing it once and for all. Now the commit log is littered by a bunch of automatic commits that format one file at a time.
    - lesuorac 2y ago
      The commit history for untouched files is mostly just cleanup CLs for reformatting or changing an import for a decade+. A lot of the history is rather useless for finding a bug but at least its generally well tagged with 'CLEANUP=TRUE' so you know to ignore them.
  - lesuorac 2y ago
    You can effectively submit two CLs at the same time where the first CL is just a formatting change and the second has just your changes. Although you would need approval for both CLs which really is no different than if you used Git/Mercurial.
  - fragmede 2y ago
    git5's been depreciated. fig/hg is well supported though.
rsc 2y 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 2y 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.
  - dilyevsky 2y ago
    Googles source control system at the time (Perforce) didn’t allow for this at least easily. Not sure about now
  - tantalor 2y ago
    All changes need to be reviewed. That's the point of code reviews. Your suggestion would allow people to bypass the code review by just saying "oh it's just cleanup don't worry".
lopkeny12ko 2y ago
[flagged]
- refulgentis 2y ago
  It's saturday night, in spring, it's really beautiful out. Another year alive. It gives me joy and reminds me to ease off a bit. In unrelated news, OP was suggesting no 100K file CL, and a presubmit. They were not disputing what the article said. They were suggesting sharding out the initial formatting change to 100K individual CLs.
y2mango 2y 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 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.

oceanstone 2y ago

Ex: Angular

valicord 2y ago

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

flymasterv 2y 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.
- valicord 2y ago
  >the presubmit can stack the changes into two commits That seems like way more complexity than just doing it once and for all. Now the commit log is littered by a bunch of automatic commits that format one file at a time.
  - lesuorac 2y ago
    The commit history for untouched files is mostly just cleanup CLs for reformatting or changing an import for a decade+. A lot of the history is rather useless for finding a bug but at least its generally well tagged with 'CLEANUP=TRUE' so you know to ignore them.
- lesuorac 2y ago
  You can effectively submit two CLs at the same time where the first CL is just a formatting change and the second has just your changes. Although you would need approval for both CLs which really is no different than if you used Git/Mercurial.
- fragmede 2y ago
  git5's been depreciated. fig/hg is well supported though.

rsc 2y 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 2y 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.
- dilyevsky 2y ago
  Googles source control system at the time (Perforce) didn’t allow for this at least easily. Not sure about now
- tantalor 2y ago
  All changes need to be reviewed. That's the point of code reviews. Your suggestion would allow people to bypass the code review by just saying "oh it's just cleanup don't worry".

lopkeny12ko 2y ago

[flagged]

refulgentis 2y ago
It's saturday night, in spring, it's really beautiful out. Another year alive. It gives me joy and reminds me to ease off a bit. In unrelated news, OP was suggesting no 100K file CL, and a presubmit. They were not disputing what the article said. They were suggesting sharding out the initial formatting change to 100K individual CLs.

y2mango 2y 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 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.