On 11/17/2017 08:45 AM, Michael Beckerle (Confluence) wrote: > <https://cwiki.apache.org/confluence/display/~mbeckerle?src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147> > > Michael Beckerle *commented* on a page > > comment icon > <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?focusedCommentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=view#comment-74687418> > > > > Re: Code Contributor Workflow > <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?focusedCommentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=view#comment-74687418> > > > > I want to fold these learnings into the flow above. > > Many of our patch sets are large. We're still refactoring the internals of > Daffodil to improve maintainability and performance. So a patch set might > modify > 80 files. Generally review for large sets of changes like this requires > several > iterations of a review-fix cycle. > > So what we learned is that each time you want code reviewed, you want to push > (without --force) a single separate new commit to your branch. This commit > should not be squashed together with any commit from a prior review, but > should > squash together all commits for changes since the prior review. Your work may > go > through several cycles of commit, push, get review comments, make changes to > respond to them (doing local commits as often as you want), squash local > commmits into a "next review commit", push, repeat. > > Let's say your review-fix cycle takes 3 iterations. Then at the end there > should > be 3 commits on that branch that become part of the pull-request for review. > Each commit gathers comments as part of reviewing, and the response to those > comments is a new separate commit on the branch. > > Many developers use a "commit often" discipline. Those commits are to your > local > clone of your fork repository. Commit as often as you want there. When it is > time to code review, squash all the commits together into a single commit of > changes *since the prior review*. > > * It's very important that each time you add a new commit for review, that > it > be separate, not squashed into anything already reviewed before it. This > preserves the commentary on prior commits for your pull request. It allows > reviewers to see how your new changes addressed the prior comments. > * It's very important that you will never need to "git push --force ...." > anything during the review-fix cycle. If you do, you have done something > wrong - like squashed reviewed-commits together with post-review ones. > > When review comments from two reviewers come back +1, then it is time to > incorporate the change into the master branch. > > At this point, squash together all the review commits so you have one commit. > > This one you must 'git push --force' to your branch on origin (aka your fork > repo). > > * Note: doing this will make it impossible to revisit review comments from > the > commits that have been squashed together - basically the commentary is > lost > once the commit is squashed together. > > That code-review UI where you can see changes and comments interleaved into > the > code,... that UI no longer can retrieve those comments once the commits they > are > on have been squashed together. > > That means review comments are *not a matter of permanent record* - though > every > one is sent to the dev mailing list, so they're recorded in that way, but you > won't be able to open the pull request and revisit comments on the individual > review commits any longer. > > This means we need to follow this policy: > > * If a comment contains any description/discussion that wants to be > maintained/remembered, it should be edited into a comment in the code > (perhaps with a TODO or FIXME tag so it's easy to find.) > > That means when doing code review - it's useful to remind contributors to put > things into code comments. > > Reply Icon > <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?replyToComment=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=reply#comment-74687418> > > Reply > <https://cwiki.apache.org/confluence/display/DAFFODIL/Code+Contributor+Workflow?replyToComment=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=reply#comment-74687418> > > • > > Like Icon > <https://cwiki.apache.org/confluence/plugins/likes/like.action?contentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=like> > > Like > <https://cwiki.apache.org/confluence/plugins/likes/like.action?contentId=74687418&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=like> > > > > Stop watching page > <https://cwiki.apache.org/confluence/users/removepagenotification.action?pageId=74683912&src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=stop-watching> > > • > > Manage notifications > <https://cwiki.apache.org/confluence/users/editmyemailsettings.action?src=mail&src.mail.timestamp=1510926334067&src.mail.notification=com.atlassian.confluence.plugins.confluence-content-notifications-plugin%3Acomment-created-notification&src.mail.recipient=8aa980875ebf6d24015f106394930147&src.mail.action=manage> > > > > > Confluence logo big > > This message was sent by Atlassian Confluence 5.8.17 >
I think this is a really important note about how to do code reviews, probably belongs either in the Code Contributor Workflow (not just a comment, people may not see it) or a new page about how to do reviews. I think this can all be summarized into a few bullet points about code reviews and fit in the workflow page somewhere.
