[PATCH wayland 1/2] contributing: add review guidelines

[Daniel Stone]

Hi Pekka,

On Mon, 18 Jun 2018 at 14:43, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> This sets up the standards for patch review, and defines when a patch
> can be merged. I believe these are the practises we have been using
> already for a long time, now they are just written down explicitly.
>
> It's not an exhaustive list of criteria and likely cannot ever be, but
> it should give a good idea of what level of review we want to have.
>
> It has been written in general terms, so that we can easily apply the
> same text not just to Wayland, but also Weston and other projects as
> necessary.
>
> This addition is not redundant with
> https://wayland.freedesktop.org/reviewing.html .
>
> The web page is a friendly introduction and encouragement for people to
> get involved. The guidelines here are more specific and aimed for people
> who seek commit rights or maintainership.

Thanks a lot for tackling this! Having it written down is fantastic,
and I like how you've put it. I have two minor suggestions below:

> +- The code is correct and does not ignore corner-cases.

In general this is what we aim for, but the presence of 'XXX'/'FIXME'
comments in the code shows we don't always live up to that ideal. ;)
Maybe this could instead be something like:

The code is correct and does not introduce new failures for existing
users, nor add new corner-case bugs.

When fixing bugs, 'root cause' fixes are preferred rather than hiding
symptoms: identify why the issue has occurred, and fix it as close as
possible to the source. If it is not practical to completely fix the
issue, partial fixes should be accompanied by a comment in proximate
code, as well as a new issue opened in GitLab clearly noting known
corner cases with a suggestion on how to fix them.

> +- The code does what it says in the commit message and changes nothing else.

Which leads naturally to something like:

If the commit message starts getting too long and addresses multiple
points, this may be a sign that the commit should be broken into
smaller individual commits.

I'm happy to take alternate wording, since what you've written here is
far more concise than mine. Anyway, both patches are:
Reviewed-by: Daniel Stone <daniels at collabora.com>

Could this later be used as the basis for similar Weston patches?

Cheers,
Daniel