PATCH: 3.1.5: ``***'' symlink follow broken

PATCH: 3.1.5: ``***'' symlink follow broken

[Geoff Wing]

Heyla,
``***'' recursive globbing with symlink follow has been broken since at
least 3.1.2 (maybe before):

% mkdir 1 2
% ln -s ../2 1/
% echo > 2/foo
% zsh-3.1.5 -fc 'echo 1/***/foo'
zsh: no matches found: 1/***/foo
% zsh-3.1.2 -fc 'echo 1/***/foo'
zsh: no matches found: 1/***/foo
% zsh-3.1.0-test3 -fc 'echo 1/***/foo'
1/2/foo
% zsh-3.0.5 -fc 'echo 1/***/foo'
1/2/foo


This patch is probably suboptimal (possibly wrong) and may encourage someone
to make a real patch.


*** Src/glob.c.org	Wed Oct 14 07:51:55 1998
--- Src/glob.c	Thu Nov 12 16:56:25 1998
***************
*** 351,357 ****
  	int subdirlen = 0;
  
  	fn = pathbuf[pathbufcwd] ? unmeta(pathbuf + pathbufcwd) : ".";
! 	if (dirs) {
  	    struct stat st;
  	    stat(fn, &st);
  	    /* a directory with subdirectories has link count greater than 2 */
--- 351,357 ----
  	int subdirlen = 0;
  
  	fn = pathbuf[pathbufcwd] ? unmeta(pathbuf + pathbufcwd) : ".";
! 	if (dirs && !q->follow) {
  	    struct stat st;
  	    stat(fn, &st);
  	    /* a directory with subdirectories has link count greater than 2 */

-- 
Geoff Wing   <gcw@pobox.com>            Mobile : 0412 162 441
Work URL: http://www.primenet.com.au/   Ego URL: http://pobox.com/~gcw/

Re: PATCH: 3.1.5: ``***'' symlink follow broken

[Bart Schaefer]

On Nov 12,  5:20pm, Geoff Wing wrote:
} Subject: PATCH: 3.1.5: ``***'' symlink follow broken
}
} ``***'' recursive globbing with symlink follow has been broken since at
} least 3.1.2 (maybe before):
[...]
} This patch is probably suboptimal (possibly wrong) and may encourage someone
} to make a real patch.
[...]
} ! 	if (dirs && !q->follow) {

I think the semantically correct test is

	if (dirs && !closure) {

but it may be that the only time it makes a difference is when all of
the subdirectories are symlinks.  I keep thinking there must be some
other case that will fail if !q->follow is tested instead, but I can't
quite figure out what.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: 3.1.5: ``***'' symlink follow broken

[Peter Stephenson]

Geoff Wing wrote:
> Heyla,
> ``***'' recursive globbing with symlink follow has been broken since at
> least 3.1.2 (maybe before):

It's amazing how the shell can totally break down without anyone
noticing.

> This patch is probably suboptimal (possibly wrong) and may encourage someone
> to make a real patch.

It works fine, but there's one minor tweak: you can still reject
things that aren't directories (the test correctly uses stat(), not
lstat(), so it may be a link to a directory), but the link count test
doesn't apply when the directory could contain only symbolic links.
The following patch is a replacement which does that.  I can't imagine
the difference is that important -- in fact, since the opendir() will
fail if the thing isn't a directory, it's hard to see that the stat()
at this point gains much at all.  Unless someone knows something
sinister about opendir()?

*** Src/glob.c.follow	Sun Nov  8 16:01:28 1998
--- Src/glob.c	Thu Nov 12 14:59:26 1998
***************
*** 365,372 ****
  	if (dirs) {
  	    struct stat st;
  	    stat(fn, &st);
! 	    /* a directory with subdirectories has link count greater than 2 */
! 	    if (!S_ISDIR(st.st_mode) || st.st_nlink == 2)
  		return;
  	}
  	lock = opendir(fn);
--- 365,374 ----
  	if (dirs) {
  	    struct stat st;
  	    stat(fn, &st);
! 	    /* a directory with subdirectories has link count greater than 2
! 	     * (unless the `subdirectory' is a symbolic link).
! 	     */
! 	    if (!S_ISDIR(st.st_mode) || (st.st_nlink == 2 && !q->follow))
  		return;
  	}
  	lock = opendir(fn);


-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56100 Pisa, Italy

Re: PATCH: 3.1.5: ``***'' symlink follow broken

[Bruce Stephens]

Peter Stephenson <pws@ibmth.df.unipi.it> writes:

> Geoff Wing wrote:
> > Heyla,
> > ``***'' recursive globbing with symlink follow has been broken since at
> > least 3.1.2 (maybe before):
> 
> It's amazing how the shell can totally break down without anyone
> noticing.

Not really.  I suspect there are features introduced in zsh years ago
that I've never learned how to use.  Indeed, I print out and read the
manual every few months, on average, and find half a dozen useful
features I'd never noticed before.  Maybe it would be a good idea to
construct some regression tests?

In this specific case, although I use ** every day, I never use ***.

Re: PATCH: 3.1.5: ``***'' symlink follow broken

[Bart Schaefer]

On Nov 12,  3:10pm, Peter Stephenson wrote:
} Subject: Re: PATCH: 3.1.5: ``***'' symlink follow broken
}
} Geoff Wing wrote:
} > ``***'' recursive globbing with symlink follow has been broken
} 
} It's amazing how the shell can totally break down without anyone
} noticing.

I must sheepishly admit that I only really use 3.1.x when I'm playing
with it or changing its code.  The machine I use most of the day at
work has 3.0.5-extended installed.

} > This patch is probably suboptimal (possibly wrong)
} 
} It works fine, but there's one minor tweak: you can still reject
} things that aren't directories (the test correctly uses stat(), not
} lstat(), so it may be a link to a directory), but the link count test
} doesn't apply when the directory could contain only symbolic links.

