rc-list - mailing list for the rc(1) shell
 help / Atom feed
* Re: Bug#62339: rc bug
       [not found] ` <QDwAALDY+jgTYAcA@ltsun0.star.le.ac.uk>
@ 2000-04-17 19:59   ` Decklin Foster
  2000-04-18  4:45     ` oops, let's try that one again Decklin Foster
  0 siblings, 1 reply; 4+ messages in thread
From: Decklin Foster @ 2000-04-17 19:59 UTC (permalink / raw)
  To: Tim Goodwin; +Cc: rc

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

Tim Goodwin writes:

> Thanks for the report.  I can reproduce this.
> 
> It's not been reported before, so I don't know what's going on, but I'll
> let you know as soon as I have a fix.

I did send something to to mailing list on Saturday, but it hasn't
gotten through yet. I seem to have figured out the use of m in a List
structure: if there are no metacharacters, NULL, and if there are, we
put a \0 where there's a non-metacharacter in the corresponding part
of n, and a \001 where there is.

Now in lmatch() when we go through looking for '*'s, we have this
loop:

	for (i = 0; p->w[i] != '\0'; i++)
		if (p->w[i] != '*' || p->m[i] != 1) {

Which assumes that m is as big as w (i.e, not NULL). I came up with
this replacement, which works for all the test cases I can think of:

        if (s == NULL) {
                if (p == NULL) /* null matches null */
                        return TRUE; 
                for (; p != NULL; p = p->n) /* one or more stars match null */
                        if (p->w && strspn(p->w, "*") == strlen(p->w) &&
                            p->m && strspn(p->m, "\001") == strlen(p->m))
                                return TRUE;
                return FALSE;
        }

I was also able to get rid of 'okay', which i found hard to follow.
The comment about the null string is gone too, because IMO the null
string fails for the same reason 'foo' fails. There's really no need
for an explicit check.

A patch is attached. Out of curiosity, why was the decision made to
use '\000' and '\001' instead of, say, '0' and '1'? For example in the
old code, we have "p->m[i] != 1", and it wouldn't be that hard to
write "p->m[i] != '1'" instead. I'm sure there's a reason, but I can't
guess it.

-- 
Written with 100% free software. Please support the following websites:
www.debian.org www.noamazon.com www.gnu.org www.opendvd.org lpf.ai.mit.edu

[-- Attachment #2: rc.patch --]
[-- Type: text/plain, Size: 837 bytes --]

--- glob.c.old	Wed Oct 21 07:01:46 1998
+++ glob.c	Mon Apr 17 15:55:30 2000
@@ -34,23 +34,13 @@
 
 extern bool lmatch(List *s, List *p) {
 	List *q;
-	int i;
-	bool okay;
 	if (s == NULL) {
 		if (p == NULL) /* null matches null */
 			return TRUE;
-		for (; p != NULL; p = p->n) { /* one or more stars match null */
-			if (*p->w != '\0') { /* the null string is a special case; it does *not* match () */
-				okay = TRUE;
-				for (i = 0; p->w[i] != '\0'; i++)
-					if (p->w[i] != '*' || p->m[i] != 1) {
-						okay = FALSE;
-						break;
-					}
-				if (okay)
-					return TRUE;
-			}
-		}
+		for (; p != NULL; p = p->n) /* one or more stars match null */
+			if (p->w && strspn(p->w, "*") == strlen(p->w) &&
+			    p->m && strspn(p->m, "\001") == strlen(p->m))
+				return TRUE;
 		return FALSE;
 	}
 	for (; s != NULL; s = s->n)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* oops, let's try that one again
  2000-04-17 19:59   ` Bug#62339: rc bug Decklin Foster
@ 2000-04-18  4:45     ` Decklin Foster
  0 siblings, 0 replies; 4+ messages in thread
From: Decklin Foster @ 2000-04-18  4:45 UTC (permalink / raw)
  To: rc

Decklin Foster writes:

> if (p->w && strspn(p->w, "*") == strlen(p->w) &&
>     p->m && strspn(p->m, "\001") == strlen(p->m))
>         return TRUE;

OK, raise your hand if you can spot the bug here. The test case I came
up with after posting this code, which breaks it, is:

~ () *'*'

(any number of quoted stars will do.) Not a crasher, but still
obviously wrong. Thankfully, the fix involves changing only one
character:

	if (p->w && strspn(p->w, "*") == strlen(p->w) &&
	    p->m && strspn(p->m, "\001") == strlen(p->w))
		return TRUE;

But this of course can be simplified to

	if (p->w && p->m && strspn(p->w, "*") == strspn(p->m, "\001")
					      == strlen(p->w))
		return TRUE;

Which is somewhat more efficient, anyway. If you would like me to stop
thinking aloud, just speak up, but I don't think this list is
suffering from too much traffic ;-)

[Note: my normal style would be to pull "== strlen(p->w))" back to the
first tab stop and then put "return TRUE;" after it on the same line,
but the rc Way seems to be to use

	if (foo)
		bar;

instead of

	if (foo) bar;

which is what I do for short statements. In short, reformat as you see
fit.]

-- 
Written with 100% free software. Please support the following websites:
www.debian.org www.noamazon.com www.gnu.org www.opendvd.org lpf.ai.mit.edu


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Bug#62339: rc bug
@ 2000-04-18  6:30 Byron Rakitzis
  2000-04-19 14:39 ` Tim Goodwin
  0 siblings, 1 reply; 4+ messages in thread
