Re: Policy for reverting changes
tor 2012-08-16 klockan 23:11 -0600 skrev Alex Rousskov: I am not annoyed at the failures -- we all commit broken code once in a while, and there is currently no good way to prevent even simple build failures. Yes.. I am somewhat annoyed that some failures result in commit reversal (with unusable or contradictory information as to the reason for the reversal) while others are not (even after detailed reasons have been provided), Reversals are not routine. There is no automatic reversal, instead it is up to each and everyone to decide on when reversal is appropriate and when not. Ok, here is a proposal on a little policy on the topic on how to handle others bad commits: trunk: 1. If possible, fix it. 2. If not, nag the person who committed the problematic code. 3a. If no response in 24 hours and it's blocking other things (including test farm builds) then revert the commit with a notice to squid-dev. 3b. If still broken after 7 days of discussion and attempts to fix then revert the change. 4. Let it mature in a development branch, and submit for merge again when the issues have been resolved. numbered branches: 1. If possible, fix it. 2. If not, nag the person who committed the problematic code. 3. Let the release manager decide on reverting It's a real pity bzr do not have a unmerge operation. Currently reversals are quite intrusive and may merge to the branch where the feature was developed erasing it from there as well. but this is _not_ the reason I am asking who is responsible for fixing trunk -- I do really need to know whether this is something I should be working on (to avoid two different people working on the same bug). responsible.. is the one who takes on the bug in question. Regards Henrik
Re: Policy for reverting changes
On Fri, Aug 17, 2012 at 6:49 PM, Henrik Nordström hen...@henriknordstrom.net wrote: tor 2012-08-16 klockan 23:11 -0600 skrev Alex Rousskov: I am not annoyed at the failures -- we all commit broken code once in a while, and there is currently no good way to prevent even simple build failures. Yes.. I am somewhat annoyed that some failures result in commit reversal (with unusable or contradictory information as to the reason for the reversal) while others are not (even after detailed reasons have been provided), Reversals are not routine. There is no automatic reversal, instead it is up to each and everyone to decide on when reversal is appropriate and when not. Ok, here is a proposal on a little policy on the topic on how to handle others bad commits: trunk: 1. If possible, fix it. 2. If not, nag the person who committed the problematic code. 3a. If no response in 24 hours and it's blocking other things (including test farm builds) then revert the commit with a notice to squid-dev. 3b. If still broken after 7 days of discussion and attempts to fix then revert the change. 4. Let it mature in a development branch, and submit for merge again when the issues have been resolved. Sounds very reasonable to me. In this case, I took path 1, complicated by the fact that the breakage wasn't visible to me. (the patchlet I sent earlier on is confirmed as good to fix the last remaining issues, including making the breakage visible in layer-02-maximus.) It's a real pity bzr do not have a unmerge operation. Currently reversals are quite intrusive and may merge to the branch where the feature was developed erasing it from there as well. Have you considered bzr uncommit? Uncommitting the last revision(s) it's very easy: bzr uncommit -r revno-to-return-to, bzr revert But a process such as the follwoing should do the trick to uncommit a merge which has occurred at revision N: bzr uncommit -r N+1 bzr shelve --all bzr uncommit bzr revert bzr unshelve --apply bzr commit Note: this doesn't take into account commit messages, conflicts etc. Robert can certainly comment on the feasibility of the approach, or any better approaches. -- /kinkie
Re: Policy for reverting changes
On 08/17/2012 10:49 AM, Henrik Nordström wrote: Reversals are not routine. There is no automatic reversal, instead it is up to each and everyone to decide on when reversal is appropriate and when not. Ok, here is a proposal on a little policy on the topic on how to handle others bad commits: trunk: 1. If possible, fix it. 2. If not, nag the person who committed the problematic code. 3a. If no response in 24 hours and it's blocking other things (including test farm builds) then revert the commit with a notice to squid-dev. 3b. If still broken after 7 days of discussion and attempts to fix then revert the change. 4. Let it mature in a development branch, and submit for merge again when the issues have been resolved. numbered branches: 1. If possible, fix it. 2. If not, nag the person who committed the problematic code. 3. Let the release manager decide on reverting The above policy places places a 24 hour wait time that may not be appropriate in emergencies. On the other hand, it may be interpreted as a permission to revert any change that a person considers broken after 7 days of squid-dev discussions, which may result in painful and unnecessary reversals. Let's step back a little and look at what we are trying to achieve here. A good reversal: (a) Balances the reversal effects with the effects of keeping the broken code. For example, reversing a high-demand but optional feature that breaks a default farm build on an obscure platform may not be such a good idea. On the other hand, reversing a commit that breaks build on on all platforms should not be frowned upon, especially if error reports are not followed by immediate fixes (indicating that the responsible party is unavailable to fix things promptly). (b) Details the reason for reversal so that the responsible party has a reasonable path forward to fix the problem or object to the reversal. For example, does not seem to work for me is a bad reversal commit message, but breaks 'make check' on Fedora, with the following error is a good one. Since balance and broken are relative terms requiring judgment calls, most reversal decisions should be driven by squid-dev consensus except in emergencies. Also, a quick reversal of the last commit is usually a far less painless action for most parties than reversing a 7-day old code with other changes piled on top of it (in trunk and in developers' branches). The policy should reflect that as well, perhaps by focusing on fresh commits and emergency reversals while allowing the usual squid-dev consensus process to handle other/older issues. With trunk focus, I would suggest the following: 1. The party responsible for the committed code should monitor farm build errors, squid-dev, and bugzilla for signs of trouble after the commit. If you are responsible and notice error reports, either promptly fix the bugs or inform squid-dev that you will not (with some justification, of course). [ Thus, it is usually a bad idea to commit changes when the responsible party cannot monitor for signs of trouble and promptly address problems. ] If you are responsible, expect people to pummel you and demand corrective actions -- *you* are at fault if your code breaks things (but you may disagree that something is broken and/or refuse to make prompt corrections for various reasons, of course). Problem reports do not imply folks do not appreciate your contributions. Others should report problems to squid-dev or bugzilla. For fresh trunk problems, squid-dev is the preferred route. Fixes may be helpful as well, but keep in mind that the responsible party may be working on the same kind of fix already so some coordination may be desirable. The committer is not necessarily the responsible party. 2. If you think it is an emergency (which is rather unlikely for trunk!), you may reverse any recent commit. If you do, make sure you document why the commit was reversed: detail the bug/problem in the commit message and provide emergency justification (and notification) on squid-dev. You become responsible for the reversal. You will need a committer assistance if you lack commit privileges, of course. 3. If you do not think this is an emergency, discuss the problem on squid-dev and act based on the consensus there. In your first post, CC the responsible party. Thank you, Alex.
Re: Policy for reverting changes
fre 2012-08-17 klockan 16:03 -0600 skrev Alex Rousskov: The above policy places places a 24 hour wait time that may not be appropriate in emergencies. On the other hand, it may be interpreted as a permission to revert any change that a person considers broken after 7 days of squid-dev discussions, which may result in painful and unnecessary reversals. If 7 days of discussions and attempted fixing can not fix the code to an acceptable level then it's not yet ready for trunk and should go back to live in a developer feature branch and resubmitted for merge when in better shape. (a) Balances the reversal effects with the effects of keeping the broken code. For example, reversing a high-demand but optional feature that breaks a default farm build on an obscure platform may not be such a good idea. On the other hand, reversing a commit that breaks build on on all platforms should not be frowned upon, especially if error reports are not followed by immediate fixes (indicating that the responsible party is unavailable to fix things promptly). It's very hard to say exactly where the limit is. For as long as that code breaks that obscure platform the build tests for other code on that platform is offline. Repeat this a number of times and we soon have no platforms that builds. (b) Details the reason for reversal so that the responsible party has a reasonable path forward to fix the problem or object to the reversal. For example, does not seem to work for me is a bad reversal commit message, but breaks 'make check' on Fedora, with the following error is a good one. Agreed. Since balance and broken are relative terms requiring judgment calls, most reversal decisions should be driven by squid-dev consensus except in emergencies. I do not see reversals of trunk changes as such as that intrusive to the development process. Not even when it's a bit older commit. But we need to get the hang of how to handle this proper in bzr so that the reversal propagates only to the development branches where it should propagate. Regards Henrik
