PATCH: 3.0.6-pre-4: COLUMNS/LINES environment handling

["Bart Schaefer" <schaefer@xxxxxxxxxxxxxxxxxxxxxxx>]

This took far longer than it should have, but I think this is right.  If PWS
agrees that it looks OK, I can produce an equivalent patch for 3.1.5-pws-21.
Here's what's going on:

The problem with the before-this-patch adjustwinsize() is mostly that it's
trying to do too much at once.  When the user sets LINES and COLUMNS, they
must be set one at a time, but adjustwinsize() behaves as if both always
change together -- which led to to the `userlines' and `usercols' statics,
trying to keep track of whether the user explicitly set one or the other
on a previous call.  This also makes it problematic to allow a recursive
call to the function, as would happen if setiparam() were used to update
the LINES and COLUMNS parameters on a SIGWINCH.

So after mucking about for far too long trying to keep the changes inside
adjustwinsize(), I finally concluded that the only right thing was to rip
management of LINES and COLUMNS out into their own functions that could be
called independently from adjustwinsize().  Hence were born adjustlines()
and adjustcolumns(), which encapsulate synchnronizing LINES and COLUMNS
with the tty driver window size structure, and return true if the values
changed or false if they remained unchanged.

Now adjustwinsize() calls both of the other adjust*() functions when its
parameter is 0 or 1, and only one of them when 2 or 3.  Thus setting the
value of LINES can't poke a new columns value into the tty driver, and
vice versa.  Happily, this also makes recursive calls to adjustwinsize()
into no-ops, so it's possible to call setiparam() to export any changes in
the 0 or 1 case.  This is done only if zgetenv() finds the parameter name
in the environment, so it can't accidentaly reverse an "unset LINES" (or
COLUMNS).

As an optimization, I used a static local to avoid doing the ioctl() again
on a recursive call; the behavior is unchanged without the static, but see
the comment in the code regarding possible race conditions.

The behavior should be as discussed in 6562 and its predecessors, to wit:
1. At startup, LINES/COLUMNS are determined by trying in this order:
   - import from the environment;
   - read from the tty driver (TIOCGWINSZ);
   - read from the termcap/terminfo entry;
   - assume 24 lines and 80 columns.
   Note that LINES/COLUMNS are not implicitly exported, so if they were
   not imported from the environment in the first place you must export
   them yourself later.  If you don't like the value imported from the
   environment, see (5) below.
2. On SIGWINCH, the tty driver values are used unless they're zeroes.
   I put in some debugging statements to warn if driver values are bad.
3. After running an external command or subshell, the tty driver values
   are used if they've changed; otherwise see (1).
