The future of commit access policy for core Firefox

Eric Rescorla ekr at rtfm.com
Sun Mar 12 12:18:23 UTC 2017


On Fri, Mar 10, 2017 at 9:03 PM, L. David Baron <dbaron at 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)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/firefox-dev/attachments/20170312/1ed2738d/attachment.html>


More information about the firefox-dev mailing list