[Geany-Devel] Zombified pull requests

[Matthew Brush]

On 2016-01-06 02:44 AM, Thomas Martitz wrote:
> Am 06.01.2016 um 04:32 schrieb Matthew Brush:
>>
>> Agree, I sometimes avoid putting LGTM when I think something is a good
>> idea, because I don't want to give the impression that I have (or even
>> will) reviewed or tested it. Maybe just a "thumbs up" could mean "good
>> idea, though I haven't reviewed or tested it"?
>>
>
> Github is a terrible code review platforms. Other platforms do much better:
> 1) Differentiate between [x] I have reviewed [x] I have tested [x] I
> like the change (no test or review)
> 2) Handle updated changesets without losing comments, to the point that
> you can even browse older revisions
>
> 1) is a problem for reviewers and 2) is a problem for everyone
>
> There are a lot of other, much better code review systems, but I guess
> we're stuck with github (bought into proprietary solution, anyone?)
>

Not strictly. Given enough demand, and buy-in, we could switch to most 
other such tools. We do have adequate hosting for something better, 
though I doubt there's much desire to switch to a solution that doesn't 
support Git as the VCS. For the most part I find Github to be basically 
adequate myself, though it's not without issues.

>
>>> 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.
>>>
>>
>> Agree, and I've actually wanted for this before.
>>
>>> 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 like a good idea, I just added those labels. We also already
>> have "reviewed", which I believe means whoever added the label has
>> fully reviewed a PR and it's ready to be merged.
>
> I can't put labels on github. The feature seems to be limited to those
> with write access to the repository, so it's useless if we want more
> reviewers.
>

If not a misconfiguration, I think this is a bummer.

>>
>>> 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. 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.
>>>
>>
>> This is a big one too. Since we don't have a "development" branch (or
>> rather "master" is the development branch), we often have too high
>> standards, only merging stuff that is 100% finished, debugged and
>> ready to release. This kind of defeats the purpose of a development
>> branch.
>>
>> Assuming there's a general consensus that a PR is a good idea, I agree
>> we should start integrating it early, even if there's some possibility
>> it could break things. This way it can get tested in real-life by more
>> than just the person who initiated the PR and others could/would make
>> their own PRs to fixup any perceived issues. When a PR sits off on
>> someone else's repo, it doesn't get real testing, and people (rarely)
>> make PRs against the repo where it came from, where they more likely
>> would if it was in the main repo.
>>
>
> I agree. Currently there is a culture that everything must be perfect
> prior to merging. This is fine in theory but simply doesn't scale. It
> requires endless iterations which in turn require a lot of re-testing,
> and results in PRs sitting for way longer than necessary. Up to the
> point where the rate of new PRs can't be handled.
>
> I agree that PRs should be merged earlier, with possible
> fix-up/follow-up commits in a new PR. This way changes acutally get the
> testing they need.
>
>>> 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?
>>>
>>
>> 4. Lack of collaboration; We tend not to use Git to its full potential
>> by making pull requests against active pull requests. I don't know if
>> it's a lack of Git knowledge or that it's too much hassle, but we
>> rarely seem to do this (I've only done it a couple times, Colomban
>> does it more often I think). Instead we make comments like "you should
>> do this or that", assuming that the only person who must make changes
>> is the pull requester. If instead of making "do this" comments, we
>> made pull requests to show/implement it, it would give something
>> concrete to discuss and increase collaboration on the code.
>
> It's awkward to use because it's not visible on the original PR. It's
> often hard enough to monitor one PR, let alone recursively monitoring
> PRs in other repos. Also if there are more than 2 people involved at
> least some of them don't see the PR-ontop-of-PR. Lastly, if you actually
> merge those PRs you get a weird history with "Merged Pull Request #XX"
> from different repos (of course XX changes widely).
>

I agree with the visibility issue. I don't think the sub-history is a 
big deal, especially since we have a tendency/ability to touch-up such 
commits to make the branch/PR cleaner (for better or worse). Who cares 
what PRs were made against a sub-sub-sub-PR, as long as the correct 
authorship is maintained where warranted for individual patches, and 
those commits get added to the original PR for full visibility to those 
watching the original PR.

