[1/1] contributing: use Gitlab merge request workflow

Submitted by Pekka Paalanen on Feb. 26, 2019, 1:42 p.m.

Details

Message ID 20190226134233.23435-2-ppaalanen@gmail.com
State Superseded
Headers show
Series "Switch to Gitlab MR workflow for Wayland" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Pekka Paalanen Feb. 26, 2019, 1:42 p.m.
From: Pekka Paalanen <pekka.paalanen@collabora.com>

The experience from Weston shows that the Gitlab merge request based workflow
works really well. Recently there have also been issues with the mailing list
that have made the email based workflow more painful than it used to be. Those
issues might have been temporary or occasional, but they probably are only
going to increase.

The MR workflow is different, it has its issues
(https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/74) and we
likely lose the explicit Reviewed-by etc. tags from commit messages, but it is
also much easier to work with: no more whitespace damaged patches, lost email,
setting up git-send-email; we gain automated CI before any human reviewer even
looks at anything, and people can jump in to an ongoing discussion even if they
weren't subscribed before.

If you still want email, you can subscribe to that selectively(!) in Gitlab
yourself.

This text has been copied from Weston's CONTRIBUTING.md of the 5.0.91 release
and slightly altered for Wayland.

Fixes: https://gitlab.freedesktop.org/wayland/wayland/issues/49

Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
---
 CONTRIBUTING.md | 147 ++++++++++++++++++++++++------------------------
 1 file changed, 72 insertions(+), 75 deletions(-)

Patch hide | download patch | download mbox

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 686ed63..09fa9ba 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -4,8 +4,46 @@  Contributing to Wayland
 Sending patches
 ---------------
 
