* Should we be avoiding "zstyle -m" ? @ 2024-01-07 20:06 Bart Schaefer 2024-01-08 16:52 ` Peter Stephenson 0 siblings, 1 reply; 5+ messages in thread From: Bart Schaefer @ 2024-01-07 20:06 UTC (permalink / raw) To: Zsh hackers list 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Should we be avoiding "zstyle -m" ? 2024-01-07 20:06 Should we be avoiding "zstyle -m" ? Bart Schaefer @ 2024-01-08 16:52 ` Peter Stephenson 2024-01-09 4:22 ` [PATCH] " Bart Schaefer 0 siblings, 1 reply; 5+ messages in thread From: Peter Stephenson @ 2024-01-08 16:52 UTC (permalink / raw) To: Zsh hackers list > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Re: Should we be avoiding "zstyle -m" ? 2024-01-08 16:52 ` Peter Stephenson @ 2024-01-09 4:22 ` Bart Schaefer 2024-01-09 9:17 ` Mikael Magnusson 0 siblings, 1 reply; 5+ messages in thread From: Bart Schaefer @ 2024-01-09 4:22 UTC (permalink / raw) To: Zsh hackers list [-- 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Re: Should we be avoiding "zstyle -m" ? 2024-01-09 4:22 ` [PATCH] " Bart Schaefer @ 2024-01-09 9:17 ` Mikael Magnusson 2024-01-09 18:30 ` Bart Schaefer 0 siblings, 1 reply; 5+ messages in thread From: Mikael Magnusson @ 2024-01-09 9:17 UTC (permalink / raw) To: Bart Schaefer; +Cc: Zsh hackers list 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Re: Should we be avoiding "zstyle -m" ? 2024-01-09 9:17 ` Mikael Magnusson @ 2024-01-09 18:30 ` Bart Schaefer 0 siblings, 0 replies; 5+ messages in thread From: Bart Schaefer @ 2024-01-09 18:30 UTC (permalink / raw) To: Zsh hackers list 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-09 18:30 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-01-07 20:06 Should we be avoiding "zstyle -m" ? Bart Schaefer 2024-01-08 16:52 ` Peter Stephenson 2024-01-09 4:22 ` [PATCH] " Bart Schaefer 2024-01-09 9:17 ` Mikael Magnusson 2024-01-09 18:30 ` Bart Schaefer
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).