New candidate JEP: 369: Migrate to GitHub

[Andrew Dinn]

On 13/11/2019 11:42, Erik Helin wrote:
> Thanks Andrew for reading through it all and providing feedback! Please
> see my replies inline.

You are very welcome. It definitely merits a considered and fair review.

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

I am happy to hear that you will be supplementing the standard GitHub
experience with extra tooling. I also must apologize for not looking at
Skara before writing a critique that perhaps does not do it justice. I
will take a look and post my thoughts.

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

I'm afraid that makes it sound worse not better. This bifurcates the
review process into 'general comments' vs 'critique on the code per se',
with the former forced into a  linear frame while the latter can benefit
from divide and conquer distinctions. I fear that's an artificial
division of concerns that will make it harder still to reconcile general
points with the evidence base for deriving them.

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

Well, I agree that sometimes that will work ok. However, I may be wrong
here but I believe that the code comments are tied to a specific point
in the file/change set. That's ok when a comment only relates to one
change. When you wish comment on the combined effect of two or more
changes at different points in the file the requirement to attach a
comment to one specific change doesn't really work. Punting such
comments to the 00 thread doesn't do it either. Quoting the relevant
lines in a follow-up note does bring the relevant changes into the scope
of preceding and subsequent replies.

The root point here is that the GitHub PR presentation model is going to
impose constraints on the way we structure and link our review comments
because it needs them to conform to it's information structure that is
essentially driven by it's web page layout. email is inherently much
more flexible because it is just a hierarchically linked set of texts.
Of course, GitHub provides aids to simplify the task of creating and
linking information in its format. Whereas with email one has to
explicitly structure the information by editing it and placing it in a
reply sequence. I can see GitHub making some things a lot easier.
However, my concern is that it may well make some important things that
we do difficult or maybe even impossible (at the least impractical).

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

Well, I will of course look into this further. Thanks you for pointing
me at the relevant matter to consider.

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

Yes, of course. However, rather than express that as 'email is less
structured' one might equally state it as 'email is much more flexible'
or 'GitHub is much more rigid' ;-)

> 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

Thanks for clarifying

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

No, I was not addressing that at you -- apologies if ti came across like
that as I very much appreciate that you do take this seriously -- rather
at all other OpenJDK project members to try to raise awareness of the
importance of getting a change like this right.

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