On 21/03/15 20:49, Ian Clarke wrote: > On Sat, Mar 21, 2015 at 1:58 PM, Matthew Toseland <matt...@toselandcs.co.uk> > wrote: >> Most of the above boils down to "review the diff, not the history". That >> is probably sensible. > 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! >> The last point is "everyone can commit anything without review". That's >> the fundamental question here: Do we want to require that some >> responsible person (release manager, person with push rights) reviews >> and signs off on the code before it is pushed? > I think the question is moot, since (so far as I'm aware) we don't have > anyone that can commit to reviewing all code reliably and quickly, so such > a requirement would only serve to create a severe bottleneck in our > development process. > > All commits are public, all commits can be reviewed by anyone, but in the > event that nobody is in a position to review something we can't allow > development to grind to a halt. If people care about reviewing code then > they can and should review code. After it's been committed. 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. >> There are 2 main reasons for this: >> 1. Security. How useful this is is debatable. E.g. introducing a rootkit which will be run on developer's boxes by editing the build scripts, or the tests. It may be possible to prevent this via automated tools. Subtle changes and dirty tricks may be hard to detect, but some things *do* show up in a quick code review. >> 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. I agree that review capacity is potentially a bottleneck. There are different ways to solve this: - Have paid staff who review and merge stuff. - Don't accept pull requests if nobody can review them right now. - Allow the reviewers to make reasonable demands for clear code. A pull request is a negotiation between the contributor and the maintainer. Many OSS projects use processes like this...
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
