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 perform [[code review|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 ==
-=== Arcanist 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.
-=== Enabling <tt>git push</tt> to our forge ===
-The way we've configured our review setup for continuous integration needs you to configure git to allow pushes to our forge. There's two ways you can do this : setting a ssh key to push over ssh, or setting a specific password for git pushes over https.
-==== SSH key for pushes ====
-In your forge User settings page (On the top right, click on your avatar, then click ''Settings''), you have access to a ''Authentication'' > ''SSH Public Keys'' section (Direct link: <tt>hxxps://forge.softwareheritage.org/settings/user/'''<your username>'''/page/ssh/</tt>). You then have the option to upload a SSH public key, which will authenticate your pushes.
-You then need to configure ssh/git to use that key pair, for instance by editing the <tt>~/.ssh/config</tt> file.
-Finally, you should configure git to push over ssh when pushing to https://forge.softwareheritage.org, by running the following command:
- git config --global url.git@forge.softwareheritage.org:.pushInsteadOf https://forge.softwareheritage.org
-This lets git know that it should use <tt>git@forge.softwareheritage.org:</tt> as a base url when pushing repositories cloned from forge.softwareheritage.org over https.
-==== VCS password for pushes ====
-If you're not comfortable setting up SSH to upload your changes, you have the option of setting a VCS password. This password, ''separate from your account password'', allows Phabricator to authenticate your uploads over HTTPS.
-In your forge User settings page (On the top right, click on your avatar, then click ''Settings''), you need to use the ''Authentication'' > ''VCS Password'' section to set your VCS password (Direct link: <tt>hxxps://forge.softwareheritage.org/settings/user/'''<your username>'''/page/vcspassword/</tt>).
-== Workflow ==
-* 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