Code review comment for lp:~mbp/launchpad/612171-diff-generation-spam

On 14 October 2011 11:47, Aaron Bentley <email address hidden> wrote:
> On 11-10-13 07:49 PM, Martin Pool wrote:
>> Oh, perhaps I was unclear, I'm not suppressing mail for all user errors.
>> That would be bad.
>
> I'm glad you're not suppressing all user errors.  The point of a user error is that the user should be notified.  If it's not something the user should be notified about, then it shouldn't be a user error, and conversely, no user errors should be suppressed.
>
> You are suppressing errors about pending writes, but you are also suppressing errors about missing source and target revisions.  I believe that kind of error is something the user should be notified about, because either they made a mistake, or they need to push to the source or target.

OK, so we could certainly handle them differently.

On the case of empty branches:

 - that seems like a pretty obscure case; I make mistakes with mps all
the time (even the previous iteration of this one) but I don't think
I've ever done that; it's more common to target the wrong branch
 - when it does happen, are we sure it's a user error not something else?
 - I don't really understand the user story by which someone would try
to merge to or from an empty branch
 - it seems more useful to tell them about it at the time they are
proposing the merge rather than some time later
 - if we are going to tell them about it, we ought to say which branch
and what they are expected to do about it
 - won't the problem be pretty obvious when they look at the mp page?

If we didn't send mail about it, I doubt it would be a high bug.

> The obvious solution is to raise a different exception when there are pending writes.

yep

>
>> I'm logging all user errors
>
> That's great; I have a branch to do that, also: lp:~abentley/launchpad/log-user-errors
>
> I haven't submitted mine yet, because it doesn't have tests.  Unfortunately, it appears you haven't tested that, either.  The bug is #850974: No indication when a job resulted in a user error

Personally I'm not very rigorous about testing dev/ops oriented
logging, other than making sure it does not crash (eg when formatting
the string.) If we're not getting the information we want, we can
add more logging. You might say, what if something changes that means
the logs are unintentionally not emitted, but I think often those
changes happen at a level that means a unit test won't catch them (eg
different code is run, or not run.) I would never block adding
logging to wait for tests, but of course I don't decide lp's review
policy.

>
>> Then for this particular error, diff generation failure, which we may agree
>> is not really a user error, I'm suppressing the mail. Perhaps I should also
>> formally reclassify it.
>
> You should definitely formally reclassify it.  I think you should do that by raising a different exception for pending writes, but if you want to reclassify all UpdatePreviewDiffNotReady exceptions, you can just delete UpdatePreviewDiffJob.user_error_types

ok

> It appears that you've deleted the original merge proposal, which makes it hard to continue the conversation.  In the future, please consider resubmitting in these circumstances.  Resubmitting will allow you to change the target branch.

Sorry about that, I normally would. I realized the diff was wrong
because I targeted ~mbp/launchpad/devel, and I deleted that branch
without heeding the warning about it deleting the mp.