On Sat, Mar 21, 2015 at 7:42 PM, Matthew Toseland <[email protected]> wrote:
> On 21/03/15 20:49, Ian Clarke wrote: > > That's part of it, but also that a branch should be created for each > > bugfix/feature, which ideally should be as small a unit of work as > possible > > (that can be merged without breaking stuff). > Absolutely. See e.g.: > http://nvie.com/posts/a-successful-git-branching-model/ > > We try to stick to this ... or at least, that was the plan/consensus! > Hmm, it doesn't sound like we do in practice, given that I believe there were pull requests with hundreds of commits which sounded like it was for much more than a single feature or bugfix. Anyway, the point isn't to complain about what happened in the past, just to agree on our process going forward. > The problem is then we spend a lot of time undoing stuff. This can and > does happen, and has happened, even relatively recently. It's part of > the reason why most projects use a branch-and-merge model, rather than > giving all the devs push rights. > Ian. > Why we might need to revert or reject changes: > 0. Stupid stuff. E.g. committing jars to repositories. > Committing jars to repositories is kind of careless, don't people have sensible .gitignore files? Can a .gitignore be committed to the repo? But regardless, nothing in the process I've outlined would inhibit correcting problems like this. Ideally they'd be corrected at the code review stage, but if they make it past that then they can be corrected with a new branch, just like any other bugfix. > >> 2. Disruptive changes to APIs. > This has also happened. Especially if the description is incomplete, > there is a significant risk of refactoring breaking other code (e.g. > plugins; this was part of the problem with Xor's changes), introducing > new APIs that don't make sense etc. > Both code review and a decent continuous integration system should address this. This is a good continuous integration service that is free for OSS projects: https://travis-ci.org/ You can see how I use it on this other OSS project of mine: http://quickml.org/ > I agree that review capacity is potentially a bottleneck. There are > different ways to solve this: > - Have paid staff who review and merge stuff. > That seems very unlikely to happen, we barely have budget for actual developers, let alone dedicated QA people. And really, who'd want that job anyway? Even setting aside budget and finding suitable people, coders should have primary responsibility for ensuring that their code is good. It's unhealthy to have a situation where coders think that ensuring their code is clean and robust is someone else's problem. > - Don't accept pull requests if nobody can review them right now. > This will absolutely cause a severe bottleneck, causing development to grind to a halt, and probably destroying morale in the process. Who wants to work their ass of on some code only for it to sit in a branch indefinitely? > - Allow the reviewers to make reasonable demands for clear code. A pull > request is a negotiation between the contributor and the maintainer. > Of course, reviewers can point out unclean code, and a conscientious developer will want to commit good code so they'll fix it. If a coder is ignoring reasonable code review feedback then that might be grounds for removing commit access. Ian. _______________________________________________ Devl mailing list [email protected] https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
