<div dir="ltr">Hi Matthew,<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 6, 2016 at 4:32 AM, Matthew Brush <span dir="ltr"><<a href="mailto:mbrush@codebrainz.ca" target="_blank">mbrush@codebrainz.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 2016-01-05 12:46 PM, Jiří Techet wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi,<br>
<br>
happy new year and let's celebrate it with something cheerful - zombies!<br>
<br>
</blockquote>
<br></span>
Wouldn't they only be zombies if we closed them and they re-opened themselves? :)</blockquote><div><br></div><div>Ah, right, I wasn't fully aware of the zombie lifecycle :-).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I've noticed there are more and more pull requests on github which don't<br>
get merged to Geany. It's clear that people are fighting with time to make<br>
reviews of pull requests (and Colomban does a great job here!), however, it<br>
seems to me there are quite many patches where all the hard work has<br>
already been done (review, patch updates) and the only missing thing is the<br>
merge - which doesn't happen.<br>
<br>
I think this is quite unfortunate - there are many patches which might be<br>
useful for users; at the same time it might be discouraging for<br>
contributors to see their patches unmerged.<br>
<br>
I've been thinking about what may be the cause of this and several things<br>
come to my mind:<br>
<br>
1. Not enough feedback from other developers. Typically I am for instance<br>
in the mode "I don't want to add too much noise" so unless I love or hate<br>
something, I just don't write anything. I think Colomban's own patches<br>
suffer from this most as he reviews other people's patches but there's no<br>
feedback for his own patches.<br>
<br>
Maybe it would be a good idea if regular Geany contributors go through the<br>
pull requests from time to time and just LGTM those that sound reasonable<br>
functionality-wise. I'm not talking about full code review here, just a<br>
very quick assessment whether the given feature makes sense for Geany (we<br>
should distinguish somehow "I made a review of the patch and LGTM" and "the<br>
functionality LTGM to be in Geany").<br>
<br>
</blockquote>
<br></span>
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"?</blockquote><div><br></div><div>Yeah, sounds better than LGBI's :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
To give an example of such a "functionality review", the fractional font<br>
sizes patch<br>
<br>
<a href="https://github.com/geany/geany/pull/407" rel="noreferrer" target="_blank">https://github.com/geany/geany/pull/407</a><br>
<br>
LGTM - even though I probably won't use it myself, I understand it may be<br>
useful for someone and it modifies just 23 lines so it's nothing intrusive<br>
and can go in from my point of view.<br>
<br>
</blockquote>
<br></span>
Agree, and I've actually wanted for this before.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Such a review can be done in a few seconds so if everyone goes through the<br>
new pull requests from time to time, the patches will receive some feedback<br>
and it will be clearer whether it's something others want it in Geany.<br>
<br>
2. Unclear status of some patches. Sometimes it might not be clear in what<br>
state the pull request is - I'd suggest adding at least the following two<br>
tags:<br>
<br>
needs-work (reviewed with some comments that need to be addressed)<br>
work-in-progress (not meant for review in the current state)<br>
<br>
This will help to distinguish pull requests awaiting merge and pull<br>
requests that aren't there yet.<br>
<br>
</blockquote>
<br></span>
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.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3. Fear that the pull request isn't tested enough. I believe that if a<br>
patch did undergo a review and there doesn't seem anything obviously wrong<br>
with it, it can be merged to master. I think there's no need for some<br>
long-term private testing of a patch before it gets merged - people using<br>
the development versions of Geany should be aware it may contain bugs and<br>
should know how to deal with them. There are also more eyes so a potential<br>
bug is spotted earlier. Of course it makes sense getting more conservative<br>
towards the end of the development cycle to stabilise things.<br>
<br>
</blockquote>
<br></span>
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.<br>
<br>
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.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
OK, these are things that come into my mind regarding the zombie pull<br>
requests but there may be other problems. What do you think? Do the points<br>
above look reasonable to you and do you think they might help a bit? Is<br>
there anything else that might help killing the bloody zombies?<br>
<br>
</blockquote>
<br></span>
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.<br>
<br>
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.<br></blockquote><div><br></div><div>Yeah, the TM stuff is an exception - the code is a bit tricky and it definitely needs some more time to get familiar with what TM actually does. I agree that it may be hard to assess just by quick looking at the patches whether it's "something we want" and whether "the patch does the right thing". Fortunately it's mostly reviewed by Colomban now.</div><div><br></div><div>In this area my long-term plan is to _completely_ sync the universal-ctags and geany ctags implementations so the ctags maintenance would become just a matter of gettting the latest code from universal-ctags. But there will be many patches on the way to be reviewed...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br></div><div>Again, this is the case of many of my TM patches (I think a similar situation was with the spawn module where the problems were very subtle too). In these cases I think someone just has to be the hero and make the review (and I really appreciate Colomban's work here).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br></div><div>I'm definitely the one to blame here as I have been resisting becoming a "full" developer with a commit access. I was worried I could easily make mess in master by doing "git push origin master" without much thinking but I think I can overcome this by not having the master repo at "origin". So yeah, maybe it's a good idea if I get commit access too. Now I'm afraid the same problems like others are facing apply to me too:</div><div><br></div><div>1. Lack of time</div><div>2. Lack of knowledge of some stuff (will definitely stay away from anything autotools-related :-)</div><div>3. Incapability of producing as great reviews as Colomban :-)</div><div><br></div><div>Cheers,</div><div><br></div><div>Jiri</div></div></div></div>