Page MenuHomePhorge
Files F2893001

No OneTemporary
Actions

View Options

diff --git a/src/docs/user/userguide/differential.diviner b/src/docs/user/userguide/differential.diviner
index edf992f720..414ad87a4f 100644
--- a/src/docs/user/userguide/differential.diviner
+++ b/src/docs/user/userguide/differential.diviner
@@ -1,68 +1,73 @@
@title Differential User Guide
@group userguide
Guide to the Differential (pre-push code review) tool and workflow.
= Overview =
Phabricator supports two code review workflows, "review" (pre-push) and
"audit" (post-push). To understand the differences between the two, see
@{article:User Guide: Review vs Audit}.
This document summarizes the pre-push "review" workflow implemented by the tool
//Differential//.
= How Review Works =
Code review in Phabricator is a lightweight, asynchronous web-based process. If
you are familiar with GitHub, it is similar to how pull requests work:
- An author prepares a change to a codebase, then sends it for review. They
specify who they want to review it (additional users may be notified as
well, see below). The change itself is called a "Differential Revision".
- The reviewers receive an email asking them to review the change.
- The reviewers inspect the change and either discuss it, approve it, or
request changes (e.g., if they identify problems or bugs).
- In response to feedback, the author may update the change (e.g., fixing
the bugs or addressing the problems).
- Once everything is satisfied, some reviewer accepts the change and the
author pushes it to the upstream.
The Differential home screen shows two sets of revisions:
- **Action Required** is revisions you are the author of or a reviewer for,
which you need to review, revise, or push.
- **Waiting on Others** is revisions you are the author of or a reviewer for,
which someone else needs to review, revise, or push.
= Creating Revisions =
The preferred way to create revisions in Differential is with `arc`
(see @{article:Arcanist User Guide}). You can also create revisions from the
web interface, by navigating to Differential, pressing the "Create Revision"
button, and pasting a diff in.
= Herald Rules =
If you're interested in keeping track of changes to certain parts of a codebase
(e.g., maybe changes to a feature or changes in a certain language, or there's
just some intern who you don't trust) you can write a Herald rule to
automatically CC you on any revisions which match rules (like content, author,
files affected, etc.)
-= Differential Tips =
+Inline Comments
+===============
- - You can leave inline comments by clicking the line numbers in the diff.
- - You can leave a comment across multiple lines by dragging across the line
- numbers.
- - Inline comments are initially saved as drafts. They are not submitted until
- you submit a comment at the bottom of the page.
- - Press "?" to view keyboard shortcuts.
+You can leave inline comments by clicking the line number next to a line. For
+an in-depth look at inline comments, see
+@{article:Differential User Guide: Inline Comments}.
-= Next Steps =
- - Read the FAQ at @{article:Differential User Guide: FAQ}; or
- - learn about handling large changesets at
+Next Steps
+==========
+
+Continue by:
+
+ - diving into the details of inline comments in
+ @{article:Differential User Guide: Inline Comments}; or
+ - reading the FAQ at @{article:Differential User Guide: FAQ}; or
+ - learning about handling large changesets in
@{article:Differential User Guide: Large Changes}; or
- - learn about test plans at @{article:Differential User Guide: Test Plans}; or
- - learn more about Herald at @{article:Herald User Guide}.
+ - learning about test plans in
+ @{article:Differential User Guide: Test Plans}; or
+ - learning more about Herald in @{article:Herald User Guide}.
diff --git a/src/docs/user/userguide/differential_inlines.diviner b/src/docs/user/userguide/differential_inlines.diviner
new file mode 100644
index 0000000000..80ac9891ed
--- /dev/null
+++ b/src/docs/user/userguide/differential_inlines.diviner
@@ -0,0 +1,168 @@
+@title Differential User Guide: Inline Comments
+@group userguide
+
+Guide to inline comments in Differential.
+
+Overview
+========
+
+Differential allows reviewers to leave feedback about changes to code inline,
+within the body of the diff itself. These comments are called "inline
+comments", and can be used to discuss specific parts of a change.
+
+(NOTE) Click the line number next to a line to leave an inline comment.
+
+To leave an inline comment, click the line number next to a line when reviewing
+a change in Differential. You can also leave a comment on a range of adjacent
+lines by clicking one line number and dragging to a nearby line number.
+
+(NOTE) Other users can't see your comments right away!
+
+When you make a comment, it is initially an **unsubmitted draft**, indicated by
+an "Unsubmitted" badge in the comment header. Other users can't see it yet.
+This behavior is different from the behavior of other software you may be
+familiar with.
+
+To publish your inline comments, scroll to the bottom of the page and submit
+the form there. All your unsubmitted inline comments will be published when you
+do, alongside an optional normal comment and optional action (like accepting
+the revision).
+
+Differential doesn't publish inlines initially because having a draft phase
+gives reviewers more time to revise and adjust the inlines, and make their
+feedback more cohesive, and contextualize their inlines with discussion in a
+normal comment. It also allows Differential to send fewer, more relevant emails
+and notifications.
+
+
+Porting / Ghost Comments
+========================
+
+When a revision is updated, we attempt to port inline comments forward and
+display them on the new diff. Ported comments have a pale, ghostly appearance
+and include a button which allows you to jump back to the original context
+where the comment appeared.
+
+Ported comments sometimes appear in unexpected locations. There are two major
+reasons for this:
+
+ - In the general case, it is not possible to always port comments to the same
+ lines humans would automatically.
+ - We are very aggressive about porting comments forward, even in the presence
+ of heavy changes. This helps prevent mistakes and makes it easier to verify
+ feedback has been addressed.
+
+You can disable this behavior in
+{nav Settings > Diff Preferences > Show Older Inlines} if you prefer comments
+stay anchored to their original diff.
+
+To understand why porting comments forward is difficult and can produce
+unexpected results, and why we choose to be aggressive about it, let's look at
+a case where the right behavior is ambiguous. Imagine this code is added as
+part of a change:
+
+```name=important.c
+111 ...
+112
+113 if (a() || b() || c()) {
+114 return;
+115 }
+116
+117 ...
+```
+
+Suppose a reviewer leaves this comment on line 113:
+
+> important.c:113 This is a serious security vulnerability!
+
+The author later updates the diff, and the code now looks like this, with some
+other changes elsewhere so the line numbers have also shifted:
+
+```name=important.c
+140 ...
+141
+142 if (a()) {
+143 return;
+144 }
+145
+146 if (b()) {
+147 return;
+148 }
+149
+150 ...
+```
+
+If we port the inline forward from the first change to the second change, where
+should it go? A human would probably do one of three things:
+
+ # Put it on line 142, with the call to `a()`.
+ # Put it on line 146, with the call to `b()`.
+ # Don't bring it forward at all.
+
+A human would choose between (1) and (2) based on context about what `a()` and
+`b()` are and what the reviewer meant. The algorithm can not possibly read the
+comment and understand which part of the statement it talked about. Humans
+might not even agree on which line is the better fit.
+
+When we choose one of these behaviors, humans will sometimes think the other
+behavior was the better one, because they have more information about what
+`a()` and `b()` are and what the comment actually meant. The line we choose may
+be unexpected, but it is the best the algorithm can do without being able to
+actually read the code or understand what the comment means.
+
+A human might also choose not to bring the comment forward if they knew that
+removing `c()` addressed it, but the algorithm can not know that. The call to
+`a()` or `b()` may be the dangerous thing the reviewer was talking about, and
+we err on the side of caution by bringing comments forward aggressively.
+
+When a line of code with an inline comment on it changes, we can not know if
+the change addressed the inline or not. We take the cautious route and
+//always// assume it did not, and that humans need to verify problems have
+really been addressed.
+
+This means that inlines are sometimes ported forward into places that don't
+make sense (the code has changed completely), but we won't drop important
+inlines just because the structure of the code has changed.
+
+Here's another case where bringing inlines forward seems desirable. Imagine
+this code is added as part of a change:
+
+```name=warpgate.c
+12 ...
+13 function_x();
+14 ...
+```
+
+Suppose a reviewer leaves this comment on line 13:
+
+> warpgate.c:13 This should be function_y() instead.
+
+The author later updates the diff, and the code now looks like this:
+
+```name=warpgate.c
+12 ...
+13 function_y();
+14 ...
+```
+
+For the reasons discussed above, we port the comment forward, so it will appear
+under line 13.
+
+We think this is desirable: it makes it trivial for an author or reviewer to
+look through the changes and verify that the feedback has really been
+addressed, because you can see that the code now uses the proper function.
+
+This isn't to say that we always do the best we can. There may be cases where
+the algorithm can be smarter than it currently is or otherwise produce a better
+result. If you find such a case, file a bug report. But it's expected that the
+algorithm will sometimes port comments into places that aren't optimal or no
+longer make sense, because this is frequently the best behavior available among
+the possible alternatives.
+
+
+Next Steps
+==========
+
+Continue by:
+
+ - returning to the @{article:Differential User Guide}.

File Metadata

Mime Type
text/x-diff
Expires
Sun, Jan 19, 17:41 (2 w, 5 d ago)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
1127071
Default Alt Text
(10 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0