Yes, I agree. We need to have a guideline for PR cherry-picking. I'm not sure if we have other tools for cherry-picking. We can provide the Git cherry-pick procedure in the document And point out what we should not do, such as push a test fix or code-style fix without CR.
I will try to draft a document. Thanks, Penghui On Wed, Nov 30, 2022 at 2:14 AM Michael Marshall <mmarsh...@apache.org> wrote: > On a practical note, do we understand what went wrong and how to > prevent it in the future? Do we need to improve cherry-picking > documentation or tooling for committers? > > - Michael > > On Tue, Nov 29, 2022 at 3:41 AM PengHui Li <peng...@apache.org> wrote: > > > > It's not only two commits > > There are more than 10 commits with meaningless commit messages. > > Please check the commit logs > > https://github.com/apache/pulsar/tree/branch-2.9-backup > > > > > Prettifying the git commit history isn't a proper reason to do force > > pushes > > > > I don't think it should be `Prettifying` here. You are not able to grep > the > > commit logs > > with the keyword. It's an error commit message, not an unpretty message. > > It will be inconvenient for users to check whether the fix is backported > to > > branch-2.9. > > If we revert all of them, we will see 20~30 unreasonable commits. > > > > And the commit only happened these days, and we don't have a release for > it. > > I think it makes sense to correct it before we release it. Otherwise, the > > error commits > > message will persist for a long time. > > > > Additionally, we have a backup for branch-2.9. > > > > This does not mean that I support force push to branch at any time. > > For some occasional cases, I think we can talk case by case. > > > > Thanks, > > Penghui > > > > On Tue, Nov 29, 2022 at 3:21 PM Lari Hotari <lhot...@apache.org> wrote: > > > > > +1 to what Michael said. > > > > > > If an invalid commit gets pushed into one of the public branches, it > will > > > simply need to be reverted. Prettifying the git commit history isn't a > > > proper reason to do force pushes. There could be special exceptions, > such > > > as when someone accidentally pushes confidential information in the git > > > repository. That wasn't the case this time. I hope that the next time > that > > > there's a need for force pushes that priv...@pulsar.apache.org is > > > consulted first. > > > > > > -Lari > > > > > > On 2022/11/28 22:51:18 Michael Marshall wrote: > > > > I do not think we should have done a force push to rewrite two commit > > > > messages. The problematic messages had commit hashes in their names, > > > > so it was easy enough to figure out what went wrong and how to find > > > > the original commit. If it was really important to fix the messages, > I > > > > think we should have reverted the problematic commits and re-done the > > > > cherry pick with the desired commit messages. > > > > > > > > I also do not think we should allow force pushing to release branches > > > > or to master branch in the future. In my opinion, the git history > > > > should be immutable. > > > > > > > > Thanks, > > > > Michael > > > > > > > > > > > > On Fri, Nov 25, 2022 at 11:39 PM 丛搏 <bog...@apache.org> wrote: > > > > > > > > > > +3 binding(Penghui Li, JiWei guo, Hang Chen) +2 nonbing(Zike Yang, > > > > > Yubiao Feng), so I will close this discussion and merge > > > > > https://github.com/apache/pulsar/pull/18623. thanks for your > votes. > > > > > > > > > > Thanks, > > > > > bo > > > > > > > > > > 丛搏 <congbobo...@gmail.com> 于2022年11月25日周五 18:55写道： > > > > > > > > > > > > > > > > > > > > One thing I'd like to make clear: this is a temporary solution, > > > and we'll re-enable branch protection after the actions are performed > (or > > > relax to 2.9.4 released). > > > > > > yes, after this pr: https://github.com/apache/pulsar/pull/18623 > > > merged > > > > > > and reset branch-2.9 to commit 5ed247de3a, I will push a new PR > to > > > the > > > > > > master branch to open the branch-2.9 force push > > > > > > > > > > > > > > If it's the case, please file an issue and assign yourself to > > > revert on time :) > > > > > > if no one is against it in 24 hours, this discussion will be > approved > > > > > > then merge https://github.com/apache/pulsar/pull/18623 then > reset > > > > > > branch-2.9 to commit 5ed247de3a > > > > > > > >
