On Wed, Dec 19, 2018 at 7:47 AM Francois Saint-Jacques <fsaintjacq...@gmail.com> 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 <wesmck...@gmail.com> 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 > >
