SGTM then, my new vote is +1 to (a) as it preserves the most information, just the reviewer need to keep in mind to tell the author to squash the noise commit before merge.
On Wed, Nov 29, 2017 at 12:09 PM Kenneth Knowles <k...@google.com> wrote: > Let's assume that when I say (a) the author has arranged commits to be > meaningful. That's what I meant to say in each of my descriptions of the > option. If they are noise, it doesn't apply. > > On Tue, Nov 28, 2017 at 8:04 PM, James <xumingmi...@gmail.com> wrote: > >> Thanks Kenn for bring up this expanded discussion, my vote is: >> >> (a) -1 this preserves noise log like 'fix review comments' >> (b) +0 this keeps the commit log clean, but without a rebase >> (c) -1 similar to option a), it preserves noise log like 'fix review >> comments' >> >> My ideal option is the current manual merge process: `rebase + squash`, >> maybe we should consider introducing mergebot? >> >> >> On Wed, Nov 29, 2017 at 4:01 AM Raghu Angadi <rang...@google.com> wrote: >> >>> 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 >>>> >>> >>> It is better to preserve the commit history of the PR at least in the >>> committer branch (and PR). >>> In addition having to force push squashed commit to remote git branch >>> each time is quite painful. If we squash at all, final merge into master >>> seems like the best place. >>> >>> >>>> (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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >
