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

[Wes McKinney]

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/contributing.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
>> >
>
>