[Reviewers] dropped a couple changesets and flushed the queue
Augie Fackler
raf at durin42.com
Thu Dec 15 22:24:30 UTC 2016
> On Dec 15, 2016, at 3:40 PM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
>
>> I didn’t ever say it was unreasonable to have dropped these patches.
>> I just haven’t personally observed any attempt to contact me before discarding a patch I’ve queued, so I wanted to bring it up as a potential policy point for discussion.
>
> I'm sorry but this is definitely not how I read your initial reply. I triple checked before writing my previous reply, and I doubled checked again before writing this one. To me the opening part (quoted below), just read like "I don't see why this was reasonable, especially in regard with Mads latency".
>
>>>> It was not clear to me that there was consensus that the changes here were bad - I agree it would have been better as two patches, but other than that what am I missing that makes this worth dropping? History indicates it’ll be weeks-to-never that Mads comes back to it and it’s a clear defect fix on the whole, so I’d have liked it to be clearer on the public review thread what’s wrong here
>
> That part (above) is about 70% of the total message payload and came first. This two things alone probably guaranteed I received reply as "aggressive reaction to the drop" and not "just wanting to bring a policy point to discussion”.
[...]
> A short rephrasing of that feedback is "please try to balance positivish/negativish ration in your email to match your mind state).
>
> To make it very clear, I don't bring this up to paint Augie badly and I trust he meant good. I'm spending time explaining all this because that as also been a recurring issues in our community (from various people and affecting various people) and a significant causes for my burn out earlier this year.
Something that would be productive for both of us would be to try and take the most charitable possible interpretation of any email - I probably shouldn’t have sent my message without significant re-editing after your message on the review thread landed, and I’m sorry for that.
>
>>> On policy around dropping patch
>>> -------------------------------
>>>
>>> I agree that getting a green light from another reviewers before
>>> dropping is a good thing. Ideally, you get the approval of reviewers who
>>> initially push the change (and any additional reviewer who approved it).
>>>
>>> However I trust other reviewer to be thoughtful and flexible on that.
>>> If there is a legitimate reason to bypass it after careful
>>> consideration, they should got for it.
>>
>> Note that I agree in this case the patches should have been discarded, but would have appreciated even a cursory attempt to contact me and ask before doing so. It’s a bad feeling for me to have my review work discarded without me having a chance understand why or even know it’s about to happen.
>
> Yes, I should have pinged you beforehand. I understand it could have been frustrating for you.
It was deeply frustrating, because it felt like you were unilaterally discounting my review. I responded too quickly (and was unclear), and in so doing upset you (and made you feel undervalued). Sorry about that. :(
> I'll make my best to put more effort into that if such situation arise.
>
> It is probably worth to note that your ban of all communications between us outside public channels, adds a significant barrier to communicating such notice. That was part of why I skipped prior-pinging and directly moved to drop + "notification email”
I don’t want the “this patch might need to be dropped” discussion to be private - I think it should be on the original review thread (if feasible), and at least on mercurial-devel at . That’s what I had in mind as far as a policy goes (so everything comes back to the same mailing list). Does that make sense?
> (Still would have been better to ping you beforehand instead of picking the easy option)
>
>>> Dropping a patches is not a big deal, it is easy to requeue it as draft
>>> on the top of the start and resume the discussion. Right
>>> now, the main pain around dropping is our tooling.
>>
>> Is dropping any more complicated than a prune, stabilize and push at this point? (With a warning and/or helping hand to the contributor so they don’t lose their change)?
>
> Yes, currently, the stabilization means changeset get rewritten and repushed. As a result:
You misunderstood my question a bit: I meant “prune && stabilize --all && push” is the right set of hg commands? I’m aware of the (nasty) implications for our review tooling. :/
>
> 1) dropping existing accept stamps on everything we touched,
> 2) implicitly accept everything we repushed.
>
> The problems gets bigger as the stack above what we drop growth (in our case 92, but this become painful much sooner).
I’ve been thinking about this some, and I don’t yet have any ideas that merit wider sharing. The only thing I’ve come up with (and mostly discarded) is pushing lots of anonymous heads, and as things get automatically accepted a robot would do the merge and (absent any conflicts or test failures) auto-land the merge of the accepted series. It’s not that much code, and it might be kind of nice, but the pile of anonymous heads could be really annoying.
Suggestions welcomed - I might be able to spend some time on this in Q1, as it’s starting to be a big problem for FB/GOOG/MOZ.
Thanks!
More information about the Reviewers
mailing list