Hey all, We recently released 1.0 for the core libraries, so now it's a good time to review how we worked in the past, draw conclusions and hopefully also improve our work-flow. When using the word "patches" in the following mail I also mean commits done by developers with commit access (when that applies).
Although we obviously get things done and get them good, there are a couple of major flaws in the way we currently work: 1. Only very few people review and apply patches. 2. Because of #1 some people review and apply patches to code they don't manage/know well which is bad. 3. Patches often contain whitespace/coding style errors. 4. Patches often introduce new compilation warnings. 5. Patch reviewing is very hard because there are many patches but the guidelines aren't well known/enforced. 6. SVN gets broken on a weekly basis. 7. SVN log messages are just awful and it's impossible to search through the logs. 8. Patches usually don't contain a good change summary and seldom contain a nice svn log message that the reviewer can just use. 9. Patches are usually too big to review. 10. Patches often don't include docs/changelog. 11. Patches often include unrelated changes - especially formatting and logic changes. As mike_m nicely put it, "there are many more submitters than committers" thus the only sane way to handle patches is let the submitters do more and thus the committers do less. I want to be able to open an email containing a patch, read a nice summary about what the patch does, go through the patch, compile and check that everything works and that there are no additional warnings (with the default configure flags) and then just copy and paste the log message (with credit to the submitter already provided in there). If the patch is not up to par (i.e fails any of the stages above), just reply to the submitter with a "sorry, your patch does not comply to our patch standards please fix a, b and c and resend it. Thank you." instead of the current way of trying to figure out and fix a patch for hours and then apply it without saying anything. You can't expect our submitters to follow rules we aren't willing to enforce, or at least point out. We should make it more annoying for submitters (also developers with access) to send broken patches than sending correct and clean patches, and the way to achieve that is sending them back to the drawing board. Using good log messages in SVN is not hard, you just need to explain in a couple of lines exactly what you did and in a way that will be easy to understand by other people, even not developers that want to go through the logs and figure out what have changed and what commit they think introduced the bug they stumbled upon. I suggest using the following template: "Lib/app module: Short summary. Long summary." For example: "Evas font-engine: Fixed a bug in the query_size function. The old query_size function didn't take kerning into account." This is very easy to write, takes <10 seconds and you get a very nice svn log in return. People can also help review patches in code they don't know, it's fast and easy and can take out a lot of the load from the other devs. You don't even need to be a developer to see someone didn't follow the guidelines, and denying such patches will mean the maintainer won't have to read through them which in turn means you saved everyone work. Spanking each other is easy and will benefit everyone. If you know the code and have commit access, that's even better, just review and commit. If you are unsure, send an email and say why, this will make sure other reviewers who better know the code will surely notice the issues you have already spotted. All of those take seconds when split among all the members in the e-devel ML but take minutes/hours when there's only one reviewer. We should also make it very clear that we will not accept any patches that introduce new compilation warnings (with default configure options and -Wall -Wextra turned on or some other warning flags we decide). Code that doesn't follow the conventions will also be denied, same goes for the svn log or any other violation I pointed out above. Also, patches should be clean and split to small manageable pieces (with each having it's own svn log message and summary). It's ok to say "no way I'm going to review this patch, it's too long and messy, REWRITE!", don't be afraid to do so. In some cases huge patches are ok (I guess) but those who send them will really have to convince you this is one of those cases. Furthermore we should not allow mixing of unrelated changes in the same patch. I think the fewer the committers the better, but I don't think that will work well in e, at least not in the near future. But we do kinda know who maintains things, for example, I maintain the textie stuff and discomfitor maintains eeze, cedric maintains many parts of edje, raster maintains everything that doesn't have a specific maintainer and etc. I don't think anyone should commit text changes without at least letting me know about it before, nor will I commit stuff into edje before getting them approved by cedric (for example, I sent my recent edje changes to him to review before I committed them). This can either be enforced by an svn hook, or by public spanking. I talked to raster about it, and he's ok with accepting GIT patches, and since many of the contributors/developers use git-svn, they can use git send-email to easily send patches that already include the changelog. People who don't use git-svn can just use 'patch -p1' to apply them and just take the changelog out of them. Either way, it's easier for everyone. In conclusion, I think we should just enforce the guidelines I suggested above. I don't mind being the "bad-man" and just revert people's commits if they behave badly and don't follow the guidelines or deny patches in the ML. Just to make sure you understand, those guidelines apply on *both* people with commit access and people without. What do you guys think? -- Tom. ------------------------------------------------------------------------------ The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE: Pinpoint memory and threading errors before they happen. Find and fix more than 250 security defects in the development cycle. Locate bottlenecks in serial and parallel code that limit performance. http://p.sf.net/sfu/intel-dev2devfeb _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