> I think gists linked to from the original PR would be more natural.
>

For smaller stuff, I agree.

>>
>> 5. This one applies to many of your PRs Jiří :) Speaking only for
>> myself here, any PRs made against CTags or TagManager I pretty much
>> ignore. Not only that I'm completely unfamiliar with their code, but
>> also the changes are often very subtle and require in-depth knowledge
>> of those code bases, and I'm neither qualified nor inclined to make
>> any judgments about the PR, let alone review or merge them.
>
> We need other means to ensure quality if nobody is up to reviewing them
> properly. These code areas could make good use unit tests so that merge
> changes there can be merged with confidence even if they are not deeply
> reviewed.
>

Coincidentally, this is the only area of Geany's source code that 
contains any kind of tests :)

I wouldn't say nobody is up to reviewing though, Jiří and Colomban, 
among others, seem to have a good grasp of these off-shoot libraries, I 
was strictly speaking for myself there.

>>
>> 6. Monster PRs. This goes along with #3. Sometimes PRs are just too
>> big to be digested by volunteer developers all at once. If we started
>> integrating such larger changes earlier, and in smaller pieces, I
>> think it would increase the likelihood of getting the feature/changes
>> merged into the master branch. When a PR is so big, it basically takes
>> as much time to fully review and test it as it did to make the changes
>> in the first place. I personally rarely merge pull requests unless
>> I've reviewed and understand each line of code that changed, and
>> tested all the cases I can think of. As volunteer like the rest of us,
>> I just simply can't afford to spend days/weeks reviewing,
>> understanding, and testing such large PRs, especially if I'm rather
>> indifferent on the feature/improvement they implement.
>>
>
> Yes, monster PRs are difficult to deal with. But on the other hand I get
> the response that "I don't like to merge this if nothing uses it right
> now" (when making a groundwork change required by a later one) when
> attempting to make smaller, incremental changes. Both views are
> reasonable. Plus, changes that merely lay the groundwork and don't have
> any impact itself seem to have very low review priority and are merged
> with large delay, if at all.
>

I think this is where some kind of recognized "road-map" would be 
useful. Whether it be on the wiki, or some other appropriate venue,if it 
is documented what the goals and general milestones of larger PRs like 
this are, then it not only makes it more clear to what end the 
individual PRs exists, but also lets us track the progress on these 
larger changes.

> I also have learned that breaking up large changes into lots of small
> commits doesn't help either. It still makes the PR as a whole rather
> large and lots of commits are not handy to deal with either.
>
>> 7. Lack of committers/commits. There's 7 people who have write access
>> to the repo. Of them, Colomban probably does > 95% of the actual
>> merges/commits. IMO, we either need more people able to assign
>> themselves and merge pull requests, or we need to spread the workload
>> out between those who already can. Personally, when I get some time, I
>> go through and cherry pick the very simple/trivial PRs and try and
>> merge them, but it's not very fair to leave all the really hard ones
>> for Colomban (not to put words in his mouth). If this existing
>> committers don't have enough time or interest to keep on top of pull
>> requests, then all we can do (besides status quo) is to have more
>> interested, trusted developers able to merge pull requests.
>>
>
> I agree. The more the merrier, if it helps spreading the workload. Jiří
> is an awesome candidate, especially since he's the official MAC guy.
>

+1

> I'd volunteer as well. However, given how I'm constantly failing to do
> good PRs initially or even after 2 or 3 revisions (by Colomban's
> standards), I'm not sure I'm suitable.
>

I have no problem with you being added either. Quite a few of your 
changes are relatively major and you would make PRs before merging 
anyway. It's not surprising, given the magnitude, that some of your PRs 
get a little bogged down, it's natural  (see "road map" above).

IMO, the main thing is to trust committers not to make significant 
changes without first going through a PR/review, and not merging stuff 
until they've reviewed/tested as much as feasible, even for their own 
changes, whenever the PR is deemed "ready".

Cheers,
Matthew Brush