Re: Rebase and merge

2017-08-15 Thread Masakazu Kitajo
On Fri, Aug 11, 2017 at 7:05 AM, Matteo Merli <matteo.me...@gmail.com>
wrote:

> I am ok in having rebased pull-requests and to allow the "Rebase & Merge"
> option in github.
> That can be helpful in cases in which there are multiple logical (though
> related) commits for
> which you want to keep separation.
> For most cases (eg: PRs comments and incremental fixes on the PR) I think
> the squash option
> is still better, because the history of the PR is anyway available, with
> all the comments and commits.
>

I agree. On Traffic Server project, we only use "Rebase & Merge", and
reviewers are expected to check if the commits are reasonably separated /
squashed. Actually most PRs contains only one commit, but sometimes the
case happens, as you know:
https://github.com/apache/incubator-pulsar/pull/524

And it's not just for commit history. In case of revert / cherry-pick /
bisect, small commits are easy to deal with.


>
> My only concern is that once you chose that option and "rebase & merge" one
> PR , it becomes your default, until you change it back.
> So one has to really pay attention to not forget to go back to the squash
> option.
>

Hmm, I didn't know that. That could be a problem.


>
> One other option would be to have a customized script to merge PRs. This
> can help in enforce a uniform style in the
> commit logs (as well as adding the name of the reviewers). For example,
> this is what we use in BookKeeper:
> https://github.com/apache/bookkeeper/blob/master/dev/bk-merge-pr.py
>
> It's a bit more involved than just hitting "Squash & merge" on github, but
> nothing really major.
>

I guess this is the reason all PRs on BookKeeper project are marked as
closed (not merged) and the commits page on GitHub doesn't have a link for
the PRs. I'd rather use features on GitHub.


> What do other people think about this?
>

Anyone?



>
>
> Matteo
>
> On Tue, Aug 8, 2017 at 4:50 AM Masakazu Kitajo <mas...@apache.org> wrote:
>
> > Hi,
> >
> > I'd like to propose enabling "Rebase and merge"[1] as an option to merge
> > pull requests.
> >
> > Currently, we have only one option, "Squash and merge". However, I'm not
> a
> > big fun of this option because it makes commit history too clean. Even if
> > you made several meaningful commits on your branch, they would be
> squashed
> > when we merge the pull-request.
> >
> > What if we want to revert a part of the changes?
> > What if we want to cherry-pick a part of changes?
> >
> > These sometimes happen, and in general, it's a good practice to keep
> > commits small.
> >
> > As long as commits in a pull request seem meaningful, I think we should
> > keep the commits separated.
> >
> > What do you think?
> >
> > [1] https://github.com/blog/2243-rebase-and-merge-pull-requests
> >
> > Masakazu
> >
> --
> Matteo Merli
> <mme...@apache.org>
>


Masakazu


Re: Rebase and merge

2017-08-10 Thread Matteo Merli
I am ok in having rebased pull-requests and to allow the "Rebase & Merge"
option in github.
That can be helpful in cases in which there are multiple logical (though
related) commits for
which you want to keep separation.
For most cases (eg: PRs comments and incremental fixes on the PR) I think
the squash option
is still better, because the history of the PR is anyway available, with
all the comments and commits.

My only concern is that once you chose that option and "rebase & merge" one
PR , it becomes your default, until you change it back.
So one has to really pay attention to not forget to go back to the squash
option.

One other option would be to have a customized script to merge PRs. This
can help in enforce a uniform style in the
commit logs (as well as adding the name of the reviewers). For example,
this is what we use in BookKeeper:
https://github.com/apache/bookkeeper/blob/master/dev/bk-merge-pr.py

It's a bit more involved than just hitting "Squash & merge" on github, but
nothing really major.

What do other people think about this?


Matteo

On Tue, Aug 8, 2017 at 4:50 AM Masakazu Kitajo <mas...@apache.org> wrote:

> Hi,
>
> I'd like to propose enabling "Rebase and merge"[1] as an option to merge
> pull requests.
>
> Currently, we have only one option, "Squash and merge". However, I'm not a
> big fun of this option because it makes commit history too clean. Even if
> you made several meaningful commits on your branch, they would be squashed
> when we merge the pull-request.
>
> What if we want to revert a part of the changes?
> What if we want to cherry-pick a part of changes?
>
> These sometimes happen, and in general, it's a good practice to keep
> commits small.
>
> As long as commits in a pull request seem meaningful, I think we should
> keep the commits separated.
>
> What do you think?
>
> [1] https://github.com/blog/2243-rebase-and-merge-pull-requests
>
> Masakazu
>
-- 
Matteo Merli
<mme...@apache.org>