<div dir="ltr"><div><div><div>Definitely "broken" commits shouldn't be merged. They should either be sent back to the author for revisions (the model I'm accustomed to) or fixed by the reviewer. <br><br></div>I would prefer related but separable changes to be in independent (non-broken) commits. Interdependent changes should be in one commit.<br><br></div>I'm curious what the inherent benefit is of having multiple people collaborate on a patch, as opposed to having a series of patches by different people, each of which advances the product incrementally. At least, it sounded like you (Rob) were advocating collaboration for its own sake. <br><br></div>I wonder if my heavy Test-Driven Development and heavy Refactoring preferences are what is putting me on the opposite side of this debate. I have no problem with a commit followed immediately by a rename commit followed immediately by a commit that rearranges the code sequence for clarity, followed immediately by a refactor extract method commit. Whether those are all done by one person, or by 4, doesn't matter to me (as long as everyone is communicating clearly to avoid stepping on each other). <br><br></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><span><font color="#888888"><br>Kevin Smith<br>Agile Coach, Wikimedia Foundation<br></font></span><font><font><i><font color="#888888"><br></font></i></font></font></div></div></div></div></div></div></div></div></div>
<br><div class="gmail_quote">On Wed, Mar 16, 2016 at 8:39 AM, Antoine Musso <span dir="ltr"><<a href="mailto:hashar+wmf@free.fr" target="_blank">hashar+wmf@free.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Le 16/03/2016 00:48, Kevin Smith a écrit :<br>
> There are lots of different cases here, as well as existing norms, and<br>
> also preferences.<br>
><br>
> I would mention that in some cases, I would prefer to accept the commit<br>
> as is, and then perform minor refactoring, such as changing a name,<br>
> fixing a typo, or rearranging the code. Not only does that clearly<br>
> separate authorship, but it would also encourage those changes to be<br>
> reviewed by someone other than that author.<br>
><br>
> Disclaimer: As someone who has carefully reviewed many hundreds<br>
> (thousands?) of commits, I am a big fan of smaller commits, or even<br>
> micro commits. I would much rather review 10 trivial changes than one<br>
> monolithic commit that does 10 different things.<br>
<br>
</span>Hello,<br>
<br>
One of the main reason for adopting Gerrit for MediaWiki development was<br>
the pre merge review that enforces related modifications to land as a<br>
single commit.  So one can see a Gerrit change as a short lived feature<br>
branch but the end result is a single commit.<br>
<br>
When we were using subversion and post commit review, it was a nightmare<br>
whenever we had to revert a modification.<br>
<br>
<br>
To illustrate:<br>
<br>
r113688 change some method<br>
<a href="https://www.mediawiki.org/wiki/Special:Code/MediaWiki/113688" rel="noreferrer" target="_blank">https://www.mediawiki.org/wiki/Special:Code/MediaWiki/113688</a><br>
<br>
113691 update related tests<br>
(we were not automatically running tests at that time)<br>
<a href="https://www.mediawiki.org/wiki/Special:Code/MediaWiki/113691" rel="noreferrer" target="_blank">https://www.mediawiki.org/wiki/Special:Code/MediaWiki/113691</a><br>
<br>
The second patch is a catch up.  In a pre merge review that would have<br>
been caught and the first patch amended.  Nowadays that specific use<br>
case would be automatically prevented by CI preventing Gerrit from<br>
submitting the change (Verified=-1).<br>
<br>
<br>
When we did the 1.18 or 1.19 branch I clearly remember we had an<br>
Etherpad listing the tree dependencies for revert purposes.  That was no<br>
fun.  Lot of those trees of patches were really reflecting a feature<br>
being developed straight in the branch :-\<br>
<br>
<br>
I also prefer unrelated modifications to be split in tiny commits. That<br>
makes review faster and get them merged faster.  When the modifications<br>
are related, they are better done in a single commit though.<br>
<br>
<br>
( just giving some context, not shooting the messenger :D  )<br>
<span class="HOEnZb"><font color="#888888"><br>
<br>
--<br>
Antoine "hashar" Musso<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
_______________________________________________<br>
teampractices mailing list<br>
<a href="mailto:teampractices@lists.wikimedia.org">teampractices@lists.wikimedia.org</a><br>
<a href="https://lists.wikimedia.org/mailman/listinfo/teampractices" rel="noreferrer" target="_blank">https://lists.wikimedia.org/mailman/listinfo/teampractices</a><br>
</div></div></blockquote></div><br></div>