[haiku] Re: How to improve Haiku
- From: Pawel Dziepak <pdziepak@xxxxxxxxxxx>
- To: haiku@xxxxxxxxxxxxx
- Date: Sun, 27 Apr 2014 20:50:02 +0200
2014-04-27 17:51 GMT+02:00 pulkomandy <pulkomandy@xxxxxxxxx>:
> On Sun, Apr 27, 2014 at 02:42:40PM +0200, Pawel Dziepak wrote:
>> Such approach, in my opinion, is much
>> better than reviewing patches on trac, which I find very inconvenient
>> for such purpose as it is not possible to add comments to an
>> individual line or a part of the code. Also, I am not a big fan of
>> github pull requests and I prefer the simplicity of mailing list and
>> basic git tools, but I will be happy if any solution that improves
>> code quality by mandatory code review will be adopted. Last, but not
>> least, there is actually a reason why I didn't proposed such changes
>> to our workflow earlier. While this may work very well with project
>> when the vast majority of developers is employed full-time I am not
>> sure it will work for us with our limited resources.
>
> May I, once again, suggest we setup Gerrit and use the git workflow that
> goes with it? Patches are submitted using a simple git push operation.
> They enter the Gerrit code review system where line-by-line reviews and
> votes (+1/-1) are possible. Gerrit can even be configured to enforce
> patches to get a score of at least "+2" and no "-" from reviewers before
> it can be applied. It seems the patches are properly rebased when merged
> to the main tree.
I think that it is also very important that all patches have to wait
for a while (1-2 days) before they got merged even if they are
positively reviewed much sooner. Potential mistakes and errors has to
get a chance to get caught and such delay shouldn't reduce the
progress of the project in any significant way.
> The original patch author can make modifications to the patch, and
> re-submit it, if done properly, this is appended to the existing Gerrit
> entry so it's easy to review the new patch in context.
>
> Gerrit also handles dependencies between patches ("this patch needs xxx
> to be merged first").
Well, that's what I would expect from Gerrit after pushing more than
one commit in a single push. I wonder what would happen if e.g. I push
a series of 3 patches, 1st and 3rd one get positively review and the
second one is rejected. Do I have to resend the whole series or just
the second patch? How Gerrit would know that the new patch is a part
of the series? (or - how gerrit would know that the remaining two
patches had been already reviewed?) What if the change in second patch
requires also some modification to the 3rd patch?
> I think this would lead to a much more solid state for the trunk, making
> it much easier to get a version of it tagged "release", with a MUCH
> shorter release cycle than we currently have, and more stable nightlies.
>
> With such a great tool in place, reviewing patches will become a much
> easier and rewarding job, and we can finally reply to people fast enough
> so we have a chance of them fixing the issues. Getting a review after
> two weeks that says "sorry, this patch you probably already gave up on
> getting applied has some coding style violations..." isn't a good way to
> get more contributors.
>
> With a tool like that, I think it's possible to make code reviews
> systematic, at least for "potentially breaking" changes.
That's actually the most important point of what I proposed earlier:
that all patches regardless whether authored by a committer or not
have to be reviewed before merge. Whether we use mailing list, github
pull requests, gerrit or anything else is just a matter of preference.
Obviously, the mailing has the advantage that we already have
everything we need for such workflow and people may start using git
send-email instead of git push right now. If someone sets up Gerrit it
will be nice to try it and check whether such workflow works out in
our situation.
> I'll add that Gerrit also supports checking that the patch doesn't break
> the build, and could easily be combined with an automated coding style
> checker.
Automated style checking would be nice, but not very important since
careful code review would caught such problems anyway. Checking if the
patch (or a series of patches) is a must though, there is no point
having an automated tool for reviewing patches if before merging
anything the developer that wants to do the merge has to manually
check if the build is not broken.
Paweł
Other related posts:
