Process for cherry-picking patches from other projects

Process for cherry-picking patches from other projects

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

Starting a new thread to make it clear that we’re discussing a wider policy here.

This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.

There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.

The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.

The questions seem to be:

1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together, or should each patch be backported individually?

2) If “batches” are permissible, when?  When would individual patches be preferred?

3) For “batch updates”, what tags are necessary?  Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags?  Do we need to include anything from the original commits — commit messages?  Signed-off-by’s?

And a related question:

4) When importing an entire file from an upstream like Linux, what tags do we need?

My recollection is that we often to a “accumulated patch” to update, say, the Kconfig tooling; so it seems like the answer to this is sometimes “yes”.

It seems to me that in a case where you’re importing a handful of patches — say 5-10 — that importing them one-by-one might be preferred; but in this case, since the submission was already made as a batch, I’d accept having it as a batch.

I think if I were writing this patch, I’d make a separate “Origin” tag for each commit.

I wouldn’t include the upstream commit messages or S-o-b’s; I would write my own commit message summarizing why I’m importing the commits, then have the ‘origin’ tags, then my own S-o-b to indicate that I am attesting that it comes from an open-source project (and for whatever copyright can be asserted on the commit message and the patch as a collection).

And for #4, I would do something similar: I would write my own commit message describing what the file is for and why we’re importing it; have the Origin tag point to the commit at the point I took the file; and my own S-o-b.

Andy and Jan, what do you guys normally do?

Thanks,
 -George

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Process for cherry-picking patches from other projects

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3589 bytes --]

On 13.05.22 16:33, George Dunlap wrote:
> Starting a new thread to make it clear that we’re discussing a wider policy here.
> 
> This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.
> 
> There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.
> 
> The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.
> 
> The questions seem to be:
> 
> 1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together, or should each patch be backported individually?
> 
> 2) If “batches” are permissible, when?  When would individual patches be preferred?
> 
> 3) For “batch updates”, what tags are necessary?  Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags?  Do we need to include anything from the original commits — commit messages?  Signed-off-by’s?
> 
> And a related question:
> 
> 4) When importing an entire file from an upstream like Linux, what tags do we need?
> 
> My recollection is that we often to a “accumulated patch” to update, say, the Kconfig tooling; so it seems like the answer to this is sometimes “yes”.
> 
> It seems to me that in a case where you’re importing a handful of patches — say 5-10 — that importing them one-by-one might be preferred; but in this case, since the submission was already made as a batch, I’d accept having it as a batch.
> 
> I think if I were writing this patch, I’d make a separate “Origin” tag for each commit.
> 
> I wouldn’t include the upstream commit messages or S-o-b’s; I would write my own commit message summarizing why I’m importing the commits, then have the ‘origin’ tags, then my own S-o-b to indicate that I am attesting that it comes from an open-source project (and for whatever copyright can be asserted on the commit message and the patch as a collection).
> 
> And for #4, I would do something similar: I would write my own commit message describing what the file is for and why we’re importing it; have the Origin tag point to the commit at the point I took the file; and my own S-o-b.

IMO we should add another tag for that purpose, e.g.:

File-origin: <repository> <tag> <path> [# <local-path>]

Specifying the repository the file(s) are coming from, the tag (e.g. a
tagged version, or the top git commit), and the path of the original
file(s) in that repository (<path> could either be a common directory
of multiple files, or a single file; multiple "File-origin:" tags should
be possible). In case the file is being renamed locally, its new name
can be added as <local-path>.

This variant should be used to add _new_ files to Xen. In case of
updating a file which has seem lots of commits since its last update or
introduction, it might be easier to just use the "File-origin:" tag,
probably with a note below the "---" marker that listing more than <x>
patches (x > 10?) or splitting into more than <x> patches would be
just useless work (common sense should apply here, especially regarding
the readability of the patch and the related review effort).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: Process for cherry-picking patches from other projects

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]

