EXT: Re: [PATCH wayland v2] contributing: use Gitlab merge request workflow

Wed Mar 6 13:33:01 UTC 2019

On Wed, 6 Mar 2019 at 04:49, Ray, Ian (GE Healthcare) <ian.ray at ge.com> wrote:
>
>
>
> > On 6 Mar 2019, at 11.28, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> >
> > Going once, going twice...
> >
> > Any objections? More acks?

Acked-by: Derek Foreman <derek.foreman.wayland at gmail.com>

Personally, I really see no harm in pushing this during code freeze,
since it's not code. :)

Thanks,
Derek

> >
> > Let's say I'll push this and enable MRs on Friday if there are no
> > further comments. More acks and I might do it sooner. :-)
> >
> >
>
> LGTM
>
> Acked-by: Ian Ray <ian.ray at ge.com>
>
>
> > Thanks,
> > pq
> >
> >
> > On Wed, 27 Feb 2019 12:35:09 +0200
> > Pekka Paalanen <ppaalanen at gmail.com> wrote:
> >
> >> From: Pekka Paalanen <pekka.paalanen at 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
> >>
> >> v2: fixed two left-over mentions of Weston
> >>
> >> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.com>
> >> v1 Reviewed-by: Simon Ser <contact at emersion.fr>
> >> Reviewed-by: Daniel Stone <daniels at collabora.com>
> >> ---
> >> CONTRIBUTING.md | 147 ++++++++++++++++++++++++------------------------
> >> 1 file changed, 72 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> >> index 686ed63..dcc9f56 100644
> >> --- a/CONTRIBUTING.md
> >> +++ b/CONTRIBUTING.md
> >> @@ -4,8 +4,46 @@ Contributing to Wayland
> >> Sending patches
> >> ---------------
> >>
> >> -Patches should be sent to **wayland-devel at 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 at 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 Wayland
> >> +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 elsewhere
> >> +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
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel