On Wed, 09 Feb 2011 08:52:00 +0000 Tom Hacohen <tom.haco...@partner.samsung.com> said:
> 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? +1 ... as long as we understand that these are guidelines and we SHOULD stick to them as much as possible - but people will make mistakes and stuff will slip through - but a lot LESS will slip through that if we didn't. :) -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ 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
