|
|
|
|
|
|
by kiallmacinnes
3730 days ago
|
|
|
> Not very flexible, I see a lot of churn of invalid pull requests with this design if they aren't allowed to grow into complete features.. Actually, Gerrit really encourages growing a patchset ("pull request") into a complete feature. It allows you update your change over and over, addressing review comments as they come in. Once done, you have a clean "Add support for use of XYZ by ABC" commit - and not a pile of half baked commits - I cringe when I see things like this: "Add framework for XYZ", "Define Config for XYZ", "Correct typos", "Add tests", "Rework XYZ to be standards compliant", "Correct typos", "Fix tests" |
|
|
> Actually, Gerrit really encourages growing a patchset ("pull request") into a complete feature. It allows you update your change over and over, addressing review comments as they come in.
> Once done, you have a clean "Add support for use of XYZ by ABC" commit - and not a pile of half baked commits - I cringe when I see things like this: "Add framework for XYZ", "Define Config for XYZ", "Correct typos", "Add tests", "Rework XYZ to be standards compliant", "Correct typos", "Fix tests"
I don't like commits like that either. But almost all real pull requests require more than one commit (so future generations can bisect the repo properly without then needing to bisect patches as well). A nice pull request is something like this:
server: add statistics monitoring framework
server: component a: hook into statistics monitoring
api: expose statistics monitoring
integration: add tests for statistics
Each commit works as intended and does exactly one thing. I tried to do something like this on Gerrit (was contributing to the TWRP recovery) and it was such a pain that I collapsed everything to one commit. That's not how things should be dealt with (I needed to improve the pattern decryption to support N*N patterns and it required a bunch of UI, internals and other changes that all got squashed together).
I also didn't like the fact that anybody could overwrite your PR's commit with their own crap. Why is that a feature?