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 <[email protected]> wrote: > On Tue, Nov 28, 2017 at 11:47 AM, Thomas Weise <[email protected]> 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 <[email protected]> wrote: >> >>> On Tue, Nov 28, 2017 at 11:16 AM, Raghu Angadi <[email protected]> >>> 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 <[email protected]> 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 <[email protected]> >>>>> wrote: >>>>> >>>>>> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers <[email protected]> >>>>>> 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 <[email protected]> >>>>>>> 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 >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >>