4. When LINES or COLUMNS is assigned a nonzero value, that value is
   written to the tty driver if possible (TIOCSWINSZ); but only the one
   that changed is written.  E.g. even if the tty driver and the internal
   value of COLUMNS disagree, assigning to LINES will change only the
   tty driver rows, not both the rows and the columns.  (It also won't
   cause the tty driver's columns value to be assigned to COLUMNS!)
5. When LINES or COLUMNS is assigned a zero value, a nonzero value is
   computed as in (1) [skipping the environment] and that becomes the
   actual value of the parameter.  Note that without (4), assigning a
   zero to either of LINES or COLUMNS would have clobbered the tty driver
   value of the other one, making this feature a lot less useful.

Now, somebody who has a copy of the actual POSIX shell spec should tell
us if the above is reasonable.  Zoltan, are you out there?

How much of all that needs to go in the manual?  Lots of it should go in
the FAQ, including remarks about how it is broken in various versions if
we can figure that out.

Another thing I tossed in was forcing a refresh on SIGWINCH even if the
OS appears not to support TIOCGWINSZ.  I don't know whether it's even
possible for that case to come up, but ...

Finally, I noticed that `winchanged' was declared only if TIOCGWINSZ but
used without such a condition.  Apparently zsh doesn't get built on any
systems that don't have TIOCGWINSZ?

Index: Src/utils.c
===================================================================
@@ -832,61 +832,131 @@
 extern winchanged;
 #endif
 
-/* check the size of the window and adjust if necessary. *
- * The value of from:					 *
- *   0: called from update_job or setupvals		 *
- *   1: called from the SIGWINCH handler		 *
- *   2: the user have just changed LINES manually	 *
- *   3: the user have just changed COLUMNS manually      */
-
-/**/
-void
-adjustwinsize(int from)
+static int
+adjustlines(int signalled)
 {
-    int oldcols = columns, oldrows = lines;
+    int oldlines = lines;
 
 #ifdef TIOCGWINSZ
-    static int userlines, usercols;
-
-    if (SHTTY == -1)
-	return;
-
-    if (from == 2)
-	userlines = lines > 0;
-    if (from == 3)
-	usercols = columns > 0;
-
-    if (!ioctl(SHTTY, TIOCGWINSZ, (char *)&shttyinfo.winsize)) {
-	if (!userlines || from == 1)
-	    lines = shttyinfo.winsize.ws_row;
-	if (!usercols || from == 1)
-	    columns = shttyinfo.winsize.ws_col;
+    if (signalled || lines <= 0)
+	lines = shttyinfo.winsize.ws_row;
+    else
+	shttyinfo.winsize.ws_row = lines;
+#endif /* TIOCGWINSZ */
+    if (lines <= 0) {
+	DPUTS(signalled, "BUG: Impossible TIOCGWINSZ rows");
+	lines = tclines > 0 ? tclines : 24;
     }
-#endif   /* TIOCGWINSZ */
 
-    if (lines <= 0)
-	lines = tclines > 0 ? tclines : 24;
-    if (columns <= 0)
-	columns = tccolumns > 0 ? tccolumns : 80;
     if (lines > 2)
 	termflags &= ~TERM_SHORT;
     else
 	termflags |= TERM_SHORT;
+
+    return (lines != oldlines);
+}
+
+static int
+adjustcolumns(int signalled)
+{
+    int oldcolumns = columns;
+
+#ifdef TIOCGWINSZ
+    if (signalled || columns <= 0)
+	columns = shttyinfo.winsize.ws_col;
+    else
+	shttyinfo.winsize.ws_col = columns;
+#endif /* TIOCGWINSZ */
+    if (columns <= 0) {
+	DPUTS(signalled, "BUG: Impossible TIOCGWINSZ cols");
+	columns = tccolumns > 0 ? tccolumns : 80;
+    }
+
     if (columns > 2)
 	termflags &= ~TERM_NARROW;
     else
 	termflags |= TERM_NARROW;
 
+    return (columns != oldcolumns);
+}
+
+/* check the size of the window and adjust if necessary. *
+ * The value of from:					 *
+ *   0: called from update_job or setupvals		 *
+ *   1: called from the SIGWINCH handler		 *
+ *   2: called from the LINES parameter callback	 *
+ *   3: called from the COLUMNS parameter callback	 */
+
+/**/
+void
+adjustwinsize(int from)
+{
+    static int getwinsz = 1;
+    int ttyrows = shttyinfo.winsize.ws_row;
+    int ttycols = shttyinfo.winsize.ws_col;
+    int resetzle = 0;
+
+    if (getwinsz || from == 1) {
 #ifdef TIOCGWINSZ
-    if (interact && from >= 2) {
-	shttyinfo.winsize.ws_row = lines;
-	shttyinfo.winsize.ws_col = columns;
+	if (SHTTY == -1)
+	    return;
+	if (ioctl(SHTTY, TIOCGWINSZ, (char *)&shttyinfo.winsize) == 0) {
+	    resetzle = (ttyrows != shttyinfo.winsize.ws_row ||
+			ttycols != shttyinfo.winsize.ws_col);
+	    if (from == 0 && resetzle && ttyrows && ttycols)
+		from = 1; /* Signal missed while a job owned the tty? */
+	    ttyrows = shttyinfo.winsize.ws_row;
+	    ttycols = shttyinfo.winsize.ws_col;
+	} else {
+	    /* Set to unknown on failure */
+	    shttyinfo.winsize.ws_row = 0;
+	    shttyinfo.winsize.ws_col = 0;
+	    resetzle = 1;
+	}
+#else
+	resetzle = from == 1;
+#endif /* TIOCGWINSZ */
+    } /* else
+	 return; */
+
+    switch (from) {
+    case 0:
+    case 1:
+	getwinsz = 0;
+	/* Calling setiparam() here calls this function recursively, but  *
+	 * because we've already called adjustlines() and adjustcolumns() *
+	 * here, recursive calls are no-ops unless a signal intervenes.   *
+	 * The commented "else return;" above might be a safe shortcut,   *
+	 * but I'm concerned about what happens on race conditions; e.g., *
+	 * suppose the user resizes his xterm during `eval $(resize)'?    */
+	if (adjustlines(from) && zgetenv("LINES"))
+	    setiparam("LINES", lines);
+	if (adjustcolumns(from) && zgetenv("COLUMNS"))
+	    setiparam("COLUMNS", columns);
+	getwinsz = 1;
+	break;
+    case 2:
+	resetzle = adjustlines(0);
+	break;
+    case 3:
+	resetzle = adjustcolumns(0);
+	break;
+    }
+
+#ifdef TIOCGWINSZ
+    if (interact && from >= 2 &&
+	(shttyinfo.winsize.ws_row != ttyrows ||
+	 shttyinfo.winsize.ws_col != ttycols)) {
+	/* shttyinfo.winsize is already set up correctly */
 	ioctl(SHTTY, TIOCSWINSZ, (char *)&shttyinfo.winsize);
     }
-#endif
+#endif /* TIOCGWINSZ */
 
-    if (zleactive && (from >= 2 || oldcols != columns || oldrows != lines)) {
-	resetneeded = winchanged = 1;
+    if (zleactive && resetzle) {
+#ifdef TIOCGWINSZ
+	winchanged =
+#endif /* TIOCGWINSZ */
+	    resetneeded = 1;
 	refresh();
     }
 }


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