4 ms·

I disagree. 1 PR should map approximately to 1 benefit (ie. a new feature or bugfix). If the feature is huge, the PR can be huge. If reviewing huge PR's is

I disagree. 1 PR should map approximately to 1 benefit (ie. a new feature or bugfix). If the feature is huge, the PR can be huge. If reviewing huge PR's is a burden, I will make better tooling for it. What I don't want is a big feature to be split into 10 PR's, where the first 9 provide no user benefit and are just refactors or adding dead code ready for the last PR.

bayindirh 3y ago
Also, A PR can contain tons of commits, and if the commits are well cut, it's very easy to understand implementation of a new thing step by step. This is why Linux kernel workflow contains n patches per feature. To understand what's done in a clear manner. Lastly, adding feature flags to mask incomplete feature code paths is very risky, and is a bug by itself I think.
Zacharias030 3y ago
Wouldn’t it be beneficial to get the 9 refactors merged even though the 10th commit needs iteration? It would make it much easier for colleagues that can then branch off of the tested and merged refactors without running into merge conflicts later.
- oooyay 3y ago
  No. If the feature requires 10 commits and only 9 are universally agreed on then under this model you would merge 9 commits. Those 9 commits, despite doing nothing, would have agency and real estate in the code. There's nothing that says that 10th commit will even get merged if it's so controversial. To be frank, PRs are almost never universally controversial. It's the details that make them controversial, so this outcome is likely more often than not.
grumple 3y ago
If the feature is huge, you should be having incremental reviews throughout the process. The PRs should be made to the feature branch.
aqme28 3y ago
If your “better tooling” is to split this PR into multiple smaller chunks, then you’re just reinventing the original workflow
barrkel 3y ago
Big changes are risky. The core of making continuous deployment work safely is ensuring changes are small. If you have CD after a huge PR lands you are in risky territory, the chance you need to unwind is higher, but the size of the PR means it's more likely to have entanglements. It's lower risk to make small changes under feature flags. The final step of deployment can then be turning on the feature, and turning off is trivial. If something breaks along the way, undoing the commit is easy because it'll be small. Small changes frequently applied are the path to low risk continuous deployment.
aseipp 3y ago
> If reviewing huge PR's is a burden, I will make better tooling for it. Systems like Gerrit and Graphite already have that, because they show you an incremental diff between versions of the change. If you have a change with 500 lines, you only typically read that whole thing once. Incremental modifications result in incremental diffs, so if it takes 50 lines of code to address review comments, the reviewer only needs to read that 50 line change. This is loosely the same as git range-diff. I talked about this here: https://news.ycombinator.com/item?id=38371748 https://news.ycombinator.com/item?id=38371748 > What I don't want is a big feature to be split into 10 PR's, where the first 9 provide no user benefit and are just refactors or adding dead code ready for the last PR. These tools do not use "PRs." They do not use branches. They use changes. You are not proposing that a branch is merged. You're proposing a single, atomic change (one that can pass integration and the testsuite), to the codebase. You review individual changes. You may review a sequence of related changes, but you do not review "branches", and the tool largely ignores branches. This is a subtle but very important distinction. Tools like Gerrit or Graphite will show you the "relation chain" which is basically just the child/parent relationship between these changes. So, if you have 3 commits, those become 3 changes to review in the UI, and at any point, while looking at any of them, you can see the other 2 changes for context, quickly. You can do something similar on GitHub. Go to a multi-commit PR on github. Click any of the commits. You can now hit "Next" (shortcut 'n') or "Prev" (shortcut 'p') in order to flip between commits, like the pages of a book. This is a review style that some projects adopt. In reality, stacking commits "well" requires a bit of practice. It's a "garbage in, garbage out" process, so if you're just throwing 50 changes up there a day it isn't going to work well. There is a bit of an art and also a science to make concise, easily readable and easily reviewable changes. In practice I'd say the "ideal stack" is anywhere from 2-7 commits; line counts I don't think are important. (Sometimes a single atomic change really does need to be 500 lines.) 7 isn't a magical number. It's just that having too large of a stack is often easy to lose track of. But that's roughly the same problem as a branch on Github that has 80 commits or whatever.

