code review merge status and docs need clarity, particularly for 'tweak' cases

We talked about this, but what you are saying is

"This is approved but I'd like you to fix X"

This means "approved" and I trust you. If I don't trust you it is "needs
fixing".

2009/5/7 Tim Penhey <email address hidden>:
> We talked about this, but what you are saying is
>
> "This is approved but I'd like you to fix X"
>
> This means "approved" and I trust you.  If I don't trust you it is "needs
> fixing".

Yes, that's write. The assertion of this bug report is that it's
useful to distinguish proposals which can be immediately merged from
those that require further work to do so. I find it a bit useful in
planning what work to do.

but this is not urgent - you can leave this report open and see what
others think.

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

I think that we should have a Tweak status, but I also think we should review our existing statuses in the light of that.

To move this bug further along, Tim needs to make a decision, or arrange to talk with the various stakeholders which will enable him to make a decision.

mpt, why did you change the status to New? Is there some policy I'm misunderstanding or not aware of?

Sorry, I didn't see any mention of what information was missing from the report, so I assumed the Incomplete status was a mistake. (If it wasn't a mistake, that seems different from how other projects in Launchpad are using the Incomplete status, so maybe you should discuss your needs with the Launchpad Bugs developers.)

Needs fixing is meant to mean "This is approved but I'd like you to fix X"
Resubmit is meant to be used when the reviewer is withholding approval until changes are made.

2009/6/24 Aaron Bentley <email address hidden>:
> Needs fixing is meant to mean "This is approved but I'd like you to fix X"
> Resubmit is meant to be used when the reviewer is withholding approval until changes are made.

OK, so that does seem to cover the options. At least to me, "needs
fixing" doesn't very obviously imply they have permission to merge
after fixing it, but maybe I'm mentally biased by BB and previous
projects.

Maybe this could be explained more in
<https://help.launchpad.net/Code/Review> and then that page could be
linked from the form?

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

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

Martin Pool wrote:
> 2009/6/24 Aaron Bentley <email address hidden>:
>> Needs fixing is meant to mean "This is approved but I'd like you to fix X"
>> Resubmit is meant to be used when the reviewer is withholding approval until changes are made.
>
> OK, so that does seem to cover the options. At least to me, "needs
> fixing" doesn't very obviously imply they have permission to merge
> after fixing it, but maybe I'm mentally biased by BB and previous
> projects.

I'll give you one guess what *I* suggested we call "needs fixing" :-)

> Maybe this could be explained more in
> <https://help.launchpad.net/Code/Review> and then that page could be
> linked from the form?

Couldn't hurt.

Code Review in Launchpad is currently in flux, because its model is
different from BB. In BB, you propose changes to be merged. In Code
Review, you propose a branch to be merged. Because branches, not
specific changes, are proposed for merging, it's not clear whether or
not "resubmit" will become a common action. If it doesn't, then
"resubmit" is a poor label for "Make these changes and I'll have another
look".

As a result, the Launchpad team itself is using "needs fixing" to mean
"Make these changes and I'll have another look".

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

iEYEARECAAYFAkpBrXMACgkQ0F+nu1YWqI3nkACfUKjjRIv1kvfjVHA+K9v21UFh
v8wAn2bvwDbuqwmo9D/gTChvwDp0GVe8
=NSww
-----END PGP SIGNATURE-----

Oh and of course (as discussed in another bug) resubmit in the overall
proposal status means not "this needs to be resubmitted" but "start
another review now", which perhaps complicates things.

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

There's a couple of things here: one being that 'tweak' is probably a more clear name for a vote than 'needs fixing'. People do seem to be confused about this overall.

The other is that for the overall status rather than votes, there is no 'tweak' or 'merge with changes' status: only 'needs review', 'in progress' and 'approved'. I think it would be useful to make that distinction so that people can see those branches that could be mindlessly/automatically merged on the one hand, and those that can be merged with no further discussion but with a bit of manual intervention on the other, as distinct from those that require a lot more work.

This is a regular irritation I'm encountering with reviews of patches for bzr. I've started saying directly "I'm voting Needs Fixing, but once you have addressed these issues please consider my vote upgraded to Approved" in the comments.

I see Aaron's remark that:

> Needs fixing is meant to mean "This is approved but I'd like you to fix X"
> Resubmit is meant to be used when the reviewer is withholding
> approval until changes are made.

But that isn't how Needs Fixing is being interpreted in practice for the bzr project (either by reviewers or reviewees). Is the experience in other projects different? I gather the idea is that the presence of "Resubmit" is meant to disambiguate the meaning of "Needs Fixing", but I think instead it just confuses.

I have fond memories of the word "tweak" from BundleBuggy, but I'd be about as happy for the vote to be called "Conditional Approve", and think reviewees would find it at least as clear as "tweak". (Conditionally Approved would make a reasonable status description too, as BundleBuggy showed). Either "Tweak" or "Conditional Approve" would be an improvement over the current ambiguity of "Needs Fixing".

I'd be open to us reviewing the code review statuses. I'd rather us look at all the statuses again and come up with a good set based on what we know now than adding the occasional status here and there.

I also have fond memories of "tweak"

I'd love to see this done; basically needs someone with some time to shepard through an email discussion, get consensus, then JFDI the code changes to match. We could mentor someone interested.