[Geany-Devel] Zombified pull requests

[Thomas Martitz]

Am 06.01.2016 um 12:47 schrieb Lex Trotman:
>> 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.
>>
> Please just add a comment, its actually clearer anyway since we don't
> have a well defined semantics for the labels :)


The advantage of labels is that you see them on the PR overview. Whereas 
with comments you have to open every single PR and look for strings that 
may indicate a review result. While labels can be named arbitrarily, in 
the end there's going to be a handful common ones which can be looked 
out for (I disagree that free-form comments are clearer than labels for 
indicating actual results).

So comments don't scale, whereas labels could be a real help when 
looking through the list of open PRs for potentially ready PRs.


>
>>>> 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.
> The problem with a development branch and with making master more
> buggy/less stable is that they will be used less, since its dangerous
> to use them day to day.  So the testing will go down even more.

You don't use git head even now with supposedly-perfect merges, so you 
are not lost if it would become a bit less stable :-)

There are lots of other people which are brave enough, and that's more 
than the 2 people that test each PR (the reviewer and the author). So 
it's a still a win.

Of course there should be a minimum level of stability that should be kept.

>
>>>> 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.
> If there is not enough effort to keep up with the commits, there is
> even less effort available to make pull requests on top of other
> peoples.
>
>>
>> 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 think gists linked to from the original PR would be more natural.
> Don't know a good solution to this one, maybe if you want to actually
> make changes, just mail the patches to the OP and let them put them up
> on the PR.
>
>>> 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.
> Yes, better tests for the non-GUI areas would make them easier to
> change.  Your offer to write the tests is accepted :)

Tests are written when 1) a change to the code is prepared and the test 
verifies that change or b) when a bug is fixed and the test verifies the 
bug is truly fixed. Just writing tests out of the blue (I don't intend 
to make a change or fix a bug there) don't work.

However we could start demanding tests as part of PRs (where applicable) 
and perhaps integrate with Travis / other CI.

On the other hand, demanding tests might drive contributors away.

>
>>> 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 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.
> You are right that individual commits on a monster PR don't help much
> unless they can be reviewed, tested and committed individually.
>
> The way massive changes are handled on Julia is for the OP to put up a
> PR (or just an issue) with a list of the steps required for the whole
> change.  Individual PRs that make preparatory changes then refer to
> this so there is a context for them and they can be smaller and easier
> to review/test.

That's a good idea. IIRC you can even draw checkboxes / todo lists in 
github comments?

However there is another catch: I'm hesitent to make PRs on top of other 
PRs (even in the same repository). Heck, I don't know even it's 
possible. The catch is that even if I break large changes into multiple 
PRs, I can still open the PRs only sequientially, so the 2nd PR after 
the 1st one is complete and merged. This makes it hard to see upcoming 
changes in the context of of the early PRs. Is there a solution to this 
catch?

Best regards.