bayindirh 3y ago

Also, A PR can contain tons of commits, and if the commits are well cut, it's very easy to understand implementation of a new thing step by step. This is why Linux kernel workflow contains n patches per feature. To understand what's done in a clear manner. Lastly, adding feature flags to mask incomplete feature code paths is very risky, and is a bug by itself I think.

Zacharias030 3y ago

Wouldn’t it be beneficial to get the 9 refactors merged even though the 10th commit needs iteration? It would make it much easier for colleagues that can then branch off of the tested and merged refactors without running into merge conflicts later.

oooyay 3y ago
No. If the feature requires 10 commits and only 9 are universally agreed on then under this model you would merge 9 commits. Those 9 commits, despite doing nothing, would have agency and real estate in the code. There's nothing that says that 10th commit will even get merged if it's so controversial. To be frank, PRs are almost never universally controversial. It's the details that make them controversial, so this outcome is likely more often than not.

grumple 3y ago

If the feature is huge, you should be having incremental reviews throughout the process. The PRs should be made to the feature branch.

aqme28 3y ago

If your “better tooling” is to split this PR into multiple smaller chunks, then you’re just reinventing the original workflow

barrkel 3y ago

Big changes are risky. The core of making continuous deployment work safely is ensuring changes are small. If you have CD after a huge PR lands you are in risky territory, the chance you need to unwind is higher, but the size of the PR means it's more likely to have entanglements. It's lower risk to make small changes under feature flags. The final step of deployment can then be turning on the feature, and turning off is trivial. If something breaks along the way, undoing the commit is easy because it'll be small. Small changes frequently applied are the path to low risk continuous deployment.

aseipp 3y ago

> If reviewing huge PR's is a burden, I will make better tooling for it. Systems like Gerrit and Graphite already have that, because they show you an incremental diff between versions of the change. If you have a change with 500 lines, you only typically read that whole thing once. Incremental modifications result in incremental diffs, so if it takes 50 lines of code to address review comments, the reviewer only needs to read that 50 line change. This is loosely the same as git range-diff. I talked about this here: https://news.ycombinator.com/item?id=38371748 https://news.ycombinator.com/item?id=38371748 > What I don't want is a big feature to be split into 10 PR's, where the first 9 provide no user benefit and are just refactors or adding dead code ready for the last PR. These tools do not use "PRs." They do not use branches. They use changes. You are not proposing that a branch is merged. You're proposing a single, atomic change (one that can pass integration and the testsuite), to the codebase. You review individual changes. You may review a sequence of related changes, but you do not review "branches", and the tool largely ignores branches. This is a subtle but very important distinction. Tools like Gerrit or Graphite will show you the "relation chain" which is basically just the child/parent relationship between these changes. So, if you have 3 commits, those become 3 changes to review in the UI, and at any point, while looking at any of them, you can see the other 2 changes for context, quickly. You can do something similar on GitHub. Go to a multi-commit PR on github. Click any of the commits. You can now hit "Next" (shortcut 'n') or "Prev" (shortcut 'p') in order to flip between commits, like the pages of a book. This is a review style that some projects adopt. In reality, stacking commits "well" requires a bit of practice. It's a "garbage in, garbage out" process, so if you're just throwing 50 changes up there a day it isn't going to work well. There is a bit of an art and also a science to make concise, easily readable and easily reviewable changes. In practice I'd say the "ideal stack" is anywhere from 2-7 commits; line counts I don't think are important. (Sometimes a single atomic change really does need to be 500 lines.) 7 isn't a magical number. It's just that having too large of a stack is often easy to lose track of. But that's roughly the same problem as a branch on Github that has 80 commits or whatever.