src/docs/user/userguide/differential_inlines.diviner

   1 @title Differential User Guide: Inline Comments
   2 @group userguide
   3
   4 Guide to inline comments in Differential.
   5
   6 Overview
   7 ========
   8
   9 Differential allows reviewers to leave feedback about changes to code inline,
  10 within the body of the diff itself. These comments are called "inline
  11 comments", and can be used to discuss specific parts of a change.
  12
  13 (NOTE) Click the line number next to a line to leave an inline comment.
  14
  15 To leave an inline comment, click the line number next to a line when reviewing
  16 a change in Differential. You can also leave a comment on a range of adjacent
  17 lines by clicking one line number and dragging to a nearby line number.
  18
  19 (NOTE) Other users can't see your comments right away!
  20
  21 When you make a comment, it is initially an **unsubmitted draft**, indicated by
  22 an "Unsubmitted" badge in the comment header. Other users can't see it yet.
  23 This behavior is different from the behavior of other software you may be
  24 familiar with.
  25
  26 To publish your inline comments, scroll to the bottom of the page and submit
  27 the form there. All your unsubmitted inline comments will be published when you
  28 do, alongside an optional normal comment and optional action (like accepting
  29 the revision).
  30
  31 Differential doesn't publish inlines initially because having a draft phase
  32 gives reviewers more time to revise and adjust the inlines, and make their
  33 feedback more cohesive, and contextualize their inlines with discussion in a
  34 normal comment. It also allows Differential to send fewer, more relevant emails
  35 and notifications.
  36
  37
  38 Porting / Ghost Comments
  39 ========================
  40
  41 When a revision is updated, we attempt to port inline comments forward and
  42 display them on the new diff. Ported comments have a pale, ghostly appearance
  43 and include a button which allows you to jump back to the original context
  44 where the comment appeared.
  45
  46 Ported comments sometimes appear in unexpected locations. There are two major
  47 reasons for this:
  48
  49   - In the general case, it is not possible to always port comments to the same
  50     lines humans would automatically.
  51   - We are very aggressive about porting comments forward, even in the presence
  52     of heavy changes. This helps prevent mistakes and makes it easier to verify
  53     feedback has been addressed.
  54
  55 You can disable this behavior in
  56 {nav Settings > Diff Preferences > Show Older Inlines} if you prefer comments
  57 stay anchored to their original diff.
  58
  59 To understand why porting comments forward is difficult and can produce
  60 unexpected results, and why we choose to be aggressive about it, let's look at
  61 a case where the right behavior is ambiguous. Imagine this code is added as
  62 part of a change:
  63
  64 ```name=important.c
  65 111 ...
  66 112
  67 113 if (a() || b() || c()) {
  68 114   return;
  69 115 }
  70 116
  71 117 ...
  72 ```
  73
  74 Suppose a reviewer leaves this comment on line 113:
  75
  76 > important.c:113 This is a serious security vulnerability!
  77
  78 The author later updates the diff, and the code now looks like this, with some
  79 other changes elsewhere so the line numbers have also shifted:
  80
  81 ```name=important.c
  82 140 ...
  83 141
  84 142 if (a()) {
  85 143   return;
  86 144 }
  87 145
  88 146 if (b()) {
  89 147   return;
  90 148 }
  91 149
  92 150 ...
  93 ```
  94
  95 If we port the inline forward from the first change to the second change, where
  96 should it go? A human would probably do one of three things:
  97
  98   # Put it on line 142, with the call to `a()`.
  99   # Put it on line 146, with the call to `b()`.
 100   # Don't bring it forward at all.
 101
 102 A human would choose between (1) and (2) based on context about what `a()` and
 103 `b()` are and what the reviewer meant. The algorithm can not possibly read the
 104 comment and understand which part of the statement it talked about. Humans
 105 might not even agree on which line is the better fit.
 106
 107 When we choose one of these behaviors, humans will sometimes think the other
 108 behavior was the better one, because they have more information about what
 109 `a()` and `b()` are and what the comment actually meant. The line we choose may
 110 be unexpected, but it is the best the algorithm can do without being able to
 111 actually read the code or understand what the comment means.
 112
 113 A human might also choose not to bring the comment forward if they knew that
 114 removing `c()` addressed it, but the algorithm can not know that. The call to
 115 `a()` or `b()` may be the dangerous thing the reviewer was talking about, and
 116 we err on the side of caution by bringing comments forward aggressively.
 117
 118 When a line of code with an inline comment on it changes, we can not know if
 119 the change addressed the inline or not. We take the cautious route and
 120 //always// assume it did not, and that humans need to verify problems have
 121 really been addressed.
 122
 123 This means that inlines are sometimes ported forward into places that don't
 124 make sense (the code has changed completely), but we won't drop important
 125 inlines just because the structure of the code has changed.
 126
 127 Here's another case where bringing inlines forward seems desirable. Imagine
 128 this code is added as part of a change:
 129
 130 ```name=warpgate.c
 131 12 ...
 132 13 function_x();
 133 14 ...
 134 ```
 135
 136 Suppose a reviewer leaves this comment on line 13:
 137
 138 > warpgate.c:13 This should be function_y() instead.
 139
 140 The author later updates the diff, and the code now looks like this:
 141
 142 ```name=warpgate.c
 143 12 ...
 144 13 function_y();
 145 14 ...
 146 ```
 147
 148 For the reasons discussed above, we port the comment forward, so it will appear
 149 under line 13.
 150
 151 We think this is desirable: it makes it trivial for an author or reviewer to
 152 look through the changes and verify that the feedback has really been
 153 addressed, because you can see that the code now uses the proper function.
 154
 155 This isn't to say that we always do the best we can. There may be cases where
 156 the algorithm can be smarter than it currently is or otherwise produce a better
 157 result. If you find such a case, file a bug report. But it's expected that the
 158 algorithm will sometimes port comments into places that aren't optimal or no
 159 longer make sense, because this is frequently the best behavior available among
 160 the possible alternatives.
 161
 162
 163 Next Steps
 164 ==========
 165
 166 Continue by:
 167
 168   - returning to the @{article:Differential User Guide}.