Difference between revisions of "Code review in Phabricator"

Revision as of 07:52, 18 March 2021

This page moved to the main developer documentation

@@ Line 1: / Line 1: @@
-We use the [[Differential]] application of [[Phabricator]] to perform [[code review|code reviews]] in the context of [[Software Heritage]].
+This page [https://docs.softwareheritage.org/devel/contributing/code-review.html#code-review moved to the main developer documentation]
-* 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>).
-If you still get a 403 error on push, this means you need a forge administrator to enable HTTPS pushes for the repository (which wasn't done by default in historical repositories). Please drop by on IRC and let us know!
-== 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'' ([https://wiki.softwareheritage.org/wiki/Git_style_guide following those conventions]).
-Please open one diff per logical commit to keep the diff size to a minimum.
-<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>
-Please, follow the
-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)
-* pulling on master and rebasing your branch against it if meanwhile someone landed commits on master:
-<pre>
-git checkout master
-git pull
-git checkout my-shiny-feature
-git rebase master
-</pre>
-When you're ready to '''update your review request''':
-<pre>
-arc diff --update Dxx HEAD~
-</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.
-=== Dependencies between diffs ===
-Note that you can manage diff dependencies within the same module with the following keyword in the diff description:
-<pre>
-Depends on Dxx
-</pre>
-That allows to keep a logical view in your diff.
-It's not strictly necessary (because the tooling now deals with it properly) but it might help reviewers or yourself to do so.
-=== 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.
-Update the diff [https://wiki.softwareheritage.org/wiki/Code_review_in_Phabricator#Updating_your_branch_to_reflect_requested_changes following the prior section].
-(It'd be good to let the ci build finish to make sure everything is still green).
-Once you're happy you can '''push to origin/master''' directly, e.g.:
-<pre>
-git checkout master
-git merge --ff-only my-shiny-feature
-git push
-</pre>
-<code>--ff-only</code> is optional, and makes sure you don't unintentionally create a merge commit.
-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>
-or, alternatively:
-<pre>
-arc land --squash
-</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"

Revision as of 07:52, 18 March 2021

Navigation menu

Search