[Qt-creator] [Development] [FYI] commit/review policy refactored

[Kai Koehne]

> -----Original Message-----
> From: Qt-creator [mailto:qt-creator-bounces+kai.koehne=qt.io at qt-
> project.org] On Behalf Of Oswald Buddenhagen
> Sent: Wednesday, July 13, 2016 12:49 PM
> To: development at qt-project.org; qt-creator at qt-project.org
> Subject: Re: [Qt-creator] [Development] [FYI] commit/review policy
> refactored
> 
> On Wed, Jul 13, 2016 at 12:15:44PM +0200, Kai Köhne wrote:
> > The page is obviously written from the viewpoint of a maintainer. I'd
> > prefer to write It in a less intimidating way to the contributor, and
> > make it helpful for first time, inexperienced contributors.
> >
> first-timers can ask in case of doubt. this is meant to be a policy, not an intro.

If we'll have a separate intro page (which is linked from here), then fine with me :)

> somewhat unsurprisingly, i finally took the effort to do this work because i
> witnessed several unrelated violations in the last weeks/ months.

That's what I suspected :)
 
> > > As a Contributor
> >
> > Maybe link back to https://wiki.qt.io/Commit_Policy .
> >
> it's linked from the reviewer section, which i find more logical.

Fair enough.

> > "Make sure that your commit matches the Qt Commit Policy.
> >
> > > 1. Invite relevant reviewers.
> > > * Always invite the respective domain experts, not somebody
> convenient.
> >
> > Scrap the 'not somebody convenient'. It's the job of the reviewer to decide
> what he can approve.
> >
> it's the responsibility of both sides. don't pretend that these social dynamics
> don't exist.

Well, in doubt the blame is always on the approver, not on the contributor. Also,
it's perfectly fine to add 'convenient' people, be it only to check commit logs,
or notify them about stuff. The point is just that they shouldn't approve it if they're 
not 'domain experts' - which is covered in the Approver's section.

> > Instead, mention how one can find the 'domain expert'. Something like
> >
> >  * Domain experts can usually be found by inspecting the git log, and
> mailing lists. If in doubt also add the [https://wiki.qt.io/Maintainers
> Maintainer] of an area if there is one.
> >
> that's already listed in the contribution guidelines and i wanted to avoid
> excessive redundancy. i may reconsider, or move parts of the content. some
> more linking is necessary anyway.

Fine with me if there's another page that's linked.

> > > 2. Give reviewers ample time to respond.
> > > * Unless everyone who can be reasonably expected to have a relevant
> opinion to offer has already done so, a full working day waiting time is the
> absolute minimum; three days are reasonable.
> > > * In particular, give watchers (usually higher-level maintainers) enough
> time to voice concerns even if you did not explicitly invite them.
> >
> > The sub-points are only valid if you have a +2 already. So maybe move
> them down to a section ("If your change got approved"). Rather mention
> that it can take some working days until added people respond.
> >
> that's a good idea. a section about staging and re-staging is needed anyway.
> 
> > > 3. Discuss objections. Do not override a -1 unless there is a broad expert
> consensus that the objection is unfounded.
> >
> > A Contributor cannot usually 'override' a -1 - that can only be done by an
> approver. Maybe your point is though that, if you got a +2, and somebody
> voiced objections, one shouldn't stage it?
> >
> every approver is a contributor by definition. ;) 

Not every contributor is an approver though, so it makes IMO more sense to put stuff specific to approvers into the reviewers section.

Or prefix them with "For contributors that are also approvers:".

> but i guess "disregard" is a
> better word than "override" in this case.

Fine with me. I think 'consensus' instead of 'broad expert consensus' is enough too :)

> > > 4. Do not ignore/fight the Early Warning System. Justify each override.
> > > [...]
> >
> > Isn't that limited to Approvers, too?
> >
> ...
> 
> > > 5. Do not approve your own changes.
> > [...]
> >
> > Again that's limited to Approvers,
> >
> yes
> 
> > and should be in the 'As a Reviewer' section.
> >
> no. it's addressing the contributor.

A contributor that's also an approver, which is the minority.

My point is: This text reads like a proverbial stick you want to use against fellows that overstep their limits (as you perceive it). I'm somewhat skeptical that putting down draconical rules somewhere on the wiki helps on this , but whatever.

My fear is that this is rather read, and taken literally, by first-time/seldom contributors that already feel intimidated by the whole process in the first place ('can't I just append my patch to bugreports?'). So let's make it clear what they have to take care of, and what not.

Regards

Kai