+1 to Kenneth proposal, using reviewer and asignee, for the merge strategy (a) +1 with the same arguments (preserving commits when they are meaningful and isolated, ask committers to do extra squash if needed.
I don't really favor having one big commit per PR (in particular if the change is big) because you lose information with this approach. We should encourage contributors to do meaningful and isolated commits, of course if this is not the case then we can go and squash them, but this is something to review per case. Regards, Ismaël On Wed, Nov 29, 2017 at 4:37 PM, Aljoscha Krettek <[email protected]> wrote: > 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é <[email protected]> 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é >> [email protected] >> http://blog.nanthrax.net >> Talend - http://www.talend.com >
