Re: AW: [openssl] OpenSSL_1_1_1-stable update
I have always implicitly assumed Matt view, but I am happy to conform to what the consensus is. I believe this discussion is very useful and could contribute a new entry in the commiter guidelines. Nicola On Fri, May 24, 2019, 07:21 Matt Caswell wrote: > > > On 24/05/2019 15:10, Richard Levitte wrote: > > Not sure I see it as picking nits, it's rather about some fundamental > > difference in what we thinking we're approving, and how we actually > > act around that. > > > > My idea has always been that I approve a code change, i.e. essentially > > a patch or a set of patches, without regard for exact branches it ends > > up in. With the in mind, the exact branches it gets applied to is a > > *separate* question. > > That's not the way I've ever thought of it. In my mind an approval is for a > change applied to a specific branch. Where a PR lists more than one branch > in it > and you approve the PR then effectively you are approving it multiple > times all > in one go - once for each branch. > > > > If we go with the idea that an approval also involves approving what > > branches it goes to, then what happens if someone realises after some > > time that a set of commits (a PR) that was applied to master only > > should really also be applied to 1.1.1? Should the approval process > > start over from scratch, i.e. all approvals that went to master should > > be scratched and replaced with a new set of approvals (in principle)? > > No. If the PR was approved for master and applied to master then no > problem - it > stays in master. If it is later realised that it needs to be backported to > other > branches then, yes, new approvals need to be sought for that change to > *those > branches*. > > As far as I was aware we've always done this. > > This is essential in my mind. A change for one branch does not always make > sense > in another branch. So you can't just say "I approve this change" and *then* > worry about what branches it applies to. A change only makes sense in the > context of the branch it applies to. > > Matt >
Re: AW: [openssl] OpenSSL_1_1_1-stable update
I forgot one word: On 24.05.19 17:45, Matthias St. Pierre wrote: (Otherwise, the missing approvers need to be from the Reviewed-by list and additional approvals would be needed). need to be _removed_ from the Reviewed-by list
Re: AW: [openssl] OpenSSL_1_1_1-stable update
On 24.05.19 16:54, Richard Levitte wrote: On Fri, 24 May 2019 16:39:51 +0200, Matt Caswell wrote: On 24/05/2019 15:30, Richard Levitte wrote: Not in practice. We *do* ask on the PR in question if it should be cherry-picked to 1.1.1 and seek approval for that action, but then it hasn't at all been clear what should happen regarding Received-By tags. I have personally never touched them when cherry-picking, even in this scenario. I do not know what others do in that case...> In principle I agree with Matt, his arguments are reasonable. Maybe a compromise would be to allow the practice of asking for an informal reconfirmation when cherry-pick tags were forgotten, but only if one receives reconfirmation from _all_ original reviewers? (Otherwise, the missing approvers need to be from the Reviewed-by list and additional approvals would be needed). Matthias
Re: AW: [openssl] OpenSSL_1_1_1-stable update
On Fri, 24 May 2019 16:39:51 +0200, Matt Caswell wrote: > > > > On 24/05/2019 15:30, Richard Levitte wrote: > > > > Not in practice. We *do* ask on the PR in question if it should be > > cherry-picked to 1.1.1 and seek approval for that action, but then it > > hasn't at all been clear what should happen regarding Received-By > > tags. > > > > I have personally never touched them when cherry-picking, even in this > > scenario. I do not know what others do in that case...> > > In the vast majority of the cases the reviewers are the same. Yes, because cherry-picking as an after-though rarely happens. It's mostly been along the lines of "hey, why was this bug-fix only applied to master???" > I wouldn't want other people putting my name in a reviewed-by tag > where I have not approved it and I have not considered the > implications of that change in that branch. What if it resulted in a > critical CVE? I haven't given possible CVEs much though, quite frankly. But tell you what, I can certainly change my ways if this is what we all think is the way to go. Not a problem. And I'm glad that we talked about it, rather than staying with "I thought everyone else did the same". Cheers, Richard -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/
Re: AW: [openssl] OpenSSL_1_1_1-stable update
On 24/05/2019 15:30, Richard Levitte wrote: > On Fri, 24 May 2019 16:20:59 +0200, > Matt Caswell wrote: >> On 24/05/2019 15:10, Richard Levitte wrote: >>> If we go with the idea that an approval also involves approving what >>> branches it goes to, then what happens if someone realises after some >>> time that a set of commits (a PR) that was applied to master only >>> should really also be applied to 1.1.1? Should the approval process >>> start over from scratch, i.e. all approvals that went to master should >>> be scratched and replaced with a new set of approvals (in principle)? >> >> No. If the PR was approved for master and applied to master then no problem >> - it >> stays in master. If it is later realised that it needs to be backported to >> other >> branches then, yes, new approvals need to be sought for that change to *those >> branches*. >> >> As far as I was aware we've always done this. > > Not in practice. We *do* ask on the PR in question if it should be > cherry-picked to 1.1.1 and seek approval for that action, but then it > hasn't at all been clear what should happen regarding Received-By > tags. > > I have personally never touched them when cherry-picking, even in this > scenario. I do not know what others do in that case...> In the vast majority of the cases the reviewers are the same. In the rare circumstances where they are different I have always changed them. I thought everyone did. IMO that is the correct action. By putting your name as a reviewer against a PR you are effectively saying "I have reviewed this and agree that it is appropriate for this to be merged". I wouldn't want other people putting my name in a reviewed-by tag where I have not approved it and I have not considered the implications of that change in that branch. What if it resulted in a critical CVE? Matt
Re: AW: [openssl] OpenSSL_1_1_1-stable update
On Fri, 24 May 2019 16:20:59 +0200, Matt Caswell wrote: > On 24/05/2019 15:10, Richard Levitte wrote: > > If we go with the idea that an approval also involves approving what > > branches it goes to, then what happens if someone realises after some > > time that a set of commits (a PR) that was applied to master only > > should really also be applied to 1.1.1? Should the approval process > > start over from scratch, i.e. all approvals that went to master should > > be scratched and replaced with a new set of approvals (in principle)? > > No. If the PR was approved for master and applied to master then no problem - > it > stays in master. If it is later realised that it needs to be backported to > other > branches then, yes, new approvals need to be sought for that change to *those > branches*. > > As far as I was aware we've always done this. Not in practice. We *do* ask on the PR in question if it should be cherry-picked to 1.1.1 and seek approval for that action, but then it hasn't at all been clear what should happen regarding Received-By tags. I have personally never touched them when cherry-picking, even in this scenario. I do not know what others do in that case... -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/
Re: AW: [openssl] OpenSSL_1_1_1-stable update
Hello, On Fri, May 24, 2019 at 5:21 PM Matt Caswell wrote: > > On 24/05/2019 15:10, Richard Levitte wrote: > > If we go with the idea that an approval also involves approving what > > branches it goes to, then what happens if someone realises after some > > time that a set of commits (a PR) that was applied to master only > > should really also be applied to 1.1.1? Should the approval process > > start over from scratch, i.e. all approvals that went to master should > > be scratched and replaced with a new set of approvals (in principle)? > > No. If the PR was approved for master and applied to master then no > problem - it > stays in master. If it is later realised that it needs to be backported to > other > branches then, yes, new approvals need to be sought for that change to > *those > branches*. > > As far as I was aware we've always done this. > > This is essential in my mind. A change for one branch does not always make > sense > in another branch. So you can't just say "I approve this change" and *then* > worry about what branches it applies to. A change only makes sense in the > context of the branch it applies to. > > I agree with Matt. For example, a patch providing new functionality cat be cleanly applicable to master and stable branches, but if it is applied to a stable branch, it breaks the policy. -- SY, Dmitry Belyavsky
Re: AW: [openssl] OpenSSL_1_1_1-stable update
On 24/05/2019 15:10, Richard Levitte wrote: > Not sure I see it as picking nits, it's rather about some fundamental > difference in what we thinking we're approving, and how we actually > act around that. > > My idea has always been that I approve a code change, i.e. essentially > a patch or a set of patches, without regard for exact branches it ends > up in. With the in mind, the exact branches it gets applied to is a > *separate* question. That's not the way I've ever thought of it. In my mind an approval is for a change applied to a specific branch. Where a PR lists more than one branch in it and you approve the PR then effectively you are approving it multiple times all in one go - once for each branch. > If we go with the idea that an approval also involves approving what > branches it goes to, then what happens if someone realises after some > time that a set of commits (a PR) that was applied to master only > should really also be applied to 1.1.1? Should the approval process > start over from scratch, i.e. all approvals that went to master should > be scratched and replaced with a new set of approvals (in principle)? No. If the PR was approved for master and applied to master then no problem - it stays in master. If it is later realised that it needs to be backported to other branches then, yes, new approvals need to be sought for that change to *those branches*. As far as I was aware we've always done this. This is essential in my mind. A change for one branch does not always make sense in another branch. So you can't just say "I approve this change" and *then* worry about what branches it applies to. A change only makes sense in the context of the branch it applies to. Matt
Re: AW: [openssl] OpenSSL_1_1_1-stable update
Not sure I see it as picking nits, it's rather about some fundamental difference in what we thinking we're approving, and how we actually act around that. My idea has always been that I approve a code change, i.e. essentially a patch or a set of patches, without regard for exact branches it ends up in. With the in mind, the exact branches it gets applied to is a *separate* question. If we go with the idea that an approval also involves approving what branches it goes to, then what happens if someone realises after some time that a set of commits (a PR) that was applied to master only should really also be applied to 1.1.1? Should the approval process start over from scratch, i.e. all approvals that went to master should be scratched and replaced with a new set of approvals (in principle)? So far, we have never acted that way. On Fri, 24 May 2019 15:24:48 +0200, Dr. Matthias St. Pierre wrote: > > Matt and Richard, I think you are mixing up cherry-picking and nit-picking > here. (Sorry for the pun ;-) > > Matthias > > > To be picky, may I assume that you meant a reviewed-by tag for you > > > should be *added*? The commit itself (its contents) having been > > > reviewed by those already there, I cannot see a reason why they should > > > be removed. > > > > You approved for master but did not approve for 1.1.1. This commit was > > merged to > > 1.1.1 and so strictly speaking was not approved by you for that branch. > > Therefore you should have been removed, and I should have been added. > > > > Matt > -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/
