Personal experiences/anecdotes of the bzr review processes

[Martin Pool]

On Tue, Sep 23, 2008 at 9:26 PM, Mark Hammond <mhammond at skippinet.com.au> wrote:
> I noticed that the bazaar review process has been discussed here recently,
> and a recent experience of mine is a reasonable example of my experiences
> submitting patches, so I thought it might be worth sharing my perception of
> the process.
>
> The anecdote is the thread "[MERGE] add
> win32utils.get_local_appdata_location()" and I hope I don't come across as
> pointing fingers at any people, just at processes.  Here is some background
> of the issue itself and the patch:
>
> * bzr provides a function to query the windows API for the "app data"
> directory.  It does this by calling a single windows API function with a
> particular param.
> * It turns out Windows provides *two* app-data locations, and a bzr-svn
> feature request correctly suggests some data from bzr-svn be stored in the
> second.
> * Jelmer suggests bzr provide this location, I agree to assist by providing
> a patch in an effort to help both bzr-svn and bzr itself.
> * Importantly, this entire feature involves calling the *exact same* API
> function as in the first point, but with a different param.
> * Possibly foolishly, I add tests for both the existing calls to that API,
> and for the new calls to that same API with the different param and for
> exposing a different way of calling the exact same windows API function
> (ctypes vs pywin32).  OTOH, I feel confident that if I submitted that patch
> *without* adding tests even for the existing non-tested cases, it would have
> been rejected.
>
> The timeline for this patch:
>
> * 23 August, I submit a patch
> * 27 August, John votes "resubmit" with (mostly valid; some even pep8)
> stylistic issues.
> * 27 August, I ask for clarification.
> * 29 August, John clarifies.
> * 30 August, I resubmit
> * 4 Sept, John asks for another tweak in a comment block
> * 4 Sept, I resubmit
> [Note: no 'approve' from John ever comes]
> * 19 Sept, Ian reviews and notes tests fail on non-windows platform.
> * 19 Sept, I resubmit
> * 19 Sept, Ian votes approve
> [first approval!]
> * 23 Sept, Aaron re-reviews, picks up on various stylistic issues, it seems
> we are back at square one.
> * 23 Sept, my frustration grows :)
>
> At this point the patch is fundamentally the same as I submitted it (give or
> take some double blank lines as per pep8 and skipping the new tests in some
> cases), bzr-svn has long ago submitted the change to take advantage of this
> feature when it deigns to appear, and I'm awaiting instructions for yet
> another resubmission request.  While I understand the importance of keeping
> a close eye on quality, the reality is from my personal POV, the cost of
> submitting improvements that aren't *critical* for my work are very close to
> being too high to bother with...
>
> I hope this is constructive,

It is, thanks for posting it.

(Quoted from the other thread)
>> Anyhow, we want to provide a great API, and if you have to use a private
>> symbol, we're failing at that.  Please help us make our API great.
>
> I tried to use an internal API function, and when I did it appeared to break
> in a fairly fundamental way.  I diagnosed the problem and submitted a patch.
> In response to a suggestion that I propose the API as being public, I
> pointed out that many more mature plugins don't use the symbol, so maybe its
> not that suitable for becoming public, and these plugins *do* use other
> private symbols, and have done so for far longer than for my code has
> existed.
>
> I think I've done my duty :)
>
> Martin: thanks for agreeing to merge it...

So I think (and we can take this up in your other thread) this is a
bit of a case where our reviews by trying to make things better
overall are slowing down merges of any particular patch.  The actual
substantial point here, that perhaps this function should be renamed,
is not wrong.  But raising it during reviews seems to be causing a few
problems (and these are not specific to the people in this review):

 * A kind of priority-inversion where less-important changes get
attached to more-important ones
(<http://en.wikipedia.org/wiki/Priority_inversion>)
 * It makes each review a longer thread so sometimes it's harder to
determine what needs to be done to finish it, and it can be harder for
other people to tell if it's settled or not
 * It can stop the momentum of the person doing the original patch,
and discourage them from doing small or non-critical patches in
future.  This effect can be pretty strong, and both Mark and John have
mentioned it recently.
 * Sometimes I suspect the cleanups are raised because they happen to
be noticed here, not because they're really high on our notional
priority list

I think the main concern here is that if we don't pick up on things at
the time they originally come in, the code will gradually get messy
and untested, which would be bad.  I think there's a further
assumption that if we don't make these things happen at the time the
patch is coming in, we won't ever get back to them.  There may be some
truth to that but I think another way is possible: if we get patches
reviewed and settled more quickly we might have more time to do
cleanups.  I think there's a greater-than-unity payoff in avoiding
people's frustration and keeping reviews straightforward.

I'm thinking that perhaps cleanup-type work, as opposed to actual bugs
or obviously wrong stuff, should be more strictly separated.  Of
course people can mention that they noticed it but let's do it later.
Yes, it's a grey area, but the balance can be better than it is now.

-- 
Martin <http://launchpad.net/~mbp/>