New candidate JEP: 369: Migrate to GitHub
Erik Helin
erik.helin at oracle.com
Wed Nov 13 11:42:21 UTC 2019
On 11/13/19 11:36 AM, Andrew Dinn wrote:
> On 12/11/2019 16:00, mark.reinhold at oracle.com wrote:
>> https://openjdk.java.net/jeps/369
> Firstly, thanks to Joe and Erik for producing a very comprehensive JEP
> that addresses so many important questions. There are many aspects of
> this proposed move that are very appealing.
Thanks Andrew for reading through it all and providing feedback! Please
see my replies inline.
> However, I am afraid I am
> still going to start by expressing my severe reservations about one
> proposed change: replacing email review with GitHub PRs (sorry guys :-/).
>
> The JEP has this laudable goal:
>
> "Ensure that workflows structurally similar to the existing e-mail and
> webrev based workflows are supported."
>
> and further qualifies that with:
>
> "Today, OpenJDK contributors collaborate via mailing lists, they push
> changes to Mercurial repositories, they test changes via the jdk/submit
> service, and they file bug reports via the JDK Bug System (JBS).
> Contributors can also make use of several command-line interface (CLI)
> tools, primarily jcheck, webrev, and defpath. This is a workflow that
> many experienced contributors enjoy and find productive. It's therefore
> critical to preserve as much of this workflow as possible if we move to
> an external provider."
>
> which one cannot really disagree with.
>
> The JEP then explains that:
>
> "Reviewers can now discuss the changes in the PR, using multiple workflows:
>
> By replying to the RFR email sent to the mailing list(s), in which
> case the contents of replies are copied into the PR on GitHub (no GitHub
> account is required);
> . . .
> Any comment made in any of the workflows is reflected in all of the
> workflows. One reviewer can add a comment via the mailing list, another
> via the web browser, and a third via the command-line and they will all
> see each others' comments."
>
> My experience of GitHub use in the Graal project suggest that this is
> not entirely the full picture. My view derived from that experience is
> that the GitHub PR is very much an inferior medium for review
> discussion. In particular, it definitely fails to be "structurally
> similar to the existing e-mail and webrev based workflows".
I think the key sentence here is "My experience of GitHub use in the
Graal project...". Having worked with project on GitHub using Skara
tooling and projects on GitHub _not_ using Skara tooling, my view is
that the experiences differs quite a bit.
> Firstly, I'll note that email discussions consist of a tree of dependent
> commentary. That commentary often contains quoted material which,
> especially when carefully edited (as it normally is), tracks relevant
> context. The structuring that results from this use of the reply
> hierarchy and quoting permits a given discussion to split into a group
> of related subordinate discussions where required while still retaining
> a continued thread of relevance to a specific review. Crucially, most
> email readers allow such branching discussions to be followed and
> extended in /parallel/.
>
> By contrast discussions in GitHub PRs are essentially linearized (modulo
> a single level of nesting of commentary on top level comments). Worse,
> the presentation forces all commentary to be flattened into a single
> browser pane view. This linear mode of presentation results in a severe
> hindrance to separation of, and attention to, separate concerns. It also
> fails to preserve and display source relationships that clarify the
> provenance and relevance of quoted text i.e. it not only fails to record
> but actually obfuscates important context.
This does not paint the whole picture. You are probably thinking of the
"Conversation" tab in a GitHub pull request which is meant for *general*
discussion of the patch that is not attached to any particular change in
the patch. GitHub's own help documentation [0] states that the
"Conversation" tab is meant to be used for:
You can comment on a pull request's Conversation tab to leave
general comments, questions, or props.
You are right that comments in the "Conversation" tab are linearized and
does not support a "tree style" view of comments.
The good news are that the _other_ form of comments available on a
GitHub pull request, a so-called "review comment" or "file comment" [1],
does support replies in the form of a tree. These comments are filed
under the "Files changed" tab in a pull request.
Personally I think of the comments in the "Conversation" tab as replies
to the "00" email in a classic patch series email and the "review
comments" in the "Files changed" tab as replies to the "0X" emails
actually containing patches. Do you follow?
Now the interesting question is of course how the Skara tooling
translates these kinds of comments back-and-forth between mailing lists
and GitHub. Here I'm happy to report that the Skara tooling does proper
replying, which will cause the "review comments" created under the
"Files changed" tab on a pull request to result in a tree-view (in email
clients that support such views).
You can see of some of this work on the openjfx-dev mailing list [2].
Now keep in mind if you are looking at Pipermail rendering of a mailing
list, which lacks some features that most email clients supports (such
as folding quoted text and nested replies beyond three levels). A
mailing list version of a pull request will in general have the
following structure:
- RFR: Fix a bug <-- Email describing patch
- Re: RFR: Fix a bug <-- This is a general comment
- Re: RFR: Fix a bug <-- This is a reply to the general comment
- Re: [Rev 01]: RFR: Fix a bug <-- The author updated the patch
- Re: [Rev 01]: RFR: Fix a bug <-- comment from reviewer A on 01
- Re: [Rev 01]: RFR: Fix a bug <-- reply to comment from A on 01
- Re: [Rev 01]: RFR: Fix a bug <-- comment from reviewer B on 01
- Re: [Rev 01]: RFR: Fix a bug <-- reply to comment from B on 01
- Re: [Rev 02]: RFR: Fix a bug <-- The author updated again
- Re: [Approved] RFR: Fix a bug <-- Reviewer A approved the patch
- Re: [Approved] RFR: Fix a bug <-- Reviewer B approved the patch
- Re: [Integrated] RFR: Fix a bug <-- Author integrated the patch
We are tweaking this structure as we get more and more experience with
it, but at the moment I'm fairly satisfied with threading and the tree
layout. If you have any feedback on this structure/layout, then we are
happy to hear it.
Going in the other direction, mailing list -> GitHub, we also try to
preserve the mailing list structure as much as possible. This is
actually a harder challenge, since an email is less structured compared
to a comment on GitHub. An example of this direction can be found in
pull request 231 for the Skara repository [3] where you can see the
thread (which is a tree) from skara-dev at openjdk.java.net [4] being
"flattened" and uses quotation to try to preserve the flow.
> Of course, discussions that remain in email may continue to profit from
> the multi-focus aspect of the current workflow. However, I believe that
> contributions to the discussion via a GitHub PR will inevitably bypass
> and, therefore, invalidate any such structuring contributed via the
> email workflow. Not only do I currently see that effect with Graal PRs
> but I also see the damage it imposes on flow and quality of discussion.
>
> Secondly, email reviews are conventionally directed to readers with
> different domains of interest and expertise. That sometimes requires
> re-routing a discussion to a different list from the one on which it was
> first initiated. It also sometimes requires posting comments to multiple
> lists, either when discussion is initiated or in mid-stream as the scope
> of discussion develops. Managing distribution when using emails lists is
> both easily achieved and well understood (n.b. that's not to imply that
> deciding which list to send to is easy).
Here we are currently working on the /cc command that can be used in a
comment on pull request, for example:
/cc build-dev hotspot-dev
> The JEP suggests that discussions raised via PRs will be routed to lists
> 1) derived from the affected files and/or 2) specified by whoever raises
> the PR. It doesn't explain how (whether) target lists are to be (may be)
> updated as review progresses. It also doesn't state clearly whether
> whoever raises the PR or comments on it will have the ability to
> override those default choices (I fully expect that option will be
> necessary in some cases).
Agree, see the command above that we are working on :)
> I need to stress that these concerns should not be considered as minor
> details of how the project operates. Easy management of the flow and
> direction of discussion is critical to being able to consume and respond
> to commentary at the rate needed to keep up with a project as large and
> rapidly changing as OpenJDK currently is. Given the central importance
> of our review process to ensuring the health of the project and its
> products I think it is right to be very concerned about the potential
> for problems caused by this switch of medium.
I hope that is does not come across that we are taking mailing list
interopability as a minor detail? I think it is fair to say that this is
the Skara feature we have spent the most time working on and are
constantly improving in order to provide a good experience.
Thanks,
Erik
[0]:
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/commenting-on-a-pull-request
[1]:
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/reviewing-proposed-changes-in-a-pull-request
[2]:
https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-November/thread.html
[3]: https://git.openjdk.java.net/skara/pull/231
[4]:
https://mail.openjdk.java.net/pipermail/skara-dev/2019-October/000998.html
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill
>
More information about the discuss
mailing list