I prefer either (a) or (b) depending on the content of the commit history. Some PRs have well-curated, useful distinct commits, whereas others consist mostly of fixups and other minor changes that are better squashed.
On Tue, Nov 28, 2017 at 11:47 AM, Thomas Weise <t...@apache.org> wrote: > > (a) -0 due to extra noise in the commit log > (b) -1 (as standard/default) this should be done by contributor as there may > be few situation where individual commits should be preserved > (c) +1 the rebase will also record the committer (which would be merge > commit author otherwise) > > In general the process should result in "merged" status for a merged PR as > opposed to "closed" as seen often currently. > > Thanks, > Thomas > > > > On Tue, Nov 28, 2017 at 11:23 AM, Kenneth Knowles <k...@google.com> wrote: >> >> 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 >>>>> >>>>> >>>> >>> >> >
