Difference between revisions of "Code review in Phabricator"

Latest revision as of 12:44, 31 March 2021

Redirect to:

swhdocs:devel/contributing/phabricator.html#patch-submission

@@ Line 1: / Line 1: @@
-We use the [[Differential]] application of [[Phabricator]] to do code reviews in the context of [[Software Heritage]].
+#REDIRECT [[swhdocs:devel/contributing/phabricator.html#patch-submission]]
-* we use Git and history.immutable=true (but beware as that is partly a Phabricator misnomer, read on)
-* when code reviews are required, developers will be allowed to push directly to master once an accepted Differential diff exists
-== Configuration ==
-When using git, [[Arcanist]] by default mess with the local history, rewriting commits at the time of first submission.<br />
-To avoid that we use so called [https://secure.phabricator.com/book/phabricator/article/arcanist_new_project/#history-mutability-git history immutability].
-To that end, you shall configure your <tt>arc</tt> accordingly:
-<pre>
-arc set-config history.immutable true
-</pre>
-Note that this does ''not'' mean that you are forbidden to rewrite your local branches (e.g., with <tt>git rebase</tt>).
-Quite the contrary: you are encouraged to locally rewrite branches before pushing to ensure that commits are logically separated and your commit history easy to bisect.
-The above setting just means that ''arc'' will not rewrite commit history under your nose.
-== Workflow ==
-'''TL;DR:'''
-* work in a feature branch: <tt>git checkout -b my-feat</tt>
-* initial review request: hack/commit/hack/commit ; <tt>arc diff origin/master</tt>
-* react to change requests: hack/commit/hack/commit ; <tt>arc diff --update Dxx origin/master</tt>
-* landing change: <tt>git checkout master ; git merge my-feat ; git push</tt>
-=== Starting a new feature and submit it for review ===
-Use a '''one branch per feature''' workflow, with well-separated ''logical commits'':
-<pre>
-git checkout -b my-shiny-feature
-... hack hack hack ...
-git commit -m 'architecture skeleton for my-shiny-feature'
-... hack hack hack ...
-git commit -m 'my-shiny-feature: implement module foo'
-... etc ...
-</pre>
-To '''submit your code for review''' the first time:
-<pre>
-arc diff origin/master
-</pre>
-arc will prompt for a '''code review message'''. Provide the following information:
-* first line: ''short description'' of the overall work (i.e., the feature you're working on). This will become the title of the review
-* ''Summary'' field (optional): ''long description'' of the overall work; the field can continue in subsequent lines, up to the next field. This will become the "Summary" section of the review
-* ''Test Plan'' field (optional): write here if something special is needed to test your change
-* ''Reviewers'' field (optional): the (Phabricator) name(s) of desired reviewers. If you don't specify one (recommended) the default reviewers will be chosen
-* ''Subscribers'' field (optional): the (Phabricator) name(s) of people that will be notified about changes to this review request. In most cases it should be left empty
-For example:
-<pre>
-mercurial loader
-Summary: first stab at a mercurial loader (T329)
-The implementation follows the plan detailed in F2F discussion with @foo.
-Performances seem decent enough for a first trial (XXX seconds for YYY repository
-that contains ZZZ patches).
-Test plan:
-Reviewers:
-Subscribers: foo
-</pre>
-After completing the message arc will submit the review request and tell you its number and URL:
-<pre>
-[...]
-Created a new Differential revision:
-        Revision URI: https://forge.softwareheritage.org/Dxx
-</pre>
-=== Updating your branch to reflect requested changes ===
-Your feature might get accepted as is, YAY!
-Or, reviewers might request changes; no big deal!
-Use the Differential web UI to follow-up to received comments, if needed.
-To implement requested changes in the code, hack on your branch as usual by:
-* adding new commits, and/or
-* rewriting old commits with git rebase (to preserve a nice, easy to bisect history)
-When you're ready to '''update your review request''':
-<pre>
-arc diff --update Dxx origin/master
-</pre>
-Arc will prompt you for a message: describe what you've changed w.r.t. the previous review request, free form.
-Your message will become the changelog entry in Differential for this new version of the diff.
-Differential only care about the code diff, and not about the commits or their order.
-Therefore each "update" can be a completely different series of commits, possibly rewritten from the previous submission.
-=== Landing your change onto master ===
-Once your change has been approved in Differential, you will be able to land it onto the master branch.
-Before doing so, you're encouraged to '''clean up your git commit history''', reordering/splitting/merging commits as needed to have separate logical commits and an easy to bisect history.
-The correspondence between the accepted review and pushed code is checked by looking only at the code diff, so commit fiddling will not impact your ability to push to master.
-Once you're happy you can '''push to origin/master''' directly, e.g.:
-<pre>
-git checkout master
-git merge my-shiny-feature
-git push
-</pre>
-Optionally you can then delete your local feature branch:
-<pre>
-git branch -d my-shiny-feature
-</pre>
-=== Reviewing locally / landing someone else's changes ===
-You can do local reviews of code with arc patch:
-<pre>
-arc patch Dxyz
-</pre>
-This will create a branch '''arcpatch-Dxyz''' containing the changes on your local checkout.
-You can then merge those changes upstream with
-<pre>
-git checkout master
-git merge --ff arcpatch-Dxyz
-git push origin master
-</pre>
-== See also ==
-* [[Code review]] for guidelines on how code is reviewed when developing for Software Heritage
 [[Category:Software development]]

Difference between revisions of "Code review in Phabricator"

Latest revision as of 12:44, 31 March 2021

Navigation menu

Search