Hi A few direct responses and my 2 cents at the end. This is all my humble opinion, feel free to disagree or ignore it :) 2013/8/6 Daniel Vetter <daniel@xxxxxxxx>: > > I often throw in a refactoring suggestion when people work on a > feature, that's right. Often it is also a crappy idea, but imo for > long-term maintainance a neat&tidy codebase is really important. So > I'll just throw them out and see what sticks with people. > The problem is that if you throw and it doesn't stick, then people feel you won't merge it. So they kinda feel they have to do it all the time. Another thing is that sometimes the refactoring is just plain bikeshedding, and that leads to demotivated workers. People write things on their way, but then they are forced to do it in another way, which is also correct, but just different, and wastes a lot of time. And I'm not talking specifically about Daniel's suggestions, everybody does this kind of bikeshedding (well, I'm sure I do). If someone gives a bikeshed to a patch, Daniel will see there's an unattended review comment and will not merge the patch at all, so basically a random reviewer can easily block someone else's patch. I guess we all should try to give less bikeshedding, including me. > > One prime example is Ville's watermark patches, which have been ready > (he only did a very few v2 versions for bikesheds) since over a month > ago. But stuck since no one bothered to review them. Actually I subscribed myself to review (on review board) and purposely waited until he was back from vacation before I would start the review. I also did early 0-day testing on real hardware, which is IMHO way much more useful than just reviewing. Something that happened many times for me in the past: I reviewed a patch, thought it was correct, then decided to boot the patch before sending the R-B email and found a bug. And my 2 cents: Daniel and Jesse are based on different premises, which means they will basically discuss forever until they realize that. In an exaggerated view, Daniel's premises: - Merging patches with bugs is unacceptable - Colorary: users should never have to report bugs/regressions - Delaying patch merging due to refactoring or review comments will always make it better In the same exaggerated view, Jesse's premises: - Actual user/developer testing is more valuable than review and refactoring - Colorary: merging code with bugs is acceptable, we want the bug reports - Endless code churn due to review/refactoring may actually introduce bugs not present in the first version Please tell me if I'm wrong. >From my point of view, this is all about tradeoffs and you two stand on different positions in these tradeoffs. Example: - Time time you save by not doing all the refactoring/bikeshedding can be spent doing bug fixing or reviewing/testing someone else's patches. - But the question is: which one is more worth it? An hour refactoring/rebasing so the code behaves exactly like $reviewer wants, or an hour staring at bugzilla or reviewing/testing patches? - From my point of view, it seems Daniel assumes people will always spend 0 time fixing bugs, that's why he requests people so much refactoring: the tradeoff slider is completely at one side. But that's kind of a vicious/virtuous cycle: the more he increases his "quality standards", the more we'll spend time on the refactorings, so we'll spend even less time on bugzilla", so Daniel will increase the standards even more due to even less time spent on bugzilla, and so on. One thing which we didn't discuss explicitly right now and IMHO is important is how people *feel* about all this. It seems to me that the current amount of reworking required is making some people (e.g., Jesse, Ben) demotivated and unhappy. While this is not really a measurable thing, I'm sure it negatively affects the rate we improve our code base and fix our bugs. If we bikeshed a feature to the point where the author gets fed up with it and just wants it to get merged, there's a high chance that future bugs discovered on this feature won't be solved that quickly due the stressful experience the author had with the feature. And sometimes the unavoidable "I'll just implement whatever review comments I get because I'm so tired about this series and now I just want to get it merged" attitude is a very nice way to introduce bugs. And one more thing. IMHO this discussion should all be on how we deal with the people on our team, who get paid to write this code. When external people contribute patches to us, IMHO we should give them big thanks, send emails with many smileys, and hold all our spotted bikesheds to separate patches that we'll send later. Too high quality standards doesn't seem to be a good way to encourage people who don't dominate our code base. My possible suggestions: - We already have drm-intel-next-queued as a barrier to protect against bugs in merged patches (it's a barrier to drm-intel-next, which external people should be using). Even though I do not spend that much time on bugzilla bugs, I do rebase on dinq/nightly every day and try to make sure all the regressions I spot are fixed, and I count this as "bug fixing time". What if we resist our OCDs and urge to request reworks, then merge patches to dinq more often? To compensate for this, if anybody reports a single problem in a patch or series present on dinq, it gets immediately reverted (which means dinq will either do lots of rebasing or contain many many reverts). And we try to keep drm-intel-next away from all the dinq madness. Does that sound maintainable? - Another idea I already gave a few times is to accept features more easily, but leave them disabled by default until all the required reworks are there. Daniel rejected this idea because he feels people won't do the reworks and will leave the feature disabled by default forever. My counter-argument: 99% of the features we do are somehow tracked by PMs, we should make sure the PMs know features are still disabled, and perhaps open sub-tasks on the feature tracking systems to document that the feature is not yet completed since it's not enabled by default. In other words: this problem is too hard, it's about tradeoffs and there's no perfect solution that will please everybody. My just 2 cents, I hope to not have offended anybody :( Cheers, Paulo -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
