Merging pull requests before 72 hours

[Mojca Miklavec]

Hi,

This conversation comes from
    https://github.com/macports/macports-ports/pull/2785
and I wanted to bring it up to the mailing list.

In summary: Ryan objected merging openmaintainer pull requests prematurely.
I'm with Ryan on this one.

On Fri, 12 Oct 2018 at 15:04, Perry E. Metzger wrote:
> On Fri, 12 Oct 2018 at 07:39, Ryan Schmidt wrote:
>> On Thu, 11 Oct 2018 at 18:01, Perry E. Metzger wrote:
>>>
>>> I had little confidence that he would know it happened,

If this is ever the case, we would need to change our system. But I
agree that the maintainer must have seen this. If he didn't, we need
to let him know about the problem (there are some who disabled all
email traffic and then never get any notifications; those maintainers
should fix their preferences to include email for cases when they are
mentioned).

>>> plus it's listed as
>>> openmaintainer, and, like it or not, your updates are almost always perfect,
>>> there's rarely any dissent against them, so you get treated differently.
>>
>> Why? I assigned the PR to him, and macportsbot also notified him.
>>
>>> plus it's listed as openmaintainer,
>>
>> Yes but in the past some maintainers have objected even to simple version
>> updates being performed on their openmaintainer ports without their consent.
>> Developers who are not the maintainer might not be aware all the implications
>> that updating the version of one port might have on other ports for example.

Precisely. What may seem like a totally trivial and straight update to
you, consisting of nothing else but version bump and checksum changes,
may introduce dramatic changes or incompatibilities.

(Oh, look, python 2.7 has now been update to python 3.x, let's update
it. Oh, see, a new version of poppler just came out. Let's update just
this one; the travis job succeeds, that's sufficient right?)

Also, many people hardly test anything when submitting a PR. I need to
confess that is often true for me even for commits that circumvent the
PR. If a version of a port I trust changes, I would often only check
whether the build succeeds, or if the test suite passes in case there
is one (which is most often not the case). The cases when I go to the
trouble of verifying the consequences on all the dependent ports
beyond whether or not they build are very very very rare.

A while ago we had something that looked like a completely innocent
update of Lua (5.2 to 5.3) which literally broke EVERY SINGLE
dependency, most of them beyond repair. I knew this would happen, but
someone simply went forward and committed the update without asking
anyone for opinion. Circumventing the maintainer might have similar
consequences.

>> And this PR was more than just updating the version: I also reformatted the
>> entire port's whitespace. While I stand by those changes, reformatting someone
>> else's port is not something we should probably do without asking permission first;
>> this PR was how I was asking Toby's permission, and by merging the
>> PR before he had a chance to look at it, he didn't get the opportunity to give it.
>>
>>> and, like it or not, your updates are almost always perfect, there's rarely any
>>> dissent against them, so you get treated differently.
>>
>> If I want to commit an update, I'll do that. If I go to the effort of filing a PR,
>> it's because I want to give the maintainer of the port the opportunity to be aware
>> of the proposed change and to voice any objections before it gets committed.
>> I appreciate that you reviewed my changes and found them suitable, but if a
>> port has a maintainer, we should really give them the opportunity to fulfill that role.

Agreed.

> Then they should remove openmaintainer from their port.

While I'm not entirely sure about the exact difference between
maintainer and non-maintainer, I disagree.

> openmaintainer means
> that you can perform simple version updates without explicit approval.

Simple changes. What looks like a simple version update might not be
that (let's update python 2.7 to python 3.7, just version change and
checksum updates, simple ...)

> If they don't want that, they can just commit a change removing it.
>
> We are in a situation where most, if not the overwhelming bulk, of our ports with
> maintainers are de facto abandoned.

Which means that we should address that particular problem (maybe more
aggressively than we do now) rather than ignore maintainer field. If
you commit within a few hours, we don't even know whether a port is
abandoned or not. Before PRs were a thing, we would always append
"maintainer timeout" to commit messages. This happened to be super
useful. When I would want to update a port, I would run svn/git log
and if all the commit messaged said "maintainer timeout" a couple of
times, this would send a very strong signal that I should probably
file a port abandoned ticket.

Maybe we should reintroduce this and always when there's a 72-hour
maintainer timeout:
- change the commit message
- check whether the person is otherwise (in)active (when was he/she
last active either or the list or doing a commit, ...)
- file a port abandoned ticket in case it seems appropriate

And maybe we should, just once:
- collect a list of all maintainers
- remove those whose last commit was within the last year
- send an email to everyone else, asking if they are still maintaining
their ports, repeat that email two or three times
- remove everyone else whose email bounced or got no reply

But let's not ignore our existing maintainer policy just because we
have issues elsewhere.

Our main problem are tickets which have been opened for 2-10 years. I
really really really do appreciate the work you Perry are doing to our
PR queue, but waiting 72 hours won't actually make the PR queue any
shorter. Yes, there might be 30 PR open for the last three days, but
those are usually not the problematic ones. After three days it's
perfectly fine to merge them.

> The fraction of the time when someone
> ever comments on a proposed update is very, very low.
> I don't know why this is, but it's indeed the case. Keeping MacPorts functioning
> requires that we operate even in the presence of this. openmaintainer means that
> we can keep the pull queue reasonably clear when we have relatively high
> assurance that an update is harmless and well tested. If it becomes meaningless,
> we'll have a lot of trouble functioning. If people don't want others touching their
> ports, they should remove openmaintainer and not insist that it means the same
> thing as closed maintainer.

Maybe we can discuss this again later, but let's first fix the real
problem (clean up the inactive maintainers etc) ...

Mojca

PS: I would actually be in favour of replacing openmaintainer by
closemaintainer. That is: to any port with missing "openmaintainer"
keyword, add "closemaintainer". Then remove the "openmaintainer" tag
from all other ports. The we wouldn't need to ask each individual
first-time contributor to please add the "openmaintainer" tag.