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

[Steve Borho]

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?

> (Note that the current ANSI approach will similarly run into trouble if
> someone tries to archive, say, an ANSI animation!)

Yep.

-- 
Steve Borho