際際滷

際際滷Share a Scribd company logo

Code review vs pull request

?
?
Bryan Liu
Bryan Liu

This document discusses code reviews and pull requests/merge requests. It notes that large diffs are difficult to review due to issues spotting bugs and reverting or merging changes. It recommends avoiding overloading or overcrowding reviews by keeping changes concise, contained, and guided. The document describes an Arc workflow for code reviews using Phabricator and compares post-push audits to pre-push reviews. It also discusses benefits of rebasing vs merging and integrating Phabricator with Jenkins.

1 of 33
Downloaded 24 times
Code Reviews vs. Merge Requests - Continuous Code Review
Why Phabricator ? Tool is not our focus today, process is ? Git?ow (gitlab, github) is not TBD friendly ? Use PR/MR for (post-commit) pre-push code review is hard
Why Code Review It¨s not just for catching bugs ´
Ef?ciency
But How to do it more ef?ciently ´
Large Diffs Hurts (I) ? Large diffs won¨t get reviewed ? It¨s hard to spot bugs in large diffs
Code review vs pull request
src: http://blogs.atlassian.com/2011/07/creating_optimal_reviews/
Large Diffs Hurts (II) ? Large diffs won¨t get reviewed ? It¨s hard to spot bugs in large diffs ? Reverting large diffs is hard src: https://medium.com/@kurtisnusbaum/large-diffs-are-hurting-your-ability-to-ship- e0b2b41e8acf#.3l2trqfsr
git commit -m "?xed issue with fan" [Pull Request code review]
Large Diffs Hurts (III) ? Large diffs won¨t get reviewed ? It¨s hard to spot bugs in large diffs ? Reverting large diffs is hard ? Rebasing/merging large diffs is error-prone
Code review vs pull request
TBD
The Right Direction Avoid overloading ? concise C don¨t let cruft get in the way ? contained C if there are two parts, create two reviews ? guided C tell people how to review it
The Right Direction Avoid overcrowding ? selected with care C don¨t blindly choose reviewers ? focused C if its a specialist change, choose a specialist dev ? guided C tell people individually what they should look out for
Arc Work?ow git co -b <branchname> <edit edit edit> git commit arc diff --rr reviewer1,reviewer2 [--cc person3] --verbatim <edit based on review feedback> git commit arc diff <maybe run git pull if you want some updates> <edit + commit + arc diff some more> arc land
Arc Work?ow <edit edit edit> No new branch git commit arc diff --rr reviewer1,reviewer2 [--cc person3] --verbatim <edit based on review feedback> git commit arc diff <maybe run git pull if you want some updates> <edit + commit + arc diff some more> arc amend git push
Code Review vs. Push ? Differential: Pre-push code review ? Diffusion/Audit: Post-push audit (non-blocking) ? User Guide: Review vs Audit
Post-push Audit <edit edit edit> git commit # put 'Auditors: foo,bar,baz' in the commit message git push Fix the foobaz test now that we no longer frob our foos. I can't believe I forgot to run this test when I changed the foo-frobbing code! Auditors: chris, james Test plan: tools/runtests.py foobaz_test.py
Rebase vs Merge ? The major bene?t of rebasing is that you get a much cleaner project history. First, it eliminates the unnecessary merge commits required by git merge. Second, as you can see in the above diagram, rebasing also results in a perfectly linear project history ！you can follow the tip of feature all the way to the beginning of the project without any forks. This makes it easier to navigate your project with commands like git log, git bisect, and gitk. ? But, there are two trade-offs for this pristine commit history: safety and traceability. If you don¨t follow the Golden Rule of Rebasing, re-writing project history can be potentially catastrophic for your collaboration work?ow. And, less importantly, rebasing loses the context provided by a merge commit！you can¨t see when upstream changes were incorporated into the feature.
Merge PR ? Branches ? Merge con?icts ? Guitar Hero
Rebase ? linearized, rebased, fast-forward commits is the default behavior of Phabricator.
Bene?ts code evolution history vs commit logs
Import Gitlab Repo
Community Resources Arcanist-support: additional testing and linting engines ? SBTTestEngine: Runs unit tests for Scala using SBT ? Scalastyle Lint Engine: Runs Scalastyle linter Phabricator Jenkins Plugin ? Integrates with Phabricator's Differential and Harbormaster apps
Jenkins Integration
Current Work?ow
Diff Build
Diff by Commit vs master
Other Usage Examples Test plan of a diff: ? https://secure.phabricator.com/D16974 Phab TBD vs wikimedia branches model: ? https://secure.phabricator.com/source/libphutil/repository/master/ ? https://phabricator.wikimedia.org/diffusion/ECOE/repository/master/ Diff with feature-toggle example: ? https://secure.phabricator.com/D17200 Use Phab wiki for release note example: ? https://secure.phabricator.com/w/changelog/2016.49/
Phabricator vs. GitHub vs. Atlassian ? https://rekinyz.wordpress.com/2015/01/18/ phabricator/
Goal ? Test driven ? Code review small and early ? Test before delivery ? Continuous Improvement
Reference ? Writing Reviewable Code (Facebook)!! ? Pull Requests vs Code Reviews ? Branch recommendation in Phabricator ? Using phabricator guide [khanacademy] ? Phabricator Project Mgmt Example