From: Byron Rakitzis @ 2000-04-18  6:30 UTC (permalink / raw)
  To: fosterd, tjg; +Cc: rc

Thanks for tracking this down.

I'm the original miscreant as far as the documentation style of rc is
concerned.

Much of this code was written roughly 10 years ago, I'd certainly write
it differently now.

The metacharacter array is properly an array of booleans. Hence the
explicit checks against 0 and 1, which should be cleaned up somehow
to reflect their function. A macro "ismeta(p, i)", perhaps. I don't
know where the tests against '\001' snuck in. I think it's a mistake.

> Now in lmatch() when we go through looking for '*'s, we have this
> loop:
>
> 	for (i = 0; p->w[i] != '\0'; i++)
> 		if (p->w[i] != '*' || p->m[i] != 1) {
>
> Which assumes that m is as big as w (i.e, not NULL). I came up with
> this replacement, which works for all the test cases I can think of:
>
>         if (s == NULL) {
>                 if (p == NULL) /* null matches null */
>                         return TRUE; 
>                 for (; p != NULL; p = p->n) /* one or more stars match null */
>                         if (p->w && strspn(p->w, "*") == strlen(p->w) &&
>                             p->m && strspn(p->m, "\001") == strlen(p->m))
>                                 return TRUE;
>                 return FALSE;
>         }
>
> I was also able to get rid of 'okay', which i found hard to follow.
> The comment about the null string is gone too, because IMO the null
> string fails for the same reason 'foo' fails. There's really no need
> for an explicit check.

This is good.

I'm picking nits here, but since I obsessed over this code a while ago,
I might suggest two small changes to your fix:

>                         if (p->w && strspn(p->w, "*") == strlen(p->w) &&
>                             p->m && strspn(p->m, "\001") == strlen(p->m))

to:

	if (p->m != NULL &&
		strspn(p->w, "*") == strlen(p->w) &&
		strlen(p->m) == strlen(p->w))

Firstly, p->w can not be NULL here; there can be no null words in
a word list, only empty strings which are represented as "".

As long as you are getting sneaky about using the string library to find
zeroes in a character array, you may also rephrase the metacharacter
test as two calls to strlen.

Byron.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: Bug#62339: rc bug
  2000-04-18  6:30 Bug#62339: rc bug Byron Rakitzis
@ 2000-04-19 14:39 ` Tim Goodwin
  0 siblings, 0 replies; 4+ messages in thread
From: Tim Goodwin @ 2000-04-19 14:39 UTC (permalink / raw)
  To: rc

A big "thank you!" to Byron and Decklin for getting to the bottom of
this bug.  I've incorporated their fix, plus a couple of other recent
fixes into a new snapshot, which you can get from here.

    http://www.star.le.ac.uk/~tjg/rc/snap/rc-1.6s20000419.tar.gz

This has only been very lightly tested; there might well be some
booboos.  After the Easter break, I'll scrutinize the changes again, and
wrap them into a new beta release.

Changes since the last beta are summarized below.

Tim.


2000-04-19

  Bug: isatty() tests in input.c are relevant to any fd, not just 0.
  Now `. -i /dev/tty' works right.

  Bug: fn sigexit wasn't always cleared in child shells.

  Bug: `~ () '*'' dumped core.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <Pine.LNX.3.96.1000425010821.264C-100000@dwarf>
     [not found] ` <QDwAALDY+jgTYAcA@ltsun0.star.le.ac.uk>
2000-04-17 19:59   ` Bug#62339: rc bug Decklin Foster
2000-04-18  4:45     ` oops, let's try that one again Decklin Foster
2000-04-18  6:30 Bug#62339: rc bug Byron Rakitzis
2000-04-19 14:39 ` Tim Goodwin

rc-list - mailing list for the rc(1) shell

Archives are clonable: git clone --mirror http://inbox.vuxu.org/rc-list

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.rc-list


AGPL code for this site: git clone https://public-inbox.org/ public-inbox