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 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>
