On Tue, Nov 28, 2017 at 11:16 AM, Raghu Angadi <rang...@google.com> wrote:
> -1 for (a): no need to see all the private branch commits from > contributor. It often makes me more conscious of local commits. > I want to note that on my PRs these are not private commits. Each one is a meaningful isolated change that can be rolled back and is useful to keep separate when looking at `git blame` or the history of a file. I would encourage every contributor to also do this. A PR is the unit of code review, but the unit of meaningful change to a repository is often much smaller. Kenn > +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 >>>>> >>>> >>> >> >
