-1 for (a): no need to see all the private branch commits from contributor. It often makes me more conscious of local commits. +1 for (b): with committer replacing the squashed commit messages with '[BEAM-jira or PRID]: Brief cut-n-paste (or longer if it contributor provided one)'. -1 for (c): This is quite painful for contributors to work with if there has been merge conflict with master. Especially for larger PRs with multiple updates.
On Tue, Nov 28, 2017 at 10:24 AM, Lukasz Cwik <lc...@google.com> wrote: > Is it possible for mergebot to auto squash any fixup! and perform the > merge commit as described in (a), if so then I would vote for mergebot. > > Without mergebot, I vote: > (a) 0 I like squashing fixup! > (b) -1 > (c) +1 Most of our PRs are for focused singular changes which is why I > would rather squash everything over not squashing anything > > > > On Tue, Nov 28, 2017 at 9:57 AM, Kenneth Knowles <k...@google.com> wrote: > >> 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 >>>> >>> >> >
