When to do a review for a downported change?
Andrew John Hughes
gnu.andrew at redhat.com
Wed May 15 21:45:23 UTC 2019
On 14/05/2019 17:22, Aleksey Shipilev wrote:
> On 5/14/19 5:52 PM, Andrew Haley wrote:
>> On 5/14/19 4:02 PM, Aleksey Shipilev wrote:
>>> Aside, I think it is a good style (though optional) to post the diff
>>> between the upstream patch and the backport -- it seems low-overhead
>>> when there is the mq patch on top. This would also make reviews much
>>> easier, and probably fits the backporting workflow too. That is what
>>> I do anyway.
>> Post in what format, though? A diff of a diff?
> Depends. Whatever fits the workflow, and maybe with workflow adjustments:
> a) Sometimes just copy-paste *.rej and explain why those are not applicable.
> b) Sometimes just inline the "addon" patches that fix the original change: in my workflow with mq
> there are two patches: the original "backport" change with rejects, and the follow up patch that
> resolves those rejects, hg qdiff the second one and paste it.
> c) Sometimes just the copy-pasted affected block "before" / "after", showing the adjustment made;
> d) Sometimes just the diff of a diff is enough to highlight the changes. If that is not available,
> I would do that during review for a non-trivial backport _anyway_.
> e) Sometimes just pointing out which files and methods have differences vs upstream version, so we
> can eyeball them.
> I mean, anything that _you_ as reviewer, who sees the backport for the first time, would like to see
> in order to understand what was done, quickly and exactly. If backport differences are indeed
> trivial, then any of the ways above would amount to a copy-pasted paragraph in RFR, and it would
> breeze through the review, making the whole thing low-overhead.
> (This is not to say people here are not doing that. Just describing "the common sense" out loud.)
Indeed. When reviewing backports (which I do for jdk8u-fix-request as
well), I diff the original changeset against the webrev patch. Having
that in a review e-mail would help.
As to trivial changes, I'd lump copyright headers in with the filename
shuffling as trivial stuff that doesn't warrant a review. It is worth
noting though that copyright changes should only ever be applied when
they move to a later year (e.g. patch says 2019, code is 2018, so
changed to 2019). We shouldn't roll backwards (e.g. patch says 2018,
code is 2019, so changed to 2018).
Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
More information about the jdk-updates-dev