Re: PATCH: autoload with explicit path

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

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]

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

[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

[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

[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 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: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

[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: 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 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

[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 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

[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

[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

[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 */
 }
 
 /**/