-Patches should be sent to **wayland-devel@lists.freedesktop.org**, using
-`git send-email`. See [git documentation] for help.
+Patches should be sent via
+[GitLab merge requests](https://docs.gitlab.com/ce/gitlab-basics/add-merge-request.html).
+Wayland is
+[hosted on freedesktop.org's GitLab](https://gitlab.freedesktop.org/wayland/wayland/):
+in order to submit code, you should create an account on this GitLab instance,
+fork the core Wayland repository, push your changes to a branch in your new
+repository, and then submit these patches for review through a merge request.
+
+Wayland formerly accepted patches via `git-send-email`, sent to
+**wayland-devel@lists.freedesktop.org**; these were
+[tracked using Patchwork](https://patchwork.freedesktop.org/project/wayland/).
+Some old patches continue to be sent this way, and we may accept small new
+patches sent to the list, but please send all new patches through GitLab merge
+requests.
+
+
+Formatting and separating commits
+---------------------------------
+
+Unlike many projects using GitHub and GitLab, Wayland has a
+[linear, 'recipe' style history](http://www.bitsnbites.eu/git-history-work-log-vs-recipe/).
+This means that every commit should be small, digestible, stand-alone, and
+functional. Rather than a purely chronological commit history like this:
+
+    connection: plug a fd leak
+    plug another fd leak
+    connection: init fds to -1
+    close all fds
+    refactor checks into a new function
+    don't close fds we handed out
+
+we aim to have a clean history which only reflects the final state, broken up
+into functional groupings:
+
+    connection: Refactor out closure allocation
+    connection: Clear fds we shouldn't close to -1
+    connection: Make wl_closure_destroy() close fds of undispatched closures
+
+This ensures that the final patch series only contains the final state,
+without the changes and missteps taken along the development process.
 
 The first line of a commit message should contain a prefix indicating
 what part is affected by the patch followed by one sentence that
@@ -45,7 +83,7 @@  We won't reject patches that lack S-o-b, but it is strongly recommended.
 
 When you re-send patches, revised or not, it would be very good to document the
 changes compared to the previous revision in the commit message and/or the
-cover letter. If you have already received Reviewed-by or Acked-by tags, you
+merge request. If you have already received Reviewed-by or Acked-by tags, you
 should evaluate whether they still apply and include them in the respective
 commit messages. Otherwise the tags may be lost, reviewers miss the credit they
 deserve, and the patches may cause redundant review effort.
@@ -54,78 +92,37 @@  deserve, and the patches may cause redundant review effort.
 Tracking patches and following up
 ---------------------------------
 
-[Wayland Patchwork](http://patchwork.freedesktop.org/project/wayland/list/) is
-used for tracking patches to Wayland. Xwayland patches are tracked with the
-[Xorg project](https://patchwork.freedesktop.org/project/Xorg/list/)
-instead. Weston uses
-[GitLab merge requests](https://gitlab.freedesktop.org/wayland/weston/merge_requests)
-for code review, and does not use mailing list review at all.
-
-Libinput patches, even though they use the same mailing list as
-Wayland, are not tracked in the Wayland Patchwork.
-
-The following applies only to Wayland.
-
-If a patch is not found in Patchwork, there is a high possibility for it to be
-forgotten. Patches attached to bug reports or not arriving to the mailing list
-because of e.g. subscription issues will not be in Patchwork because Patchwork
-only collects patches sent to the list.
-
-When you send a revised version of a patch, it would be very nice to mark your
-old patch as superseded (or rejected, if that is applicable). You can change
-the status of your own patches by registering to Patchwork - ownership is
-identified by email address you use to register. Updating your patch status
-appropriately will help maintainer work.
-
-The following patch states are found in Patchwork:
-
-- **New**:
-    Patches under discussion or not yet processed.
-
-- **Under review**:
-    Mostly unused state.
-
-- **Accepted**:
-    The patch is merged in the master branch upstream, as is or slightly
-    modified.
-
-- **Rejected**:
-    The idea or approach is rejected and cannot be fixed by revising
-    the patch.
-
-- **RFC**:
-    Request for comments, not meant to be merged as is.
-
-- **Not applicable**:
-    The email was not actually a patch, or the patch is not for Wayland.
-    Libinput patches are usually automatically ignored by Wayland
-    Patchwork, but if they get through, they will be marked as Not
-    applicable.
-
-- **Changes requested**:
-    Reviewers determined that changes to the patch are needed. The
-    submitter is expected to send a revised version. (You should
-    not wait for your patch to be set to this state before revising,
-    though.)
-
-- **Awaiting upstream**:
-    Mostly unused as the patch is waiting for upstream actions but
-    is not shown in the default list, which means it is easy to
-    overlook.
-
-- **Superseded**:
-    A revised version of the patch has been submitted.
-
-- **Deferred**:
-    Used mostly during freeze periods before releases, to temporarily
-    hide patches that cannot be merged during a freeze.
-
-Note, that in the default listing, only patches in *New* or *Under review* are
-shown.
-
-There is also a command line interface to Patchwork called `pwclient`, see
-http://patchwork.freedesktop.org/project/wayland/
-for links where to get it and the sample `.pwclientrc` for Wayland.
+Once submitted to GitLab, your patches will be reviewed by the Weston
+development team on GitLab. Review may be entirely positive and result in your
+code landing instantly, in which case, great! You're done. However, we may ask
+you to make some revisions: fixing some bugs we've noticed, working to a
+slightly different design, or adding documentation and tests.
+
+If you do get asked to revise the patches, please bear in mind the notes above.
+You should use `git rebase -i` to make revisions, so that your patches follow
+the clear linear split documented above. Following that split makes it easier
+for reviewers to understand your work, and to verify that the code you're
+submitting is correct.
+
+A common request is to split single large patch into multiple patches. This can
+happen, for example, if when adding a new feature you notice a bug in Weston's
+core which you need to fix to progress. Separating these changes into separate
+commits will allow us to verify and land the bugfix quickly, pushing part of
+your work for the good of everyone, whilst revision and discussion continues on
+the larger feature part. It also allows us to direct you towards reviewers who
+best understand the different areas you are working on.
+
+When you have made any requested changes, please rebase the commits, verify
+that they still individually look good, then force-push your new branch to
+GitLab. This will update the merge request and notify everyone subscribed to
+your merge request, so they can review it again.
+
+There are also
+[many GitLab CLI clients](https://about.gitlab.com/applications/#cli-clients),
+if you prefer to avoid the web interface. It may be difficult to follow review
+comments without using the web interface though, so we do recommend using this
+to go through the review process, even if you use other clients to track the
+list of available patches.
 
 
 Coding style

Comments

Hi Pekka,

On Tue, 26 Feb 2019 at 13:42, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> The experience from Weston shows that the Gitlab merge request based workflow
> works really well. Recently there have also been issues with the mailing list
> that have made the email based workflow more painful than it used to be. Those
> issues might have been temporary or occasional, but they probably are only
> going to increase.
>
> The MR workflow is different, it has its issues
> (https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/74) and we
> likely lose the explicit Reviewed-by etc. tags from commit messages, but it is
> also much easier to work with: no more whitespace damaged patches, lost email,
> setting up git-send-email; we gain automated CI before any human reviewer even
> looks at anything, and people can jump in to an ongoing discussion even if they
> weren't subscribed before.

Yes, I totally agree. It's been night and day since we switched Weston
over. We seem to do a much better job of not letting MRs fall between
the cracks, we're getting MRs from contributors we didn't previously
see. As a reviewer, just being able to track the discussions over the
various iterations of the patch without having to read the whole mail
thread over again and mentally keep track of what has and hasn't been
fixed, is magical. Plus actually having comments attached to
particular points in code, with context.

> -There is also a command line interface to Patchwork called `pwclient`, see
> -http://patchwork.freedesktop.org/project/wayland/
> -for links where to get it and the sample `.pwclientrc` for Wayland.
> +Once submitted to GitLab, your patches will be reviewed by the Weston

s/Weston/Wayland/

> +A common request is to split single large patch into multiple patches. This can
> +happen, for example, if when adding a new feature you notice a bug in Weston's

And again.

But the rest looks good to me and I'm thrilled to see it, so:
Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
On Tuesday, February 26, 2019 2:42 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>
> The experience from Weston shows that the Gitlab merge request based workflow
> works really well. Recently there have also been issues with the mailing list
> that have made the email based workflow more painful than it used to be. Those
> issues might have been temporary or occasional, but they probably are only
> going to increase.
>
> The MR workflow is different, it has its issues
> (https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/74) and we
> likely lose the explicit Reviewed-by etc. tags from commit messages, but it is
> also much easier to work with: no more whitespace damaged patches, lost email,
> setting up git-send-email; we gain automated CI before any human reviewer even
> looks at anything, and people can jump in to an ongoing discussion even if they
> weren't subscribed before.
>
> If you still want email, you can subscribe to that selectively(!) in Gitlab
> yourself.
>
> This text has been copied from Weston's CONTRIBUTING.md of the 5.0.91 release
> and slightly altered for Wayland.
>
> Fixes: https://gitlab.freedesktop.org/wayland/wayland/issues/49
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>

This seems to make it easier and friendlier for maintainers to review and merge
patches. I think this is pretty important. The CI integration is also a big win.

Reviewed-by: Simon Ser <contact@emersion.fr>

Thanks!
On Tue, 26 Feb 2019 14:13:03 +0000
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi Pekka,
> 
> On Tue, 26 Feb 2019 at 13:42, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > The experience from Weston shows that the Gitlab merge request based workflow
> > works really well. Recently there have also been issues with the mailing list
> > that have made the email based workflow more painful than it used to be. Those
> > issues might have been temporary or occasional, but they probably are only
> > going to increase.
> >
> > The MR workflow is different, it has its issues
> > (https://gitlab.freedesktop.org/freedesktop/freedesktop/issues/74) and we
> > likely lose the explicit Reviewed-by etc. tags from commit messages, but it is
> > also much easier to work with: no more whitespace damaged patches, lost email,
> > setting up git-send-email; we gain automated CI before any human reviewer even
> > looks at anything, and people can jump in to an ongoing discussion even if they
> > weren't subscribed before.  
> 
> Yes, I totally agree. It's been night and day since we switched Weston
> over. We seem to do a much better job of not letting MRs fall between
> the cracks, we're getting MRs from contributors we didn't previously
> see. As a reviewer, just being able to track the discussions over the
> various iterations of the patch without having to read the whole mail
> thread over again and mentally keep track of what has and hasn't been
> fixed, is magical. Plus actually having comments attached to
> particular points in code, with context.
> 
> > -There is also a command line interface to Patchwork called `pwclient`, see
> > -http://patchwork.freedesktop.org/project/wayland/
> > -for links where to get it and the sample `.pwclientrc` for Wayland.
> > +Once submitted to GitLab, your patches will be reviewed by the Weston  
> 
> s/Weston/Wayland/
> 
> > +A common request is to split single large patch into multiple patches. This can
> > +happen, for example, if when adding a new feature you notice a bug in Weston's  
> 
> And again.
> 
> But the rest looks good to me and I'm thrilled to see it, so:
> Reviewed-by: Daniel Stone <daniels@collabora.com>

Ha, I could swear I read it through a couple times to find the weston
mentions. :-D

I'll fix that and update.


Thanks,
pq