Is "read -AE" buggy?

Is "read -AE" buggy?

[Bart Schaefer]

Via fooling around with the "wc and leading spaces" thread on -users:

schaefer<535> wc config.modules | read -AE foo
64
461
4569
config.modules
64
461
4569
config.modules

Why did it print everything twice?  Doesn't happen without -A:

schaefer<529> wc config.modules | read -E       
64  461 4569 config.modules

Doesn't happen with -e:

schaefer<540> wc config.modules | read -Ae                         
64
461
4569
config.modules

"cvs annotate" pegs this for rev 1.82 (workers/17582) but I suspect that
is an unrelated syntactic change and really it's been this way since
before the CVS repository was first set up in April 1999.  Indeed, I
can reproduce this in zsh 2.4 (no, that's not a typo for 4.2).

Re: Is "read -AE" buggy?

[Peter Stephenson]

On Tue, 23 Aug 2011 00:18:27 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> schaefer<535> wc config.modules | read -AE foo
> 64
> 461
> 4569
> config.modules
> 64
> 461
> 4569
> config.modules

I think this is down to the fact that bin_read() is a complete mess
where people dump in new features without any attempt to impose
structure (fancy that happening in zsh...) so the obvious fix is as good
as any.

We can at least test this, although in general getting this sort of
thing right with read is (at least) a quadratic problem.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.252
diff -p -u -r1.252 builtin.c
--- Src/builtin.c	3 Jun 2011 19:54:44 -0000	1.252
+++ Src/builtin.c	28 Aug 2011 16:46:33 -0000
@@ -5549,7 +5549,7 @@ bin_read(char *name, char **args, Option
 	*bptr = '\0';
 #endif
 	/* dispose of word appropriately */
-	if (OPT_ISSET(ops,'e') || OPT_ISSET(ops,'E')) {
+	if (OPT_ISSET(ops,'e')) {
 	    zputs(buf, stdout);
 	    putchar('\n');
 	}
@@ -5581,7 +5581,7 @@ bin_read(char *name, char **args, Option
 	     : (char **)zalloc((al + 1) * sizeof(char *)));
 
 	for (pp = p, n = firstnode(readll); n; incnode(n)) {
-	    if (OPT_ISSET(ops,'e') || OPT_ISSET(ops,'E')) {
+	    if (OPT_ISSET(ops,'E')) {
 		zputs((char *) getdata(n), stdout);
 		putchar('\n');
 	    }
Index: Test/B04read.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/B04read.ztst,v
retrieving revision 1.5
diff -p -u -r1.5 B04read.ztst
--- Test/B04read.ztst	7 Jan 2011 10:05:35 -0000	1.5
+++ Test/B04read.ztst	28 Aug 2011 16:46:33 -0000
@@ -93,3 +93,20 @@
   read foo) <<<bar
 1:return status on failing to set parameter
 ?(eval):2: read-only variable: foo
+
+  read -AE array <<<'one two three'
+  print ${(j.:.)array}
+0:Behaviour of -A and -E combination
+>one
+>two
+>three
+>one:two:three
+
+  array=()
+  read -Ae array <<<'four five six'
+  print ${(j.:.)array}
+0:Behaviour of -A and -e combination
+>four
+>five
+>six
+>

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: Is "read -AE" buggy?

[Bart Schaefer]

On Aug 28,  5:50pm, Peter Stephenson wrote:
} Subject: Re: Is "read -AE" buggy?
}
} I think this is down to the fact that bin_read() is a complete mess
} where people dump in new features without any attempt to impose
} structure (fancy that happening in zsh...)

I'd have guessed that too except that this bug has apparently been
around since before the first CVS import in 1999.

Thanks for working out the patch.

Re: Is "read -AE" buggy?

[Peter Stephenson]

I think I should be a bit more careful in the non '-A' case.

Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.253
diff -p -u -r1.253 builtin.c
--- Src/builtin.c	28 Aug 2011 17:06:27 -0000	1.253
+++ Src/builtin.c	29 Aug 2011 17:05:24 -0000
@@ -5549,7 +5549,14 @@ bin_read(char *name, char **args, Option
 	*bptr = '\0';
 #endif
 	/* dispose of word appropriately */
-	if (OPT_ISSET(ops,'e')) {
+	if (OPT_ISSET(ops,'e') ||
+	    /*
+	     * When we're doing an array assignment, we'll
+	     * handle echoing at that point.  In all other
+	     * cases (including -A with no assignment)
+	     * we'll do it here.
+	     */
+	    (OPT_ISSET(ops,'E') && !OPT_ISSET(ops,'A'))) {
 	    zputs(buf, stdout);
 	    putchar('\n');
 	}


-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/