[mb-devel] Reverting another developer's changes -- is it ok?

[Alexander Dupuy]

musicbrainz-devel-request at lists.musicbrainz.org wrote:

>In this case nikki reverted some changes made by Stefan because this  
>is the only way she could see how to fix the issue. Ok, I am not  
>interested in settling or getting involved in this particular  
>instance. Since we did not have a commit/dev policy in place, I don't  
>want to revisit this issue. I mainly want to see what people think of  
>this.
>
>In the future, is this acceptable behaviour for MB developers?
>  
>

While it's certainly not ideal, I would hesitate to put in a blanket 
statement saying you should never do this.  In this case, Nikki reverted 
only part of a larger commit, in an attempt to fix behavior that was 
arguably broken in an objective way (incorrect information displayed).  
Without addressing the question of whether this was in fact the case (I 
have no idea), this doesn't seem, a priori, to be unacceptable behavior.

On the other hand, in a hypothetical example, a developer who reverts 
another developer's commit entirely, because s/he "doesn't like the 
pixel spacing" would probably fall into the category of unacceptable 
behavior.  But I don't see any place to draw a clear line between the 
two cases.  There is one item in the proposed commit policy that might 
have some bearing:

5. Any disputed change must be backed out pending resolution of the dispute if requested by a maintainer.


Since a revert is also a change, and Keschte would probably have fallen 
into the category of "maintainer" at that time, if this policy had been 
in place, and he requested reinstatement of his version of the code, 
Nikki would have been obliged to do so (under these new rules).  But 
some more clarification of this rule 5 would be helpful - who can 
"dispute" a change?  Only maintainers?  Or also developers?  What about 
users?  And I think that in such cases, while it may be reasonable to 
require a disputed change to be backed out of a release branch, or the 
trunk, it should be allowed on another branch.

There are some problems with the commit that don't seem to have been 
resolved in the comments, notably about the correct branch for the 
commit.  It wasn't clear to Nikki that she should have committed to the 
release branch, let alone how to commit anything to any place other than 
the trunk.  This may be, more than anything else, a documentation 
failure.  Perhaps it would make sense to add another item (right after 
Don't commit code you don't understand) about "Understand how to commit 
to branches before you commit to the trunk"?


>    3. Discuss any significant change before committing.
>  
>

I would go a bit further on this, and say that before you commit to a 
release branch or the trunk, you ought to create a Trac ticket for 
discussion of the problem (or enhancement) and the proposed change.  If 
you want to make a whole mess of commits for a major enhancement, over a 
long period (days, weeks, or even months) you can do so on a branch 
without any need for discussion etcetera (or even a ticket) - it would 
only be necessary when merging onto the trunk (or a release branch).

>    4. Respect existing maintainers (ruaok: I think this should  
>include not reverting other people's changes)
>  
>

I think my comments above address this, but while certainly not 
reverting changes would sometimes be covered by this, in other cases, it 
might not.

>    5. Any disputed change must be backed out pending resolution of  
>the dispute if requested by a maintainer. Security related changes  
>may override a maintainer's wishes at the Security Officer's discretion.
>    8. Respect all code freezes and read the committers and  
>developers mailing lists in a timely manner so you know when a code  
>freeze is in effect.
>    9. When in doubt on any procedure, ask first!
>   10. Test your changes before committing them.
>
> From the KDE list I like (augmenting the above):
>
>   # Think twice before committing.
>   # Double check what you commit.
>   # Always add descriptive log messages.
>   # The code you commit must adhere to the KDE coding policies.
>   # Respect special commit policies set by the release plans.
>   # Take responsibilty for your commits.
>   # Don't commit code you don't understand.
>   # Don't abuse your SVN account to push in changes with which other  
>developers disagree.
>   # Don't add generated files to the repository.
>   # Don't use SVN keywords like Id or Log in the source files.
>
>  
>

It might help to divide these points into principles and practices.  
Respect other maintainers is a principle.  Don't use SVN keywords is a 
practice; other projects may (quite reasonably, for other reasons) 
require them.  I'm curious why KDE (and, apparently MB) prefer to avoid 
them.

>I'd probably start with these points and build our own commit policy  
>from this.
>  
>

I'd say a Wiki page would probably be in order about now.

@alex

-- 
mailto:alex.dupuy at mac.com