Merging a Pull Request

[Nick Coghlan]

On 20 February 2015 at 23:06, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 02/20/2015 01:08 PM, Andrew Miller wrote:
>>
>> I can issue pull requests and comment on them, but I cannot see how to
>> merge a pull request.
>>
>> Has this functionality been implemented yet?
>
> Correct, this has not been implemented. You can / have to do that locally.
> (And I think that is a good/better way of doing it.)

FWIW, I'm genuinely surprised that the "pull request" term has been
used in Rhodecode/Kallithea without providing online merging
functionality - in my view, those aren't really pull requests, they're
more in the vein of traditional online code reviews.

> I think 'automatic merge' is something that github and bitbucket do in a way
> where they pretty much enforce one work flow that is good enough for the 80%
> of "market" that are using them. I am more in the last 20%. I care what goes
> into my repository and I want to keep it clean. I want to tweak what I am
> merging, not just merge it.

Online pull request merging doesn't prevent that approach, although I
do think the Gerrit model is superior to the GitHub/BitBucket pull
request model in this particular regard. Because Gerrit identifies
change proposals through a stable hash-based ID, and stores them as a
branch in the backing git repo, anyone with the appropriate
permissions on the destination repo is able to clone a proposed
change, update it, and then submit it back for review as a modified
version of the existing change proposal.

That could actually be a reasonable way to go here (assuming it's
technically feasible). Rather than implement a GitHub/BitBucket style
"merge pull request" feature directly, it may be possible to instead
introduce a separate Gerrit-style notion of a "change proposal" that
isn't coupled to a particular origin fork/branch, but is instead more
like a standalone patch (or patch series) stored under version control
on the server (and hence able to be cloned, rebased, etc, using
appropriate obsolescence markers in Mercurial, and destructive history
editing in Git).

It would require a bit of UX design work to get right, but the general
idea of what I see as being possible here is that, by default, a
Kallithea repo could work like a GitHub or BitBucket repo: pull
requests would get a simple "Merge" button that, behind the scenes,
created and automatically accepted a change proposal.

Repo owners that wanted more control over the process of integrating
changes could then enable a "Change Proposals" feature on their repo.
This would expose the otherwise hidden additional step between someone
submitting their PR (and perhaps iterating on it themselves), and the
change actually being ready for inclusion into the main line of
development.

The key difference here is that whereas a PR is owned primarily by the
person submitting it, a change proposal would be owned primarily by
the team accepting it. It would be where they make the changes that
they want made before accepting the change, but that they don't want
to coach the external contributor through making.

Essentially, we'd be taking the tweaking step that core developers are
currently doing offline, and making it possible to do it online.

This approach would also give more selectivity in when particular
automated checks run. Some might run automatically on every PR, while
others might only run on submissions that have been promoted to being
Change Proposals.

Another advantage of introducing within-repo change proposals as a
distinct concept is that they could also be generated by uploading a
patch directly, rather than having to follow the
fork-or-branch-and-send-PR approach enforced by the GitHub/BitBucket
model.

(Tangent: I've long realised that the Gerrit model was fundamentally
different from the GitHub/BitBucket model, but I think the "change
proposal" vs "pull request" distinction above is the closest I've ever
come to being able to explain it clearly)

> Some thoughts in that area were dumped on
> http://lists.sfconservancy.org/pipermail/kallithea-general/2015q1/000149.html
>
> That being said, it could make sense to implement a merge option that could
> work in the simple cases. So far nobody considered it important enough to
> invest in implementing it.

This is a capability I'm very interested in, as one of the key goals
of the forge.python.org proposal is to enable using online editing and
pull requests to permit entirely in-the-browser typo fixes and other
small changes to documentation, Python Enhancement Proposals, the
developer guide, etc. You'd still want to work offline for more
complex changes, but for tiny non-functional changes, the idea is to
make the barriers to submitting fixes as low as is feasible.

I also see offering online change approval functionality as being
essential to making the best possible use of CPython core developer
time, as it means reviews can be completed and changes merged from any
device where the review can be read easily, rather than requiring a
local clone (if the change needs tweaks, and we're not personally
worried about whether or not the change makes it in, then we usually
ask the contributor to make any required tweaks. If we do care, then
yes, we'll revise the change ourselves, and will need to update it
using an offline workflow.

Finally, I see online merging support as being essential to enabling
an OpenStack style "all changes go through the review process" model,
where you have "core reviewers" (who approve merges) rather than "core
committers" (who push changes to the main repo directly). That's not
going to be appropriate for every project (e.g. it's almost entirely
pointless for personal projects), but it's a valuable maintainability
and knowledge-sharing policy in larger teams (as it means most changes
will have been both read and reviewed for readability by at least 1
person other than the original author). The other nice part of this
particular approach is that people don't have any new workflows to
learn when they become a core reviewer - they just gain the power to
push a button that they previously had to persuade someone else should
be pushed.

Regards,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia