[Pandas-dev] Proposal to change PR update policy from rebase to merge

Fri Jan 26 12:19:25 EST 2018

To come back to this, I chatted with Jeff a whole time ago, and I think he
agrees with the idea of promoting merge instead of rebase (Jeff, if I
misinterpreted you, say so!)

So next step is to do a PR with an update of the contributing docs (Marc
Garcia just opened a related issue about this:
https://github.com/pandas-dev/pandas/issues/19412), and then we can discuss
there more about the exact details.

Joris

2017-11-14 17:46 GMT+01:00 Stephan Hoyer <shoyer at gmail.com>:

> The GitHub pull request model does have some virtues: there is a 1-1
> relationship between git commits on a branch and what you see on the pull
> request. This makes it easier to review/edit a change's history without the
> review tool, and for multiple users to work on a single changes at once --
> as long as contributors stick to adding new commits and merging in updates
> from master instead of rebasing. The PR interface itself is designed around
> this, e.g., you can click "Changes since my last review" (or any particular
> change), get email notifications when new commits are pushed, etc.
>
> It would be nice to have the option of Gerrit's single commit model, but
> as long as we are stuck on GitHub we should use the process that works well
> with the tool.
>
> On Tue, Nov 14, 2017 at 7:56 AM Wes McKinney <wesmckinn at gmail.com> wrote:
>
>> Having now experienced Gerrit and other professional code review
>> tools, it's very hard for me to see GitHub's review in anything but a
>> negative light. I haven't found any version of contributor behavior to
>> make incremental reviews any easier -- this "has the contributor
>> updated the PR" is a problem that is completely solved / a non-issue
>> in tools like Gerrit (the other issues you cited also do not exist in
>> Gerrit). Since I'm not actively maintaining pandas PRs, the solution
>> that makes your lives as maintainers easiest is OK by me.
>>
>> It's too bad we're in a position of advocating "messy" PR branches in
>> order to improve the UX of code reviews in GitHub. FWIW, I talked with
>> GitHub employees in person about these exact sort of problems in
>> October and I don't think it's likely to improve anytime soon.
>>
>> - Wes
>>
>> On Tue, Nov 14, 2017 at 9:49 AM, Joris Van den Bossche
>> <jorisvandenbossche at gmail.com> wrote:
>> > Another advantage (IMO) that I forgot:
>> >
>> > - when a contributor adds a commit to a branch instead of
>> > squash+rebasing/amending new additions, you get notified of that by
>> github.
>> > In that way, I know the contributor has actually pushed updates related
>> to
>> > my review, and I know I have to look again at the PR (otherwise I have
>> to
>> > check the PR from time to time to see if the contributor pushed new
>> > changes).
>> >
>> > 2017-11-14 5:34 GMT+01:00 Wes McKinney <wesmckinn at gmail.com>:
>> >>
>> >> I think as long as clean atomic commits end up in master (which the
>> >> merge/squash tool takes care of) then whatever is convenient for the
>> >> contributor is fine. I personally prefer clean rebases but some users
>> >> struggle with rebasing.
>> >
>> >
>> > My preference to not rebase is not only from a contributor point of
>> view,
>> > but mainly from reviewer point of view.
>> > So my point is basically that, for me personally, this "whathever is
>> > convenient for the contributor" is not fine for me as a reviewer.
>> >
>> > But of course, if the different reviewers don't share this preference,
>> we
>> > can't ask something specific from the contributors. That's the reason I
>> > opened this discussion to see if there would be agreement.
>> >
>> >>
>> >> - Wes
>> >>
>> >> On Mon, Nov 13, 2017 at 5:49 PM, Joris Van den Bossche
>> >> <jorisvandenbossche at gmail.com> wrote:
>> >> > 2017-11-13 23:05 GMT+01:00 Jeff Reback <jeffreback at gmail.com>:
>> >> >>
>> >> >> i don’t care what people actually do in there own branches;
>> >> >
>> >> >
>> >> > The point is a bit that I actually do care about what you do in your
>> >> > branches. I would find it easier for the PRs I am reviewing that
>> people
>> >> > would add commits (and merge master to sync with latest changes)
>> than to
>> >> > rebase and amend or squash commits, or add commit + rebase.
>> >> >
>> >> >>
>> >> >> though i find rebase much easier to read
>> >> >>
>> >> >> as long as github squashes then it’s fine
>> >> >>
>> >> >> the issue is that when i need to look at there branches locally they
>> >> >> are
>> >> >> always a mess and very hard to follow
>> >> >
>> >> >
>> >> > Can you give an example of what is hard? Eg if the branch is out of
>> >> > date, I
>> >> > typically just merge master in it.
>> >> >
>> >> >>
>> >> >> i would still recommend rebasing
>> >> >>
>> >> >> On Nov 13, 2017, at 5:02 PM, Tom Augspurger
>> >> >> <tom.augspurger88 at gmail.com>
>> >> >> wrote:
>> >> >>
>> >> >> Yes, recommending merging instead of rebasing seems OK now that
>> Github
>> >> >> has
>> >> >> squash on merge.
>> >> >>
>> >> >> Tom
>> >> >>
>> >> >> On Mon, Nov 13, 2017 at 3:58 PM, Stephan Hoyer <shoyer at gmail.com>
>> >> >> wrote:
>> >> >>>
>> >> >>> +1 for merging instead of rebasing. Not losing comment history in
>> PRs
>> >> >>> is
>> >> >>> a major bonus, and the end result (when we squash on merge) is
>> >> >>> basically
>> >> >>> looks the same.
>> >> >>>
>> >> >>> In fact, I would say with this workflow GitHub almost works as
>> well as
>> >> >>> Gerrit ;).
>> >> >>>
>> >> >>> On Mon, Nov 13, 2017 at 1:54 PM Joris Van den Bossche
>> >> >>> <jorisvandenbossche at gmail.com> wrote:
>> >> >>>>
>> >> >>>> Hi all,
>> >> >>>>
>> >> >>>> Currently when PRs get outdated, we often ask to "rebase and
>> update"
>> >> >>>> (although to be honest, it mainly Jeff that is doing most of this
>> >> >>>> work to
>> >> >>>> ping stale PRs), and rebasing is also how it is explained in the
>> docs
>> >> >>>>
>> >> >>>> (http://pandas-docs.github.io/pandas-docs-travis/contributin
>> g.html#creating-a-branch).
>> >> >>>> And also many active contributors use rebasing while working on a
>> PR
>> >> >>>> to
>> >> >>>> get in sync with changes in master.
>> >> >>>>
>> >> >>>> I would like to propose to change this policy from rebasing to
>> >> >>>> merging
>> >> >>>> (= merging master in the feature branch, creating a merge commit).
>> >> >>>>
>> >> >>>> Some reasons for this:
>> >> >>>>
>> >> >>>> - I personally think this is easier to do (certainly for less
>> >> >>>> experienced git users; conflicts can be easier to solve, ..)
>> >> >>>> - It makes it easier to follow what has changed (certainly if we
>> >> >>>> extend
>> >> >>>> 'not rebasing' with 'not squash rebasing'), making it easier
>> review
>> >> >>>> - It doesn't destroy links to github PR comments
>> >> >>>> - Since we squash on merge in the end, we don't care about the
>> >> >>>> additional merge commits in the PR's history
>> >> >>>>
>> >> >>>> Thoughts on this?
>> >> >>>>
>> >> >>>> If we would agree on this, that would mean: update the docs +
>> start
>> >> >>>> doing it ourselves + start asking that of contributors
>> consistently.
>> >> >>>>
>> >> >>>> Regards,
>> >> >>>> Joris
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>> _______________________________________________
>> >> >>>> Pandas-dev mailing list
>> >> >>>> Pandas-dev at python.org
>> >> >>>> https://mail.python.org/mailman/listinfo/pandas-dev
>> >> >>>
>> >> >>>
>> >> >>> _______________________________________________
>> >> >>> Pandas-dev mailing list
>> >> >>> Pandas-dev at python.org
>> >> >>> https://mail.python.org/mailman/listinfo/pandas-dev
>> >> >>>
>> >> >>
>> >> >> _______________________________________________
>> >> >> Pandas-dev mailing list
>> >> >> Pandas-dev at python.org
>> >> >> https://mail.python.org/mailman/listinfo/pandas-dev
>> >> >>
>> >> >>
>> >> >> _______________________________________________
>> >> >> Pandas-dev mailing list
>> >> >> Pandas-dev at python.org
>> >> >> https://mail.python.org/mailman/listinfo/pandas-dev
>> >> >>
>> >> >
>> >> >
>> >> > _______________________________________________
>> >> > Pandas-dev mailing list
>> >> > Pandas-dev at python.org
>> >> > https://mail.python.org/mailman/listinfo/pandas-dev
>> >> >
>> >
>> >
>> _______________________________________________
>> Pandas-dev mailing list
>> Pandas-dev at python.org
>> https://mail.python.org/mailman/listinfo/pandas-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/pandas-dev/attachments/20180126/b14f5919/attachment.html>