On Fri, 13 May 2022, Juergen Gross wrote:
> On 13.05.22 16:33, George Dunlap wrote:
> > Starting a new thread to make it clear that we’re discussing a wider policy
> > here.
> > 
> > This question is aimed at Jan and Andy in particular, as I think they’ve
> > probably done the most of this; so I’m looking to them to find out what our
> > “standard practice” is.
> > 
> > There have recently been some patches that Bertrand has submitted which pull
> > in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with
> > Linux 5.18-rc3”), which has caused a discussion between him, Julien, and
> > Stefano about the proper way to do such patches.
> > 
> > The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc
> > suggests that there are some standards, but doesn’t spell them out.
> > 
> > The questions seem to be:
> > 
> > 1) When doing this kind of update, is it permissible to send a single patch
> > which “batches” several upstream commits together, or should each patch be
> > backported individually?
> > 
> > 2) If “batches” are permissible, when?  When would individual patches be
> > preferred?
> > 
> > 3) For “batch updates”, what tags are necessary?  Do we need to note the
> > changesets of all the commits, and if so, do we need multiple “Origin” tags?
> > Do we need to include anything from the original commits — commit messages?
> > Signed-off-by’s?
> > 
> > And a related question:
> > 
> > 4) When importing an entire file from an upstream like Linux, what tags do
> > we need?
> > 
> > My recollection is that we often to a “accumulated patch” to update, say,
> > the Kconfig tooling; so it seems like the answer to this is sometimes “yes”.
> > 
> > It seems to me that in a case where you’re importing a handful of patches —
> > say 5-10 — that importing them one-by-one might be preferred; but in this
> > case, since the submission was already made as a batch, I’d accept having it
> > as a batch.
> > 
> > I think if I were writing this patch, I’d make a separate “Origin” tag for
> > each commit.
> > 
> > I wouldn’t include the upstream commit messages or S-o-b’s; I would write my
> > own commit message summarizing why I’m importing the commits, then have the
> > ‘origin’ tags, then my own S-o-b to indicate that I am attesting that it
> > comes from an open-source project (and for whatever copyright can be
> > asserted on the commit message and the patch as a collection).
> > 
> > And for #4, I would do something similar: I would write my own commit
> > message describing what the file is for and why we’re importing it; have the
> > Origin tag point to the commit at the point I took the file; and my own
> > S-o-b.
> 
> IMO we should add another tag for that purpose, e.g.:
> 
> File-origin: <repository> <tag> <path> [# <local-path>]
> 
> Specifying the repository the file(s) are coming from, the tag (e.g. a
> tagged version, or the top git commit), and the path of the original
> file(s) in that repository (<path> could either be a common directory
> of multiple files, or a single file; multiple "File-origin:" tags should
> be possible). In case the file is being renamed locally, its new name
> can be added as <local-path>.

+1

> This variant should be used to add _new_ files to Xen. In case of
> updating a file which has seem lots of commits since its last update or
> introduction, it might be easier to just use the "File-origin:" tag,
> probably with a note below the "---" marker that listing more than <x>
> patches (x > 10?) or splitting into more than <x> patches would be
> just useless work (common sense should apply here, especially regarding
> the readability of the patch and the related review effort).

+1

Re: Process for cherry-picking patches from other projects

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 4385 bytes --]



> On May 13, 2022, at 3:52 PM, Juergen Gross <JGross@suse.com> wrote:
> 
> On 13.05.22 16:33, George Dunlap wrote:
>> Starting a new thread to make it clear that we’re discussing a wider policy here.
>> This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.
>> There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.
>> The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.
>> The questions seem to be:
>> 1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together, or should each patch be backported individually?
>> 2) If “batches” are permissible, when? When would individual patches be preferred?
>> 3) For “batch updates”, what tags are necessary? Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags? Do we need to include anything from the original commits — commit messages? Signed-off-by’s?
>> And a related question:
>> 4) When importing an entire file from an upstream like Linux, what tags do we need?
>> My recollection is that we often to a “accumulated patch” to update, say, the Kconfig tooling; so it seems like the answer to this is sometimes “yes”.
>> It seems to me that in a case where you’re importing a handful of patches — say 5-10 — that importing them one-by-one might be preferred; but in this case, since the submission was already made as a batch, I’d accept having it as a batch.
>> I think if I were writing this patch, I’d make a separate “Origin” tag for each commit.
>> I wouldn’t include the upstream commit messages or S-o-b’s; I would write my own commit message summarizing why I’m importing the commits, then have the ‘origin’ tags, then my own S-o-b to indicate that I am attesting that it comes from an open-source project (and for whatever copyright can be asserted on the commit message and the patch as a collection).
>> And for #4, I would do something similar: I would write my own commit message describing what the file is for and why we’re importing it; have the Origin tag point to the commit at the point I took the file; and my own S-o-b.
> 
> IMO we should add another tag for that purpose, e.g.:
> 
> File-origin: <repository> <tag> <path> [# <local-path>]
> 
> Specifying the repository the file(s) are coming from, the tag (e.g. a
> tagged version, or the top git commit), and the path of the original
> file(s) in that repository (<path> could either be a common directory
> of multiple files, or a single file; multiple "File-origin:" tags should
> be possible). In case the file is being renamed locally, its new name
> can be added as <local-path>.
> 
> This variant should be used to add _new_ files to Xen. In case of
> updating a file which has seem lots of commits since its last update or
> introduction, it might be easier to just use the "File-origin:" tag,
> probably with a note below the "---" marker that listing more than <x>
> patches (x > 10?) or splitting into more than <x> patches would be
> just useless work (common sense should apply here, especially regarding
> the readability of the patch and the related review effort).

You don’t mention what to do about SoB’s — I assume you agree with my assessment above, that a single  SoB from the submitter of the patch to Xen, asserting that they’re satisfied that all of the code has been asserted by other people as having a suitable license, is sufficient.

In which case, barring a contradiction from Andy or Jan as to our standard practice, I think that we don’t need to collect SoBs from the original commits; a single SoB by Bertrand (or whomever) that it all comes from Linux and that suitable SoBs can be tracked down should it become necessary, will be sufficient.

That should be enough to get the specific patch currently under discussion from the ARM maintainers un-stuck.

 -George

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Process for cherry-picking patches from other projects

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4552 bytes --]

On 16.05.22 15:12, George Dunlap wrote:
> 
> 
>> On May 13, 2022, at 3:52 PM, Juergen Gross <JGross@suse.com> wrote:
>>
>> On 13.05.22 16:33, George Dunlap wrote:
>>> Starting a new thread to make it clear that we’re discussing a wider policy here.
>>> This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.
>>> There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.
>>> The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.
>>> The questions seem to be:
>>> 1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together, or should each patch be backported individually?
>>> 2) If “batches” are permissible, when? When would individual patches be preferred?
>>> 3) For “batch updates”, what tags are necessary? Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags? Do we need to include anything from the original commits — commit messages? Signed-off-by’s?
>>> And a related question:
>>> 4) When importing an entire file from an upstream like Linux, what tags do we need?
>>> My recollection is that we often to a “accumulated patch” to update, say, the Kconfig tooling; so it seems like the answer to this is sometimes “yes”.
>>> It seems to me that in a case where you’re importing a handful of patches — say 5-10 — that importing them one-by-one might be preferred; but in this case, since the submission was already made as a batch, I’d accept having it as a batch.
>>> I think if I were writing this patch, I’d make a separate “Origin” tag for each commit.
>>> I wouldn’t include the upstream commit messages or S-o-b’s; I would write my own commit message summarizing why I’m importing the commits, then have the ‘origin’ tags, then my own S-o-b to indicate that I am attesting that it comes from an open-source project (and for whatever copyright can be asserted on the commit message and the patch as a collection).
>>> And for #4, I would do something similar: I would write my own commit message describing what the file is for and why we’re importing it; have the Origin tag point to the commit at the point I took the file; and my own S-o-b.
>>
>> IMO we should add another tag for that purpose, e.g.:
>>
>> File-origin: <repository> <tag> <path> [# <local-path>]
>>
>> Specifying the repository the file(s) are coming from, the tag (e.g. a
>> tagged version, or the top git commit), and the path of the original
>> file(s) in that repository (<path> could either be a common directory
>> of multiple files, or a single file; multiple "File-origin:" tags should
>> be possible). In case the file is being renamed locally, its new name
>> can be added as <local-path>.
>>
>> This variant should be used to add _new_ files to Xen. In case of
>> updating a file which has seem lots of commits since its last update or
>> introduction, it might be easier to just use the "File-origin:" tag,
>> probably with a note below the "---" marker that listing more than <x>
>> patches (x > 10?) or splitting into more than <x> patches would be
>> just useless work (common sense should apply here, especially regarding
>> the readability of the patch and the related review effort).
> 
> You don’t mention what to do about SoB’s — I assume you agree with my assessment above, that a single  SoB from the submitter of the patch to Xen, asserting that they’re satisfied that all of the code has been asserted by other people as having a suitable license, is sufficient.

Yes.

> In which case, barring a contradiction from Andy or Jan as to our standard practice, I think that we don’t need to collect SoBs from the original commits; a single SoB by Bertrand (or whomever) that it all comes from Linux and that suitable SoBs can be tracked down should it become necessary, will be sufficient.

Yes, this is the idea by specifying a tag of the source repository.
Using the correct git commands it should always be possible to get
the complete list of SoBs for an imported file.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: Process for cherry-picking patches from other projects

[Andrew Cooper]

On 13/05/2022 15:33, George Dunlap wrote:
> Starting a new thread to make it clear that we’re discussing a wider policy here.
>
> This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.
>
> There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.
>
> The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.
>
> The questions seem to be:
>
> 1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together,

Yes, absolutely.

We do this all over the place.

>  or should each patch be backported individually?
>
> 2) If “batches” are permissible, when?  When would individual patches be preferred?

