Re: 'read -q -t X'

["Michael B. Trausch" <mbt@zest.trausch.us>]

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

On Fri, 24 Jul 2009, Peter Stephenson wrote:

> On Fri, 24 Jul 2009 09:43:57 +0100
> Peter Stephenson <pws@csr.com> wrote:
>> This is a bug: -q and -k should be similar enough that the shell does
>> essentially that internally.  Unfortunately, every time anyone has added
>> a "great new feature" to the read builtin they've simply hacked it in as
>> a completely separate case without fitting it properly into the existing
>> code, so the -t code isn't wired up to treat -q as a special case, unlike
>> -k.
>
> It doesn't actually seem to be that hard to fix, in fact.  Why wasn't it
> done this way before?  People do seem bent on making my job harder (or not
> proleptically making it easier, or whatever).
>
> I see the original code suggested the default on a timeout should be
> "y"---however, a timeout would normally return the same status as "n".
> I've made it return status 2 for a timeout for -q only.  There's an
> argument for a timeout always being status 2, but that's a bigger change
> for other options (i.e. the ones where the timeout already worked properly
> :-/).  It's not usually such an issue elsewhere since status 1 means
> the read failed, not that the user explicit said "no".

Awesome, thanks for the patch.  I will apply it when I get the chance.  Much 
appreciated!

 	--- Mike

