git.net

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

Re: Arrow pull requests: please limit squashing your commits


On Wed, Dec 19, 2018 at 7:47 AM Francois Saint-Jacques
<fsaintjacques@xxxxxxxxx> wrote:
>
> No issue with this.
>
> When the final squash is done, which title/body is preserved?

The PR title (in GitHub) and the PR description are what matter. The
commit messages don't really matter

>
> 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
> >