That's a matter of taste.  If it's several patches of a complicated
bugfix, then it probably wants splitting up in the same way.

If it's a bunch of misc changes, then batching is fine.


> 3) For “batch updates”, what tags are necessary?  Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags?  Do we need to include anything from the original commits — commit messages?  Signed-off-by’s?

"Update $FOO to something resembling $PROJECT, $VERSION" is perfectly good.

>
> And a related question:
>
> 4) When importing an entire file from an upstream like Linux, what tags do we need?

Any clear reference to where it came from.

Nothing is ever imported verbatim.  If nothing else, paths have to be
changed, and usually more than that.

Given that, I do question whether it is appropriate to retain original
authorship.  The original author did not write a patch for Xen, and what
gets committed wasn't the patch they wrote.

Any issues with the port into Xen should be sent to the person who did
the port into Xen, not the original author who most likely has no idea
that their patch has been borrowed by Xen.

IMO, a commit message saying "port $X from project $Y" makes it crystal
clear that the original code change isn't mine, but the porting effort
is.  Amongst other things, porting invalidates any review/ack/test chain
because those tags were given in the context of the original project,
not Xen.

~Andrew

Re: Process for cherry-picking patches from other projects

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]



> On May 16, 2022, at 4:04 PM, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
>> 
>> 4) When importing an entire file from an upstream like Linux, what tags do we need?
> 
> Any clear reference to where it came from.
> 
> Nothing is ever imported verbatim.  If nothing else, paths have to be
> changed, and usually more than that.
> 
> Given that, I do question whether it is appropriate to retain original
> authorship.  The original author did not write a patch for Xen, and what
> gets committed wasn't the patch they wrote.

Not sure what you meant by authorship here — do you mean in the git commit?  In the GPL header of the file?

The original author (or the company they work for) may own a copyright on the code; if the owner of the copyright comes around and accuses us of infringement, we need to be able to a) demonstrate that we are generally trying to respect copyright (by requiring people to assert that the copyright question is all in order for the handled code) b) be able to track back to find where the infringement happened so that we can take appropriate remedial action (either education or sanction as appropriate).

I think if the GPL header of the file contains “Copyright YYYY by ${AUTHOR}”, that copyright notice should be retained when importing the file.

 -George

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Process for cherry-picking patches from other projects

[Julien Grall]

