I think I agree with Kenn on the "merge question": - There should be a merge commit because this records important information, for example, I like having the option of figuring out what PR certain commits came from - Individual meaningful commits of a PR should be preserved, I think having commits as small as possible is nice and the git history tells a story of where the code came from - fixup commits should be squashed
The question of whether to keep or squash commits could also be solved by enforcing 1 PR = 1 commit and making people open several PRs where they would previously open one PR with several distinct and meaningful commits. This might introduce quite some overhead, though. Best, Aljoscha > On 29. Nov 2017, at 09:40, Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > > Hi, > > I don't see why gitbox merge button change what we are doing. > > I agree with Kenn for 1 (reviewer field) & 2 (assignee field). > > IMHO, for 3, I think the reviewer should only use rebase & merge. The squash > should be under the contributor scope. The reviewer can ask to squash some > commits, but he should not do it himself (the contributor should update the > PR with the squashes). > > My $0.01 ;) > > Regards > JB > > On 11/28/2017 06:45 PM, Kenneth Knowles 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 > > -- > Jean-Baptiste Onofré > jbono...@apache.org > http://blog.nanthrax.net > Talend - http://www.talend.com
