New candidate JEP: 369: Migrate to GitHub
Erik Helin
erik.helin at oracle.com
Thu Nov 14 11:34:48 UTC 2019
On 11/13/19 6:39 PM, Andrew Dinn wrote:
> 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).
Yes, email is more free form (and thus more flexible) than essentially
any other medium. The interesting question to ponder here is if this
flexibility helps or hurts the majority of reviews being conducted in
OpenJDK on a daily basis?
I think we all have experienced where the flexibility of mailing lists
and free-form emails have resulted in less than stellar experiences as well:
- a thread gets forked to two different mailing lists because one
participant forgets to press "Reply all" and instead presses
"Reply List"
- a reviewer joins a "RFR" thread after the patch has been pushed since
there is no way see on a mailing list whether the patch has been
pushed or not
- a contributor forgets to mentions one more important facts in a RFR
email such as links to JBS issue(s), which commit the patch should
be applied upon or does not supply an incremental webrev
Looking at the jdk/jdk repository [0] we see that over 95% of all
commits have 3 or less reviewers [1]. Manually skimming the last weeks
of emails on core-libs-dev and hotspot-dev seems (to me) to confirm
this: the majority of the conversations have 1 - 4 participants (one
author and up to three reviewers). Looking at the conversations it also
seems (again, to me) that a majority of them take place on a singe list
and do not move/transfer to other mailing lists.
With the above in mind I do believe that flexibility and expressiveness
of pull requests combined with Skara's mailing list interoperability is
powerful enough. The experience from OpenJFX transitioning to GitHub +
Skara seem to confirm this, but this of course my interpretation based
on their feedback.
Now, as I have pointed in some other replies, we do *not* lose the
mailing lists just because we migrate to GitHub + Skara. If you have a
really large patch where you are expecting 5+ reviewers spanning 3+
mailing lists, then it is more than fine to send an RFC email with a
webrev, just like we do today. The patch will eventually have to become
one more pull requests and subject to the review process, but there is
nothing stopping you from using the mailing lists for the initial feedback.
>> 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.
Thanks for digging into this, we are longing for feedback :)
>> 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' ;-)
:) Please see my reply above for my thoughts on this matter.
>> 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
No problem at all.
>> 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.
No feelings hurt, I just wanted to stress how deeply we care about this
matter. And for the record, yes, I'm well aware of the importance of
getting this change right. I have worked in this community for more than
7 years now and have a deep respect for it and its members.
Thanks,
Erik
[0]: https://git.openjdk.java.net/jdk
[1]: https://gist.github.com/edvbld/a03dec6c84bad054d7529461a38efdf5
> 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