On 16/05/2022 16:04, Andrew Cooper wrote:
> On 13/05/2022 15:33, George Dunlap wrote:
>> Starting a new thread to make it clear that we’re discussing a wider policy here.
>>
>> This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.
>>
>> There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.
>>
>> The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.
>>
>> The questions seem to be:
>>
>> 1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together,
> 
> Yes, absolutely.
> 
> We do this all over the place.
> 
>>   or should each patch be backported individually?
>>
>> 2) If “batches” are permissible, when?  When would individual patches be preferred?
> 
> That's a matter of taste.  If it's several patches of a complicated
> bugfix, then it probably wants splitting up in the same way.
> 
> If it's a bunch of misc changes, then batching is fine.
> 
> 
>> 3) For “batch updates”, what tags are necessary?  Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags?  Do we need to include anything from the original commits — commit messages?  Signed-off-by’s?
> 
> "Update $FOO to something resembling $PROJECT, $VERSION" is perfectly good.
> 
>>
>> And a related question:
>>
>> 4) When importing an entire file from an upstream like Linux, what tags do we need?
> 
> Any clear reference to where it came from.
> 
> Nothing is ever imported verbatim.  If nothing else, paths have to be
> changed, and usually more than that.
> 
> Given that, I do question whether it is appropriate to retain original
> authorship.  The original author did not write a patch for Xen, and what
> gets committed wasn't the patch they wrote.
> 
> Any issues with the port into Xen should be sent to the person who did
> the port into Xen, not the original author who most likely has no idea
> that their patch has been borrowed by Xen.

I agree with that. But I don't view the "Author" line as a way to 
achieve it.

Even for Xen, it is possible that the patch was written by A but then 
fully upstreamed by B (they may be different company). In which case, 
the practice so far has been to use A as the author and add a 2nd 
Signed-off-by for B.

I view porting a patch from Linux the same. If the changes are minor, 
the original author should be credited.

> 
> IMO, a commit message saying "port $X from project $Y" makes it crystal
> clear that the original code change isn't mine, but the porting effort
> is.  Amongst other things, porting invalidates any review/ack/test chain
> because those tags were given in the context of the original project,
> not Xen.

This seems to contradict our documentation:

"All tags **above** the `Origin:` tag are from the original patch (which
should all be kept), while tags **after** `Origin:` are related to the
normal Xen patch process as described here."

Cheers,

-- 
Julien Grall

Re: Process for cherry-picking patches from other projects

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]



> On May 16, 2022, at 4:20 PM, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/05/2022 16:04, Andrew Cooper wrote:
>> On 13/05/2022 15:33, George Dunlap wrote:
> 
>> IMO, a commit message saying "port $X from project $Y" makes it crystal
>> clear that the original code change isn't mine, but the porting effort
>> is.  Amongst other things, porting invalidates any review/ack/test chain
>> because those tags were given in the context of the original project,
>> not Xen.
> 
> This seems to contradict our documentation:
> 
> "All tags **above** the `Origin:` tag are from the original patch (which
> should all be kept), while tags **after** `Origin:` are related to the
> normal Xen patch process as described here."

And indeed, part of the the point of that paragraph is almost certainly to clue you in that the “Tested-by:” is for the original commit, not the Xen commit.

That paragraph is clearly expecting the case where individual commits are cherry-picked, not batched or done with an entire file.  In large part we seem to be in agreement, so we just need someone to do some wordsmithing of some text to update that file for the “batch update” and “import a full file” cases.

 -George

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Process for cherry-picking patches from other projects

[Jan Beulich]

On 13.05.2022 16:33, George Dunlap wrote:
> Starting a new thread to make it clear that we’re discussing a wider policy here.
> 
> This question is aimed at Jan and Andy in particular, as I think they’ve probably done the most of this; so I’m looking to them to find out what our “standard practice” is.
> 
> There have recently been some patches that Bertrand has submitted which pull in code from Linux ("[PATCH 1/3] xen/arm: Sync sysregs and cpuinfo with Linux 5.18-rc3”), which has caused a discussion between him, Julien, and Stefano about the proper way to do such patches.
> 
> The “Origin:” tag section of xen.git/docs/process/sending-patches.pandoc suggests that there are some standards, but doesn’t spell them out.
> 
> The questions seem to be:
> 
> 1) When doing this kind of update, is it permissible to send a single patch which “batches” several upstream commits together, or should each patch be backported individually?
> 
> 2) If “batches” are permissible, when?  When would individual patches be preferred?
> 
> 3) For “batch updates”, what tags are necessary?  Do we need to note the changesets of all the commits, and if so, do we need multiple “Origin” tags?  Do we need to include anything from the original commits — commit messages?  Signed-off-by’s?

