Should we be avoiding "zstyle -m" ?

Should we be avoiding "zstyle -m" ?

[Bart Schaefer]

I'm the main culprit here, using zstyle -m to test whether a style
exists before asserting a default, something I first did in
url-quote-magic 20 years ago -- but recently while working on
something else I realized that -m invokes code defined by zstyle -e,
which may be inappropriate at the point where the test appears.

One alternative is to use zstyle -g, but that retrieves only a
specific pattern rather than matching a context.  The other
possibility is zstyle -L, which requires capturing stdout because -L
always returns zero, and further it's uncertain whether the
metapattern matched by -L is similar enough to a context match.  Based
on a quick glance at the code I think they may be identical (that is,
that
  zstyle -L ":completion:$curcontext"
would give the desired result), but there's a lot going on in there.

So what to do here?  Ideas that occur to me ...

1) Allow -L and -g to be combined, to return the list in an array
instead of printing it.
2) Add a -q option to -L, to exit 0 or 1 on found or not found,
without printing.
3) Add an option to -m to only match the context, without interpreting
the value.  Maybe -n ?  In every case where -m is currently used in
Functions/ and Completion/, the value to match is '*'.
4) Variant of #3, which is to skip interpreting the value when
searching for '*' ... but it's possible someone is depending on -e
evaluating with -m.
5) Add an option to go along with -a/-b/-s that installs the style
only if it's not already there.  This still runs the risk of adding a
more-specific style that overrides a user's generic style covering the
context ... unless this new option also takes a context as an
argument?  Which gets ugly.

The first two require altering the way scanhashtable() is used.  The
third and fourth alter the way that lookupstyle() works.

Any thoughts?

Re: Should we be avoiding "zstyle -m" ?

[Peter Stephenson]

> On 07/01/2024 20:06 GMT Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
>  
> I'm the main culprit here, using zstyle -m to test whether a style
> exists before asserting a default, something I first did in
> url-quote-magic 20 years ago -- but recently while working on
> something else I realized that -m invokes code defined by zstyle -e,
> which may be inappropriate at the point where the test appears.
> 
> One alternative is to use zstyle -g, but that retrieves only a
> specific pattern rather than matching a context.  The other
> possibility is zstyle -L, which requires capturing stdout because -L
> always returns zero, and further it's uncertain whether the
> metapattern matched by -L is similar enough to a context match.  Based
> on a quick glance at the code I think they may be identical (that is,
> that
>   zstyle -L ":completion:$curcontext"
> would give the desired result), but there's a lot going on in there.
> 
> So what to do here?  Ideas that occur to me ...
> 
> 1) Allow -L and -g to be combined, to return the list in an array
> instead of printing it.
> 2) Add a -q option to -L, to exit 0 or 1 on found or not found,
> without printing.
> 3) Add an option to -m to only match the context, without interpreting
> the value.  Maybe -n ?  In every case where -m is currently used in
> Functions/ and Completion/, the value to match is '*'.
> 4) Variant of #3, which is to skip interpreting the value when
> searching for '*' ... but it's possible someone is depending on -e
> evaluating with -m.
> 5) Add an option to go along with -a/-b/-s that installs the style
> only if it's not already there.  This still runs the risk of adding a
> more-specific style that overrides a user's generic style covering the
> context ... unless this new option also takes a context as an
> argument?  Which gets ugly.

The -q and -L combination sounds like it ought to be reasonably easy
(pass through a flag to suppress output) and ought to be quite flexible
as -L already has a lot of search configurability built into it, maybe?

pws

[PATCH] Re: Should we be avoiding "zstyle -m" ?

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]

On Mon, Jan 8, 2024 at 8:52 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> > On 07/01/2024 20:06 GMT Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > on a quick glance at the code I think they may be identical (that is,
> > that
> >   zstyle -L ":completion:$curcontext"
> > would give the desired result), but there's a lot going on in there.
> >
> > So what to do here?  Ideas that occur to me ...
> >
> > 2) Add a -q option to -L, to exit 0 or 1 on found or not found,
> > without printing.
>
> The -q and -L combination sounds like it ought to be reasonably easy
> (pass through a flag to suppress output) and ought to be quite flexible
> as -L already has a lot of search configurability built into it, maybe?

Sadly all zstyle options are singletons, that is, it doesn't use the
normal option parsing but only looks at argv[0][1].  Combining options
is not friendly.

Also -L acts more like -g than like -m, and what's wanted here is -m.
That is, -L doesn't apply the zstyle pattern to the context, it treats
the context as a pattern (which is why the doc calls it a
"metapattern") and looks for a zstyle pattern that matches it.  Which
could be shoehorned into working, I guess, but is not straightforward
for this use case.

