PATH: autoload with explicit path

PATH: autoload with explicit path

[Peter Stephenson]

Not to be committed until 5.3 is safely done and dusted, but you might
as well see it now.

I wanted to autoload a function from an explicit path.  (Not so auto, in
fact, but there isn't a trivial way of manually loading an autoload -z
format file without using autoload.)  I could rig something up in a
function, saving and restoring fpath or wrapping the contents of the
file, but it strikes me it's a bit mean of the shell not simply to do
something sensible with

autoload +X /path/to/myfunc

That doesn't do anything sensible at the moment --- relative paths to
functions do, with the full relative path as the name, which is entirely
consistent, but absolute paths don't and in fact it tries the name as a
relative path anyway which seems Just Plain Wrong.

Then I found it was pretty much as easy (easier, actually, since fewer
special cases) to implement this generally:

autoload -Uz /path/to/myfunc

defines myfunc to be found in the directory /path/to by reusing the
filename element of the shfunc structure that's currently unused at this
stage.

This seemed quite a useful feature.  No doc or tests at this stage.

Also TBD: output from "autoload" with no name.  Just needs to update the
output of the autoload -X line appropriately, so this is a minor detail,
I think.

Also TBD: I should clearly document that autoload has its normal
semantics i.e. if the function already exists that definition is kept.
Explicit path != forced reload.

Also TBD: should output a special error message if not found so
user knows where it was looking.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 65e0cb1..96571df 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2962,6 +2962,27 @@ listusermathfunc(MathFunc p)
 }
 
 
+static void
+add_autoload_function(Shfunc shf, char *funcname)
+{
+    char *nam;
+    if (*funcname == '/' && funcname[1] &&
+	(nam = strrchr(funcname, '/')) && nam[1]) {
+	char *dir;
+	nam = strrchr(funcname, '/');
+	if (nam == funcname) {
+	    dir = "/";
+	} else {
+	    *nam++ = '\0';
+	    dir = funcname;
+	}
+	shf->filename = ztrdup(dir);
+	shfunctab->addnode(shfunctab, ztrdup(nam), shf);
+    } else {
+	shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
+    }
+}
+
 /* Display or change the attributes of shell functions.   *
  * If called as autoload, it will define a new autoloaded *
  * (undefined) shell function.                            */
@@ -3195,7 +3216,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 			  "BUG: Calling autoload from empty function");
 		} else {
 		    shf = (Shfunc) zshcalloc(sizeof *shf);
-		    shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
+		    add_autoload_function(shf, funcname);
 		}
 		shf->node.flags = on;
 		ret = eval_autoload(shf, funcname, ops, func);
@@ -3266,6 +3287,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 		printshfuncexpand(&shf->node, pflags, expand);
 	} else if (on & PM_UNDEFINED) {
 	    int signum = -1, ok = 1;
+	    char *nam;
 
 	    if (!strncmp(*argv, "TRAP", 4) &&
 		(signum = getsignum(*argv + 4)) != -1) {
@@ -3282,7 +3304,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	    shf->node.flags = on;
 	    shf->funcdef = mkautofn(shf);
 	    shfunc_set_sticky(shf);
-	    shfunctab->addnode(shfunctab, ztrdup(*argv), shf);
+	    add_autoload_function(shf, *argv);
 
 	    if (signum != -1) {
 		if (settrap(signum, NULL, ZSIG_FUNC)) {
diff --git a/Src/exec.c b/Src/exec.c
index a439aec..6e4e99c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5155,12 +5155,23 @@ loadautofn(Shfunc shf, int fksh, int autol)
 {
     int noalias = noaliases, ksh = 1;
     Eprog prog;
-    char *fname;
+    char *fname, **alt_path, *spec_path[2];
 
     pushheap();
 
+    if (shf->filename && shf->filename[0] == '/')
+    {
+	spec_path[0] = dupstring(shf->filename);
+	spec_path[1] = NULL;
+	alt_path = spec_path;
+    }
+    else
+    {
+	alt_path = NULL;
+    }
+
     noaliases = (shf->node.flags & PM_UNALIASED);
-    prog = getfpfunc(shf->node.nam, &ksh, &fname);
+    prog = getfpfunc(shf->node.nam, &ksh, &fname, alt_path);
     noaliases = noalias;
 
     if (ksh == 1) {
@@ -5607,7 +5618,7 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
 
 /**/
 Eprog
-getfpfunc(char *s, int *ksh, char **fname)
+getfpfunc(char *s, int *ksh, char **fname, char **alt_path)
 {
     char **pp, buf[PATH_MAX+1];
     off_t len;
@@ -5616,7 +5627,7 @@ getfpfunc(char *s, int *ksh, char **fname)
     Eprog r;
     int fd;
 
-    pp = fpath;
+    pp = alt_path ? alt_path : fpath;
     for (; *pp; pp++) {
 	if (strlen(*pp) + strlen(s) + 1 >= PATH_MAX)
 	    continue;
diff --git a/Src/parse.c b/Src/parse.c
index 50a0d5f..38c6da2 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -3324,7 +3324,7 @@ cur_add_func(char *nam, Shfunc shf, LinkList names, LinkList progs,
 	    return 1;
 	}
 	noaliases = (shf->node.flags & PM_UNALIASED);
-	if (!(prog = getfpfunc(shf->node.nam, NULL, NULL)) ||
+	if (!(prog = getfpfunc(shf->node.nam, NULL, NULL, NULL)) ||
 	    prog == &dummy_eprog) {
 	    noaliases = ona;
 	    zwarnnam(nam, "can't load function: %s", shf->node.nam);
diff --git a/Src/zsh.h b/Src/zsh.h
index f22d8b1..4fdd09a 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1233,7 +1233,9 @@ struct cmdnam {
 
 struct shfunc {
     struct hashnode node;
-    char *filename;             /* Name of file located in */
+    char *filename;             /* Name of file located in.
+				   For not yet autoloaded file, name
+				   of explicit directory, if not NULL. */
     zlong lineno;		/* line number in above file */
     Eprog funcdef;		/* function definition    */
     Eprog redir;                /* redirections to apply */

Re: PATH: autoload with explicit path

[Bart Schaefer]

On Dec 11, 10:18pm, Peter Stephenson wrote:
}
} I wanted to autoload a function from an explicit path.
} [...] it strikes me it's a bit mean of the shell not simply to do
} something sensible with
} 
} autoload +X /path/to/myfunc

For +X only, this can be accomplished by

    FPATH=/path/to autoload +X myfunc

Sebastian's plugin manager uses this.

} That doesn't do anything sensible at the moment --- relative paths to
} functions do, with the full relative path as the name, which is entirely
} consistent, but absolute paths don't and in fact it tries the name as a
} relative path anyway which seems Just Plain Wrong.

If I understand what you're saying, that means

    autoload this/here/file

creates a function named "this/here/file" by loading "file" from
$^fpath/this/here  ... yes?

And then (before this patch)

    autoload /this/here/file

creates a function named "/this/here/file" from the same location?

So after this patch there is no way to autoload a function whose name
begins with a slash.  Not that it was especially useful to be able to
do so, just confirming.

} autoload -Uz /path/to/myfunc
} 
} defines myfunc to be found in the directory /path/to by reusing the
} filename element of the shfunc structure that's currently unused at this
} stage.
} 
} This seemed quite a useful feature.

Yes, this is specifically what Sebastian and (I think) Ray were asking
for several months ago.

If I may make a suggestion -- an equally useful variation would be for
autoload to search the $fpath at the time of the autoload command and
store the path where the file is found *iff* it is found.  This would
be sort of like doing

    autoload -Uz $^fpath/myfunc(N)

except that it handles the situation where $fpath does not yet contain
the "myfunc" file.

Another useful variation might be to fall back to the current fpath if
the function isn't found at the specified location.

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Mon, 12 Dec 2016 08:05:50 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> If I understand what you're saying, that means
> 
>     autoload this/here/file
> 
> creates a function named "this/here/file" by loading "file" from
> $^fpath/this/here  ... yes?
> 
> And then (before this patch)
> 
>     autoload /this/here/file
> 
> creates a function named "/this/here/file" from the same location?
> 
> So after this patch there is no way to autoload a function whose name
> begins with a slash.  Not that it was especially useful to be able to
> do so, just confirming.

Yes, that all looks like it's correct.

> If I may make a suggestion -- an equally useful variation would be for
> autoload to search the $fpath at the time of the autoload command and
> store the path where the file is found *iff* it is found.  This would
> be sort of like doing
> 
>     autoload -Uz $^fpath/myfunc(N)
> 
> except that it handles the situation where $fpath does not yet contain
> the "myfunc" file.
>
> Another useful variation might be to fall back to the current fpath if
> the function isn't found at the specified location.

These need some form of new syntax, but it looks like options are
probably good enough.  -r for remember the path where found (or -s for
save/store?) and -c for use current fpath as default (or -d for default?
anything with -f or -p being a bit vague in this context).

-R for remember and also ensure it's there with a hard error if it isn't
there would be possible if it seems useful --- it gives you a way you
don't currently have of expressing the fact the function is required and
you might as well bomb out immediately rather than once you're committed
to doing whatever.

Presumably -c should remember the path, even if it found it on $fpath
rather than the specified location, to avoid surprises.

We're at the point where autoload isn't really a synonym of functions -u
any more.  I'd be tempted to document "functions -u" as "implements a
limited subset of..." rather than "equivalent to...".  (Does anyone use
that interface except for compatibility?)

pws

Re: PATH: autoload with explicit path

[Bart Schaefer]

On Dec 12,  4:31pm, Peter Stephenson wrote:
}
} We're at the point where autoload isn't really a synonym of functions -u
} any more.  I'd be tempted to document "functions -u" as "implements a
} limited subset of..." rather than "equivalent to...".  (Does anyone use
} that interface except for compatibility?)

There's
  autoload
  functions -u
  typeset -f -u

but there are aready some variations of each that are not supported by
the others, so which ones say they are "equivalent to" which others is
probably overdue for an overhaul.

For example I don't think there's any way to autoload a math function in
a single command -- you have to autoload it and then mark it as math, or
something like that.

Re: PATH: autoload with explicit path

[Peter Stephenson]

Here is an improved patch for this with the features noted in the commit
message from the branch.

commit 6f74a54d0cc48e9d9928f18eaead0460963f7d95
Author: Peter Stephenson <p.w.stephenson@ntlworld.com>
Date:   Thu Dec 15 18:57:57 2016 +0000

    Add features associated with autoloading a function using an absolute
    path.
    
    -d defaults to normal fpath
    
    -r remembers the path without actually loading.  May be combined with -d.
    
    -R does the same but it's an error if not found
    
    -X can now take a directory path: this is used to output not yet loaded
    functions that have an associated path.

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index af336ef..23ccf76 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -147,20 +147,39 @@ ifnzman(noderef(Aliasing)).
 findex(autoload)
 cindex(functions, autoloading)
 cindex(autoloading functions)
