Re: [teampractices] Patch review culture of Wikimedia teams

2016-03-19 Thread Kevin Smith
Perhaps we should split this into "code review tied to a phab task" vs. "code review that does not have an associated phab task". TPG probably has no visibility into the latter (if those even exist). The former would be handled normally by my (and probably other TPGer's) regular processes. The ta

Re: [teampractices] Code review social norms

2016-03-19 Thread 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 commi

Re: [teampractices] Code review social norms

2016-03-19 Thread Gergo Tisza
On Wed, Mar 16, 2016 at 4:25 AM, Mukunda Modell wrote: > Tweaking the code during merge is actually something differential / > arcanist already supports. It's amending someone else's revision (outside > of the merge process) that is currently prevented in arcanist. > Nontrivial changes in a merg

Re: [teampractices] Code review social norms

2016-03-19 Thread Antoine Musso
Le 18/03/2016 01:47, Gergo Tisza a écrit : > > Nontrivial changes in a merge commit is a bad practice, as the changes > are invisible to git diff / git log -p unless you explicitly tell git to > diff to the non-default parent, so they inevitably cause confusion. Hello, It is slightly more compli

[teampractices] Apologies to Greg (Re: Code review social norms)

2016-03-19 Thread Rob Lanphier
Hi folks, Thanks to everyone who privately pointed out that my email to Greg had an overly sarcastic and belittling tone. Greg, sorry. :-( To everyone else, my apologies for how my email negatively affected the tone of this list; please call me out on it if I do something like this again. Greg

Re: [teampractices] Code review social norms

2016-03-19 Thread Gergo Tisza
On Wed, Mar 16, 2016 at 12:48 AM, Kevin Smith wrote: > 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 w

Re: [teampractices] Apologies to Greg (Re: Code review social norms)

2016-03-19 Thread Greg Grossmeier
> Thanks to everyone who privately pointed out that my email to Greg had an > overly sarcastic and belittling tone. Greg, sorry. :-( Thank you for the apology, Rob. Greg -- | Greg GrossmeierGPG: B2FA 27B1 F7EB D327 6B8E | | identi.ca: @gregA18D 1138 8E47 FAC8 1C7

Re: [teampractices] Code review social norms

2016-03-19 Thread Gergo Tisza
On Wed, Mar 16, 2016 at 6:33 PM, Kevin Smith wrote: > 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. > I don't think there is a benefit

Re: [teampractices] Patch review culture of Wikimedia teams

2016-03-19 Thread Andre Klapper
Hej, thanks for the reply! On Tue, 2016-03-15 at 17:20 -0700, Kevin Smith wrote: > TPG generally focuses on process and inter-personal issues, and > doesn't get into the tech itself. > So our focus tends to be at the phab task level, rather than at the > gerrit patch level. As engineering tasks

Re: [teampractices] Code review social norms

2016-03-19 Thread Kevin Smith
On Thu, Mar 17, 2016 at 6:08 PM, Gergo Tisza wrote: > some large changes simply cannot be broken up into independent small ones > due to circular dependencies, unless you want to leave the code in a broken > state between commits (in which case you lose most of the advantage you > would get from

Re: [teampractices] Code review social norms

2016-03-19 Thread Subramanya Sastry
On 03/16/2016 01:33 PM, Kevin Smith wrote: 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 advocati

Re: [teampractices] Code review social norms

2016-03-19 Thread Dan Garry
On 16 March 2016 at 10:33, Kevin Smith wrote: > > 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 commi