So amend the above to say "add a -q option that acts like -m except ..."

It's possible this could be refactored but that felt uglier than
duplicating a couple of tests.

[-- Attachment #2: zstyle-q.txt --]
[-- Type: text/plain, Size: 2805 bytes --]

diff --git a/Doc/Zsh/mod_zutil.yo b/Doc/Zsh/mod_zutil.yo
index 3cf9e5028..9946618d6 100644
--- a/Doc/Zsh/mod_zutil.yo
+++ b/Doc/Zsh/mod_zutil.yo
@@ -11,6 +11,7 @@ xitem(tt(zstyle) [ tt(-L) [ var(metapattern) [ var(style) ] ] ])
 xitem(tt(zstyle) [ tt(-e) | tt(-) | tt(-)tt(-) ] var(pattern) var(style) var(string) ...)
 xitem(tt(zstyle -d) [ var(pattern) [ var(style) ... ] ])
 xitem(tt(zstyle -g) var(name) [ var(pattern) [ var(style) ] ])
+xitem(tt(zstyle -q) var(context) var(style))
 xitem(tt(zstyle -){tt(a)|tt(b)|tt(s)} var(context) var(style) var(name) [ var(sep) ])
 xitem(tt(zstyle -){tt(T)|tt(t)} var(context) var(style) [ var(string) ... ])
 item(tt(zstyle -m) var(context) var(style) var(pattern))(
@@ -105,6 +106,12 @@ enditem()
 The other forms can be used to look up or test styles for a given context.
 
 startitem()
+item(tt(zstyle -q) var(context) var(style))(
+Return tt(0) if var(style) is defined in var(context).  This does not
+evaluate expressions defined by tt(zstyle -e) and does not examine any
+values set by var(style).  The expected use is to test whether a style
+has been defined for var(context) before asserting a new style.
+)
 item(tt(zstyle -s) var(context) var(style) var(name) [ var(sep) ])(
 The parameter var(name) is set to the value of the style interpreted as a
 string.  If the value contains several strings they are concatenated with
diff --git a/Src/Modules/zutil.c b/Src/Modules/zutil.c
index 8b863d5c8..293a62dcf 100644
--- a/Src/Modules/zutil.c
+++ b/Src/Modules/zutil.c
@@ -461,6 +461,28 @@ lookupstyle(char *ctxt, char *style)
     return found;
 }
 
+static int
+testforstyle(char *ctxt, char *style)
+{
+    Style s;
+    Stypat p;
+    int found = 0;
+
+    s = (Style)zstyletab->getnode2(zstyletab, style);
+    if (s) {
+	MatchData match;
+	savematch(&match);
+	for (p = s->pats; p; p = p->next)
+	    if (pattry(p->prog, ctxt)) {
+		found = 1;
+		break;
+	    }
+	restorematch(&match);
+    }
+
+    return !found;	/* 0 == success */
+}
+
 static int
 bin_zstyle(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 {
@@ -570,6 +592,7 @@ bin_zstyle(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
     case 't': min = 2; max = -1; break;
     case 'T': min = 2; max = -1; break;
     case 'm': min = 3; max =  3; break;
+    case 'q': min = 2; max =  2; break;
     case 'g': min = 1; max =  3; break;
     default:
 	zwarnnam(nam, "invalid option: %s", args[0]);
@@ -723,6 +746,15 @@ bin_zstyle(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    return 1;
 	}
 	break;
+    case 'q':
+	{
+	    int success;
+	    queue_signals();	/* Protect PAT_STATIC */
+	    success = testforstyle(args[1], args[2]);
+	    unqueue_signals();
+	    return success;
+	}
+	break;
     case 'g':
 	{
 	    int ret = 1;

Re: [PATCH] Re: Should we be avoiding "zstyle -m" ?

[Mikael Magnusson]

On 1/9/24, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Mon, Jan 8, 2024 at 8:52 AM Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
>>
>> > On 07/01/2024 20:06 GMT Bart Schaefer <schaefer@brasslantern.com>
>> > wrote:
>> >
>> > on a quick glance at the code I think they may be identical (that is,
>> > that
>> >   zstyle -L ":completion:$curcontext"
>> > would give the desired result), but there's a lot going on in there.
>> >
>> > So what to do here?  Ideas that occur to me ...
>> >
>> > 2) Add a -q option to -L, to exit 0 or 1 on found or not found,
>> > without printing.
>>
>> The -q and -L combination sounds like it ought to be reasonably easy
>> (pass through a flag to suppress output) and ought to be quite flexible
>> as -L already has a lot of search configurability built into it, maybe?
>
> Sadly all zstyle options are singletons, that is, it doesn't use the
> normal option parsing but only looks at argv[0][1].  Combining options
> is not friendly.
>
> Also -L acts more like -g than like -m, and what's wanted here is -m.
> That is, -L doesn't apply the zstyle pattern to the context, it treats
> the context as a pattern (which is why the doc calls it a
> "metapattern") and looks for a zstyle pattern that matches it.  Which
> could be shoehorned into working, I guess, but is not straightforward
> for this use case.
>
> So amend the above to say "add a -q option that acts like -m except ..."

I don't understand what the \ does in your context argument for zstyle
-m, I took a look at the docs and found nothing that explains it:
  zstyle -m context style pattern
    Match a value. Returns status zero if the pattern matches at least one
    of the strings in the value.

This doesn't imply that the context argument is treated differently
than in any of the other forms?

Using the code in url-quote-magic as an example, in a new shell the
following returns 1(false) which is expected:
zstyle -m ':url-quote-magic:\*' url-metas '*'
and after running the following, it returns 0(true), also expected:
zstyle ':url-quote-magic:*' url-metas '*?[]^(|)~#{}='

but the \* seems to just be taken literally, so for example the
following also now returns true:
zstyle -m ':url-quote-magic:asasadsaf' url-metas '*'
This makes me suspicious that maybe the code doesn't at all do what
the author expected it to do? It is of course more likely that I am
the one that's confused, the point is I don't understand what the \ is
supposed to accomplish, if anything it just seems misleading.

I am slightly more confident in my view after the following experiment
(again a new shell):
% zstyle ':url-quote-magic:\\*' url-metas '*?[]^(|)~#{}='
% zstyle -m ':url-quote-magic:\*' url-metas '*' && echo true
true

So if I've set the url-metas style for the \\* context, the code will
now override whatever my url-metas style was set for in the * context
which is bad.

On a more general note, I think the whole approach is wrong and we
should never set a style if we think it's empty; just use the fallback
value when you look up the style and it was empty later on in the
code, that will never have weird subtleties like this.

-- 
Mikael Magnusson

Re: [PATCH] Re: Should we be avoiding "zstyle -m" ?

[Bart Schaefer]

On Tue, Jan 9, 2024 at 1:17 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> I don't understand what the \ does in your context argument for zstyle -m

That is in reference to url-quote-magic.

The more typical usage would be in completion, e.g., in _sysrc
  zstyle -m ":completion:${curcontext%:*}:values" cache-policy '*'
where the context argument is built up by the chain of calls below
_main_complete and actually means something well-defined

The \* in the url-quote-magic context is just there so that the part
after the last colon is neither empty nor a legal value for a URL
scheme.

> This makes me suspicious that maybe the code doesn't at all do what
> the author expected it to do?

It's expected to return false for e.g.
  zstyle 'zstyle ':url-quote-magic:http*' url-metas '+*?[]^(|)#{}='
because that context is more specific than
  zstyle ':url-quote-magic:*' url-metas '*?[]^(|)~#{}='
for which it should return true (and does as you found).

The idea is to install the least-specific context whenever the user
has not already defined that, so that the user doesn't need to define
contexts other than the most specific.  So it needs a context that
will never match something more specific.  The actual lookup, later,
always will be in a full context, so is expected to skip the
least-specific default when the user has defined something more
specific that matches.

> I am slightly more confident in my view after the following experiment
> (again a new shell):
> % zstyle ':url-quote-magic:\\*' url-metas '*?[]^(|)~#{}='
> % zstyle -m ':url-quote-magic:\*' url-metas '*' && echo true
> true
>
> So if I've set the url-metas style for the \\* context, the code will
> now override whatever my url-metas style was set for in the * context

No, it won't ... it would only override when zstyle -m returns false
-- the tests in url-quote-magic are all || rather than &&

This is a different bug in that asserting a style for the '...::\\*'
context ends up being equivalent to setting the default url-metas to
unset, but given there are no sensible contexts that contain a leading
backslash, this would never occur unless someone were deliberately
trying to break it.

> On a more general note, I think the whole approach is wrong and we
> should never set a style if we think it's empty; just use the fallback
> value when you look up the style and it was empty later on in the
> code, that will never have weird subtleties like this.

True, but it considerably muddies the code that actually needs the
style values.  In url-quote-magic, that also means pushing the
defaults for some styles down into helper functions so they
potentially end up being mentioned more than once, and it prevents
setting up the defaults at autoload time so they don't have to be
re-asserted every time the function runs.

This could be a bit less messy now by using namespace parameters to
hold the defaults, but at the time zstyle was the only
explictly-scoped global cache.