Yes, that's exactly the problem ... but ...

} The following patch is a replacement which does that.  I can't imagine
} the difference is that important -- in fact, since the opendir() will
} fail if the thing isn't a directory, it's hard to see that the stat()
} at this point gains much at all.  Unless someone knows something
} sinister about opendir()?

I *think* what's going on here is that `dirs' means the *next* level of
comparison needs to match directories inside which the final files may
reside.  The stat() is intended to short-circuit the search when there
are no such subdirectories, before iterating through the entries in the
`while (zreaddir())' loop that follows.  It's only a bonus that it also
avoids calling opendir() on a non-directory, and it's useless if the
link count test is invalid.

So *unless* somebody knows of a reason not to do opendir() on something
that isn't a directory, I think Geoff's patch is actually better [on the
assumption that a failed opendir() is faster than a successful stat()].
Late last night I was of the opinion that `closure' mattered more than
did `q->follow', but I've since revised that opinion.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: 3.1.5: ``***'' symlink follow broken

[Geoff Wing]

Heyla,
I've got a much better patch for it now.   Just above the line ``if (dirs) {''
put ``#if 0''  and after the closing brace for it, put "#endif"

% mkdir foo bar
% cd foo
% I=0; repeat 10 do mkdir $I; ln ../foo/$I ../bar/; echo > ../bar/$I/$I; let I++; done
% cd ../bar
% zsh-3.1.5 -fc 'echo */*'
zsh: no matches found: */*
% zsh-3.1.2 -fc 'echo */*'
zsh: no matches found: */*
% zsh-3.1.0-test3 -fc 'echo */*'
0/0 1/1 2/2 3/3 4/4 5/5 6/6 7/7 8/8 9/9
% zsh-3.0.5 -fc 'echo */*'
0/0 1/1 2/2 3/3 4/4 5/5 6/6 7/7 8/8 9/9

Maybe the whole  if (dirs)  thing is a case of trying to be too clever :-)
-- 
Geoff Wing   <gcw@pobox.com>            Mobile : 0412 162 441
Work URL: http://www.primenet.com.au/   Ego URL: http://pobox.com/~gcw/

Re: PATCH: 3.1.5: ``***'' symlink follow broken

[Peter Stephenson]

"Bart Schaefer" wrote:
> So *unless* somebody knows of a reason not to do opendir() on something
> that isn't a directory, I think Geoff's patch is actually better [on the
> assumption that a failed opendir() is faster than a successful stat()].

I thought about it some more last night and came to this conclusion
too.  Only if you can test the number of subdirectories, and hence
prune empty subdirectories, is the stat() worthwhile.  I'm very
tempted by Geoff's second suggestion, though.

> Late last night I was of the opinion that `closure' mattered more than
> did `q->follow', but I've since revised that opinion.

I think it's written so that `closure' matters as little as possible
when it's analysing individual directories.

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56100 Pisa, Italy

PATCH: */* broken (Re: PATCH: 3.1.5: ``***'' symlink follow broken)

[Bart Schaefer]

On Nov 13,  5:59pm, Geoff Wing wrote:
} Subject: Re: PATCH: 3.1.5: ``***'' symlink follow broken
}
} I've got a much better patch for it now.   Just above the line ``if (dirs) {''
} put ``#if 0''  and after the closing brace for it, put "#endif"

Here's a patch that makes the code similar (not identical, there are other
changes that had to be kept) to the 3.0.5 version of the same section.

Index: Src/glob.c
===================================================================
--- glob.c	1998/11/12 17:56:26	1.8
+++ glob.c	1998/11/13 17:06:09
@@ -355,21 +355,12 @@
 	    insert(c->str, 0);
     } else {
 	/* Do pattern matching on current path section. */
-	char *fn;
+	char *fn = pathbuf[pathbufcwd] ? unmeta(pathbuf + pathbufcwd) : ".";
 	int dirs = !!q->next;
-	DIR *lock;
+	DIR *lock = opendir(fn);
 	char *subdirs = NULL;
 	int subdirlen = 0;
 
-	fn = pathbuf[pathbufcwd] ? unmeta(pathbuf + pathbufcwd) : ".";
-	if (dirs) {
-	    struct stat st;
-	    stat(fn, &st);
-	    /* a directory with subdirectories has link count greater than 2 */
-	    if (!S_ISDIR(st.st_mode) || st.st_nlink == 2)
-		return;
-	}
-	lock = opendir(fn);
 	if (lock == NULL)
 	    return;
 	while ((fn = zreaddir(lock, 1)) && !errflag) {

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: */* broken (Re: PATCH: 3.1.5: ``***'' symlink follow broken)

[Geoff Wing]

Bart Schaefer <schaefer@brasslantern.com> typed:
:On Nov 13,  5:59pm, Geoff Wing wrote:
:} Subject: Re: PATCH: 3.1.5: ``***'' symlink follow broken
:} I've got a much better patch for it now.   Just above the line ``if (dirs) {''
:} put ``#if 0''  and after the closing brace for it, put "#endif"
:Here's a patch that makes the code similar (not identical, there are other
:changes that had to be kept) to the 3.0.5 version of the same section.

Wow, that patch looked suspiciously like it had _exactly_ the same
functionality as my descriptive ``#if 0'' patch.  Isn't it amazing
how you can have a total lack a sobriety and still type properly?
-- 
Geoff Wing   <gcw@pobox.com>            Mobile : 0412 162 441
Work URL: http://www.primenet.com.au/   Ego URL: http://pobox.com/~gcw/