Code review in Phabricator: Difference between revisions

From Software Heritage Wiki
Jump to navigation Jump to search
(add local review/merge instructions)
(Fix link)
Tag: Redirect target changed
 
(19 intermediate revisions by 6 users not shown)
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>




[[Category:Software development]]
[[Category:Software development]]

Latest revision as of 12:44, 31 March 2021

Redirect to:

Retrieved from "https://wiki.softwareheritage.org/index.php?title=Code_review_in_Phabricator&oldid=1551"