Code review: pull requests versus independent review

[Mads Kiilerich]

On 01/28/2015 02:06 PM, Thomas De Schampheleire wrote:
> Hi,
>
> Currently, reviewing a set of commits in Kallithea is based on the
> concept of a pull request.

Let me share some general thoughts related to this:

"Pull request" is a very bad name.

The contributed code might in some cases be linear on top of the target 
repo so it just can be pulled. That is however a special case. In the 
general case it will not be linear, so what you really want is a merge 
(with the special case being a "fast forward" merge). It should thus be 
"Merge request".

Some projects prefer a linear history that shows how the project evolved 
upstream instead of what happened 'downstream' to get there. What they 
really want is "Rebase request".

Some people might want to have support for cherry picking changes 
without having to rebase them just for the PR. They want "Graft requests".

Some people might want to put something out there for review and 
feedback, without really intending/expecting it to get accepted upstream 
yet. They want "Review request".

Also: the Compare and Show Changesets views are very similar to how you 
would create "merge requests" or "cherry pick requests", and some kind 
of naming of them would be close to an actual PR.

I guess I would like to rename "Pull Request" and merge it with compare. 
I am not sure what to name it. Something that doesn't suggest a wrong 
intent. It is not necessarily a "request" and especially not for "pull". 
It is just a bunch of changesets that you want someone to do something 
with. "Change collection"?

> 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?

Initially, RhodeCode Pull requests were pretty much like that. It did 
however not work well for us, especially because we were merging messy 
history and not really wanted to review per changeset. The data model do 
however still store the list of changesets (in a string field!). I guess 
it would be quite easy to add/restore "cherry pick per changeset review" 
by using the "show changeset" view instead of "compare".

/Mads