[teampractices] Code review social norms

[Antoine Musso]

Le 16/03/2016 00:48, Kevin Smith a écrit :
> There are lots of different cases here, as well as existing norms, and
> also preferences.
> 
> I would mention that in some cases, I would prefer to accept the commit
> as is, and then perform minor refactoring, such as changing a name,
> fixing a typo, or rearranging the code. Not only does that clearly
> separate authorship, but it would also encourage those changes to be
> reviewed by someone other than that author.
> 
> Disclaimer: As someone who has carefully reviewed many hundreds
> (thousands?) of commits, I am a big fan of smaller commits, or even
> micro commits. I would much rather review 10 trivial changes than one
> monolithic commit that does 10 different things.

Hello,

One of the main reason for adopting Gerrit for MediaWiki development was
the pre merge review that enforces related modifications to land as a
single commit.  So one can see a Gerrit change as a short lived feature
branch but the end result is a single commit.

When we were using subversion and post commit review, it was a nightmare
whenever we had to revert a modification.


To illustrate:

r113688 change some method
https://www.mediawiki.org/wiki/Special:Code/MediaWiki/113688

113691 update related tests
(we were not automatically running tests at that time)
https://www.mediawiki.org/wiki/Special:Code/MediaWiki/113691

The second patch is a catch up.  In a pre merge review that would have
been caught and the first patch amended.  Nowadays that specific use
case would be automatically prevented by CI preventing Gerrit from
submitting the change (Verified=-1).


When we did the 1.18 or 1.19 branch I clearly remember we had an
Etherpad listing the tree dependencies for revert purposes.  That was no
fun.  Lot of those trees of patches were really reflecting a feature
being developed straight in the branch :-\


I also prefer unrelated modifications to be split in tiny commits. That
makes review faster and get them merged faster.  When the modifications
are related, they are better done in a single commit though.


( just giving some context, not shooting the messenger :D  )


-- 
Antoine "hashar" Musso