osdir.com






















[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Arrow pull requests: please limit squashing your commits


No issue with this.

When the final squash is done, which title/body is preserved?

On Wed, Dec 19, 2018 at 8:43 AM Wes McKinney <wesmckinn@xxxxxxxxx> wrote:

> hi folks,
>
> As the contributor base has grown, our development styles have grown
> increasingly diverse.
>
> Sometimes contributors are used to working in a Gerrit-style workflow
> where patches are always squashed with `git rebase -i` into a single
> patch, and then force pushed to the PR branch.
>
> I'd like to ask you to avoid doing this, as it can make things harder
> for maintainers. Let me explain:
>
> * When you rebase and force-push, GitHub fails to generate an e-mail
> notification. I use the GitHub notifications to tell which branches
> are being actively developed and may need to be reviewed again. Many
> times now I have thought a branch was inactive only to look more
> closely and see that it's been updated via force-push. Since it took
> GitHub 10 years to start showing force push changes at all in their UI
> I'm not holding out for them to send e-mail notifications about this
>
> * GitHub is not Gerrit. We don't have the awesome incremental diff
> feature. So in lieu of this it's easier to be able to look at
> incremental diffs (e.g. responses to code review comments) by clicking
> on the individual commits
>
> * Our PR merge tool (dev/merge_arrow_py.py) squashes all the commits
> anyway, so squashing twice is redundant
>
> Sometimes I'll have commits like this in my branch
>
> * IMPLEMENTING THE FEATURE
> * lint
> * fixing CI
> * fixing toolchain issue
> * code review commits
> * fixing CI issues
> * more code review comments
> * documentation
>
> I think it's fine to combine some of the commits this so long as the
> produced commits reflect the logical evolution of your patch, for the
> purposes of code review.
>
> In the event of a gnarly rebase on master, sometimes it is easiest to
> create a single commit and then rebase that.
>
> Thanks,
> Wes
>