Re: PATCH: separate watch/log functionality out into a module

[Oliver Kiddle <opk@xxxxxxx>]

On 6 Nov, Bart Schaefer wrote:
> My thought was that the module bootstrap could examine its paramdef
> array for PM_DONTIMPORT and either warn about, or skip marking for
> autoload, any variable with that flag that is already in the
> environment.

The paramdef structure and PM_ flags are stored in the module so we
can't access them until the module is loaded. What we have to work with
is what goes into Src/bltinmods.list. That only has zmodload
feature flags like "p:WATCH", originally from the .mdd file. The final
parameter to autofeatures() is a FEAT_ flag, currently hardcoded to 1
(FEAT_IGNORE) so we could do something with that at a module rather than
parameter level of granularity. We're importing variables from the
environment before setting up features that autoload parameters.

> How was this dealt with in the base shell, before you modularized it?
> Array values ($watch) can't be imported, so something must have
> happened when WATCH alone is an environment value at shell startup.

Special parameters are setup before importing from the environment so
the imported WATCH value was used and also reflected in $watch.

The $WATCH form probably only exists to allow it to be carried around in
the environment. With a more modern security perspective, this doesn't
seem especially clever. Treating the environment as untrusted, it
adds the watch code to the attack surface. Passing it's value via the
environment isn't especially useful. So if compatibility can be restored
that'd be nice but if it is too tricky, I won't be overly concerned.

> I would think the ideal behavior would be "as if":
>
>   local W=$WATCH
>   unset WATCH
>   zmodload zsh/watch
>   [[ -n $W ]] && WATCH=$W  # triggers autoload and tie

We could use a PM flag to indicate that an existing value should be
saved and restored (assuming that value was compatible). Or even do this
for any non-readonly. Rearranging the initialisation code to import the
environment after setting up autoloadable parameters may be an option if
it doesn't have other unwanted side-effects.

The following small patch avoids the error on startup which is better
in my view. I think this was actually the intention when the code was
written. Certainly my understanding of the preceding comment is that the
message was supposed to be used if the existing variable was local. I
increased context for this patch to include the whole comment. opt_i is
set for zmodload -i or from the autoloading mechanism. pm points to an
existing parameter with the same name so pm->level would indicate that
it is local.

Oliver

diff --git a/Src/module.c b/Src/module.c
index f41b82f25..bab4d8d73 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -1035,15 +1035,15 @@ checkaddparam(const char *nam, int opt_i)
 	 * -i suppresses "it's already that way" warnings,
 	 * but not "this can't possibly work" warnings, so we print
 	 * the message anyway if there's a local parameter blocking
 	 * the parameter we want to add, not if there's a
 	 * non-autoloadable parameter already there.  This
 	 * is consistent with the way add_auto* functions work.
 	 */
-	if (!opt_i || !pm->level) {
+	if (!opt_i || pm->level) {
 	    zwarn("Can't add module parameter `%s': %s",
 		  nam, pm->level ?
 		  "local parameter exists" :
 		  "parameter already exists");
 	    return 1;
 	}
 	return 2;