-item(tt(autoload) [ {tt(PLUS())|tt(-)}tt(TUXkmtz) ] [ tt(-w) ] [ var(name) ... ])(
+item(tt(autoload) [ {tt(PLUS())|tt(-)}tt(RTUXdkmrtz) ] [ tt(-w) ] [ var(name) ... ])(
 vindex(fpath, searching)
-Equivalent to tt(functions -u), with the exception of tt(-X)/tt(+X) and
-tt(-w).  See the section `Autoloading Functions' in ifzman(zmanref(zshmisc))\
+See the section `Autoloading Functions' in ifzman(zmanref(zshmisc))\
 ifnzman(noderef(Functions)) for full details.  The tt(fpath) parameter
 will be searched to find the function definition when the function is
 first referenced.
 
-The flag tt(-X) may be used only inside a shell function, and may not be
-followed by a var(name).  It causes the calling function to be marked for
-autoloading and then immediately loaded and executed, with the current
-array of positional parameters as arguments.  This replaces the previous
-definition of the function.  If no function definition is found, an error
-is printed and the function remains undefined and marked for autoloading.
+If var(name) consists of an absolute path, the function is defined to
+load from the file given (searching as usual for dump files in the given
+location).  The name of the function is the base name (non-directory
+part) of the file.  It is normally an error if the function is not found
+in the given location; however, if the option tt(-d) is given, searching
+for the function defaults to tt($fpath).
+
+If the option tt(-r) or tt(-R) is given, the function is searched for
+immediately and the location is recorded internally for use when the
+function is executed; a relative path is expanded using the value of
+tt($PWD).  This protects against a change to tt($fpath) after the call
+to tt(autoload).  With tt(-r), if the function is not found, it is
+silently left unresolved until execution; with tt(-R), an error message
+is printed and command processing aborted immediately the search fails,
+i.e. at the tt(autoload) command rather than at function execution..
+
+The flag tt(-X) may be used only inside a shell function.  It causes the
+calling function to be marked for autoloading and then immediately
+loaded and executed, with the current array of positional parameters as
+arguments.  This replaces the previous definition of the function.  If
+no function definition is found, an error is printed and the function
+remains undefined and marked for autoloading.  If an argument is given,
+it is used as a directory (i.e. it does not include the name of the
+function) in which the function is to be found; this may be combined
+with the tt(-d) option to allow the function search to default to tt($fpath)
+if it is not in the given location.
 
 The flag tt(+X) attempts to load each var(name) as an autoloaded function,
 but does em(not) execute it.  The exit status is zero (success) if the
@@ -176,6 +195,13 @@ If the tt(-m) flag is also given each var(name) is treated as a
 pattern and all functions already marked for autoload that match the
 pattern are loaded.
 
+With the tt(-t) flag, turn on execution tracing; with tt(-T), turn on
+execution tracing only for the current function, turning it off on entry
+to any called functions that do not also have tracing enabled.
+
+With the tt(-U) flag, alias expansion is suppressed when the function is
+loaded.
+
 With the tt(-w) flag, the var(name)s are taken as names of files compiled
 with the tt(zcompile) builtin, and all functions defined in them are
 marked for autoloading.
@@ -193,6 +219,10 @@ example(emulate zsh -c 'autoload -Uz var(func)')
 
 arranges that when var(func) is loaded the shell is in native tt(zsh)
 emulation, and this emulation is also applied when var(func) is run.
+
+Some of the functions of tt(autoload) are also provided by tt(functions
+-u) or tt(functions -U), but tt(autoload) is a more comprehensive
+interface.
 )
 findex(bg)
 cindex(jobs, backgrounding)
@@ -811,7 +841,8 @@ xitem(tt(functions -M) var(mathfn) [ var(min) [ var(max) [ var(shellfn) ] ] ])
 xitem(tt(functions -M) [ tt(-m) var(pattern) ... ])
 item(tt(functions +M) [ tt(-m) ] var(mathfn) ... )(
 Equivalent to tt(typeset -f), with the exception of the tt(-x) and
-tt(-M) options.
+tt(-M) options.  For tt(functions -u) and tt(functions -U), see
+tt(autoload), which provides additional options.
 
 The tt(-x) option indicates that any functions output will have
 each leading tab for indentation, added by the shell to show syntactic
@@ -2034,7 +2065,9 @@ expansion to be suppressed when the function is loaded.  See the
 description of the `tt(autoload)' builtin for details.
 
 Note that the builtin tt(functions) provides the same basic capabilities
-as tt(typeset -f) but gives access to a few extra options.
+as tt(typeset -f) but gives access to a few extra options; tt(autoload)
+gives further additional options for the case tt(typeset -fu) and
+tt(typeset -fU).
 )
 item(tt(-h))(
 Hide: only useful for special parameters (those marked `<S>' in the table in
diff --git a/README b/README
index 414ee0b..1cce246 100644
--- a/README
+++ b/README
@@ -32,7 +32,7 @@ details, see the documentation.
 Incompatibilities since 5.3.1
 -----------------------------
 
-The default behaviour of code like the following has changed:
+1) The default behaviour of code like the following has changed:
 
   alias foo='noglob foo'
   foo() { print function body; }
@@ -52,8 +52,18 @@ ALIAS_FUNC_DEF, has been added, which can be set to make the shell
 behave as in previous versions.  It is in any case recommended to use
 the "function" keyword, as aliases are not expanded afterwards.
 
-Incompatibilities between 5.0.8 and 5.3.1
------------------------------------------
+2) It was an undocumented, and largely useless, feature that a function
+autoloaded with an absolute path was searched for along the normal fpath
+(as if the leading / was missing) and, if found, loaded under the full
+name including the leading slash.  This has been replaced with the more
+useful feature that the function is searched for only at the given
+absolute path; the name of the function is the base name of the file.
+Note that functions including a non-leading / behave as before,
+e.g. if `dir/name' is found anywhere under a directory in $fpath it is
+loaded as a function named `dir/name'.
+
+Incompatibilities between 5.0.8 and 5.3
+----------------------------------------
 
 1) In character classes delimited by "[" and "]" within patterns, whether
 used for filename generation (globbing) or other forms of pattern
@@ -182,6 +192,10 @@ following example illustrates how this differs from past versions.
      4 4 => 1  |  4 4 => 0   **
      4 5 => 1  |  4 5 => 1
 
+<<<<<<< HEAD
+=======
+
+>>>>>>> Add features associated with autoloading a function using an absolute
 The behaviour of the parameter flag (P) has changed when it appears
 in a nested parameter group, in order to make it more useful in
 such cases.  A (P) in the outermost parameter group behaves as
diff --git a/Src/builtin.c b/Src/builtin.c
index 0f04d14..78d67ca 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -46,7 +46,7 @@ static struct builtin builtins[] =
     BUILTIN(".", BINF_PSPECIAL, bin_dot, 1, -1, 0, NULL, NULL),
     BUILTIN(":", BINF_PSPECIAL, bin_true, 0, -1, 0, NULL, NULL),
     BUILTIN("alias", BINF_MAGICEQUALS | BINF_PLUSOPTS, bin_alias, 0, -1, 0, "Lgmrs", NULL),
-    BUILTIN("autoload", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "mktTUwXz", "u"),
+    BUILTIN("autoload", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "dmktrRTUwXz", "u"),
     BUILTIN("bg", 0, bin_fg, 0, -1, BIN_BG, NULL, NULL),
     BUILTIN("break", BINF_PSPECIAL, bin_break, 0, 1, BIN_BREAK, NULL, NULL),
     BUILTIN("bye", 0, bin_break, 0, 1, BIN_EXIT, NULL, NULL),
@@ -2922,9 +2922,59 @@ eval_autoload(Shfunc shf, char *name, Options ops, int func)
     }
 
     return !loadautofn(shf, (OPT_ISSET(ops,'k') ? 2 :
-			     (OPT_ISSET(ops,'z') ? 0 : 1)), 1);
+			     (OPT_ISSET(ops,'z') ? 0 : 1)), 1,
+		       OPT_ISSET(ops,'d'));
 }
 
+/* Helper for bin_functions() for -X and -r options */
+
+/**/
+static int
+check_autoload(Shfunc shf, char *name, Options ops, int func)
+{
+    if (OPT_ISSET(ops,'X'))
+    {
+	return eval_autoload(shf, name, ops, func);
+    }
+    if (OPT_ISSET(ops,'r') || OPT_ISSET(ops,'R'))
+    {
+	char *dir_path;
+	if (shf->filename) {
+	    char *spec_path[2];
+	    spec_path[0] = shf->filename;
+	    spec_path[1] = NULL;
+	    if (getfpfunc(shf->node.nam, NULL, &dir_path, spec_path, 1)) {
+		/* shf->filename is already correct. */
+		return 0;
+	    }
+	    if (!OPT_ISSET(ops,'d')) {
+		if (OPT_ISSET(ops,'R')) {
+		    zerr("%s: function definition file not found",
+			 shf->node.nam);
+		    return 1;
+		}
+		return 0;
+	    }
+	}
+	if (getfpfunc(shf->node.nam, NULL, &dir_path, NULL, 1)) {
+	    zsfree(shf->filename);
+	    if (*dir_path != '/') {
+		dir_path = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP),
+				    "/", dir_path);
+		dir_path = xsymlink(dir_path, 1);
+	    }
+	    shf->filename = ztrdup(dir_path);
+	    return 0;
+	}
+	if (OPT_ISSET(ops,'R')) {
+	    zerr("%s: function definition file not found",
+		 shf->node.nam);
+	    return 1;
+	}
+	/* with -r, we don't flag an error, just let it be found later. */
+    }
+    return 0;
+}
 
 /* List a user-defined math function. */
 static void
@@ -2962,6 +3012,28 @@ listusermathfunc(MathFunc p)
 }
 
 
+static void
+add_autoload_function(Shfunc shf, char *funcname)
+{
+    char *nam;
+    if (*funcname == '/' && funcname[1] &&
+	(nam = strrchr(funcname, '/')) && nam[1]) {
+	char *dir;
+	nam = strrchr(funcname, '/');
+	if (nam == funcname) {
+	    dir = "/";
+	} else {
+	    *nam++ = '\0';
+	    dir = funcname;
+	}
+	zsfree(shf->filename);
+	shf->filename = ztrdup(dir);
+	shfunctab->addnode(shfunctab, ztrdup(nam), shf);
+    } else {
+	shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
+    }
+}
+
 /* Display or change the attributes of shell functions.   *
  * If called as autoload, it will define a new autoloaded *
  * (undefined) shell function.                            */
@@ -3007,10 +3079,17 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	off |= PM_KSHSTORED;
 	roff |= PM_KSHSTORED;
     }
+    if (OPT_MINUS(ops,'d')) {
+	on |= PM_CUR_FPATH;
+	off |= PM_CUR_FPATH;
+    } else if (OPT_PLUS(ops,'d')) {
+	off |= PM_CUR_FPATH;
+	roff |= PM_CUR_FPATH;
+    }
 
     if ((off & PM_UNDEFINED) || (OPT_ISSET(ops,'k') && OPT_ISSET(ops,'z')) ||
 	(OPT_ISSET(ops,'x') && !OPT_HASARG(ops,'x')) ||
-	(OPT_MINUS(ops,'X') && (OPT_ISSET(ops,'m') || *argv || !scriptname))) {
+	(OPT_MINUS(ops,'X') && (OPT_ISSET(ops,'m') || !scriptname))) {
 	zwarnnam(name, "invalid option(s)");
 	return 1;
     }
@@ -3165,47 +3244,55 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	return returnval;
     }
 
