[MPlayer-dev-eng] [PATCH] suboption escaping

[Joey Parrish]

On Thu, Aug 18, 2005 at 11:51:10PM +0200, Reimar Döffinger wrote:
> On Thu, Aug 18, 2005 at 04:21:21PM -0500, Joey Parrish wrote:
> > > I think it is yet another, because it does not work exactly like any of
> > > the other methods, e.g. it doesn't support escaping via "" (like the
> > > normal option parser),
> > 
> > I don't believe this method via "" exists.  Try this:
> > $ mencoder -info 'artist="Something: Something else"'
> > 
> > You'll get:
> >   Option info: Unknown suboption  Something else"
> 
> Not for suboptions, but for options (at least in the config file from
> what I remember, but I hae to admit I rarely need it).
> 
> > > and also it won't work (from what I can tell) for
> > > specifying an option ending in \ (important for e.g. windows) - unless
> > > you make it the last option and add two \.
> > 
> > A simple change to make.
> 
> Sure, just an example. The problem I have with it is that it looks to me
> like a "brute-force" hack specifically targeted at the ':' character without
> fixing the problem in general so that specifying anything would be
> possible.
> 
> > > I also think that your patch makes it impossible to specify an option
> > > value that contains a '='?
> > 
> > Was that not already the case?  How do you propose to change the sscanf
> > format?
> 
> I think currently the first = delimits the option name, in the value you
> can have as many '=' as you want.
> And I actually wonder why sscanf is use here, it seems a bit overkill to
> me (though it is really short).
> 
> > > I am also unhappy about the fact that so many place must be changed, but
> > > that probably isn't your patches fault :-(.
> > 
> > It's not so many places!  The second version encapsulates it all in our
> > own strtok.  Outside of that function, we change _3_ lines of code.  The
> > sscanf format, and the strtok calls.
> 
> Sorry, somehow though that this was one patch, where it was actually two
> different ways to fix...
> 
> > > I'd like this to be fixed, but IMHO this way is even more a hack than my
> > > %len% method is (and I am still not entirely happy about that one, it is
> > > only really useful for GUIs).
> > 
> > I disagree strongly, because escaping using a backslash is a very common
> > and expected Unix way to do things.  %len% is something I had never
> > heard of before I saw it proposed on this list.
> 
> Yes, but the backslash escaping behaves in a certain way that your patch
> does not conform to (e.g. \\ is supposed to stand for \ etc.).

IT DOES.  For the last time, it DOES behave that way.  Read the source!
It doesn't matter what comes after the \, the next character is literal.

> Also I'd like to state my dislike of backslash escaping, since it is not
> possible to do without modifying the string and possibly lots of
> memmoves.

True.  I would love a better implementation.  However, it is only done
when parsing the options, never again.  Performance hit is one time
only.

> My %len% approach as I said is not really good, its only advantage is
> that it is extremly simple to implement in a program.

My biggest problem with that approach is that for bash scripting, I
would have to count the characters in a user-supplied string, add %len%
to the beginning, then pass that to mencoder.

> I prefer "" escaping, though it has the disadvantage that it is more
> difficult to protect from the shell and tab completion usually doesn't
> work.

> IMHO the current way to parse it is broken (first strtok and then
> sscanf). I think it would be better to first assign the subopt variable
> (by finding the end of the option string by searching for '=' or ':'),

No, this doesn't work.  If you end subopt by finding :, then we can't
have : in a value.  That's the whole reason for my patch.

> and then parse the value (if any). e.g. if it starts with " search for
> the closing " and copy all of it. if not you can do backslash escaping
> processing. Etc.

I would accept "" parsing if you have a patch to offer.  All I care
about is mencoder -info "title=Star Trek: The Next Generation".  If you
can make this work (other than the %len% method, which is a pain for
scripting) then I'll be happy.

--Joey

-- 
"Are disorders necessarily bad?" --Ike