>
> Index: Doc/Zsh/builtins.yo
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
> retrieving revision 1.124
> diff -u -r1.124 builtins.yo
> --- Doc/Zsh/builtins.yo	19 Jul 2009 19:08:48 -0000	1.124
> +++ Doc/Zsh/builtins.yo	24 Jul 2009 11:33:30 -0000
> @@ -1113,9 +1113,10 @@
> Read only one character from the terminal and set var(name) to
> `tt(y)' if this character was `tt(y)' or `tt(Y)' and to `tt(n)' otherwise.
> With this flag set the return status is zero only if the character was
> -`tt(y)' or `tt(Y)'.  Note that this always reads from the terminal, even
> -if used with the tt(-p) or tt(-u) or tt(-z) flags or with redirected input.
> -This option may also be used within zle widgets.
> +`tt(y)' or `tt(Y)'.  This option may be used with a timeout; if
> +the read times out, or encounters end of file, status 2 is returned.
> +Input is read from the terminal unless one of tt(-u)
> +or tt(-p) is present.  This option may also be used within zle widgets.
> )
> item(tt(-k) [ var(num) ])(
> Read only one (or var(num)) characters.  All are assigned to the first
> Index: Src/builtin.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
> retrieving revision 1.233
> diff -u -r1.233 builtin.c
> --- Src/builtin.c	21 Jul 2009 09:35:26 -0000	1.233
> +++ Src/builtin.c	24 Jul 2009 11:33:30 -0000
> @@ -5040,8 +5040,8 @@
>     if(OPT_ISSET(ops,'l') || OPT_ISSET(ops,'c'))
> 	return compctlreadptr(name, args, ops, reply);
>
> -    if ((OPT_ISSET(ops,'k') && !OPT_ISSET(ops,'u') &&
> -	 !OPT_ISSET(ops,'p')) || OPT_ISSET(ops,'q')) {
> +    if ((OPT_ISSET(ops,'k') || OPT_ISSET(ops,'q')) &&
> +	!OPT_ISSET(ops,'u') && !OPT_ISSET(ops,'p')) {
> 	if (!zleactive) {
> 	    if (SHTTY == -1) {
> 		/* need to open /dev/tty specially */
> @@ -5065,7 +5065,7 @@
> 		gettyinfo(&shttyinfo);
> 	    /* attach to the tty */
> 	    attachtty(mypgrp);
> -	    if (!isem && OPT_ISSET(ops,'k'))
> +	    if (!isem)
> 		setcbreak();
> 	    readfd = SHTTY;
> 	}
> @@ -5184,8 +5184,9 @@
> #endif
> 	} else {
> 	    if (readfd == -1 ||
> -		!read_poll(readfd, &readchar, keys && !zleactive, timeout)) {
> -		if (OPT_ISSET(ops,'k') && !zleactive && !isem)
> +		!read_poll(readfd, &readchar, keys && !zleactive,
> +			   timeout)) {
> +		if (keys && !zleactive && !isem)
> 		    settyinfo(&shttyinfo);
> 		else if (resettty && SHTTY != -1)
> 		    settyinfo(&saveti);
> @@ -5194,7 +5195,7 @@
> 		    shout = oshout;
> 		    SHTTY = -1;
> 		}
> -		return 1;
> +		return OPT_ISSET(ops,'q') ? 2 : 1;
> 	    }
> 	}
>     }
> @@ -5203,11 +5204,18 @@
>     memset(&mbs, 0, sizeof(mbs));
> #endif
>
> -    /* option -k means read only a given number of characters (default 1) */
> -    if (OPT_ISSET(ops,'k')) {
> +    /*
> +     * option -k means read only a given number of characters (default 1)
> +     * option -q means get one character, and interpret it as a Y or N
> +     */
> +    if (OPT_ISSET(ops,'k') || OPT_ISSET(ops,'q')) {
> 	int eof = 0;
> 	/* allocate buffer space for result */
> +#ifdef MULTIBYTE_SUPPORT
> +	bptr = buf = (char *)zalloc(nchars*MB_CUR_MAX+1);
> +#else
> 	bptr = buf = (char *)zalloc(nchars+1);
> +#endif
>
> 	do {
> 	    if (izle) {
> @@ -5295,6 +5303,17 @@
> 	    }
> 	}
>
> +	if (OPT_ISSET(ops,'q'))
> +	{
> +	    /*
> +	     * Keep eof as status but status is now whether we read
> +	     * 'y' or 'Y'.  If we timed out, status is 2.
> +	     */
> +	    if (eof)
> +		eof = 2;
> +	    else
> +		eof = (bptr - buf != 1 || (buf[0] != 'y' && buf[0] != 'Y'));
> +	}
> 	if (OPT_ISSET(ops,'e') || OPT_ISSET(ops,'E'))
> 	    fwrite(buf, bptr - buf, 1, stdout);
> 	if (!OPT_ISSET(ops,'e'))
> @@ -5306,59 +5325,6 @@
> 	return eof;
>     }
>
> -    /* option -q means get one character, and interpret it as a Y or N */
> -    if (OPT_ISSET(ops,'q')) {
> -	char readbuf[2];
> -
> -	/* set up the buffer */
> -	readbuf[1] = '\0';
> -
> -	/* get, and store, reply */
> -	if (izle) {
> -#ifdef MULTIBYTE_SUPPORT
> -	    int key;
> -	    char c;
> -
> -	    for (;;) {
> -		zleentry(ZLE_CMD_GET_KEY, izle_timeout, NULL, &key);
> -		if (key < 0)
> -		    break;
> -		c = (char)key;
> -		/*
> -		 * If multibyte, it can't be y, so we don't care
> -		 * what key gets set to; just read to end of character.
> -		 */
> -		if (!isset(MULTIBYTE) ||
> -		    mbrlen(&c, 1, &mbs) != MB_INCOMPLETE)
> -		    break;
> -	    }
> -#else
> -	    int key;
> -	    zleentry(ZLE_CMD_GET_KEY, izle_timeout, NULL, &key);
> -#endif
> -
> -	    readbuf[0] = (key == 'y' ? 'y' : 'n');
> -	} else {
> -	    readbuf[0] = ((char)getquery(NULL, 0)) == 'y' ? 'y' : 'n';
> -
> -	    /* dispose of result appropriately, etc. */
> -	    if (haso) {
> -		fclose(shout);	/* close(SHTTY) */
> -		shout = oshout;
> -		SHTTY = -1;
> -	    }
> -	}
> -
> -	if (OPT_ISSET(ops,'e') || OPT_ISSET(ops,'E'))
> -	    printf("%s\n", readbuf);
> -	if (!OPT_ISSET(ops,'e'))
> -	    setsparam(reply, ztrdup(readbuf));
> -
> -	if (resettty && SHTTY != -1)
> -	    settyinfo(&saveti);
> -	return readbuf[0] == 'n';
> -    }
> -
>     /* All possible special types of input have been exhausted.  Take one line,
>        and assign words to the parameters until they run out.  Leftover words go
>        onto the last parameter.  If an array is specified, all the words become
> Index: Test/B04read.ztst
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Test/B04read.ztst,v
> retrieving revision 1.3
> diff -u -r1.3 B04read.ztst
> --- Test/B04read.ztst	25 Apr 2005 10:41:26 -0000	1.3
> +++ Test/B04read.ztst	24 Jul 2009 11:33:30 -0000
> @@ -24,6 +24,18 @@
> 0:read specified number of chars
> >foo
>
> + for char in y Y n N X $'\n'; do
> +   read -q -u0 <<<$char
> +   print $?
> + done
> +0:read yes or no, default no
> +>0
> +>0
> +>1
> +>1
> +>1
> +>1
> +
>  read -d: <<<foo:bar
>  print $REPLY
> 0:read up to delimiter
> Index: Test/D07multibyte.ztst
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Test/D07multibyte.ztst,v
> retrieving revision 1.33
> diff -u -r1.33 D07multibyte.ztst
> --- Test/D07multibyte.ztst	8 May 2009 14:30:36 -0000	1.33
> +++ Test/D07multibyte.ztst	24 Jul 2009 11:33:30 -0000
> @@ -232,6 +232,12 @@
> <«»ignored
> >«»
>
> +  read -q -u0 mb
> +  print $?
> +0:multibyte character makes read -q return false
> +<«
> +>1
> +
>   # See if the system grokks first-century Greek...
>   ioh="Ἐν ἀρχῇ ἦν ὁ λόγος, καὶ ὁ λόγος ἦν πρὸς τὸν θεόν, καὶ θεὸς ἦν ὁ λόγος."
>   for (( i = 1; i <= ${#ioh}; i++ )); do
>
>
>