[teampractices] Code review social norms

[Kevin Smith]

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.

I would prefer related but separable changes to be in independent
(non-broken) commits. Interdependent changes should be in one commit.

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.

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).



Kevin Smith
Agile Coach, Wikimedia Foundation


On Wed, Mar 16, 2016 at 8:39 AM, Antoine Musso <hashar+wmf at free.fr> wrote:

> 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
>
>
> _______________________________________________
> teampractices mailing list
> teampractices at lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/teampractices
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/teampractices/attachments/20160316/955e2d59/attachment.html>