More Related Content

Code review vs pull request

  • 1. Code Reviews vs. Merge Requests - Continuous Code Review
  • 2. Why Phabricator ? Tool is not our focus today, process is ? Git?ow (gitlab, github) is not TBD friendly ? Use PR/MR for (post-commit) pre-push code review is hard
  • 3. Why Code Review It¨s not just for catching bugs ´
  • 5. But How to do it more ef?ciently ´
  • 6. Large Diffs Hurts (I) ? Large diffs won¨t get reviewed ? It¨s hard to spot bugs in large diffs
  • 9. Large Diffs Hurts (II) ? Large diffs won¨t get reviewed ? It¨s hard to spot bugs in large diffs ? Reverting large diffs is hard src: https://medium.com/@kurtisnusbaum/large-diffs-are-hurting-your-ability-to-ship- e0b2b41e8acf#.3l2trqfsr
  • 10. git commit -m "?xed issue with fan" [Pull Request code review]
  • 11. Large Diffs Hurts (III) ? Large diffs won¨t get reviewed ? It¨s hard to spot bugs in large diffs ? Reverting large diffs is hard ? Rebasing/merging large diffs is error-prone
  • 13. TBD
  • 14. The Right Direction Avoid overloading ? concise C don¨t let cruft get in the way ? contained C if there are two parts, create two reviews ? guided C tell people how to review it
  • 15. The Right Direction Avoid overcrowding ? selected with care C don¨t blindly choose reviewers ? focused C if its a specialist change, choose a specialist dev ? guided C tell people individually what they should look out for
  • 16. Arc Work?ow git co -b <branchname> <edit edit edit> git commit arc diff --rr reviewer1,reviewer2 [--cc person3] --verbatim <edit based on review feedback> git commit arc diff <maybe run git pull if you want some updates> <edit + commit + arc diff some more> arc land
  • 17. Arc Work?ow <edit edit edit> No new branch git commit arc diff --rr reviewer1,reviewer2 [--cc person3] --verbatim <edit based on review feedback> git commit arc diff <maybe run git pull if you want some updates> <edit + commit + arc diff some more> arc amend git push
  • 18. Code Review vs. Push ? Differential: Pre-push code review ? Diffusion/Audit: Post-push audit (non-blocking) ? User Guide: Review vs Audit
  • 19. Post-push Audit <edit edit edit> git commit # put 'Auditors: foo,bar,baz' in the commit message git push Fix the foobaz test now that we no longer frob our foos. I can't believe I forgot to run this test when I changed the foo-frobbing code! Auditors: chris, james Test plan: tools/runtests.py foobaz_test.py
  • 20. Rebase vs Merge ? The major bene?t of rebasing is that you get a much cleaner project history. First, it eliminates the unnecessary merge commits required by git merge. Second, as you can see in the above diagram, rebasing also results in a perfectly linear project history ！you can follow the tip of feature all the way to the beginning of the project without any forks. This makes it easier to navigate your project with commands like git log, git bisect, and gitk. ? But, there are two trade-offs for this pristine commit history: safety and traceability. If you don¨t follow the Golden Rule of Rebasing, re-writing project history can be potentially catastrophic for your collaboration work?ow. And, less importantly, rebasing loses the context provided by a merge commit！you can¨t see when upstream changes were incorporated into the feature.
  • 21. Merge PR ? Branches ? Merge con?icts ? Guitar Hero
  • 22. Rebase ? linearized, rebased, fast-forward commits is the default behavior of Phabricator.
  • 25. Community Resources Arcanist-support: additional testing and linting engines ? SBTTestEngine: Runs unit tests for Scala using SBT ? Scalastyle Lint Engine: Runs Scalastyle linter Phabricator Jenkins Plugin ? Integrates with Phabricator's Differential and Harbormaster apps
  • 29. Diff by Commit vs master
  • 30. Other Usage Examples Test plan of a diff: ? https://secure.phabricator.com/D16974 Phab TBD vs wikimedia branches model: ? https://secure.phabricator.com/source/libphutil/repository/master/ ? https://phabricator.wikimedia.org/diffusion/ECOE/repository/master/ Diff with feature-toggle example: ? https://secure.phabricator.com/D17200 Use Phab wiki for release note example: ? https://secure.phabricator.com/w/changelog/2016.49/
  • 31. Phabricator vs. GitHub vs. Atlassian ? https://rekinyz.wordpress.com/2015/01/18/ phabricator/
  • 32. Goal ? Test driven ? Code review small and early ? Test before delivery ? Continuous Improvement
  • 33. Reference ? Writing Reviewable Code (Facebook)!! ? Pull Requests vs Code Reviews ? Branch recommendation in Phabricator ? Using phabricator guide [khanacademy] ? Phabricator Project Mgmt Example