Cherry-pick proposal

[Richard Levitte]

I find the idea of *mandatory* separate PRs for master and 1.1.1
awful.  There's still a lot of code that hasn't changed at all.  CMS,
BIO, ...

We already have the policy that if we get a clean cherry-pick, we go
with it, otherwise we make a separate PR.  I've followed that guiding
rule for a long time.

As for always starting with master, I would say that it depends.  If
an issue has been identified in 1.1.1, I usually submit a PR against
1.1.1, because that's usually cleaner, i.e. the person making the
issue can easily pick the fixing PR and try it out without having to
wade through unrelated 3.0 things that may or may not work for them.
Or to put it in harsher words, submitting a fix against master for an
issue found in 1.1.1 is *user* *unfriendly*.
With such a PR, I then cherry-pick to master if that's clean, or
submit an up-port PR.

Cheers,
Richard

On Wed, 29 Apr 2020 15:04:57 +0200,
Nicola Tuveri wrote:
> I can agree it is a good idea to always require backport as a separate PR, with the following
> conditions:
> - unless it's a 1.1.1 only issue, we MUST always wait to open the backport-to-111 PR until after
> the master PR has been approved and merged (to avoid splitting the discussions among different
> PRs, which make review and revisiting our history very hard)
> - trivial documentation changes, such as fixing typos, can be exempted
> 
> It must be clear that although things have changed a lot in the inner plumbings, we have so far
> managed to keep crypto implementations very much in sync between 1.1.1 and master, by applying the
> policy of "first merge to master, then possibly backport".
> 
> What I am afraid of in Bernd's proposal, and recent discussions, is that committers might be
> tempted to open PRs changing implementations against 1.1.1 first (to avoid frequent rebases due to
> unrelated changes) and let the `master` PR stagnate indefinitely because it feels like too much
> hassle to keep up with the development pace of master if your PR collaterally changes certain
> files.
> 
> An example of what can go wrong if we open a 1.1.1 PR concurrently with a PR for master can be
> seen here:
> - `master` PR: https://github.com/openssl/openssl/pull/10828
> - `1.1.1` PR: https://github.com/openssl/openssl/pull/11411
> 
> I highlight the following problems related to the above example:
> - as you can see the `1.1.1` has been merged, even though the `master` one has stalled while
> discussing which implementation we should pick. (this was my fault, I should have applied the
> `hold` label after stating that my approval for 1.1.1 was conditional to approving the `master`
> counterpart)
> - discussion that is integral part of the decision process was split among the 2 PRs, with over 40
> comments each
> - there is no clear link between the `master` PR and the `1.1.1` PR for the same feature (this
> makes it very difficult to review what is the state of the "main" PR, and if we or someone else in
> some months or years needs to go check why we did things the way we did, will have a hard time
> connecting the dots)
> 
> I also think that the proposal as written is a bit misleading: I would very like to keep seeing in
> 1.1.1 a majority of commits cherry-picked from commits merged to master, with explicit tags in the
> 1.1.1 commit messages (this helps keeping the git history self-contained without a too strong
> dependency on GitHub).
> I would rephrase the proposal as follows:
> 
>     Merge to 1.1.1 should only happen after approval of a dedicated PR targeting the
> OpenSSL_1_1_1-stable branch.
> 
> Potential amendments that I'd like the OTC to consider are:
> a) before the end of the sentence add ", with the optional exclusion of trivial documentation-only
> changes"
> b) after the end of the sentence add "In composing backport pull requests, explicit cherry-picking
> (`git cherry-pick -x`) of relevant commits merged to `master` or another stable branch is
> recommended and encouraged whenever possible."
> c) adopt a more general statement:
> 
>     Merge to any stable branch should only happen after approval of a dedicated PR targeting
> specifically that branch.
> 
> So, in summary, I would agree with the proposal, as I definitely think Bernd has a good point
> about running the 1.1.1 CI for things we think should be backported, but requires careful
> consideration of additional requirements to avoid duplicating review efforts, splitting
> discussions that should be kept together, or disrupting our processes making 1.1.1 and master
> diverge more than necessary.
> 
> Cheers,
> 
> Nicola
> 
> On Wed, 29 Apr 2020 at 14:08, Matt Caswell <matt at openssl.org> wrote:
> 
>     The OTC have received this proposal and a request that we vote on it:
>    
>     I would like to request that we do not allow cherry-picks between master
>     and 1.1.1-stable because these two versions are now very different, if a
>     cherry-pick succeeds, there is no guarantee that the result will work.
>     Because we have no CI for the cherry-pick. If a cherry-pick is needed, a
>     new PR for 1.1.1 should be done and approved independently.
>    
>     Before starting a vote I'd like to provide opportunity for comments, and
>     also what the vote text should be.
>    
>     Thanks
>    
>     Matt
> 
> 
-- 
Richard Levitte         levitte at openssl.org
OpenSSL Project         http://www.openssl.org/~levitte/