When to do a review for a downported change?

[Andrew John Hughes]

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

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).
-- 
Andrew :)

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
https://keybase.io/gnu_andrew