<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 10, 2017 at 9:03 PM, L. David Baron <span dir="ltr"><<a href="mailto:dbaron@dbaron.org" target="_blank">dbaron@dbaron.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Friday 2017-03-10 19:33 -0800, Eric Rescorla wrote:<br>
> We have been using Phabricator for our reviews in NSS and its interdiffs<br>
> work pretty well<br>
> (modulo rebases, which are not so great), and it's very easy to handle LGTM<br>
> with<br>
> nits and verify the nits.<br>
<br>
</span>For what it's worth, I think proper interdiffs have two columns of<br>
[ -+] at the beginning of each line, not one column like diffs do.<br>
I've gotten used to reading interdiffs as diff -u of a diff -u, and<br>
while it takes a little getting used to, but once you're used to it,<br>
it actually represents what an interdiff is and is quite usable. I<br>
think anything that pretends that something like this:<br>
<br>
// Frame has a LayerActivityProperty property<br>
FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_<wbr>PROPERTY)<br>
<br>
+ // Frame owns anonymous boxes whose style contexts it will need to update during<br>
+ // a stylo tree traversal.<br>
+ FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES)<br>
+<br>
+// If this bit is set, then reflow may be dispatched from the current<br>
+// frame instead of the root frame.<br>
-+FRAME_STATE_BIT(Generic, 55, NS_FRAME_DYNAMIC_REFLOW_ROOT)<br>
++FRAME_STATE_BIT(Generic, 56, NS_FRAME_DYNAMIC_REFLOW_ROOT)<br>
+<br>
// Set for all descendants of MathML sub/supscript elements (other than the<br>
// base frame) to indicate that the SSTY font feature should be used.<br>
FRAME_STATE_BIT(Generic, 58, NS_FRAME_MATHML_SCRIPT_<wbr>DESCENDANT)<br>
<br>
can be represented with only one column of [+- ] at the beginning<br>
is going to fail for some substantial set of important cases (such<br>
as rebases, as you mention).<br></blockquote><div><br></div><div>The systems I am most familiar with (Phabricator, Rietveld), present interdiffs</div><div>by allowing you to look at the diff between any revision of the CL (including</div><div>the base revision that's the code as-is). This works pretty well for anything other</div><div>than rebases (and is actually rather equivalent to the UI you get with</div><div>Github when people update PRs by pushing new commits onto the branch).</div><div><br></div><div>What sort of UI you want for rebases seems like a bit more of an open</div><div>question. i can imagine several things:</div><div><br></div><div>1. For simple rebases (where there's not much interaction with the surrounding</div><div>code, you probably just want to see the patch in context as if the rebase hadn't</div><div>happened.</div><div>2. For more complicated rebases (where there is real interaction with the</div><div>code that was rebased onto), you want to see the difference between</div><div>base1 + CL1 and base2 + CL2, but with the diffs that are due to the</div><div>rebase distinguished from the ones that are due to the CL1-CL2</div><div>difference (this what Phabricator does by marking the rebase artifacts</div><div>as lighter).</div><div>3. The design you suggest (which, at least for me, seems like it's</div><div>harder to read than designs where the tools provide more support).</div><div><br></div><div>It seems like designs here are evolving and it's probably going to be a</div><div>matter of personal taste, at least for a while</div><div><br></div><div>-Ekr</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
(That's a piece of interdiff from rebasing my own patch queue<br>
earlier this week.)<br>
<div class="HOEnZb"><div class="h5"><br>
-David<br>
<br>
--<br>
𝄞 L. David Baron <a href="http://dbaron.org/" rel="noreferrer" target="_blank">http://dbaron.org/</a> 𝄂<br>
𝄢 Mozilla <a href="https://www.mozilla.org/" rel="noreferrer" target="_blank">https://www.mozilla.org/</a> 𝄂<br>
Before I built a wall I'd ask to know<br>
What I was walling in or walling out,<br>
And to whom I was like to give offense.<br>
- Robert Frost, Mending Wall (1914)<br>
</div></div></blockquote></div><br></div></div>