Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Nicola Tuveri
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

2019-05-24 Thread Matthias St. Pierre

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

2019-05-24 Thread Matthias St. Pierre




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

2019-05-24 Thread Richard Levitte
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

2019-05-24 Thread Matt Caswell



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

2019-05-24 Thread Richard Levitte
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/


AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Dr. Matthias St. Pierre
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