Code review: pull requests versus independent review

[Thomas De Schampheleire]

Hi Nick,

On Wed, Jan 28, 2015 at 2:50 PM, Nick Coghlan <ncoghlan at gmail.com> wrote:
> On 28 January 2015 at 23:06, Thomas De Schampheleire
> <patrickdepinguin at gmail.com> wrote:
>> Hi,
>>
>> Currently, reviewing a set of commits in Kallithea is based on the
>> concept of a pull request. A code author can create a pull request for
>> a set of (descendant) commits so that people can review it.
>> When Kallithea is not used for actually performing the pull/push to
>> the main repo, the pull request concept is actually not the best fit.
>>
>> One particular case where it fails is when not all commits you want
>> reviewed are descendants of one another. Maybe there is a commit in
>> the middle that is irrelevant, or relevant but you do not expect
>> reviewed by that set of people.
>>
>> In this case, being able to cherry-pick the commits part of the review
>> is more useful. Given the Kallithea way of handling reviews (which is
>> mostly coupled to individual changesets without them being coupled to
>> the corresponding pull request), it wouldn't be that hard to add an
>> extra concept in Kallithea: that of 'Review request'.
>> Such review request could contain an arbitrary number and combination
>> of commits, without mandatory parent-child relation, but is otherwise
>> very similar to the current pull request concept.
>>
>> What do you think of this idea?
>
> What would be the expected outcome of such a review request, and how
> do you see it fitting into a team's workflow?

The outcome is that the developer gets a go/no-go feedback for his
changes, giving him the permission to proceed with delivering his
changes through the existing methods (which could be outside of
Kallithea).
If comments were posted, the author is supposed to modify his code and
relaunch/update the review request.

The workflow would be
a. create changes in local repo
b. push changes to review repo in kallithea
c. launch review request in kallithea
d. receive comments from reviewers
e. adapt code, go to b. (cycle)
f. when approved, push code to delivery repo (outside of kallithea,
possibly passing some validation).

>
> With a pull request, the expected outcome is clear (merged or
> rejected), and if fits clearly into a workflow as a gating mechanism
> for inclusion into the main line of development.

Could you explain in some detail how Kallithea can be used to support
this workflow, in particular assuming there are no comments on the
pull request, how does the code get merged? I did not see any
provisioning in Kallithea to merge in the code, so the developer is
supposed to pull the changes from the pull request, perform any needed
merge, then push the changes back. Is that correct?

And (how) does Kallithea detect that the changes are pulled?
Is it possible to indicate that a pull request is merged, albeit after rebasing?

>
> Additionaly, how would such a review request differ from cherry
> picking the commits of interest to a new branch and including them in
> a new pull request?

The fact that you need to do the cherry-picking to the branch.
With the review request, the repository is not touched, only viewed in
a different way.
When cherry picking, you are effectively changing the repository, in
order to fit it into the limitations of a tool.
Note that for my own changes, a pull request would probably suffice,
but I'm thinking in a large company context where there are many
developers, some of which may not even know how to cherry-pick.

>
> Finally, if Kallithea is not the "repo of record" and someone just
> wants a standalone code review tool, wouldn't it make more sense to
> use something like ReviewBoard instead?

We did evaluate several other tools, including ReviewBoard, Crucible,
Phabricator and Upsource. But only Kallithea and Upsource meet my
basic requirement of being able to review each commit separately in
case of a multi-commit review.

Note also that although at this moment we are still using
mercurial-server to host the repositories, I do not exclude that
Kallithea could take over this job once SSH authentication is
supported.

Best regards,
Thomas