[vtk-developers] rules proposal drop "+1" ie, looks ok to me but someone else needs to review as well

[Ben Boeckel]

On Thu, Jul 07, 2016 at 08:56:14 -0400, David E DeMarle wrote:
> As Robert just pointed out, the +1 state in the review process is nothing
> but trouble. In practice, it seems to help us avoid responsibility and
> delays merging, contributing to the accumulation of stale merge requests
> and newbie frustration.
> 
> How about we say "+1 means I approve this code and assert that it is ready
> to merge". Merge requests from a authorized developers should be promptly
> merged after the +1 by the original author. Merge requests from
> unauthorized developers should be merged more or less immediately by the
> authorized reviewer after the +1.

The problem here is that for people (like me) with more domain-specific
knowledge might be able to +1 the CMake side of things, but without a
"+1", what do I do?

Responding to other bits from the thread:

On Thu, Jul 07, 2016 at 09:56:09 -0400, Brad King wrote:
> The real problem we'd like to address is stagnation of MRs that
> are ready but not merged.  The syntax for approving is not very
> important.  We should identify reasons MRs stagnate and address
> them directly:

An idea: have kwrobot scan MRs every Monday looking for +1 and +2
comments on unmerged MRs. It adds a comment asking "what's the holdup?"
pinging any reviewers. It could also use the `workflow:buildbot` tag to
see if the buildbots have reported back successfully (would need some
bugs fixed in status submission from buildbot).

On Thu, Jul 07, 2016 at 10:10:00 -0400, Will Schroeder wrote:
> It's a time problem: I see chunks of code which at a superficial level seem
> fine and I'm willing to give it a +1. The problem with a +2 is that issuing
> such a score has implications that I've jumped into the code at a deep

Copying from an earlier email:

On Wed, Mar 25, 2015 at 4:41 PM, Ben Boeckel <ben.boeckel at kitware.com>
wrote:
> Acked-by    - "I see nothing wrong with the change" (which is all
>               kwrobot's checks can do)

+1

> Reviewed-by - "I reviewed the change and agree with it" (we should
>               probably use this rather than Acked-by for humans)

+2

> Tested-by   - "I tested the change and approve that it works"

+3

What you're describing is probably closer to +3 here.

On Thu, Jul 07, 2016 at 15:00:42 -0400, Marcus D. Hanwell wrote:
> In my opinion one of the other things lacking in VTK is someone owning
> a particular module, such that if things are broken/in need of review
> the buck stops with them. This makes things a little nebulous at
> times, Qt, the Linux kernel and countless others tend to distribute
> responsibility for modules, subsystems, etc.

One idea I've batted around is to have kwrobot auto-assign MRs randomly
(being in the rotation would be voluntary). That user would at least
know who the right person to assign it to would be. The kwrobot could
also be made aware of areas maintained by certain users and
assign/mention based on that.

Gitlab upstream uses assignment in a slightly more involved way: when an
MR is waiting on the submitter, it is assigned back to the submitter to
indicate who is blocking it. Assigned MRs (and issues) increase the
number in the left sidebar in gitlab, so people should be able to use
that as a todo list of sorts. The problem with this is that
non-developers cannot be assigned anything, but it would work for the
majority of contributors.

> I think the buildbots have been very slow when I have worked on
> changes, and it can be an issue that by the time results are back you
> have switched on to other tasks. We had lower coverage but faster
> response when only 3 dashboards were required (continuous, and later
> Gerrit with CDash at Home). Hopefully this can be improved in future,
> such that we could request a branch be tested and see the results
> within an hour or so.

I think if we could have more targeted testing to start, that would
improve things. For example, working on a Windows change? Only test on
the Windows buildbots (`Do: test -i windows`) until those are green and
*then* do a full test request (`Do: test`). Fixing warnings on megas?
Use `Do: test -i megas`. And so on. This would help keep machines from
doing unnecessary work.

Another (probably additive) way to do this would be to have --oneshot be
the default so that new builds are not queued automatically on push (I
asked the list about this policy change a while ago and got ~no
response).

> It is tough balancing having enough process that things are managed,
> and not so much that it is just too hard to get anything in. I agree
> with Berk that if we stick with +1/+2 then they should be additive and
> two +1s = a +2.

For the most part, yes (two reviewers for CMake changes and none for
code is probably not enough).

--Ben