Merging pull requests before 72 hours

[Leonardo Brondani Schenkel]

> On 15 Oct 2018, at 11:18 pm, Chris Jones <jonesc at hep.phy.cam.ac.uk> wrote:
> 
>> On 15 Oct 2018, at 10:34 pm, Leonardo Brondani Schenkel <lbschenkel at macports.org> wrote:
>> I'm a committer, and if I'm doing a trivial bump of an openmaintainer port I'll push it directly.
> 
> Depends entirely on what you consider trivial. If you consider a version update a trivial bump then I disagree. This in my opinion is something that should also be submitted as a PR so the port maintainer always gets a chance to review it. If the port is open maintainer and they do not respond within the 72 hour timeout, it can then be merged without their comment, but not before. For a committer to directly commit an update to a port that isn’t trivial and they do not maintain is in my humble opinion an abuse of the member role. Members should not bypass the PR review stage just because they can.

Like it or not, that's the definition of 'openmaintainer': for simple 
version bumps, no maintainer approval is necessary. Since the maintainer 
is the one explicitly opting in to such policy, I don't see any issue 
with respecting their wish. I deliberately added 'openmaintainer' to all 
the ports I maintain so others are free to do version bumps without 
going through me first, reducing friction (but not other changes, see 
below).

Ports that tend to break on any minor version bump, or are problematic 
in other ways, should probably not be marked as 'openmaintainer' -- and 
I believe they aren't.

> 
>> If I'm opening a GitHub PR for an openmaintainer port this means that for some reason I want the maintainer's opinion/review before it gets merged.
> 
> In my opinion the ‘rules’ for when this happens should not be subjective, as they currently seem to be. There needs to be a clear policy on what is and is not trivial and thus what does and does not require PR review.

I agree that it looks like different people have different 
interpretations for the scope of 'openmaintainer' (apart from simple 
version bumps being allowed, which is the definition that is explicitly 
written in the documentation).

My own interpretation is that any change that is not a simple version 
bump, a simple revision bump due to a dependency being upgraded, or 
fixing a livecheck or a typo, do not fall within the scope of 
'openmaintainer' and should be cleared with the maintainer first.

>> My belief is that in general nobody else should merge the PRs besides the submitter of the PR and/or the maintainer.
> 
> Disagree. Any member with commit rights should be able merge PRs. As long as the procedures have been followed, so either the maintainer has oked the changes, or the 72 hour timeout has expired and another reviewer has agreed with the changes, it does not really matter who finally clicks merge.

Naturally any member can merge PRs by virtue of having the rights to do 
so, but my point was that when a PR is opened against a maintained port, 
the recipient for that PR is the maintainer although it can be 
additionally reviewed/commented by anybody else (I suspect most PRs 
don't). Barring maintainer timeouts and other special circumstances, 
it's the maintainer that ultimately decides if that PR should be merged. 
At that point, they can signal it by approving the PR -- and depending 
on who has commit rights they can merge it themselves, or ask the 
submitter to merge, or ask any non-participant member to merge.

> ( Its actually quite common practise in many circles for submitters of a PR to require third party approval, and for someone else to merge it, and for the submitter to simply do it themselves to be discouraged. )

In the scenarios I have in mind and I participated in, the submitter 
merges if they're asked to by the maintainer or when the PR is approved 
without requests for changes or other concerns. Sometimes I submit PRs 
to other ports maintained by non-members, and I'm the one who has commit 
rights.

> 
>> They can merge at any time of their choosing in case they want to proceed, and there is no need to second guess their intentions.
> 
> Only the maintainer of a port should be free to merge before the 72 hour timeout, if they have the rights to do so. If they do not have commit rights, then can signify they have agreed to the changes at which point any member could merge. 

I think I wasn't clear on that point. What I meant is that a PR is 
either on a 'waiting for review/reviewing' phase, or an 'approved/ready 
to be merged' phase, and once the PR is 'approved' then anybody can 
merge. If it's on the 'review' phase, it's up to the participants to 
transition that PR into 'approved' and merge (or ask for it to be 
merged). I see very little room here for a non-participant third-party 
to second guess a participant's intention and prematurely merge a PR -- 
and if this seems to be warranted, simply asking first will eliminate 
the vast majority of misunderstandings.

// Leonardo.