Re: Code reviews

[Phillip Lord]

Stefan Monnier <address@hidden> writes:

>> Introduce code reviews.  Don't give commit access to the "golden"
>> branches to everyone, just to a few top contributors and reviewers.
>
> We already have a fair bit of patches submitted and lingering in limbo
> forever until someone (almost always the same someone, BTW) finally
> loses hope that some of the other contributors take care of it.
>
> If we could switch to a system where every patch is reviewed before
> commit, that'd be great.  My own impression is that it will kill the
> development pace because too few people are willing to spend the
> corresponding efforts.

I think that there is a half-way house here. You extensively reviewed
the first change that I made to the core of Emacs -- as someone who
didn't know the internals or, indeed, even C, there is no way on earth
that I could have made that commit without. Other commits are less of
problem.

Giving people different permissions on different branches seems to make
sense. Overtime, people move toward master/emacs-25.

Having said that, while I found your feedback and the overall experience
very positive, I did feel the lack of a nicer pull/merge request system.
Tools such as gitlab or gerrit would have helped.



> That's why I've followed a practice of giving out write access very
> liberally, with "post-commit spot-check reviews" instead.  Indeed, it
> means that errors in commit messages can't be fixed (we can fix them in
> the ChangeLog files, admittedly, but since I don't use them it doesn't
> help me).
>
> Maybe we could have a half-way system, where commits are pushed to
> a branch that is "not fast-forward-only", and this branch is then
> auto-merged to the real (fast-forward-only) master branch after a delay
> (one day, maybe?) to give time to fix mess ups before they're cast
> in stone.

I think that this would require some considerable co-ordination. If I
push a broken commit to devel branch, and then you fix this commit
through rebase, my copy of the branch (and everyone elses) is now
broken. I'm pretty sure that this kind of fix up can only happen on a
feature branch in a sane way.

Phil