[PATCH 01 of 35] largefiles: declare commands using decorator

[Pierre-Yves David]

On 05/07/2014 10:59 PM, Gregory Szorc wrote:
> On 5/5/2014 12:38 AM, Pierre-Yves David wrote:
>>> [PATCH 01 of 35] largefiles: declare commands using decorator
>>
>> While I appreciate your contributions, the areas you clean up, the
>> quality of your patches, this is still terribly wrong.
>>
>> You cannot blindly past bomb such gigantic series. Please start talking
>> with reviewer to have a more efficient way to do this. You are currently
>> creating massive disruption to the review process, impacting all other
>> contributors
>
> OK, so I grok that the run-tests patch series at >100 patches is
> excessive. But I'm kinda scratching my head as to what's wrong here.

Usual series patchbombed series are below 10 patches. (aiming at 5-7 
split if bigger than that). There is two main reasons for that:

1. Reviewing a giant pile of patches is usually very frustrating for 
both side. Because changes requested to the first patches of the series 
may have a painful consequence proportional to the size of the series 
(This is the `do not build giant backlog` part)

2. Second, our good old review system based on mailing list emails means:
* You are flooding hundreds of people when your send such series (+V2, 
V3, V4 etc).
* You greatly increase the amount of "to process patches" that peoples 
need to jungle with while doing review.
* Matt is very sensible to patches bloat and is process choke when there 
is too much in flight. Leading to various project distribution. (This is 
the mailing list process does not like giant series)

(3. As a reviewers I'll rarely review big series in one go anyway 
because my ability to spot error falloff quickly.)

The (1) is an important argument that stand true in most case. But It 
may be modulated by the exact content of the series (example your 12 
first patch about decorating command was 12 patches, but they were 
mostly identical) and how much it has been discussed already.
I understand why you built a bit backlog during the freeze, its not 
optimal but I hope the review won't be too painful.

The (2) argument sucks. Multiple of us has started working on improving 
this as a side track, but this is not a small task. Things will be much 
better as time goes and we have better tool to support the mailing list. 
In the mean time we have to deal with it.

> While this series is 35 patches, it is mostly cosmetic and IMO should
> involve lots of rubber stamping.

Yes, from what I've seem this seems very trivial. It is still an issue 
to -patchbomb- them all at once.

> I was very liberal about using multiple
> patches because fold is easier than split. I didn't want to risk someone
> rejecting the patches because they didn't like changing multiple files
> in a single patch. I believe the patch guidelines are on my side here.
> Furthermore, I'm pretty sure I've seen series with fewer parts but more
> actual code to review. I wouldn't get too held up on patch count without
> looking at actual diff size or the nature of the diff (e.g. new code vs
> copy)

We love small patches, please keep doing small patches (there is even a 
few I would've spitted more but I'm a bit extreme on this matter). In 
case the series is bigger than 10, we do not fold more patches together, 
we just send it in smaller group of changeset one (group) after the other.

>(although this is hard to identify in vanilla diffs - tools like
> Phabricator make it easier, but you know that).

Phabricator highlight actual code change in the diff. Making it easier 
to spot pure code indentation/movement. Not day and night. But cool.

> It's also not clear to me what "talking with [a] reviewer" entails.
> Please clarify.

Whenever you have a super sized series being built, you should be 
verbose about it (ok, your actually did). And get in touch with someone 
that actually push changesets. As this is your first bg series, we can 
help you to slice your series in reasonable chunk. Morever, knowing that 
you have a big backlog will helps having your small series pushed 
faster (knowing there is more to come). Also, we can setup synchronous 
review to quickly goes through the stack of patches.

> It is fair for you to take issue with me not following procedure. I just
> ask that the procedure be better documented so future contributors don't
> fall into unmarked pits. AFAICT

The "not following the procedure" part is exactly the issue here. And 
the fact it is not documented is a very fair point ☺

> http://mercurial.selenic.com/wiki/ContributingChanges says nothing on
> the subject of series length and disrupting review processes.
>
> I apologize. I'm just trying to contribute.

Apologize accepted ;-). Please continue doing awesome work!

--
Pierre-Yves