|
(Tag: Redirect target changed) |
(20 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>
| |
| | | |
| | | |
| [[Category:Software development]] | | [[Category:Software development]] |