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