[Intel-gfx] Maintainer-review fluff (was: Re: [PATCH 01/12] drm/i915: plumb VM into object operations)

[Paulo Zanoni]

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 at ffwll.ch>:
>
> 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