[weston,4/4] doc: Use GitLab MRs for patches, not the list

Submitted by Daniel Stone on July 14, 2018, 1:09 p.m.

Details

Message ID 20180714130907.6161-5-daniels@collabora.com
State Superseded
Headers show
Series "Doc updates, GitLab MRs" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Daniel Stone July 14, 2018, 1:09 p.m.
Though Wayland and the protocols still use mail-based patch review,
Weston can now move to GitLab MRs with review through that system.

Add some documentation on how to submit patches through GitLab,
specifically targeted at people who may be familiar with GitLab review,
but not familiar with our rebasing microcommit workflow.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 CONTRIBUTING.md | 140 ++++++++++++++++++++++++------------------------
 1 file changed, 70 insertions(+), 70 deletions(-)

Patch hide | download patch | download mbox

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 91b3fffe9..33b78510e 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -4,8 +4,45 @@  Contributing to Weston
 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).
+Weston is
+[hosted on freedesktop.org's GitLab](https://gitlab.freedesktop.org/wayland/weston/):
+in order to submit code, you should create an account on this GitLab instance,
+fork the core Weston repository, push your changes to a branch in your new
+repository, and then submit these patches for review through a merge request.
+
+Weston formerly accepted patches via `git-send-email`, sent to
+**wayland-devel@lists.freedesktop.org**; these were
+[tracked using Patchwork](https://patchwork.freedesktop.org/projects/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, Weston has a
+[linear history](http://www.bitsnbites.eu/a-tidy-linear-git-history/). This
+means that every commit should be small, digestible, stand-alone, and
+functional. Rather than a chronological commit history like this:
+
+    doc: final docs for view transforms
+    fix tests when disabled, redo broken doc formatting
+    better transformed-view iteration (thanks Hannah!)
+    try to catch more cases in tests
+    tests: add new spline test
+    fix compilation on splines
+    doc: notes on reticulating splines
+    compositor: add spline reticulation for view transforms
+
+we aim to have a clean history which only reflects the final state, broken up
+into functional groupings:
+
+    compositor: add spline reticulation for view transforms
+    compositor: new iterator for view transforms
+    tests: add view-transform correctness tests
+    doc: fix Doxygen formatting for view transforms
 
 The first line of a commit message should contain a prefix indicating
 what part is affected by the patch followed by one sentence that
@@ -54,74 +91,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 and Weston. Xwayland patches are tracked
-with the [Xorg project](https://patchwork.freedesktop.org/project/Xorg/list/)
-instead. 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 and Weston.
-
-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 or
-    Weston. 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/Weston.
+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 indivdiually 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

On Sat, 14 Jul 2018 14:09:07 +0100
Daniel Stone <daniels@collabora.com> wrote:

> Though Wayland and the protocols still use mail-based patch review,
> Weston can now move to GitLab MRs with review through that system.
> 
> Add some documentation on how to submit patches through GitLab,
> specifically targeted at people who may be familiar with GitLab review,
> but not familiar with our rebasing microcommit workflow.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  CONTRIBUTING.md | 140 ++++++++++++++++++++++++------------------------
>  1 file changed, 70 insertions(+), 70 deletions(-)

Hi Daniel,

I'm happy to move to MR-based work flow immediately.

> 
> diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> index 91b3fffe9..33b78510e 100644
> --- a/CONTRIBUTING.md
> +++ b/CONTRIBUTING.md
> @@ -4,8 +4,45 @@ Contributing to Weston
>  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).
> +Weston is
> +[hosted on freedesktop.org's GitLab](https://gitlab.freedesktop.org/wayland/weston/):
> +in order to submit code, you should create an account on this GitLab instance,
> +fork the core Weston repository, push your changes to a branch in your new
> +repository, and then submit these patches for review through a merge request.
> +
> +Weston formerly accepted patches via `git-send-email`, sent to
> +**wayland-devel@lists.freedesktop.org**; these were
> +[tracked using Patchwork](https://patchwork.freedesktop.org/projects/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, Weston has a
> +[linear history](http://www.bitsnbites.eu/a-tidy-linear-git-history/). This

From the above link, I found this:
http://www.bitsnbites.eu/git-history-work-log-vs-recipe/
which explain the below in more words. Maybe include this link as well?

Linear history doesn't exactly imply "recipe" history, we want both. I
only came to think of the differences after reading both of the above
links.

> +means that every commit should be small, digestible, stand-alone, and
> +functional. Rather than a chronological commit history like this:
> +
> +    doc: final docs for view transforms
> +    fix tests when disabled, redo broken doc formatting
> +    better transformed-view iteration (thanks Hannah!)
> +    try to catch more cases in tests
> +    tests: add new spline test
> +    fix compilation on splines
> +    doc: notes on reticulating splines
> +    compositor: add spline reticulation for view transforms
> +
> +we aim to have a clean history which only reflects the final state, broken up
> +into functional groupings:
> +
> +    compositor: add spline reticulation for view transforms
> +    compositor: new iterator for view transforms
> +    tests: add view-transform correctness tests
> +    doc: fix Doxygen formatting for view transforms
>  
>  The first line of a commit message should contain a prefix indicating
>  what part is affected by the patch followed by one sentence that
> @@ -54,74 +91,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 and Weston. Xwayland patches are tracked
> -with the [Xorg project](https://patchwork.freedesktop.org/project/Xorg/list/)
> -instead. 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 and Weston.

We need a wayland patch to remove notes about Weston like the above.


...

> -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/Weston.
> +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 indivdiually look good, then force-push your new branch to

"individually"

> +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

Looks good!

Patches 1 and 2:
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

Patch 4 also R-b with fixes, and patch 3 practically as well.

Oh wait, the rename of README to README.md will probably drop the file
from the dist tar-ball, since it's not auto-magic by autotools anymore.
CONTRIBUTING.md should also be added to dist. And the screenshot.jpg.

Should we start with Gitlab MRs already, but merge the doc patches only
after the final release, as per our documented release process?


Thanks,
pq
Hi Pekka,

On Fri, 3 Aug 2018 at 12:05, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> I'm happy to move to MR-based work flow immediately.

\o/

> From the above link, I found this:
> http://www.bitsnbites.eu/git-history-work-log-vs-recipe/
> which explain the below in more words. Maybe include this link as well?
>
> Linear history doesn't exactly imply "recipe" history, we want both. I
> only came to think of the differences after reading both of the above
> links.

Yeah, good point. I've just changed it to include the recipe link:
linear history is a far less important detail than the recipe side.

> Patches 1 and 2:
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
>
> Patch 4 also R-b with fixes, and patch 3 practically as well.
>
> Oh wait, the rename of README to README.md will probably drop the file
> from the dist tar-ball, since it's not auto-magic by autotools anymore.
> CONTRIBUTING.md should also be added to dist. And the screenshot.jpg.
>
> Should we start with Gitlab MRs already, but merge the doc patches only
> after the final release, as per our documented release process?

Thanks, I'll resubmit now. I am keen to avoid a situation where we
have a mismatch between what we do (GitLab MRs) and the contribution
documentation (mailing list). I think it would be great to get these
in the release, since they carry no risk of code regression, and mean
that anyone reading from the release will immediately see how to
contribute properly.

Cheers,
Daniel