[Geany-Devel] Zombified pull requests

[Lex Trotman]

Hi Jiri,

Its a worthwhile thing to talk about. Some specific comments below,
but first a couple of general ones.

I occasionally talk to some of the Geany devs/contributors in other
forums and its clear that at this moment Geany isn't their primary
focus.  That is my situation, which is why I concentrate on ML and
issue replies, they only take a few minutes at a time, suitable for a
break from my other activities, but not a significant time usage.  And
hopefully that saves other devs and contributors from spending time on
initial obvious errors and omissions in reports.

Note that if I didn't use the latest Git version of Geany for day to
day work I would almost never test it, so testing PRs means including
them into the Geany I am using for my other work, which I consider
risky if I don't know the contributor or the PR is big, so I don't
test them much.  Primary contributors or devs PRs only.

But I will stop using the latest for day to day work if it becomes
unreliable, so I depend on all patches being inspected and tested
before commit.  And by test I don't mean 5 minutes tick and flick, but
*used* for some time, lets say at least a week.  So PRs for
features/languages I don't use won't get tested either. Colombans #852
is an example at the moment, have incorporated it and if it works fine
for a week I'll commit it.

And since I am not testing PRs much, I am not in a position to commit them much.

But not many people other than committers seem to review/test patches.
It should not be just up to the committers to review and test PRs.  If
there were more people doing that, then there is less pressure on
committers to be responsible for the whole thing.  A cheery comment "I
like this and have been using it for the last month on system XXX with
no problems" will go a long way to getting stuff committed.

What I do on Asciidoc for PRs that I can't/won't test, is to require
at least one person, other than the proposer, to acknowledge
usefulness, and to report tested ok in actual use.  Then I will commit
it, without trying it myself (Colomban spins in his bed :).  Of course
if a recommender turns out to be unreliable then they will be
subsequently ignored, but I haven't had any problems to date.

I suspect many of the comments above apply to other devs/contributors as well.

On 6 January 2016 at 06:46, Jiří Techet <techet at gmail.com> wrote:
> Hi,
>
> happy new year and let's celebrate it with something cheerful - zombies!

:)

>
> I've noticed there are more and more pull requests on github which don't get
> merged to Geany. It's clear that people are fighting with time to make
> reviews of pull requests (and Colomban does a great job here!), however, it
> seems to me there are quite many patches where all the hard work has already
> been done (review, patch updates) and the only missing thing is the merge -
> which doesn't happen.
>
> I think this is quite unfortunate - there are many patches which might be
> useful for users; at the same time it might be discouraging for contributors
> to see their patches unmerged.
>
> I've been thinking about what may be the cause of this and several things
> come to my mind:
>
> 1. Not enough feedback from other developers. Typically I am for instance in
> the mode "I don't want to add too much noise" so unless I love or hate
> something, I just don't write anything. I think Colomban's own patches
> suffer from this most as he reviews other people's patches but there's no
> feedback for his own patches.
>
> Maybe it would be a good idea if regular Geany contributors go through the
> pull requests from time to time and just LGTM those that sound reasonable
> functionality-wise. I'm not talking about full code review here, just a very
> quick assessment whether the given feature makes sense for Geany (we should
> distinguish somehow "I made a review of the patch and LGTM" and "the
> functionality LTGM to be in Geany").

I think the categories are:

1) LGTM, I agree with what this PR is doing, how it does it, and I
have reviewed and tested it with no issues.  This category should mean
"can be committed if nobody objects in a reasonable period".

2) LGBI, (looks good by inspection) I agree with what this PR is
doing, how it does it, and I have reviewed it, but I havn't tested it.

3) AWTI, (agree with the intent) I agree with the intent, but either
havn't looked at the implementation or have issues with it and have
provided comments elsewhere.

4) HI, ( despite being cheery, means Hate It :), disagree with its
intent and purpose, the implementation is irrelevant.

And again I emphasise its not just up to devs/committers to do this.

Silence means don't care, somebody else can commit it if they want to
and I won't object later (except by providing an alternative PR).

>
> To give an example of such a "functionality review", the fractional font
> sizes patch
>
> https://github.com/geany/geany/pull/407
>
> LGTM - even though I probably won't use it myself, I understand it may be
> useful for someone and it modifies just 23 lines so it's nothing intrusive
> and can go in from my point of view.

That would be a LGBI in my above taxonomy.

>
> Such a review can be done in a few seconds so if everyone goes through the
> new pull requests from time to time, the patches will receive some feedback
> and it will be clearer whether it's something others want it in Geany.
>
> 2. Unclear status of some patches. Sometimes it might not be clear in what
> state the pull request is - I'd suggest adding at least the following two
> tags:
>
> needs-work (reviewed with some comments that need to be addressed)
> work-in-progress (not meant for review in the current state)
>
> This will help to distinguish pull requests awaiting merge and pull requests
> that aren't there yet.

Sounds good, makes it easier to skip ones that don't need action,
needs work already exists, work in progress label added.

>
> 3. Fear that the pull request isn't tested enough. I believe that if a patch
> did undergo a review and there doesn't seem anything obviously wrong with
> it, it can be merged to master. I think there's no need for some long-term
> private testing of a patch before it gets merged - people using the
> development versions of Geany should be aware it may contain bugs and should
> know how to deal with them.

This is where we significantly disagree, if the development version
isn't usable for "real work" then it isn't going to get used much at
all, and "testing" becomes just a tick and flick "well it didn't crash
and seemed to do the right thing, once".

And then the development version becomes less stable has more bugs and
gets less used and less tested, and thats a downward spiral.

Thats not to say that simple things should not be easier to add, but
we get problems caused by the most innocuous looking patches.

> There are also more eyes so a potential bug is
> spotted earlier. Of course it makes sense getting more conservative towards
> the end of the development cycle to stabilise things.
>
> OK, these are things that come into my mind regarding the zombie pull
> requests but there may be other problems. What do you think? Do the points
> above look reasonable to you and do you think they might help a bit? Is
> there anything else that might help killing the bloody zombies?

Perhaps we could also just close some of the really old ones that have
objections on them.  People can still comment and they can be
resurrected if the proposer objects that they still want it.

Cheers
Lex

>
> Cheers,
>
> Jiri
>
> _______________________________________________
> Devel mailing list
> Devel at lists.geany.org
> https://lists.geany.org/cgi-bin/mailman/listinfo/devel
>