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.
Received on Fri Aug 17 2012 - 22:04:33 MDT