-    /* If no arguments given, we will print functions.  If flags *
-     * are given, we will print only functions containing these  *
-     * flags, else we'll print them all.                         */
-    if (!*argv) {
-	int ret = 0;
-
+    if (OPT_MINUS(ops,'X')) {
+	Funcstack fs;
+	char *funcname = NULL;
+	int ret;
+	if (*argv && argv[1]) {
+	    zwarnnam(name, "-X: too many arguments");
+	    return 1;
+	}
 	queue_signals();
-	if (OPT_MINUS(ops,'X')) {
-	    Funcstack fs;
-	    char *funcname = NULL;
-	    for (fs = funcstack; fs; fs = fs->prev) {
-		if (fs->tp == FS_FUNC) {
-		    /*
-		     * dupstring here is paranoia but unlikely to be
-		     * problematic
-		     */
-		    funcname = dupstring(fs->name);
-		    break;
-		}
+	for (fs = funcstack; fs; fs = fs->prev) {
+	    if (fs->tp == FS_FUNC) {
+		/*
+		 * dupstring here is paranoia but unlikely to be
+		 * problematic
+		 */
+		funcname = dupstring(fs->name);
+		break;
 	    }
-	    if (!funcname)
-	    {
-		zerrnam(name, "bad autoload");
-		ret = 1;
+	}
+	if (!funcname)
+	{
+	    zerrnam(name, "bad autoload");
+	    ret = 1;
+	} else {
+	    if ((shf = (Shfunc) shfunctab->getnode(shfunctab, funcname))) {
+		DPUTS(!shf->funcdef,
+		      "BUG: Calling autoload from empty function");
 	    } else {
-		if ((shf = (Shfunc) shfunctab->getnode(shfunctab, funcname))) {
-		    DPUTS(!shf->funcdef,
-			  "BUG: Calling autoload from empty function");
-		} else {
-		    shf = (Shfunc) zshcalloc(sizeof *shf);
-		    shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
-		}
-		shf->node.flags = on;
-		ret = eval_autoload(shf, funcname, ops, func);
+		shf = (Shfunc) zshcalloc(sizeof *shf);
+		shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
 	    }
-	} else {
-	    if (OPT_ISSET(ops,'U') && !OPT_ISSET(ops,'u'))
+	    if (*argv)
+		shf->filename = ztrdup(*argv);
+	    shf->node.flags = on;
+	    ret = eval_autoload(shf, funcname, ops, func);
+	}
+	unqueue_signals();
+	return ret;
+    } else if (!*argv) {
+	/* If no arguments given, we will print functions.  If flags *
+	 * are given, we will print only functions containing these  *
+	 * flags, else we'll print them all.                         */
+	int ret = 0;
+
+	queue_signals();
+	if (OPT_ISSET(ops,'U') && !OPT_ISSET(ops,'u'))
 		on &= ~PM_UNDEFINED;
 	    scanshfunc(1, on|off, DISABLED, shfunctab->printnode,
 		       pflags, expand);
-	}
 	unqueue_signals();
 	return ret;
     }
@@ -3231,8 +3318,8 @@ bin_functions(char *name, char **argv, Options ops, int func)
 				!(shf->node.flags & DISABLED)) {
 				shf->node.flags = (shf->node.flags |
 					      (on & ~PM_UNDEFINED)) & ~off;
-				if (OPT_ISSET(ops,'X') &&
-				    eval_autoload(shf, shf->node.nam, ops, func)) {
+				if (check_autoload(shf, shf->node.nam,
+						   ops, func)) {
 				    returnval = 1;
 				}
 			    }
@@ -3258,8 +3345,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	    if (on|off) {
 		/* turn on/off the given flags */
 		shf->node.flags = (shf->node.flags | (on & ~PM_UNDEFINED)) & ~off;
-		if (OPT_ISSET(ops,'X') &&
-		    eval_autoload(shf, shf->node.nam, ops, func))
+		if (check_autoload(shf, shf->node.nam, ops, func))
 		    returnval = 1;
 	    } else
 		/* no flags, so just print */
@@ -3282,7 +3368,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	    shf->node.flags = on;
 	    shf->funcdef = mkautofn(shf);
 	    shfunc_set_sticky(shf);
-	    shfunctab->addnode(shfunctab, ztrdup(*argv), shf);
+	    add_autoload_function(shf, *argv);
 
 	    if (signum != -1) {
 		if (settrap(signum, NULL, ZSIG_FUNC)) {
@@ -3293,8 +3379,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 		}
 	    }
 
-	    if (ok && OPT_ISSET(ops,'X') &&
-		eval_autoload(shf, shf->node.nam, ops, func))
+	    if (ok && check_autoload(shf, shf->node.nam, ops, func))
 		returnval = 1;
 	} else
 	    returnval = 1;
diff --git a/Src/exec.c b/Src/exec.c
index a439aec..a41d05b 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3124,7 +3124,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 	if (is_shfunc)
 	    shf = (Shfunc)hn;
 	else {
-	    shf = loadautofn(state->prog->shf, 1, 0);
+	    shf = loadautofn(state->prog->shf, 1, 0, 0);
 	    if (shf)
 		state->prog->shf = shf;
 	    else {
@@ -5142,7 +5142,7 @@ execautofn(Estate state, UNUSED(int do_exec))
 {
     Shfunc shf;
 
-    if (!(shf = loadautofn(state->prog->shf, 1, 0)))
+    if (!(shf = loadautofn(state->prog->shf, 1, 0, 0)))
 	return 1;
 
     state->prog->shf = shf;
@@ -5151,7 +5151,7 @@ execautofn(Estate state, UNUSED(int do_exec))
 
 /**/
 Shfunc
-loadautofn(Shfunc shf, int fksh, int autol)
+loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 {
     int noalias = noaliases, ksh = 1;
     Eprog prog;
@@ -5160,7 +5160,18 @@ loadautofn(Shfunc shf, int fksh, int autol)
     pushheap();
 
     noaliases = (shf->node.flags & PM_UNALIASED);
-    prog = getfpfunc(shf->node.nam, &ksh, &fname);
+    if (shf->filename && shf->filename[0] == '/')
+    {
+	char *spec_path[2];
+	spec_path[0] = dupstring(shf->filename);
+	spec_path[1] = NULL;
+	prog = getfpfunc(shf->node.nam, &ksh, &fname, spec_path, 0);
+	if (prog == &dummy_eprog &&
+	    (current_fpath || (shf->node.flags & PM_CUR_FPATH)))
+	    prog = getfpfunc(shf->node.nam, &ksh, &fname, NULL, 0);
+    }
+    else
+	prog = getfpfunc(shf->node.nam, &ksh, &fname, NULL, 0);
     noaliases = noalias;
 
     if (ksh == 1) {
@@ -5602,12 +5613,18 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
     unqueue_signals();
 }
 
-/* Search fpath for an undefined function.  Finds the file, and returns the *
- * list of its contents.                                                    */
+/*
+ * Search fpath for an undefined function.  Finds the file, and returns the
+ * list of its contents.
+ *
+ * If test_only is 1, don't load function, just test for it:
+ * - Non-null return means function was found
+ * - *fname points to path at which found (not duplicated)
+ */
 
 /**/
 Eprog
-getfpfunc(char *s, int *ksh, char **fname)
+getfpfunc(char *s, int *ksh, char **fname, char **alt_path, int test_only)
 {
     char **pp, buf[PATH_MAX+1];
     off_t len;
@@ -5616,7 +5633,7 @@ getfpfunc(char *s, int *ksh, char **fname)
     Eprog r;
     int fd;
 
-    pp = fpath;
+    pp = alt_path ? alt_path : fpath;
     for (; *pp; pp++) {
 	if (strlen(*pp) + strlen(s) + 1 >= PATH_MAX)
 	    continue;
@@ -5624,9 +5641,9 @@ getfpfunc(char *s, int *ksh, char **fname)
 	    sprintf(buf, "%s/%s", *pp, s);
 	else
 	    strcpy(buf, s);
-	if ((r = try_dump_file(*pp, s, buf, ksh))) {
+	if ((r = try_dump_file(*pp, s, buf, ksh, test_only))) {
 	    if (fname)
-		*fname = ztrdup(buf);
+		*fname = test_only ? *pp : ztrdup(buf);
 	    return r;
 	}
 	unmetafy(buf, NULL);
@@ -5634,6 +5651,11 @@ getfpfunc(char *s, int *ksh, char **fname)
 	    struct stat st;
 	    if (!fstat(fd, &st) && S_ISREG(st.st_mode) &&
 		(len = lseek(fd, 0, 2)) != -1) {
+		if (test_only) {
+		    close(fd);
+		    *fname = *pp;
+		    return &dummy_eprog;
+		}
 		d = (char *) zalloc(len + 1);
 		lseek(fd, 0, 0);
 		if ((rlen = read(fd, d, len)) >= 0) {
@@ -5661,7 +5683,7 @@ getfpfunc(char *s, int *ksh, char **fname)
 		close(fd);
 	}
     }
-    return &dummy_eprog;
+    return test_only ? NULL : &dummy_eprog;
 }
 
 /* Handle the most common type of ksh-style autoloading, when doing a      *
diff --git a/Src/hashtable.c b/Src/hashtable.c
index 7c33675..2a8b585 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -949,16 +949,20 @@ printshfuncnode(HashNode hn, int printflags)
 	    zoutputtab(stdout);
 	}
 	if (!t) {
-	    char *fopt = "UtTkz";
+	    char *fopt = "UtTkzc";
 	    int flgs[] = {
 		PM_UNALIASED, PM_TAGGED, PM_TAGGED_LOCAL,
-		PM_KSHSTORED, PM_ZSHSTORED, 0
+		PM_KSHSTORED, PM_ZSHSTORED, PM_CUR_FPATH, 0
 	    };
 	    int fl;;
 
 	    zputs("builtin autoload -X", stdout);
 	    for (fl=0;fopt[fl];fl++)
 		if (f->node.flags & flgs[fl]) putchar(fopt[fl]);
+	    if (f->filename) {
+		putchar(' ');
+		zputs(f->filename, stdout);
+	    }
 	} else {
 	    zputs(t, stdout);
 	    zsfree(t);
diff --git a/Src/parse.c b/Src/parse.c
index ed6c4a8..314cc09 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -3338,7 +3338,7 @@ cur_add_func(char *nam, Shfunc shf, LinkList names, LinkList progs,
 	    return 1;
 	}
 	noaliases = (shf->node.flags & PM_UNALIASED);
-	if (!(prog = getfpfunc(shf->node.nam, NULL, NULL)) ||
+	if (!(prog = getfpfunc(shf->node.nam, NULL, NULL, NULL, 0)) ||
 	    prog == &dummy_eprog) {
 	    noaliases = ona;
 	    zwarnnam(nam, "can't load function: %s", shf->node.nam);
@@ -3580,7 +3580,7 @@ load_dump_file(char *dump, struct stat *sbuf, int other, int len)
 
 /**/
 Eprog
-try_dump_file(char *path, char *name, char *file, int *ksh)
+try_dump_file(char *path, char *name, char *file, int *ksh, int test_only)
 {
     Eprog prog;
     struct stat std, stc, stn;
@@ -3589,7 +3589,7 @@ try_dump_file(char *path, char *name, char *file, int *ksh)
 
     if (strsfx(FD_EXT, path)) {
 	queue_signals();
-	prog = check_dump_file(path, NULL, name, ksh);
+	prog = check_dump_file(path, NULL, name, ksh, test_only);
 	unqueue_signals();
 	return prog;
     }
@@ -3608,14 +3608,14 @@ try_dump_file(char *path, char *name, char *file, int *ksh)
     if (!rd &&
 	(rc || std.st_mtime > stc.st_mtime) &&
 	(rn || std.st_mtime > stn.st_mtime) &&
-	(prog = check_dump_file(dig, &std, name, ksh))) {
+	(prog = check_dump_file(dig, &std, name, ksh, test_only))) {
 	unqueue_signals();
 	return prog;
     }
     /* No digest file. Now look for the per-function compiled file. */
     if (!rc &&
 	(rn || stc.st_mtime > stn.st_mtime) &&
-	(prog = check_dump_file(wc, &stc, name, ksh))) {
+	(prog = check_dump_file(wc, &stc, name, ksh, test_only))) {
 	unqueue_signals();
 	return prog;
     }
@@ -3643,7 +3643,7 @@ try_source_file(char *file)
 
     if (strsfx(FD_EXT, file)) {
 	queue_signals();
-	prog = check_dump_file(file, NULL, tail, NULL);
+	prog = check_dump_file(file, NULL, tail, NULL, 0);
 	unqueue_signals();
 	return prog;
     }
@@ -3654,7 +3654,7 @@ try_source_file(char *file)
 
     queue_signals();
     if (!rc && (rn || stc.st_mtime > stn.st_mtime) &&
-	(prog = check_dump_file(wc, &stc, tail, NULL))) {
+	(prog = check_dump_file(wc, &stc, tail, NULL, 0))) {
 	unqueue_signals();
 	return prog;
     }
@@ -3667,7 +3667,8 @@ try_source_file(char *file)
 
 /**/
 static Eprog
-check_dump_file(char *file, struct stat *sbuf, char *name, int *ksh)
+check_dump_file(char *file, struct stat *sbuf, char *name, int *ksh,
+		int test_only)
 {
     int isrec = 0;
     Wordcode d;
@@ -3709,6 +3710,11 @@ check_dump_file(char *file, struct stat *sbuf, char *name, int *ksh)
     if ((h = dump_find_func(d, name))) {
 	/* Found the name. If the file is already mapped, return the eprog,
 	 * otherwise map it and just go up. */
+	if (test_only)
+	{
+	    /* This is all we need.  Just return dummy. */
+	    return &dummy_eprog;
+	}
 
 #ifdef USE_MMAP
 
@@ -3745,7 +3751,7 @@ check_dump_file(char *file, struct stat *sbuf, char *name, int *ksh)
 
 #endif
 
-	    {
+	{
 	    Eprog prog;
 	    Patprog *pp;
 	    int np, fd, po = h->npats * sizeof(Patprog);
diff --git a/Src/zsh.h b/Src/zsh.h
index 2a41638..67c5a35 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1233,7 +1233,9 @@ struct cmdnam {
 
 struct shfunc {
     struct hashnode node;
-    char *filename;             /* Name of file located in */
+    char *filename;             /* Name of file located in.
+				   For not yet autoloaded file, name
+				   of explicit directory, if not NULL. */
     zlong lineno;		/* line number in above file */
     Eprog funcdef;		/* function definition    */
     Eprog redir;                /* redirections to apply */
@@ -1811,6 +1813,7 @@ struct tieddata {
 #define PM_UNALIASED	(1<<13)	/* do not expand aliases when autoloading   */
 
 #define PM_HIDE		(1<<14)	/* Special behaviour hidden by local        */
+#define PM_CUR_FPATH    (1<<14) /* (function): can use $fpath with filename */
 #define PM_HIDEVAL	(1<<15)	/* Value not shown in `typeset' commands    */
 #define PM_TIED 	(1<<16)	/* array tied to colon-path or v.v.         */
 #define PM_TAGGED_LOCAL (1<<16) /* (function): non-recursive PM_TAGGED      */
diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst
index ab7b429..1821b78 100644
--- a/Test/C04funcdef.ztst
+++ b/Test/C04funcdef.ztst
@@ -330,6 +330,95 @@
 0:whence -v of zsh-style autoload
 >oops is a shell function from ./oops
 
+  (
+    fpath=(.)
+    mkdir extra
+    print 'print "I have been loaded by explicit path."' >extra/spec
+    autoload -Uz $PWD/extra/spec
+    spec
+  )
+0:autoload with explicit path
+>I have been loaded by explicit path.
+
+  (
+    fpath=(.)
+    print 'print "I have been loaded by default path."' >def
+    autoload -Uz $PWD/extra/def
+    def
+  )
+1:autoload with explicit path with function in normal path, no -d
+?(eval):5: def: function definition file not found
+
+  (
+    fpath=(.)
+    autoload -dUz $PWD/extra/def
+    def
+  )
+0:autoload with explicit path with function in normal path, with -d
+>I have been loaded by default path.
+
+  (
+    cd extra
+    fpath=(.)
+    autoload -r spec
+    cd ..
+    spec
+  )
+0:autoload -r
+>I have been loaded by explicit path.
+
+  (
+    cd extra
+    fpath=(.)
+    autoload -r def
+    cd ..
+    def
+  )
+0:autoload -r is permissive
+>I have been loaded by default path.
+
+  (
+    cd extra
+    fpath=(.)
+    autoload -R def
+  )
+1:autoload -R is not permissive
+?(eval):4: def: function definition file not found
+
+  (
+    spec() { autoload -XUz $PWD/extra; }
+    spec
+  )
+0:autoload -X with path
+>I have been loaded by explicit path.
+
+# The line number 1 here and in the next test seems suspect,
+# but this example proves it's not down to the new features
+# being tested here.
+  (
+    fpath=(.)
+    cod() { autoload -XUz; }
+    cod
+  )
+1:autoload -X with no path, failure
+?(eval):1: cod: function definition file not found
+
+  (
+    fpath=(.)
+    def() { autoload -XUz $PWD/extra; }
+    def
+  )
+1:autoload -X with wrong path and no -d
+?(eval):1: def: function definition file not found
+
+  (
+    fpath=(.)
+    def() { autoload -dXUz $PWD/extra; }
+    def
+  )
+0:autoload -dX with path
+>I have been loaded by default path.
+
 %clean
 
  rm -f file.in file.out

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Tue, 10 Jan 2017 19:31:02 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>     Add features associated with autoloading a function using an absolute
>     path.

I pushed this, with the merge fixed.


The main feature most people might be interested in is that you can
now replace

fpath=(/path/to/my/functions)
autoload -Uz /path/to/my/functions/*(.:t)

with the more direct

autoload -Uz /path/to/my/functions/*(.)

The advantages are

- Your are guaranteed the right file will be autoloaded as $fpath is
not used.  If you previously autoloaded the file generically the record
is updated.

- "functions", "which" etc. tell you the directory that will be used.

The main disadvantage is each function record now contains the name of
the directory, so this takes extra memory --- it looks like a couple of
dozen k in the case of my function directory which is quite full.  This
could be optimised with indirection but I'm not sure it's worth the work.


The other possibility I can see being of general interest is that if you
have code that needs functions as a prerequisite, you can make the
calling code fail if the function isn't found immediately at the point
of the autoload by using the -R option, which also ensures that later
changes to fpath don't change the function that will be loaded.

pws

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Wed, 11 Jan 2017 11:42:32 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> The main disadvantage is each function record now contains the name of
> the directory, so this takes extra memory --- it looks like a couple of
> dozen k in the case of my function directory which is quite full.  This
> could be optimised with indirection but I'm not sure it's worth the work.

This seemed like too good an idea not to follow up.  Now "autoload
/blah/blah/*" is as efficient as the old way of doing it, too.

With a bit of work this can be expanded to paths for already loaded
functions, and possibly elsewhere though I think the pickings are
a lot smaller.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index b986dd8..0ed081a 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2957,13 +2957,13 @@ check_autoload(Shfunc shf, char *name, Options ops, int func)
 	    }
 	}
 	if (getfpfunc(shf->node.nam, NULL, &dir_path, NULL, 1)) {
-	    zsfree(shf->filename);
+	    dircache_set(&shf->filename, NULL);
 	    if (*dir_path != '/') {
 		dir_path = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP),
 				    "/", dir_path);
 		dir_path = xsymlink(dir_path, 1);
 	    }
-	    shf->filename = ztrdup(dir_path);
+	    dircache_set(&shf->filename, dir_path);
 	    return 0;
 	}
 	if (OPT_ISSET(ops,'R')) {
@@ -3026,8 +3026,8 @@ add_autoload_function(Shfunc shf, char *funcname)
 	    *nam++ = '\0';
 	    dir = funcname;
 	}
-	zsfree(shf->filename);
-	shf->filename = ztrdup(dir);
+	dircache_set(&shf->filename, NULL);
+	dircache_set(&shf->filename, dir);
 	shfunctab->addnode(shfunctab, ztrdup(nam), shf);
     } else {
 	shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
@@ -3276,8 +3276,8 @@ bin_functions(char *name, char **argv, Options ops, int func)
 		shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
 	    }
 	    if (*argv) {
-		zsfree(shf->filename);
-		shf->filename = ztrdup(*argv);
+		dircache_set(&shf->filename, NULL);
+		dircache_set(&shf->filename, *argv);
 	    }
 	    shf->node.flags = on;
 	    ret = eval_autoload(shf, funcname, ops, func);
diff --git a/Src/exec.c b/Src/exec.c
index 7bec7ce..10a5249 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4902,6 +4902,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 	shf = (Shfunc) zalloc(sizeof(*shf));
 	shf->funcdef = prog;
 	shf->node.flags = 0;
+	/* No dircache here, not a directory */
 	shf->filename = ztrdup(scriptfilename);
 	shf->lineno = lineno;
 	/*
@@ -4934,7 +4935,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 		    freeeprog(shf->funcdef);
 		    if (shf->redir) /* shouldn't be */
 			freeeprog(shf->redir);
-		    zsfree(shf->filename);
+		    dircache_set(&shf->filename, NULL);
 		    zfree(shf, sizeof(*shf));
 		    state->pc = end;
 		    return 1;
@@ -4965,7 +4966,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 	    freeeprog(shf->funcdef);
 	    if (shf->redir) /* shouldn't be */
 		freeeprog(shf->redir);
-	    zsfree(shf->filename);
+	    dircache_set(&shf->filename, NULL);
 	    zfree(shf, sizeof(*shf));
 	    break;
 	} else {
@@ -4974,7 +4975,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 		(signum = getsignum(s + 4)) != -1) {
 		if (settrap(signum, NULL, ZSIG_FUNC)) {
 		    freeeprog(shf->funcdef);
-		    zsfree(shf->filename);
+		    dircache_set(&shf->filename, NULL);
 		    zfree(shf, sizeof(*shf));
 		    state->pc = end;
 		    return 1;
@@ -5204,7 +5205,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	    else
 		shf->funcdef = dupeprog(prog, 0);
 	    shf->node.flags &= ~PM_UNDEFINED;
-	    zsfree(shf->filename);
+	    dircache_set(&shf->filename, NULL);
+	    /* Full filename, don't use dircache */
 	    shf->filename = fname;
 	} else {
 	    VARARR(char, n, strlen(shf->node.nam) + 1);
@@ -5228,7 +5230,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	else
 	    shf->funcdef = dupeprog(stripkshdef(prog, shf->node.nam), 0);
 	shf->node.flags &= ~PM_UNDEFINED;
-	zsfree(shf->filename);
+	dircache_set(&shf->filename, NULL);
+	/* Full filename, don't use dircache */
 	shf->filename = fname;
     }
     popheap();
@@ -5619,6 +5622,8 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
  * Search fpath for an undefined function.  Finds the file, and returns the
  * list of its contents.
  *
+ * If test is 0, load the function; *fname is set to zalloc'ed location.
+ *
  * If test_only is 1, don't load function, just test for it:
  * - Non-null return means function was found
  * - *fname points to path at which found (not duplicated)
diff --git a/Src/hashtable.c b/Src/hashtable.c
index 2a8b585..110cea6 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -889,7 +889,7 @@ freeshfuncnode(HashNode hn)
 	freeeprog(shf->funcdef);
     if (shf->redir)
 	freeeprog(shf->redir);
-    zsfree(shf->filename);
+    dircache_set(&shf->filename, NULL);
     if (shf->sticky) {
 	if (shf->sticky->n_on_opts)
 	    zfree(shf->sticky->on_opts,
@@ -1442,3 +1442,134 @@ freehistdata(Histent he, int unlink)
 	}
     }
 }
+
+
+/***********************************************************************
+ * Directory name cache mechanism
+ *
+ * The idea of this is that there are various shell structures,
+ * notably functions, that record the directories with which they
+ * are associated.  Rather than store the full string each time,
+ * we store a pointer to the same location and count the references.
+ * This is optimised so that retrieval is quick at the expense of
+ * searching the list when setting up the structure, which is a much
+ * rarer operation.
+ *
+ * There is nothing special about the fact that the strings are
+ * directories, except for the assumptions for efficiency that many
+ * structures will point to the same one, and that there are not too
+ * many different directories associated with the shell.
+ **********************************************************************/
+
+struct dircache_entry
+{
+    /* Name of directory in cache */
+    char *name;
+    /* Number of references to it */
+    int refs;
+};
+
+/*
+ * dircache is the cache, of length dircache_size.
+ * dircache_lastentry is the last entry used, an optimisation
+ * for multiple references to the same directory, e.g
+ * "autoload /blah/blah/\*".
+ */
+struct dircache_entry *dircache, *dircache_lastentry;
+int dircache_size;
+
+/*
+ * Set *name to point to a cached version of value.
+ * value is copied so may come from any source.
+ *
+ * If value is NULL, look for the existing value of *name (safe if this
+ * too is NULL) and remove a reference to it from the cache. If it's
+ * not found in the cache, it's assumed to be an allocated string and
+ * freed --- this currently occurs for a shell function that's been
+ * loaded as the filename is now a full path, not just a directory,
+ * though we may one day optimise this to a cached directory plus a
+ * name, too.  Note --- the function does *not* otherwise check
+ * if *name points to something already cached, so this is
+ * necessary any time *name may already be in the cache.
+ */
+
+/**/
+mod_export void
+dircache_set(char **name, char *value)
+{
+    struct dircache_entry *dcptr, *dcnew;
+
+    if (!value) {
+	if (!dircache_size || !*name)
+	    return;
+
+	for (dcptr = dircache; dcptr < dircache + dircache_size; dcptr++)
+	{
+	    /* Must be a pointer much, not a string match */
+	    if (*name == dcptr->name)
+	    {
+		--dcptr->refs;
+		if (!dcptr->refs) {
+		    ptrdiff_t ind = dcptr - dircache;
+		    zsfree(dcptr->name);
+		    --dircache_size;
+
+		    if (!dircache_size) {
+			zfree(dircache, sizeof(*dircache));
+			dircache = NULL;
+			return;
+		    }
+		    dcnew = (struct dircache_entry *)
+			zalloc(dircache_size * sizeof(*dcnew));
+		    if (ind)
+			memcpy(dcnew, dircache, ind * sizeof(*dcnew));
+		    if (ind < dircache_size)
+			memcpy(dcnew + ind, dcptr + 1,
+			       (dircache_size - ind) * sizeof(*dcnew));
+		    zfree(dircache, (dircache_size+1)*sizeof(*dcnew));
+		    dircache = dcnew;
+		    dircache_lastentry = NULL;
+		}
+		*name = NULL;
+		return;
+	    }
+	}
+	zsfree(*name);
+	*name = NULL;
+    } else {
+	/*
+	 * We'll maintain the cache at exactly the right size rather
+	 * than overallocating.  The rationale here is that typically
+	 * we'll get a lot of functions in a small number of directories
+	 * so the complexity overhead of maintaining a separate count
+	 * isn't really matched by the efficiency gain.
+ 	 */
+	if (!dircache_size) {
+	    dircache_size = 1;
+	    dcptr = dircache =
+		(struct dircache_entry *)zalloc(sizeof(*dircache));
+	} else if (dircache_lastentry &&
+		   !strcmp(value, dircache_lastentry->name)) {
+	    *name = dircache_lastentry->name;
+	    ++dircache_lastentry->refs;
+	    return;
+	} else {
+	    for (dcptr = dircache; dcptr < dircache + dircache_size; dcptr++)
+	    {
+		if (!strcmp(value, dcptr->name)) {
+		    *name = dcptr->name;
+		    ++dcptr->refs;
+		    return;
+		}
+	    }
+	    ++dircache_size;
+	    dircache = (struct dircache_entry *)
+		zrealloc(dircache, sizeof(*dircache) * dircache_size);
+	    dcptr = dircache + dircache_size - 1;
+	}
+	dcptr->name = ztrdup(value);
+	*name = dcptr->name;
+	dcptr->refs = 1;
+	dircache_lastentry = dcptr;
+    }
+}
diff --git a/Src/signals.c b/Src/signals.c
index 9e05add..2de5743 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -811,6 +811,7 @@ dosavetrap(int sig, int level)
 	    newshf->node.nam = ztrdup(shf->node.nam);
 	    newshf->node.flags = shf->node.flags;
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
+	    /* Assume this is a real filename, don't use dircache */
 	    newshf->filename = ztrdup(shf->filename);
 	    if (shf->sticky) {
 		newshf->sticky = sticky_emulation_dup(shf->sticky, 0);

Re: PATH: autoload with explicit path

[Peter Stephenson]

Completion for this.

Didn't really understand what I was doing here, but it seems to work...

pws

diff --git a/Completion/Zsh/Command/_typeset b/Completion/Zsh/Command/_typeset
index 94f63cf..0961e98 100644
--- a/Completion/Zsh/Command/_typeset
+++ b/Completion/Zsh/Command/_typeset
@@ -21,6 +21,7 @@ allargs=(
   X '+X[immediately autoload function]'
   Z "($fopts -A -E -F -i)-Z+[right justify and fill with leading zeros]:width"
   a "($fopts -A -E -F -T -i)-a[specify that arguments refer to arrays]"
+  df "-d[default absolute path autoload to fpath]"
   f "($popts)-f[specify that arguments refer to functions]"
   g "($fopts -T)-+g[do not restrict parameter to local scope]"
   h "($fopts -T)-+h[hide specialness of parameter]"
@@ -31,6 +32,8 @@ allargs=(
   m '(-A -E -F -T -i)-m[treat arguments as patterns]'
   p '-p[output parameters in form of calls to typeset]'
   r '(-f)-+r[mark parameters as readonly]'
+  rf '-r[remember autoload path]'
+  Rf '-R[remember autoload path, error if not found]'
   t '(-T)-+t[tag parameters and turn on execution tracing for functions]'
   tf '(-T)-+t[turn on execution tracing for functions]'
   tp '(-T)-+t[tag parameters]'
@@ -46,7 +49,7 @@ use="AEFHLRTUZafghiklmprtuxz"
 
 case ${service} in
   autoload)
-    use="UTXktwz"
+    use="URTXdkrtwz"
     func=f
   ;;
   float) use="EFHghlprtux";;
@@ -74,7 +77,7 @@ onopts=${(j..)${${words[1,CURRENT-1]:#^-*}##-}}
 offopts=${(j..)${${words[1,CURRENT-1]:#^+*}##+}}
 
 for ((i=1;i<=$#use;++i)); do
-  args+=( ${allargs[${use[$i]}${${(s::)use[$i]}[(r)[UutT]]:+$func}]} )
+  args+=( ${allargs[${use[$i]}${${(s::)use[$i]}[(r)[dUurRtT]]:+$func}]} )
 done
 
 _arguments -C -s -A "-*" -S "${args[@]}" '*::vars:= ->vars_eq'
@@ -90,12 +93,17 @@ if [[ "$state" = vars_eq ]]; then
     elif (( $+opt_args[-w] )); then
       _wanted files expl 'zwc file' _files -g '*.zwc(-.)'
     elif [[ $service = autoload || -n $opt_args[(i)-[uU]] ]]; then
-      args=(${^fpath}/*(-.:t))
-      # Filter out functions already loaded or marked for autoload.
-      local -a funckeys
-      funckeys=(${(k)functions})
-      args=(${args:|funckeys})
-      _wanted functions expl 'shell function' compadd -a args
+      if [[ $PREFIX[1] = / ]]; then
+	# Autoload by absolute path
+	_files
+      else
+	  args=(${^fpath}/*(-.:t))
+	  # Filter out functions already loaded or marked for autoload.
+	  local -a funckeys
+	  funckeys=(${(k)functions})
+	  args=(${args:|funckeys})
+	  _wanted functions expl 'shell function' compadd -a args
+      fi
     elif [[ -n $onopts$offopts ]]; then
       if [[ -n $offopts ]]; then
 	args=(${(f)"$(functions +${offopts//[^UXkmtTuz]/})"})

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Wed, 11 Jan 2017 20:51:22 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Wed, 11 Jan 2017 11:42:32 +0000
> Peter Stephenson <p.stephenson@samsung.com> wrote:
> > The main disadvantage is each function record now contains the name of
> > the directory, so this takes extra memory --- it looks like a couple of
> > dozen k in the case of my function directory which is quite full.  This
> > could be optimised with indirection but I'm not sure it's worth the work.
> 
> This seemed like too good an idea not to follow up.  Now "autoload
> /blah/blah/*" is as efficient as the old way of doing it, too.

Update based on the previous fix, also fix a memory leak in the cache
and add a test to check what happens when bits of the cache are
deleted.  I think this is sane now.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 716ddd4..a683032 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2958,13 +2958,13 @@ check_autoload(Shfunc shf, char *name, Options ops, int func)
 	    }
 	}
 	if (getfpfunc(shf->node.nam, NULL, &dir_path, NULL, 1)) {
-	    zsfree(shf->filename);
+	    dircache_set(&shf->filename, NULL);
 	    if (*dir_path != '/') {
 		dir_path = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP),
 				    "/", dir_path);
 		dir_path = xsymlink(dir_path, 1);
 	    }
-	    shf->filename = ztrdup(dir_path);
+	    dircache_set(&shf->filename, dir_path);
 	    shf->node.flags |= PM_LOADDIR;
 	    return 0;
 	}
@@ -3029,8 +3029,8 @@ add_autoload_function(Shfunc shf, char *funcname)
 	    *nam++ = '\0';
 	    dir = funcname;
 	}
-	zsfree(shf->filename);
-	shf->filename = ztrdup(dir);
+	dircache_set(&shf->filename, NULL);
+	dircache_set(&shf->filename, dir);
 	shf->node.flags |= PM_LOADDIR;
 	shfunctab->addnode(shfunctab, ztrdup(nam), shf);
     } else {
@@ -3280,8 +3280,8 @@ bin_functions(char *name, char **argv, Options ops, int func)
 		shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
 	    }
 	    if (*argv) {
-		zsfree(shf->filename);
-		shf->filename = ztrdup(*argv);
+		dircache_set(&shf->filename, NULL);
+		dircache_set(&shf->filename, *argv);
 		on |= PM_LOADDIR;
 	    }
 	    shf->node.flags = on;
diff --git a/Src/exec.c b/Src/exec.c
index 68c455b..7a5d2bd 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4902,6 +4902,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 	shf = (Shfunc) zalloc(sizeof(*shf));
 	shf->funcdef = prog;
 	shf->node.flags = 0;
+	/* No dircache here, not a directory */
 	shf->filename = ztrdup(scriptfilename);
 	shf->lineno = lineno;
 	/*
@@ -4934,7 +4935,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 		    freeeprog(shf->funcdef);
 		    if (shf->redir) /* shouldn't be */
 			freeeprog(shf->redir);
-		    zsfree(shf->filename);
+		    dircache_set(&shf->filename, NULL);
 		    zfree(shf, sizeof(*shf));
 		    state->pc = end;
 		    return 1;
@@ -4965,7 +4966,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 	    freeeprog(shf->funcdef);
 	    if (shf->redir) /* shouldn't be */
 		freeeprog(shf->redir);
-	    zsfree(shf->filename);
+	    dircache_set(&shf->filename, NULL);
 	    zfree(shf, sizeof(*shf));
 	    break;
 	} else {
@@ -4974,7 +4975,7 @@ execfuncdef(Estate state, Eprog redir_prog)
 		(signum = getsignum(s + 4)) != -1) {
 		if (settrap(signum, NULL, ZSIG_FUNC)) {
 		    freeeprog(shf->funcdef);
-		    zsfree(shf->filename);
+		    dircache_set(&shf->filename, NULL);
 		    zfree(shf, sizeof(*shf));
 		    state->pc = end;
 		    return 1;
@@ -5205,7 +5206,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	    else
 		shf->funcdef = dupeprog(prog, 0);
 	    shf->node.flags &= ~(PM_UNDEFINED|PM_LOADDIR);
-	    zsfree(shf->filename);
+	    dircache_set(&shf->filename, NULL);
+	    /* Full filename, don't use dircache */
 	    shf->filename = fname;
 	} else {
 	    VARARR(char, n, strlen(shf->node.nam) + 1);
@@ -5229,7 +5231,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	else
 	    shf->funcdef = dupeprog(stripkshdef(prog, shf->node.nam), 0);
 	shf->node.flags &= ~(PM_UNDEFINED|PM_LOADDIR);
-	zsfree(shf->filename);
+	dircache_set(&shf->filename, NULL);
+	/* Full filename, don't use dircache */
 	shf->filename = fname;
     }
     popheap();
@@ -5620,6 +5623,8 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
  * Search fpath for an undefined function.  Finds the file, and returns the
  * list of its contents.
  *
+ * If test is 0, load the function; *fname is set to zalloc'ed location.
+ *
  * If test_only is 1, don't load function, just test for it:
  * - Non-null return means function was found
  * - *fname points to path at which found (not duplicated)
diff --git a/Src/hashtable.c b/Src/hashtable.c
index 2a8b585..22ebe34 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -889,7 +889,7 @@ freeshfuncnode(HashNode hn)
 	freeeprog(shf->funcdef);
     if (shf->redir)
 	freeeprog(shf->redir);
-    zsfree(shf->filename);
+    dircache_set(&shf->filename, NULL);
     if (shf->sticky) {
 	if (shf->sticky->n_on_opts)
 	    zfree(shf->sticky->on_opts,
@@ -1442,3 +1442,140 @@ freehistdata(Histent he, int unlink)
 	}
     }
 }
+
+
+/***********************************************************************
+ * Directory name cache mechanism
+ *
+ * The idea of this is that there are various shell structures,
+ * notably functions, that record the directories with which they
+ * are associated.  Rather than store the full string each time,
+ * we store a pointer to the same location and count the references.
+ * This is optimised so that retrieval is quick at the expense of
+ * searching the list when setting up the structure, which is a much
+ * rarer operation.
+ *
+ * There is nothing special about the fact that the strings are
+ * directories, except for the assumptions for efficiency that many
+ * structures will point to the same one, and that there are not too
+ * many different directories associated with the shell.
+ **********************************************************************/
+
+struct dircache_entry
+{
+    /* Name of directory in cache */
+    char *name;
+    /* Number of references to it */
+    int refs;
+};
+
+/*
+ * dircache is the cache, of length dircache_size.
+ * dircache_lastentry is the last entry used, an optimisation
+ * for multiple references to the same directory, e.g
+ * "autoload /blah/blah/\*".
+ */
+struct dircache_entry *dircache, *dircache_lastentry;
+int dircache_size;
+
+/*
+ * Set *name to point to a cached version of value.
+ * value is copied so may come from any source.
+ *
+ * If value is NULL, look for the existing value of *name (safe if this
+ * too is NULL) and remove a reference to it from the cache. If it's
+ * not found in the cache, it's assumed to be an allocated string and
+ * freed --- this currently occurs for a shell function that's been
+ * loaded as the filename is now a full path, not just a directory,
+ * though we may one day optimise this to a cached directory plus a
+ * name, too.  Note --- the function does *not* otherwise check
+ * if *name points to something already cached, so this is
+ * necessary any time *name may already be in the cache.
+ */
+
+/**/
+mod_export void
+dircache_set(char **name, char *value)
+{
+    struct dircache_entry *dcptr, *dcnew;
+
+    if (!value) {
+	if (!*name)
+	    return;
+	if (!dircache_size) {
+	    zsfree(*name);
+	    *name = NULL;
+	    return;
+	}
+
+	for (dcptr = dircache; dcptr < dircache + dircache_size; dcptr++)
+	{
+	    /* Must be a pointer much, not a string match */
+	    if (*name == dcptr->name)
+	    {
+		--dcptr->refs;
+		if (!dcptr->refs) {
+		    ptrdiff_t ind = dcptr - dircache;
+		    zsfree(dcptr->name);
+		    --dircache_size;
+
+		    if (!dircache_size) {
+			zfree(dircache, sizeof(*dircache));
+			dircache = NULL;
+			dircache_lastentry = NULL;
+			return;
+		    }
+		    dcnew = (struct dircache_entry *)
+			zalloc(dircache_size * sizeof(*dcnew));
+		    if (ind)
+			memcpy(dcnew, dircache, ind * sizeof(*dcnew));
+		    if (ind < dircache_size)
+			memcpy(dcnew + ind, dcptr + 1,
+			       (dircache_size - ind) * sizeof(*dcnew));
+		    zfree(dircache, (dircache_size+1)*sizeof(*dcnew));
+		    dircache = dcnew;
+		    dircache_lastentry = NULL;
+		}
+		*name = NULL;
+		return;
+	    }
+	}
+	zsfree(*name);
+	*name = NULL;
+    } else {
+	/*
+	 * We'll maintain the cache at exactly the right size rather
+	 * than overallocating.  The rationale here is that typically
+	 * we'll get a lot of functions in a small number of directories
+	 * so the complexity overhead of maintaining a separate count
+	 * isn't really matched by the efficiency gain.
+ 	 */
+	if (dircache_lastentry &&
+	    !strcmp(value, dircache_lastentry->name)) {
+	    *name = dircache_lastentry->name;
+	    ++dircache_lastentry->refs;
+	    return;
+	} else if (!dircache_size) {
+	    dircache_size = 1;
+	    dcptr = dircache =
+		(struct dircache_entry *)zalloc(sizeof(*dircache));
+	} else {
+	    for (dcptr = dircache; dcptr < dircache + dircache_size; dcptr++)
+	    {
+		if (!strcmp(value, dcptr->name)) {
+		    *name = dcptr->name;
+		    ++dcptr->refs;
+		    return;
+		}
+	    }
+	    ++dircache_size;
+	    dircache = (struct dircache_entry *)
+		zrealloc(dircache, sizeof(*dircache) * dircache_size);
+	    dcptr = dircache + dircache_size - 1;
+	}
+	dcptr->name = ztrdup(value);
+	*name = dcptr->name;
+	dcptr->refs = 1;
+	dircache_lastentry = dcptr;
+    }
+}
diff --git a/Src/signals.c b/Src/signals.c
index 9e05add..2de5743 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -811,6 +811,7 @@ dosavetrap(int sig, int level)
 	    newshf->node.nam = ztrdup(shf->node.nam);
 	    newshf->node.flags = shf->node.flags;
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
+	    /* Assume this is a real filename, don't use dircache */
 	    newshf->filename = ztrdup(shf->filename);
 	    if (shf->sticky) {
 		newshf->sticky = sticky_emulation_dup(shf->sticky, 0);
diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst
index 7100280..370394b 100644
--- a/Test/C04funcdef.ztst
+++ b/Test/C04funcdef.ztst
@@ -429,6 +429,45 @@
 0: autoload -X interaction with absolute filename used for source location
 >Function was loaded correctly.
 
+  (
+    fpath=()
+    mkdir extra2
+    for f in fun2a fun2b; do
+      print "print $f" >extra2/$f
+    done
+    repeat 3; do
+      autoload $PWD/extra2/fun2{a,b} $PWD/extra/spec
+      fun2a
+      fun2b
+      spec
+      unfunction fun2a fun2b spec
+      autoload $PWD/extra2/fun2{a,b} $PWD/extra/spec
+      spec
+      fun2b
+      fun2a
+      unfunction fun2a fun2b spec
+    done
+  )
+0: Exercise the directory name cache for autoloads
+>fun2a
+>fun2b
+>I have been loaded by explicit path.
+>I have been loaded by explicit path.
+>fun2b
+>fun2a
+>fun2a
+>fun2b
+>I have been loaded by explicit path.
+>I have been loaded by explicit path.
+>fun2b
+>fun2a
+>fun2a
+>fun2b
+>I have been loaded by explicit path.
+>I have been loaded by explicit path.
+>fun2b
+>fun2a
+
 %clean
 
  rm -f file.in file.out

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Wed, 11 Jan 2017 20:51:22 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> With a bit of work this can be expanded to paths for already loaded
> functions, and possibly elsewhere though I think the pickings are
> a lot smaller.

This does the former of that --- it was less work than I expected.

The upshot of this is that with the new absolute path scheme, storing
the filename at the point of loading the function is a no-op since we've
already got the components and keep them.  With a traditional
fpath-style load, we use the new directory cache at the point where we
load the file from the path, so this now also avoids an extra directory
allocation (and, unlike the similar mechanism used for the command hash,
avoids the brittle association with the original path variable).

There is a heap allocation when we enter the function to put the name on
the function stack (see getshfuncfile()), but there always was --- the
only difference is a little extra calculation for the three components.
So I think the penalty is trivial to non-existent.

The only significant extra requirement is use of the heap to get the
filename in "whence -v" --- that's a bit of laziness, we could just print
out the components using the same logic as getshfuncfile().  Might fix
that later.

Note that the PM_LOADDIR flag still represents exactly what it did,
it's just that the period of possible validity now lasts throughout the
function [nearly said product] life cycle.

pws

diff --git a/Src/exec.c b/Src/exec.c
index 7a5d2bd..6c82643 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5124,12 +5124,12 @@ execautofn_basic(Estate state, UNUSED(int do_exec))
      * defined yet.
      */
     if (funcstack && !funcstack->filename)
-	funcstack->filename = dupstring(shf->filename);
+	funcstack->filename = getshfuncfile(shf);
 
     oldscriptname = scriptname;
     oldscriptfilename = scriptfilename;
     scriptname = dupstring(shf->node.nam);
-    scriptfilename = dupstring(shf->filename);
+    scriptfilename = getshfuncfile(shf);
     execode(shf->funcdef, 1, 0, "loadautofunc");
     scriptname = oldscriptname;
     scriptfilename = oldscriptfilename;
@@ -5150,13 +5150,47 @@ execautofn(Estate state, UNUSED(int do_exec))
     return execautofn_basic(state, 0);
 }
 
+/*
+ * Helper function to install the source file name of a shell function
+ * just autoloaded.
+ *
+ * We attempt to do this efficiently as the typical case is the
+ * directory part is a well-known directory, which is cached, and
+ * the non-directory part is the same as the node name.
+ */
+
+/**/
+static void
+loadautofnsetfile(Shfunc shf, char *fdir)
+{
+    /*
+     * If shf->filename is already the load directory ---
+     * keep it as we can still use it to get the load file.
+     * This makes autoload with an absolute path particularly efficient.
+     */
+    if (!(shf->node.flags & PM_LOADDIR) ||
+	strcmp(shf->filename, fdir) != 0) {
+	/* Old directory name not useful... */
+	dircache_set(&shf->filename, NULL);
+	if (fdir) {
+	    /* ...can still cache directory */
+	    shf->node.flags |= PM_LOADDIR;
+	    dircache_set(&shf->filename, fdir);
+	} else {
+	    /* ...no separate directory part to cache, for some reason. */
+	    shf->node.flags &= ~PM_LOADDIR;
+	    shf->filename = ztrdup(shf->node.nam);
+	}
+    }
+}
+
 /**/
 Shfunc
 loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 {
     int noalias = noaliases, ksh = 1;
     Eprog prog;
-    char *fname;
+    char *fdir;			/* Directory path where func found */
 
     pushheap();
 
@@ -5167,13 +5201,13 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	char *spec_path[2];
 	spec_path[0] = dupstring(shf->filename);
 	spec_path[1] = NULL;
-	prog = getfpfunc(shf->node.nam, &ksh, &fname, spec_path, 0);
+	prog = getfpfunc(shf->node.nam, &ksh, &fdir, spec_path, 0);
 	if (prog == &dummy_eprog &&
 	    (current_fpath || (shf->node.flags & PM_CUR_FPATH)))
-	    prog = getfpfunc(shf->node.nam, &ksh, &fname, NULL, 0);
+	    prog = getfpfunc(shf->node.nam, &ksh, &fdir, NULL, 0);
     }
     else
-	prog = getfpfunc(shf->node.nam, &ksh, &fname, NULL, 0);
+	prog = getfpfunc(shf->node.nam, &ksh, &fdir, NULL, 0);
     noaliases = noalias;
 
     if (ksh == 1) {
@@ -5192,7 +5226,6 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	return NULL;
     }
     if (!prog) {
-	zsfree(fname);
 	popheap();
 	return NULL;
     }
@@ -5205,10 +5238,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 		shf->funcdef = prog;
 	    else
 		shf->funcdef = dupeprog(prog, 0);
-	    shf->node.flags &= ~(PM_UNDEFINED|PM_LOADDIR);
-	    dircache_set(&shf->filename, NULL);
-	    /* Full filename, don't use dircache */
-	    shf->filename = fname;
+	    shf->node.flags &= ~PM_UNDEFINED;
+	    loadautofnsetfile(shf, fdir);
 	} else {
 	    VARARR(char, n, strlen(shf->node.nam) + 1);
 	    strcpy(n, shf->node.nam);
@@ -5220,7 +5251,6 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 		zwarn("%s: function not defined by file", n);
 		locallevel++;
 		popheap();
-		zsfree(fname);
 		return NULL;
 	    }
 	}
@@ -5230,10 +5260,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	    shf->funcdef = stripkshdef(prog, shf->node.nam);
 	else
 	    shf->funcdef = dupeprog(stripkshdef(prog, shf->node.nam), 0);
-	shf->node.flags &= ~(PM_UNDEFINED|PM_LOADDIR);
-	dircache_set(&shf->filename, NULL);
-	/* Full filename, don't use dircache */
-	shf->filename = fname;
+	shf->node.flags &= ~PM_UNDEFINED;
+	loadautofnsetfile(shf, fdir);
     }
     popheap();
 
@@ -5453,7 +5481,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	funcstack = &fstack;
 
 	fstack.flineno = shfunc->lineno;
-	fstack.filename = dupstring(shfunc->filename);
+	fstack.filename = getshfuncfile(shfunc);
 
 	prog = shfunc->funcdef;
 	if (prog->flags & EF_RUN) {
@@ -5623,16 +5651,17 @@ runshfunc(Eprog prog, FuncWrap wrap, char *name)
  * Search fpath for an undefined function.  Finds the file, and returns the
  * list of its contents.
  *
- * If test is 0, load the function; *fname is set to zalloc'ed location.
+ * If test is 0, load the function.
  *
  * If test_only is 1, don't load function, just test for it:
- * - Non-null return means function was found
- * - *fname points to path at which found (not duplicated)
+ * Non-null return means function was found
+ *
+ * *fdir points to path at which found (as passed in, not duplicated)
  */
 
 /**/
 Eprog
-getfpfunc(char *s, int *ksh, char **fname, char **alt_path, int test_only)
+getfpfunc(char *s, int *ksh, char **fdir, char **alt_path, int test_only)
 {
     char **pp, buf[PATH_MAX+1];
     off_t len;
@@ -5650,8 +5679,8 @@ getfpfunc(char *s, int *ksh, char **fname, char **alt_path, int test_only)
 	else
 	    strcpy(buf, s);
 	if ((r = try_dump_file(*pp, s, buf, ksh, test_only))) {
-	    if (fname)
-		*fname = test_only ? *pp : ztrdup(buf);
+	    if (fdir)
+		*fdir = *pp;
 	    return r;
 	}
 	unmetafy(buf, NULL);
@@ -5661,7 +5690,8 @@ getfpfunc(char *s, int *ksh, char **fname, char **alt_path, int test_only)
 		(len = lseek(fd, 0, 2)) != -1) {
 		if (test_only) {
 		    close(fd);
-		    *fname = *pp;
+		    if (fdir)
+			*fdir = *pp;
 		    return &dummy_eprog;
 		}
 		d = (char *) zalloc(len + 1);
@@ -5677,8 +5707,8 @@ getfpfunc(char *s, int *ksh, char **fname, char **alt_path, int test_only)
 		    r = parse_string(d, 1);
 		    scriptname = oldscriptname;
 
-		    if (fname)
-			*fname = ztrdup(buf);
+		    if (fdir)
+			*fdir = *pp;
 
 		    zfree(d, len + 1);
 
diff --git a/Src/hashtable.c b/Src/hashtable.c
index a3a38f7..0fc87f3 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -926,10 +926,12 @@ printshfuncnode(HashNode hn, int printflags)
 	       (f->node.flags & PM_UNDEFINED) ?
 	       " is an autoload shell function" :
 	       " is a shell function");
-	if (f->filename && (printflags & PRINT_WHENCE_VERBOSE) &&
-	    strcmp(f->filename, f->node.nam) != 0) {
-	    printf(" from ");
-	    quotedzputs(f->filename, stdout);
+	if (printflags & PRINT_WHENCE_VERBOSE) {
+	    char *fname = getshfuncfile(f);
+	    if (fname && strcmp(fname, f->node.nam) != 0) {
+		printf(" from ");
+		quotedzputs(fname, stdout);
+	    }
 	}
 	putchar('\n');
 	return;
@@ -959,7 +961,7 @@ printshfuncnode(HashNode hn, int printflags)
 	    zputs("builtin autoload -X", stdout);
 	    for (fl=0;fopt[fl];fl++)
 		if (f->node.flags & flgs[fl]) putchar(fopt[fl]);
-	    if (f->filename) {
+	    if (f->filename && (f->node.flags & PM_LOADDIR)) {
 		putchar(' ');
 		zputs(f->filename, stdout);
 	    }
@@ -1041,6 +1043,24 @@ printshfuncexpand(HashNode hn, int printflags, int expand)
     text_expand_tabs = save_expand;
 }
 
+/*
+ * Get a heap-duplicated name of the shell function, for
+ * use in tracing.
+ */
+
+/**/
+mod_export char *
+getshfuncfile(Shfunc shf)
+{
+    if (shf->node.flags & PM_LOADDIR) {
+	return zhtricat(shf->filename, "/", shf->node.nam);
+    } else if (shf->filename) {
+	return dupstring(shf->filename);
+    } else {
+	return NULL;
+    }
+}
+
 /**************************************/
 /* Reserved Word Hash Table Functions */
 /**************************************/
diff --git a/Src/signals.c b/Src/signals.c
index 2de5743..a717677 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -811,8 +811,11 @@ dosavetrap(int sig, int level)
 	    newshf->node.nam = ztrdup(shf->node.nam);
 	    newshf->node.flags = shf->node.flags;
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
-	    /* Assume this is a real filename, don't use dircache */
-	    newshf->filename = ztrdup(shf->filename);
+	    if (shf->node.flags & PM_LOADDIR) {
+		dircache_set(&newshf->filename, shf->filename);
+	    } else {
+		newshf->filename = ztrdup(shf->filename);
+	    }
 	    if (shf->sticky) {
 		newshf->sticky = sticky_emulation_dup(shf->sticky, 0);
 	    } else

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Fri, 13 Jan 2017 18:04:23 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Wed, 11 Jan 2017 20:51:22 +0000
> Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> > With a bit of work this can be expanded to paths for already loaded
> > functions, and possibly elsewhere though I think the pickings are
> > a lot smaller.
> 
> This does the former of that --- it was less work than I expected.
>...
> The only significant extra requirement is use of the heap to get the
> filename in "whence -v" --- that's a bit of laziness, we could just print
> out the components using the same logic as getshfuncfile().  Might fix
> that later.

I fixed this up when I pushed it.  One very minor change is that to
avoid the intermediate string quotedzputs() now runs separately on the
directory and the filename, e.g.

% whence -v my\ stupid\ function\ name
my stupid function name is a shell function from /export/home/pws/src/zsh-git/code/'my stupid function name'

You might even consider that a feature.

pws

Re: PATH: autoload with explicit path

[Daniel Shahaf]

Peter Stephenson wrote on Mon, Jan 16, 2017 at 10:37:08 +0000:
> One very minor change is that to
> avoid the intermediate string quotedzputs() now runs separately on the
> directory and the filename, e.g.
> 
> % whence -v my\ stupid\ function\ name
> my stupid function name is a shell function from /export/home/pws/src/zsh-git/code/'my stupid function name'
> 
> You might even consider that a feature.

Does it do the right thing when RC_QUOTES is set?

Re: PATH: autoload with explicit path

[Bart Schaefer]

On Mon, Jan 16, 2017 at 2:37 AM, Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> % whence -v my\ stupid\ function\ name
> my stupid function name is a shell function from /export/home/pws/src/zsh-git/code/'my stupid function name'
>
> You might even consider that a feature.

When was this pushed?  I don't get any quoting.

% path+=(/tmp/one\ space)
% whence -v two\ space
two space is /tmp/one space/two space
%

% git whatchanged
commit f90a0447aada3ad2fa114210c62b855a3e60cb1d
Author: Peter Stephenson <pws@zsh.org>
Date:   Mon Jan 16 10:31:56 2017 +0000

    40353 with tweaks to whence -v: extend directory cache use.

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Mon, 16 Jan 2017 15:04:52 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Peter Stephenson wrote on Mon, Jan 16, 2017 at 10:37:08 +0000:
> > One very minor change is that to
> > avoid the intermediate string quotedzputs() now runs separately on the
> > directory and the filename, e.g.
> > 
> > % whence -v my\ stupid\ function\ name
> > my stupid function name is a shell function from /export/home/pws/src/zsh-git/code/'my stupid function name'
> > 
> > You might even consider that a feature.
> 
> Does it do the right thing when RC_QUOTES is set?

You'll notice there's a "/" between the directory and the file.

pws

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Mon, 16 Jan 2017 07:22:23 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Mon, Jan 16, 2017 at 2:37 AM, Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> >
> > % whence -v my\ stupid\ function\ name
> > my stupid function name is a shell function from /export/home/pws/src/zsh-git/code/'my stupid function name'
> >
> > You might even consider that a feature.
> 
> When was this pushed?  I don't get any quoting.
> 
> % path+=(/tmp/one\ space)
> % whence -v two\ space
> two space is /tmp/one space/two space

That's not a shell function, that's a command.  Looks like that doesn't
do quoting.; I presume that's just a historical oversight.

This would handle the other verbose "is" cases.  However, in this case
the entire path is passed back by findcmd, so it's all quoted.  Updating
findcmd() would make it more efficient (don't need to copy the
directory) and more consistent with the function case, but it's too
minor for me to feel like doing.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index a683032..b1b6e2e 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3747,7 +3747,7 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 		    } else {
 			if (v && !csh)
 			    zputs(*argv, stdout), fputs(" is ", stdout);
-			zputs(buf, stdout);
+			quotedzputs(buf, stdout);
 			if (OPT_ISSET(ops,'s') || OPT_ISSET(ops, 'S'))
 			    print_if_link(buf, OPT_ISSET(ops, 'S'));
 			fputc('\n', stdout);
@@ -3779,7 +3779,7 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 	    } else {
 		if (v && !csh)
 		    zputs(*argv, stdout), fputs(" is ", stdout);
-		zputs(cnam, stdout);
+		quotedzputs(cnam, stdout);
 		if (OPT_ISSET(ops,'s') || OPT_ISSET(ops,'S'))
 		    print_if_link(cnam, OPT_ISSET(ops,'S'));
 		fputc('\n', stdout);

Re: PATCH: autoload with explicit path

[Peter Stephenson]

Currently,

autoload /path/to/foo
autoload foo

leaves foo marked as to be loaded from /path/to/foo.  I should probably
document this (and that therefore to unmark the path you need to
unfunction).  I think this is the right way of doing it as the explicit
path should continue to override the more vague autoload with no path
indicated, and this is safer in case some code decides it needs a
function and inadvertently resets the path the user carefully decided to
give the function.

However, you could argue that "autoload foo" should reset foo to reload
from $fpath, as the shell would do if it encountered that command on its
own.

You can in any case change an explicitly marked path seamlessly by
issuing a new command with an absolute path.

I have found a bug, though: "autoload /path/to/foo" takes effect
even if foo is already loaded, which isn't supposed to happen.
Test for this tomorrow.

Second hunk is a minor efficiency change: I noticed findcmd() with
second argument 1 returns a dupstring() value anyway.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index b1b6e2e..7a04a79 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3369,6 +3369,31 @@ bin_functions(char *name, char **argv, Options ops, int func)
 		removetrapnode(signum);
 	    }
 
+	    if (**argv == '/') {
+		char *base = strrchr(*argv, '/') + 1;
+		if (*base &&
+		    (shf = (Shfunc) shfunctab->getnode(shfunctab, base))) {
+		    char *dir;
+		    /* turn on/off the given flags */
+		    shf->node.flags =
+			(shf->node.flags | (on & ~PM_UNDEFINED)) & ~off;
+		    if (shf->node.flags & PM_UNDEFINED) {
+			/* update path if not yet loaded */
+			if (base == *argv + 1)
+			    dir = "/";
+			else {
+			    dir = *argv;
+			    base[-1] = '\0';
+			}
+			dircache_set(&shf->filename, NULL);
+			dircache_set(&shf->filename, dir);
+		    }
+		    if (check_autoload(shf, shf->node.nam, ops, func))
+			returnval = 1;
+		    continue;
+		}
+	    }
+
 	    /* Add a new undefined (autoloaded) function to the *
 	     * hash table with the corresponding flags set.     */
 	    shf = (Shfunc) zshcalloc(sizeof *shf);
diff --git a/Src/subst.c b/Src/subst.c
index 737a0a9..670f3f0 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -622,7 +622,7 @@ filesub(char **namptr, int assign)
 char *
 equalsubstr(char *str, int assign, int nomatch)
 {
-    char *pp, *cnam, *cmdstr, *ret;
+    char *pp, *cnam, *cmdstr;
 
     for (pp = str; !isend2(*pp); pp++)
 	;
@@ -634,10 +634,10 @@ equalsubstr(char *str, int assign, int nomatch)
 	    zerr("%s not found", cmdstr);
 	return NULL;
     }
-    ret = dupstring(cnam);
     if (*pp)
-	ret = dyncat(ret, pp);
-    return ret;
+	return dyncat(cnam, pp);
+    else
+	return cnam;		/* already duplicated */
 }
 
 /**/

Re: PATCH: autoload with explicit path

[Daniel Shahaf]

Peter Stephenson wrote on Tue, Jan 17, 2017 at 18:36:06 +0000:
> autoload /path/to/foo
> autoload foo
> 
> leaves foo marked as to be loaded from /path/to/foo.  I should probably
> document this (and that therefore to unmark the path you need to
> unfunction).  I think this is the right way of doing it as the explicit
> path should continue to override the more vague autoload with no path
> indicated, and this is safer in case some code decides it needs a
> function and inadvertently resets the path the user carefully decided to
> give the function.

Suppose $^fpath/foo/bar(N) has two matches, how do I explicitly autoload
the second one?  I.e., how do I disambiguate «autoload foo/bar» (with no
leading slash) to load a particular copy of foo/bar?

Just removing the $FPATH element containing the former foo/bar would
prevent foo/bar from autoloading functions that are only available in
that $FPATH element.  (Which might be the system default $fpath dir)

I wonder if we need «autoload -d /qux foo/bar», or perhaps cause
«autoload /qux/./foo/bar» load a function named "foo/bar" (the idea of
assigning a meaning to /./ elements is borrowed from rsyncd.conf).

(I'm assuming «autoload foo/bar» is a first-class use-case.)

Re: PATCH: autoload with explicit path

[Bart Schaefer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 790 bytes --]

On Tue, 17 Jan 2017, Daniel Shahaf wrote:

> Peter Stephenson wrote on Tue, Jan 17, 2017 at 18:36:06 +0000:
> > I think this is the right way of doing it as the explicit
> > path should continue to override the more vague autoload with no path
> > indicated, and this is safer in case some code decides it needs a
> > function and inadvertently resets the path the user carefully decided to
> > give the function.
>
> Suppose $^fpath/foo/bar(N) has two matches, how do I explicitly autoload
> the second one?  I.e., how do I disambiguate «autoload foo/bar» (with no
> leading slash) to load a particular copy of foo/bar?

How does any of this interact with searching for functions inside files
built with zcompile?

(Does zcompile even support having a function name that contains a slash?)

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Tue, 17 Jan 2017 22:17:54 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Suppose $^fpath/foo/bar(N) has two matches, how do I explicitly autoload
> the second one?  I.e., how do I disambiguate «autoload foo/bar» (with no
> leading slash) to load a particular copy of foo/bar?

You don't; the mechanism only applies to the last path component so it
will pick the latest version of "bar".

Applying resolution by means of searching along a path is exactly what
the existing $fpath mechanism is for; the new mechanism doesn't replace
that.  It gives you direct access to functions if you know for a fact
you always want the files in a particular directory loaded.  I'm
guessing this will usually apply to people's own private functions, and
it might in principle apply to an add-on that has its own ideas about
paths.  It inevitably won't be a good match for finding / resolving
things that might be anywhere along a path.  But we already have a
mechanism for that.

pws

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Tue, 17 Jan 2017 16:06:22 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> How does any of this interact with searching for functions inside files
> built with zcompile?
> 
> (Does zcompile even support having a function name that contains a slash?)

It should be seamless.  It should search the same files it always would have,
except using an explicit path rather than everywhere along $fpath.  The
function *name* doesn't contain a slash (it can't with this mechanism).
The autoload immediately gets separated into dir + name which are stored
separately.

Functions marked for autoload remain second class citizens in the sense
that any time a full definition is encountered it is immediately used
and the autoload forgotten about --- that was the point of yesterday's
patch.

pws

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Tue, 17 Jan 2017 18:36:06 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> I have found a bug, though: "autoload /path/to/foo" takes effect
> even if foo is already loaded, which isn't supposed to happen.
> Test for this tomorrow.

I'll push these two together.  This also exemplifies the behaviour I was
discussing yesterday.

pws

diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst
index 370394b..5edbe26 100644
--- a/Test/C04funcdef.ztst
+++ b/Test/C04funcdef.ztst
@@ -468,6 +468,31 @@
 >fun2b
 >fun2a
 
+  not_trashed() { print This function was not trashed; }
+  autoload -Uz /foo/bar/not_trashed
+  not_trashed
+0:autoload with absolute path doesn't trash loaded function
+>This function was not trashed
+
+  # keep spec from getting loaded in parent shell for simplicity
+  (
+    if whence spec; then print spec already loaded >&2; exit 1; fi
+    autoload -Uz $PWD/spec
+    autoload -Uz $PWD/extra/spec
+    spec
+  )
+0:autoload with absolute path can be overridden if not yet loaded
+>I have been loaded by explicit path.
+
+  (
+    if whence spec; then print spec already loaded >&2; exit 1; fi
+    autoload -Uz $PWD/extra/spec
+    autoload spec
+    spec
+  )
+0:autoload with absolute path not cancelled by bare autoload
+>I have been loaded by explicit path.
+
 %clean
 
  rm -f file.in file.out

Re: PATCH: autoload with explicit path

[Bart Schaefer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 860 bytes --]

On Wed, 18 Jan 2017, Peter Stephenson wrote:

> On Tue, 17 Jan 2017 22:17:54 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Suppose $^fpath/foo/bar(N) has two matches, how do I explicitly autoload
> > the second one?  I.e., how do I disambiguate «autoload foo/bar» (with no
> > leading slash) to load a particular copy of foo/bar?
>
> You don't; the mechanism only applies to the last path component so it
> will pick the latest version of "bar".

I think the implied question was "Given that this works if I don't use
an explicit path, why isn't there a way for it to work when I do want
to give an explicit path?  (Now that explicit paths work at all.)"

Where "this works" means having a function name with a slash in it.

I think it's a rather rare case that we may not want to bother with, but
we've bothered with all sorts of other rarities.

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Wed, 18 Jan 2017 14:26:12 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> I think the implied question was "Given that this works if I don't use
> an explicit path, why isn't there a way for it to work when I do want
> to give an explicit path?  (Now that explicit paths work at all.)"
> 
> Where "this works" means having a function name with a slash in it.
> 
> I think it's a rather rare case that we may not want to bother with, but
> we've bothered with all sorts of other rarities.

I think the official answer is "well, you'll just have to lump it".

If there's a clear use case that someone needs we can no doubt find a
way.  I hadn't even registered that this worked (with $fpath) until I
started looking.

pws

Re: PATH: autoload with explicit path

[Peter Stephenson]

On Tue, 24 Jan 2017 11:17:34 +0100
Mikael Magnusson <mikachu@gmail.com> wrote:
> This patch is probably a bad idea, it's extremely common to do this:
> PROGRAM="$(which program)"
> and with the above change that no longer works.

Yes, that was supposed to be limited to the chattier -v output.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index 2fb1a70..219fbc9 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3774,9 +3774,11 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 		    if (wd) {
 			printf("%s: command\n", *argv);
 		    } else {
-			if (v && !csh)
+			if (v && !csh) {
 			    zputs(*argv, stdout), fputs(" is ", stdout);
-			quotedzputs(buf, stdout);
+			    quotedzputs(buf, stdout);
+			} else
+			    zputs(buf, stdout);
 			if (OPT_ISSET(ops,'s') || OPT_ISSET(ops, 'S'))
 			    print_if_link(buf, OPT_ISSET(ops, 'S'));
 			fputc('\n', stdout);
@@ -3806,9 +3808,11 @@ bin_whence(char *nam, char **argv, Options ops, int func)
 	    if (wd) {
 		printf("%s: command\n", *argv);
 	    } else {
-		if (v && !csh)
+		if (v && !csh) {
 		    zputs(*argv, stdout), fputs(" is ", stdout);
-		quotedzputs(cnam, stdout);
+		    quotedzputs(cnam, stdout);
+		} else
+		    zputs(cnam, stdout);
 		if (OPT_ISSET(ops,'s') || OPT_ISSET(ops,'S'))
 		    print_if_link(cnam, OPT_ISSET(ops,'S'));
 		fputc('\n', stdout);

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Thu, 12 Jan 2017 16:23:38 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> The point is that the next time writes:
> .
>     foo(shf->filename);
> .
> that won't compile, so he will be forced to take into account the two
> distinct overloaded meanings.

OK, I can see that might be useful.

> This might have prevented the bug you fixed in 40335 from being
> written.

No, it would still have been possible to resolve all uses into one form
or the other.  The problem was it was done implicitly by location in the
code and needed discriminating by something else such as the new bit
(or the implied NULL / non-NULL test of a separate field).

> I'll see if adding it makes things clearer, will post if it does.

Fine, you might want to see what happens with the change for caching the
autoload directory first as it hits the same lines.

pws

Re: PATCH: autoload with explicit path

[Daniel Shahaf]

Peter Stephenson wrote on Thu, Jan 12, 2017 at 16:16:51 +0000:
> On Thu, 12 Jan 2017 16:09:21 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Of course it would be exactly the same machine code, but the source code
> > would be more robust against bugs.  It's a lot harder to refer to the
> > wrong union member than to use the ->filename member without testing
> > PM_LOADDIR first.
> 
> Hmm, feel free to write this if you think you can make things clearer;
> the bit setting will tell you what needs doing in each case.  But I
> don't really see how it helps.  If the bit happens to be set you will do
> it one way having exactly the same effect as if you did it the other
> way, so I don't see the gain.

The point is that the next time writes:
.
    foo(shf->filename);
.
that won't compile, so he will be forced to take into account the two
distinct overloaded meanings.  This might have prevented the bug you
fixed in 40335 from being written.

I'll see if adding it makes things clearer, will post if it does.

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Thu, 12 Jan 2017 16:09:21 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Of course it would be exactly the same machine code, but the source code
> would be more robust against bugs.  It's a lot harder to refer to the
> wrong union member than to use the ->filename member without testing
> PM_LOADDIR first.

Hmm, feel free to write this if you think you can make things clearer;
the bit setting will tell you what needs doing in each case.  But I
don't really see how it helps.  If the bit happens to be set you will do
it one way having exactly the same effect as if you did it the other
way, so I don't see the gain.

pws

Re: PATCH: autoload with explicit path

[Daniel Shahaf]

Peter Stephenson wrote on Thu, Jan 12, 2017 at 15:59:20 +0000:
> On Thu, 12 Jan 2017 15:40:57 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > We could also put these two members in a union{} if they are mutually
> > exclusive (if at least one of them is NULL at all times).
> 
> They are mutually exclusive, as one is only useful for an autoload and
> one is only useful with real source, which is why it uses the same
> pointer.  Making it a union doesn't actually change anything: it's still
> the same set of of reads and writes.

Of course it would be exactly the same machine code, but the source code
would be more robust against bugs.  It's a lot harder to refer to the
wrong union member than to use the ->filename member without testing
PM_LOADDIR first.

Re: PATCH: autoload with explicit path

[Vin Shelton]

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

On Thu, Jan 12, 2017 at 7:56 AM, Peter Stephenson <p.stephenson@samsung.com>
wrote:

> I can't reproduce this using the normal tests, but I have found a
> possible cause and added a test that does fail in the same way.  This
> patch fixes that failure.
>
> The problem is the overloading of filename in struct shfunc.  If the
> function was alrady loaded, which is the case if it contains an
> explicit autoload -X (rather than one generated internally by looking
> at the flags for an undefined function), then the filename indicates the
> location of the source for the function.  If this came from a file with
> an absolute path, and there was no explicit directory path in the
> autoload -X statement, the file path was erroneously taken as a
> directory for loading.
>
> This adds an explicit flag to indicate filename is being used for that
> purpose, unsetting it when the filename is set to the file's path.
>
> Also be a bit more careful checking if a function wasn't let loaded when
> using the new functions options.  If it's already loaded they're
> irrelevant.
>

That has fixed the failures for me: I'm now seeing 48 successful tests.

Thanks,
  Vin

Re: PATCH: autoload with explicit path

[Peter Stephenson]

On Thu, 12 Jan 2017 15:40:57 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> We could also put these two members in a union{} if they are mutually
> exclusive (if at least one of them is NULL at all times).

They are mutually exclusive, as one is only useful for an autoload and
one is only useful with real source, which is why it uses the same
pointer.  Making it a union doesn't actually change anything: it's still
the same set of of reads and writes.

pws

Re: PATCH: autoload with explicit path

[Daniel Shahaf]

Peter Stephenson wrote on Thu, Jan 12, 2017 at 12:56:02 +0000:
> The problem is the overloading of filename in struct shfunc.  If the
> function was alrady loaded, which is the case if it contains an
> explicit autoload -X (rather than one generated internally by looking
> at the flags for an undefined function), then the filename indicates the
> location of the source for the function.  If this came from a file with
> an absolute path, and there was no explicit directory path in the
> autoload -X statement, the file path was erroneously taken as a
> directory for loading.
> 
> This adds an explicit flag to indicate filename is being used for that
> purpose, unsetting it when the filename is set to the file's path.

I would suggest using two separate struct members, rather than one whose
semantics depend on a flag.  It'd be too easy to forget to check the
flag in some (existing or future) location.

E.g.,

struct shfunc {
    char *filename1;             /* Name of file located in */
    char *filename2;             /* Name of file to autoload from */
};

We could also put these two members in a union{} if they are mutually
exclusive (if at least one of them is NULL at all times).

Re: PATCH: autoload with explicit path

[Peter Stephenson]

(It's a bit late now, but that "PATH" in the subject line was a typo
because the Subject line in my email client at home is in very small
print.)

Vin is reporting failures where the symptom in both cases appears to be
not finding an autoload function when using "autoload -X" within the
function (with fpath=(.), but that aspect is probably irrelevant).

I can't reproduce this using the normal tests, but I have found a
possible cause and added a test that does fail in the same way.  This
patch fixes that failure.

The problem is the overloading of filename in struct shfunc.  If the
function was alrady loaded, which is the case if it contains an
explicit autoload -X (rather than one generated internally by looking
at the flags for an undefined function), then the filename indicates the
location of the source for the function.  If this came from a file with
an absolute path, and there was no explicit directory path in the
autoload -X statement, the file path was erroneously taken as a
directory for loading.

This adds an explicit flag to indicate filename is being used for that
purpose, unsetting it when the filename is set to the file's path.

Also be a bit more careful checking if a function wasn't let loaded when
using the new functions options.  If it's already loaded they're
irrelevant.

pws

P.S. test failure reports to the list, please, so everyone can see ---
thanks.

> From: Vin Shelton <acs@alumni.princeton.edu>
> To: Peter Stephenson <p.w.stephenson@ntlworld.com>
> Cc: Zsh hackers list <zsh-workers@zsh.org>
> Date: 11 January 2017 at 23:15
> Subject: Re: PATH: autoload with explicit path
> 
> Hi, Peter -
> 
> Thanks for this. I see two test failures:
> 
> opt/src/zsh-2017-01-11/Test/A04redirect.ztst: all tests successful.
> /opt/src/zsh-2017-01-11/Test/A05execution.ztst: starting.
> Test /opt/src/zsh-2017-01-11/Test/A05execution.ztst failed: bad status 1,
> expected 0 from:
>  unfunction functst
>  print "print Yet another version" >functst
>  functst() { autoload -X; }
>  functst
> Error output:
> (eval):1: functst: function definition file not found
> Was testing: autoloading via -X
> /opt/src/zsh-2017-01-11/Test/A05execution.ztst: test failed.
> /opt/src/zsh-2017-01-11/Test/A06assign.ztst: starting.
> 
> and
> 
> /opt/src/zsh-2017-01-11/Test/C03traps.ztst: all tests successful.
> /opt/src/zsh-2017-01-11/Test/C04funcdef.ztst: starting.
> --- /tmp/zsh.ztst.8259/ztst.out 2017-01-11 17:56:26.309239694 -0500
> +++ /tmp/zsh.ztst.8259/ztst.tout 2017-01-11 17:56:26.309239694 -0500
> @@ -1,4 +1,4 @@
> -oops was successfully autoloaded
>  oops () {
> 
> *   print oops was successfully autoloaded
> 
> *   # undefined
> *   builtin autoload -X /opt/src/zsh-2017-01-11/Test/ztst.zsh
> }
> Test /opt/src/zsh-2017-01-11/Test/C04funcdef.ztst failed: output differs
> from expected as shown above for:
> (
>  fpath=(.)
>  print "print oops was successfully autoloaded" >oops
>  oops() { eval autoload -X }
>  oops
>  which -x2 oops
> )
> Error output:
> (eval):1: oops: function definition file not found
> Was testing: autoload containing eval
> /opt/src/zsh-2017-01-11/Test/C04funcdef.ztst: test failed.
> /opt/src/zsh-2017-01-11/Test/C05debug.ztst: starting.
> /opt/src/zsh-2017-01-11/Test/C05debug.ztst: all tests successful.
> 
> I see this on a variety of linux systems. Please let me know if you need
> more details.

diff --git a/Src/builtin.c b/Src/builtin.c
index b986dd8..716ddd4 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2936,10 +2936,11 @@ check_autoload(Shfunc shf, char *name, Options ops, int func)
     {
 	return eval_autoload(shf, name, ops, func);
     }
-    if (OPT_ISSET(ops,'r') || OPT_ISSET(ops,'R'))
+    if ((OPT_ISSET(ops,'r') || OPT_ISSET(ops,'R')) &&
+	(shf->node.flags & PM_UNDEFINED))
     {
 	char *dir_path;
-	if (shf->filename) {
+	if (shf->filename && (shf->node.flags & PM_LOADDIR)) {
 	    char *spec_path[2];
 	    spec_path[0] = shf->filename;
 	    spec_path[1] = NULL;
@@ -2964,6 +2965,7 @@ check_autoload(Shfunc shf, char *name, Options ops, int func)
 		dir_path = xsymlink(dir_path, 1);
 	    }
 	    shf->filename = ztrdup(dir_path);
+	    shf->node.flags |= PM_LOADDIR;
 	    return 0;
 	}
 	if (OPT_ISSET(ops,'R')) {
@@ -3017,7 +3019,8 @@ add_autoload_function(Shfunc shf, char *funcname)
 {
     char *nam;
     if (*funcname == '/' && funcname[1] &&
-	(nam = strrchr(funcname, '/')) && nam[1]) {
+	(nam = strrchr(funcname, '/')) && nam[1] &&
+	(shf->node.flags & PM_UNDEFINED)) {
 	char *dir;
 	nam = strrchr(funcname, '/');
 	if (nam == funcname) {
@@ -3028,6 +3031,7 @@ add_autoload_function(Shfunc shf, char *funcname)
 	}
 	zsfree(shf->filename);
 	shf->filename = ztrdup(dir);
+	shf->node.flags |= PM_LOADDIR;
 	shfunctab->addnode(shfunctab, ztrdup(nam), shf);
     } else {
 	shfunctab->addnode(shfunctab, ztrdup(funcname), shf);
@@ -3278,6 +3282,7 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	    if (*argv) {
 		zsfree(shf->filename);
 		shf->filename = ztrdup(*argv);
+		on |= PM_LOADDIR;
 	    }
 	    shf->node.flags = on;
 	    ret = eval_autoload(shf, funcname, ops, func);
diff --git a/Src/exec.c b/Src/exec.c
index 7bec7ce..68c455b 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -5160,7 +5160,8 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
     pushheap();
 
     noaliases = (shf->node.flags & PM_UNALIASED);
-    if (shf->filename && shf->filename[0] == '/')
+    if (shf->filename && shf->filename[0] == '/' &&
+	(shf->node.flags & PM_LOADDIR))
     {
 	char *spec_path[2];
 	spec_path[0] = dupstring(shf->filename);
@@ -5203,7 +5204,7 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 		shf->funcdef = prog;
 	    else
 		shf->funcdef = dupeprog(prog, 0);
-	    shf->node.flags &= ~PM_UNDEFINED;
+	    shf->node.flags &= ~(PM_UNDEFINED|PM_LOADDIR);
 	    zsfree(shf->filename);
 	    shf->filename = fname;
 	} else {
@@ -5227,7 +5228,7 @@ loadautofn(Shfunc shf, int fksh, int autol, int current_fpath)
 	    shf->funcdef = stripkshdef(prog, shf->node.nam);
 	else
 	    shf->funcdef = dupeprog(stripkshdef(prog, shf->node.nam), 0);
-	shf->node.flags &= ~PM_UNDEFINED;
+	shf->node.flags &= ~(PM_UNDEFINED|PM_LOADDIR);
 	zsfree(shf->filename);
 	shf->filename = fname;
     }
diff --git a/Src/zsh.h b/Src/zsh.h
index 67c5a35..7d18333 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1823,6 +1823,7 @@ struct tieddata {
 
 /* Remaining flags do not correspond directly to command line arguments */
 #define PM_DONTIMPORT_SUID (1<<19) /* do not import if running setuid */
+#define PM_LOADDIR      (1<<19) /* (function) filename gives load directory */
 #define PM_SINGLE       (1<<20) /* special can only have a single instance  */
 #define PM_LOCAL	(1<<21) /* this parameter will be made local        */
 #define PM_SPECIAL	(1<<22) /* special builtin parameter                */
diff --git a/Test/C04funcdef.ztst b/Test/C04funcdef.ztst
index 1821b78..7100280 100644
--- a/Test/C04funcdef.ztst
+++ b/Test/C04funcdef.ztst
@@ -419,6 +419,16 @@
 0:autoload -dX with path
 >I have been loaded by default path.
 
+  (
+    fpath=(.)
+    print 'loadthisfunc() { autoload -X }' >loadthisfunc_sourceme
+    print 'print Function was loaded correctly.' >loadthisfunc
+    source $PWD/loadthisfunc_sourceme
+    loadthisfunc
+  )
+0: autoload -X interaction with absolute filename used for source location
+>Function was loaded correctly.
+
 %clean
 
  rm -f file.in file.out