[PATCH] color: labeled text should be passed to ui.write() as ui.labeled

[Matt Mackall]

On Tue, 2010-06-08 at 19:11 -0500, Steve Borho wrote:
> On Tue, Jun 8, 2010 at 6:15 PM, Matt Mackall <mpm at selenic.com> wrote:
> > On Tue, 2010-06-08 at 17:28 -0500, Steve Borho wrote:
> >> On Tue, Jun 8, 2010 at 4:42 PM, Matt Mackall <mpm at selenic.com> wrote:
> >> > On Tue, 2010-06-08 at 16:32 -0500, Steve Borho wrote:
> >> >> On Tue, Jun 8, 2010 at 3:49 PM, Steve Borho <steve at borho.org> wrote:
> >> >> > On Tue, Jun 8, 2010 at 3:44 PM, Matt Mackall <mpm at selenic.com> wrote:
> >> >> >> On Thu, 2010-06-03 at 23:53 -0500, Steve Borho wrote:
> >> >> >>> # HG changeset patch
> >> >> >>> # User Steve Borho <steve at borho.org>
> >> >> >>> # Date 1275625098 18000
> >> >> >>> # Node ID ce4224afddd4e2c3c3a138d0fdec001fe0dbd5ef
> >> >> >>> # Parent  e581f3acc3385da5d5ddf547882f274d9f7e5b6d
> >> >> >>> color: labeled text should be passed to ui.write() as ui.labeled
> >> >> >>>
> >> >> >>> Some implementations of ui.label() (HTML versions in particular) must escape
> >> >> >>> the provided text and then markup the text with their tags.  When this marked
> >> >> >>> up text is then passed to ui.write(), we must label the text as 'ui.labeled'
> >> >> >>> so the implementation knows not to escape it a second time (exposing the initial
> >> >> >>> markup).
> >> >> >>>
> >> >> >>> This required the addition of a 'ui.plain' label for text that is purposefully
> >> >> >>> not marked up.
> >> >> >>>
> >> >> >>> I was a little pedantic here, passing even ' ' strings to ui.label() when it
> >> >> >>> would be included with other labeled text in a ui.write() call.   But it seemed
> >> >> >>> appropriate to lean to the side of caution.
> >> >> >>
> >> >> >> Missed this on the list, but I see this made it into crew. I can't say I
> >> >> >> like it..
> >> >> >
> >> >> > I pinged this on IRC a couple of times as well, but got no comments.  Oh well.
> >> >> >
> >> >> >>> diff --git a/hgext/churn.py b/hgext/churn.py
> >> >> >>> --- a/hgext/churn.py
> >> >> >>> +++ b/hgext/churn.py
> >> >> >>> @@ -150,8 +150,10 @@
> >> >> >>>      if opts.get('diffstat'):
> >> >> >>>          width -= 15
> >> >> >>>          def format(name, (added, removed)):
> >> >> >>> -            return "%s %15s %s%s\n" % (pad(name, maxname),
> >> >> >>> -                                       '+%d/-%d' % (added, removed),
> >> >> >>> +            return "%s %15s %s%s\n" % (ui.label(pad(name, maxname),
> >> >> >>> +                                                'ui.plain'),
> >> >> >>> +                                       ui.label('+%d/-%d' % (added, removed),
> >> >> >>> +                                                'ui.plain'),
> >> >> >>>                                         ui.label('+' * charnum(added),
> >> >> >>>                                                  'diffstat.inserted'),
> >> >> >>>                                         ui.label('-' * charnum(removed),
> >> >> >>
> >> >> >> There are a lot of changes here that don't make any sense unless you
> >> >> >> know that there's some weird implementation detail -that you can't
> >> >> >> actually find anywhere in the source- that's getting tripped over.
> >> >> >>
> >> >> >> This is not great. It'd be better to inflict some ugly hack on the HTML
> >> >> >> labeler than on the whole of core. For instance, enclosing labeled
> >> >> >> that's gone through the wash once already in "<div class=labelled>" and
> >> >> >> then stripping that out later in a wrapper on ui.write().
> >> >>
> >> >> FWIW, this workaround cannot work.  Any changeset that introduced this
> >> >> change would itself get mangled if it went through ui.write().
> >> >
> >> > Yes, you do have to ensure that the string you add to the labelled text
> >> > doesn't appear literally in the source. But that's easy to do with +.
> >>
> >> You lost me.  If I knew the string had been formatted before, it's
> >> trivial.  But the API without my patch provides no hints whether the
> >> ui.write() arguments are raw or formatted.  Inserting special
> >> characters is a hack that artificially restricts what valid Mercurial
> >> output looks like.  But.. the ANSII version of color.py already does
> >> this so I guess it's safe for me as well.
> >
> > I thought you were saying this wouldn't work:
> >
> > def label(s):
> >    return "<labeled>%s</labeled>" % s
> >
> > ..because colorizing a diff containing the above function in it would be
> > mangled. Yes, this is true. Any in-band labeling scheme will fall afoul
> > of this problem, potentially. We can do a first order workaround thusly:
> >
> > def label(s):
> >    return "<labe" + ("led>%s</la" % s) + "beled>"
> >
> > Now the problematic strings no longer appear in the source. But
> > obviously it's possible for someone somewhere to put <labeled> in their
> > source and experience a weird bug. We can do better:
> >
> > def label(s):
> >    return "%s%s%s" % (sharedsecretstart, s, sharedsecretend)
> >
> > where sharedsecretstart and end are:
> >
> > - generated at run-time
> > - statistically unlikely to appear in any data stream
> > - known to both label and write
> 
> That was pretty much my plan; to wrap labeled output with unusual
> multi-byte start and stop tags in ui.label().  In write(), it would
> look for both tags, remove them, and skip escaping the text between
> them.
> 
> It's not clear what the runtime generation buys you, except
> unrepeatable bug reports?

Defeating hostile attackers.

-- 
Mathematics is the supreme nostalgia of our time.