On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers <bchamb...@google.com> wrote:
> One risk to "squash and merge" is that it may lead to commits that don't > have clean descriptions -- for instance, commits like "Fixing review > comments" will show up. If we use (a) these would also show up as separate > commits. It seems like there are two cases of multiple commits in a PR: > > 1. Multiple commits in a PR that have semantic meaning (eg., a PR > performed N steps, split across N commits). In this case, keeping the > descriptions and performing either a merge (if the commits are separately > valid) or squash (if we want the commits to become a single commit in > master) probably makes sense. > Keep 'em > 2. Multiple commits in a PR that just reflect the review history. In this > case, we should probably ask the PR author to explicitly rebase their PR to > have semantically meaningful commits prior to merging. (Eg., do a rebase > -i). > Ask the author to squash 'em. Kenn > > On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles <k...@google.com> wrote: > >> Hi all, >> >> James brought up a great question in Slack, which was how should we use >> the merge button, illustrated [1] >> >> I want to broaden the discussion to talk about all the new capabilities: >> >> 1. Whether & how to use the "reviewer" field >> 2. Whether & how to use the "assignee" field >> 3. Whether & how to use the merge button >> >> My preferences are: >> >> 1. Use the reviewer field instead of "R:" comments. >> 2. Use the assignee field to keep track of who the review is blocked on >> (either the reviewer for more comments or the author for fixes) >> 3. Use merge commits, but editing the commit subject line >> >> To expand on part 3, GitHub's merge button has three options [1]. They >> are not described accurately in the UI, as they all say "merge" when only >> one of them performs a merge. They do the following: >> >> (a) Merge the branch with a merge commit >> (b) Squash all the commits, rebase and push >> (c) Rebase and push without squash >> >> Unlike our current guide, all of these result in a "merged" status for >> the PR, so we can correctly distinguish those PRs that were actually merged. >> >> My votes on these options are: >> >> (a) +1 this preserves the most information >> (b) -1 this erases the most information >> (c) -0 this is just sort of a middle ground; it breaks commit hashes, >> does not have a clear merge commit, but preserves other info >> >> Kenn >> >> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/ >> >> >> >> >> >> Kenn >> >