Having seen the various other replies, I'd like to support all views
along the lines of "it depends". I don't think we have formal rules
applicable all over our code base. One model might be used in one
place (and then preferably that model would be followed there), while
another model might make more sense in a 2nd case.

> And a related question:
> 
> 4) When importing an entire file from an upstream like Linux, what tags do we need?
> 
> My recollection is that we often to a “accumulated patch” to update, say, the Kconfig tooling; so it seems like the answer to this is sometimes “yes”.
> 
> It seems to me that in a case where you’re importing a handful of patches — say 5-10 — that importing them one-by-one might be preferred; but in this case, since the submission was already made as a batch, I’d accept having it as a batch.
> 
> I think if I were writing this patch, I’d make a separate “Origin” tag for each commit.
> 
> I wouldn’t include the upstream commit messages or S-o-b’s; I would write my own commit message summarizing why I’m importing the commits, then have the ‘origin’ tags, then my own S-o-b to indicate that I am attesting that it comes from an open-source project (and for whatever copyright can be asserted on the commit message and the patch as a collection).
> 
> And for #4, I would do something similar: I would write my own commit message describing what the file is for and why we’re importing it; have the Origin tag point to the commit at the point I took the file; and my own S-o-b.

Hmm. The present rules written down in docs/process/sending-patches.pandoc
are a result of me having been accused of unduly stripping S-o-b (and maybe
a few other) tags. If that was for a real reason (and not just because of
someone's taste), how could it ever be okay to remove S-o-b? (Personally I
agree with what you propose, it just doesn't line up with that discussion
we had not all that long ago.)

Jan

Re: Process for cherry-picking patches from other projects

[Stefano Stabellini]

On Tue, 17 May 2022, Jan Beulich wrote:
> Hmm. The present rules written down in docs/process/sending-patches.pandoc
> are a result of me having been accused of unduly stripping S-o-b (and maybe
> a few other) tags. If that was for a real reason (and not just because of
> someone's taste), how could it ever be okay to remove S-o-b? (Personally I
> agree with what you propose, it just doesn't line up with that discussion
> we had not all that long ago.)

This is the meaning of the DCO: https://developercertificate.org

The relevant case is:

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

IANAL but I read this as to mean that only the submitter Signed-off-by
is required. Also consider that the code could come from a place where
Signed-off-by is not used. As long as the copyright checks out, then we
are OK.

Re: Process for cherry-picking patches from other projects

[Julien Grall]

Hi Stefano,

On 18/05/2022 04:12, Stefano Stabellini wrote:
> On Tue, 17 May 2022, Jan Beulich wrote:
>> Hmm. The present rules written down in docs/process/sending-patches.pandoc
>> are a result of me having been accused of unduly stripping S-o-b (and maybe
>> a few other) tags. If that was for a real reason (and not just because of
>> someone's taste), how could it ever be okay to remove S-o-b? (Personally I
>> agree with what you propose, it just doesn't line up with that discussion
>> we had not all that long ago.)
> 
> This is the meaning of the DCO: https://developercertificate.org
> 
> The relevant case is:
> 
> (b) The contribution is based upon previous work that, to the best
>      of my knowledge, is covered under an appropriate open source
>      license and I have the right under that license to submit that
>      work with modifications, whether created in whole or in part
>      by me, under the same open source license (unless I am
>      permitted to submit under a different license), as indicated
>      in the file; or
> 
> IANAL but I read this as to mean that only the submitter Signed-off-by
> is required. Also consider that the code could come from a place where
> Signed-off-by is not used. As long as the copyright checks out, then we
> are OK.

I don't think I can write better than what Ian wrote back then:

"
Please can we keep the Linux S-o-b.

