OOPS on BadStateTransition when reviewing code by mail

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Paul Hummer wrote:
> So I did a code review for Entertainer and sent it, then immediately
> merged the branch into trunk and pushed up to launchpad. The scanner
> apparently marked the BMP as merged before my review with ' status
> approved' could get handled. This resulted in this oops:
> https://devpad.canonical.com/~matsubara/oops.cgi/2009-02-06/CEMAIL5

It's possible for something to be merged that is not approved, and it's
possible for something to be approved that isn't merged. So I think
that our model is slightly wrong. The ideal model would have two status
flags:

code review status: inactive, pending, approved, disapproved
merge status: inactive, queued, merged

If our model reflected this, then setting the review status to approved
would not change the merge status, and we could not get errors like this.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkmMWt0ACgkQ0F+nu1YWqI22EACfad+Ksy0foLEJTuLVcFa+L2BP
ULYAn0GNmRB3Ru6JnJ78bJ99Mg+HR69L
=itMw
-----END PGP SIGNATURE-----

On Sat, 07 Feb 2009 04:44:44 Aaron Bentley wrote:
> Paul Hummer wrote:
> > So I did a code review for Entertainer and sent it, then immediately
> > merged the branch into trunk and pushed up to launchpad. The scanner
> > apparently marked the BMP as merged before my review with ' status
> > approved' could get handled. This resulted in this oops:
> > https://devpad.canonical.com/~matsubara/oops.cgi/2009-02-06/CEMAIL5
>
> It's possible for something to be merged that is not approved, and it's
> possible for something to be approved that isn't merged. So I think
> that our model is slightly wrong. The ideal model would have two status
> flags:
>
> code review status: inactive, pending, approved, disapproved

s/disapproved/rejected/ :)

> merge status: inactive, queued, merged

Would we still want to somehow record merge_failed? As in we tried to merge
but failed due to either conflicts or test failures.

> If our model reflected this, then setting the review status to approved
> would not change the merge status, and we could not get errors like this.

Now that's a damn fine idea.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tim Penhey wrote:
> On Sat, 07 Feb 2009 04:44:44 Aaron Bentley wrote:
>> Paul Hummer wrote:
>>> So I did a code review for Entertainer and sent it, then immediately
>>> merged the branch into trunk and pushed up to launchpad. The scanner
>>> apparently marked the BMP as merged before my review with ' status
>>> approved' could get handled. This resulted in this oops:
>>> https://devpad.canonical.com/~matsubara/oops.cgi/2009-02-06/CEMAIL5
>> It's possible for something to be merged that is not approved, and it's
>> possible for something to be approved that isn't merged. So I think
>> that our model is slightly wrong. The ideal model would have two status
>> flags:
>>
>> code review status: inactive, pending, approved, disapproved
>
> s/disapproved/rejected/ :)
>
>> merge status: inactive, queued, merged
>
> Would we still want to somehow record merge_failed? As in we tried to merge
> but failed due to either conflicts or test failures.

I think that would make a lot of sense, because someone has to fix that
branch and re-queue it. We'd want to flag it specially in the UI.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkmQMaQACgkQ0F+nu1YWqI2EswCggs+4m6OFs6Qz1d+ZYqlmv0ky
AKgAn2bKkSZkPmEFcRvHBjUihaeKxEiO
=r2QM
-----END PGP SIGNATURE-----

I just hit this, and it seems like it might be slightly more common than just an edge case. In particular, it seems to take several minutes from the time I send an email and it winds its way through all of the various servers involved until launchpad processes it.

On the other hand, it's quite possible for me to push a merged revision in less time than it takes for the email to work - so there's a race condition where I approve email, commit, push, email arrives.

Also, honestly, if the merge request is auto-marked approved because it's been merged, and then the merge approval comes in, I think I'd rather see the message get added than an error thrown, given that the two intended states do not conflict.

My €0.02

And at the very least... an email response saying "Oh hai! Ur branchiz can haz already merged" would be nice.

It's telling users they've screwed up when they haven't and generating OOPSes. This is enough to make it a high priority bug.

Surely we can come up with something that will stop the errors without requiring substantial changes to our data model!