Note that S-o-b is not a chain of *approval* (whose relevance to us is
debateable) but but a chain of *custody and transmission* for
copyright/licence/gdpr purposes.  That latter chain is hightly
relevant to us.

All such S-o-b should be retained.

Of course if you got the patch somewhere other than the Linux commit,
then the chain of custody doesn't pass through the Linux commit.  But
in that case I expect you to be able to say where you got it.
"

Cheers,

-- 
Julien Grall

Re: Process for cherry-picking patches from other projects

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 4332 bytes --]



> On 18 May 2022, at 08:38, Julien Grall <julien@xen.org> wrote:
> 
> Hi Stefano,
> 
> On 18/05/2022 04:12, Stefano Stabellini wrote:
>> On Tue, 17 May 2022, Jan Beulich wrote:
>>> Hmm. The present rules written down in docs/process/sending-patches.pandoc
>>> are a result of me having been accused of unduly stripping S-o-b (and maybe
>>> a few other) tags. If that was for a real reason (and not just because of
>>> someone's taste), how could it ever be okay to remove S-o-b? (Personally I
>>> agree with what you propose, it just doesn't line up with that discussion
>>> we had not all that long ago.)
>> This is the meaning of the DCO: https://developercertificate.org
>> The relevant case is:
>> (b) The contribution is based upon previous work that, to the best
>>     of my knowledge, is covered under an appropriate open source
>>     license and I have the right under that license to submit that
>>     work with modifications, whether created in whole or in part
>>     by me, under the same open source license (unless I am
>>     permitted to submit under a different license), as indicated
>>     in the file; or
>> IANAL but I read this as to mean that only the submitter Signed-off-by
>> is required. Also consider that the code could come from a place where
>> Signed-off-by is not used. As long as the copyright checks out, then we
>> are OK.
> 
> I don't think I can write better than what Ian wrote back then:
> 
> "
> Please can we keep the Linux S-o-b.
> 
> Note that S-o-b is not a chain of *approval* (whose relevance to us is
> debateable) but but a chain of *custody and transmission* for
> copyright/licence/gdpr purposes.  That latter chain is hightly
> relevant to us.
> 
> All such S-o-b should be retained.
> 
> Of course if you got the patch somewhere other than the Linux commit,
> then the chain of custody doesn't pass through the Linux commit.  But
> in that case I expect you to be able to say where you got it.
> "

So the thread in question is "[PATCH 1/7] xz: add fall-through comments to a switch statement” [1].

This effectively turned into a policy discussion that happened on a random thread about compression algorithms.  It’s likely a lot of people who might have had opinions didn’t read the thread; that’s why I started a new thread, to make sure people knew there was a policy discussion going on.

I was on parental leave when this discussion happened.  Looking at the thread, I agree with the request of Julien to just C&P the whole Linux commit message: It just seems both simpler and more… fitting? Respectful? Something like that; and additionally it saves the reviewer having to think too much about whether the removed S-o-b’s were necessary.  It’s something that we should just do because it’s easy and generally better, particularly as we now have a way of indicating “above this line is *their* way of doing things, which may have useless data in it; below this line is *ours*.”

However, the justification Ian put forward in that thread — that "S-o-b is ...a chain of *custody and transmission* for copyright/licence/gdpr purposes” — must be incorrect.  If it were true, then when we import a file from another project, we would have to check in *the entire git log for that file up to that point*, including all patches.  After all, we need to know *for each line*, the copyright provenance — even having a massive list of all S-o-b’s from the history of the file wouldn’t be of any use if a copyright dispute actually came up.  I think that shows the absurdity of the position.

What we need to be able to do is, for each line of code, to be able to track down where it came from and who originally asserted that it was GPL, in the event of some sort of challenge.  As long as we have the the Linux commit at the point of import, we can track everything else down.  In fact, it will be much easier to track down from a Linux git commit than from anything checked into the commit message.

I’ll double check with LF legal on this, but I’m pretty sure having a “pointer” to where the code came from (either a git commit or a message-id) should be fine.

 -George

[1] https://patchwork.kernel.org/project/xen-devel/patch/0ed245fa-58a7-a5f6-b82e-48f9ed0b6970@suse.com/

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]