PATCH: options for cd and other animals

[Peter Stephenson <pws@xxxxxxx>]

Here's a tenative patch to rationalise the option handling for cd, as
well as generally making the code more sane.

In general, I have tried to elimate command-specific handling in the
builtin code by introducing generic flags which should be useful in
other places.  I have rationalised XTRACE handling at the expense of
putting the whole list of arguments, including the options, into a
temporary array.

In particular, here's what I think I've done for cd and friends.

- The argument count now works, i.e. the 0 to 2 argument limit excludes
  options.
- `--' is recognised.  I have tried not to break compatibility too
  badly, so `cd -foo' or `cd -foo -bar' will still work unless the first
  argument is really --.  Strictly speaking, this is probably still
  non-conformant with something or other.  (As before, `cd -L' indicates
  an option, but there is now a workaround `cd -- -L', which there wasn't
  before.  Handling of numbers and a single `-' should not have changed.)
- The undocumented feature that pushd also takes the cd options is now
  documented.
- The undocumented feature that popd took those arguments too, and also
  silently accepted a second argument, has been removed.  (There is an
  argument for handling them to force particular behaviour on symbolic
  links but it's not all that logical when restoring previous directories.)


Index: Doc/Zsh/builtins.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/builtins.yo,v
retrieving revision 1.51
diff -u -r1.51 builtins.yo
--- Doc/Zsh/builtins.yo	27 Aug 2002 21:10:34 -0000	1.51
+++ Doc/Zsh/builtins.yo	2 Sep 2002 12:36:15 -0000
@@ -772,9 +772,9 @@
 pindex(PUSHD_MINUS, use of)
 pindex(CDABLE_VARS, use of)
 pindex(PUSHD_SILENT, use of)
-xitem(tt(pushd) [ var(arg) ])
-xitem(tt(pushd) var(old) var(new))
-item(tt(pushd) {tt(PLUS())|tt(-)}var(n))(
+xitem(tt(pushd) [ tt(-sLP) ] [ var(arg) ])
+xitem(tt(pushd) [ tt(-sLP) ] var(old) var(new))
+item(tt(pushd) [ tt(-sLP) ] {tt(PLUS())|tt(-)}var(n))(
 Change the current directory, and push the old current directory
 onto the directory stack.  In the first form, change the
 current directory to var(arg).
@@ -795,6 +795,9 @@
 
 If the option tt(PUSHD_SILENT) is not set, the directory
 stack will be printed after a tt(pushd) is performed.
+
+The options tt(-s), tt(-L) and tt(-P) have the same meanings as for the
+tt(cd) builtin.
 )
 findex(pushln)
 item(tt(pushln) [ var(arg) ... ])(
Index: Src/builtin.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/builtin.c,v
retrieving revision 1.82
diff -u -r1.82 builtin.c
--- Src/builtin.c	27 Aug 2002 21:10:34 -0000	1.82
+++ Src/builtin.c	2 Sep 2002 12:36:15 -0000
@@ -50,14 +50,14 @@
     BUILTIN("bg", 0, bin_fg, 0, -1, BIN_BG, NULL, NULL),
     BUILTIN("break", BINF_PSPECIAL, bin_break, 0, 1, BIN_BREAK, NULL, NULL),
     BUILTIN("bye", 0, bin_break, 0, 1, BIN_EXIT, NULL, NULL),
-    BUILTIN("cd", 0, bin_cd, 0, 2, BIN_CD, NULL, NULL),
-    BUILTIN("chdir", 0, bin_cd, 0, 2, BIN_CD, NULL, NULL),
+    BUILTIN("cd", BINF_SKIPINVALID | BINF_SKIPNUM | BINF_SKIPDASH | BINF_DASHDASHVALID, bin_cd, 0, 2, BIN_CD, "sPL", NULL),
+    BUILTIN("chdir", BINF_SKIPINVALID | BINF_SKIPNUM | BINF_SKIPDASH | BINF_DASHDASHVALID, bin_cd, 0, 2, BIN_CD, "sPL", NULL),
     BUILTIN("continue", BINF_PSPECIAL, bin_break, 0, 1, BIN_CONTINUE, NULL, NULL),
     BUILTIN("declare", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL, bin_typeset, 0, -1, 0, "AE:%F:%HL:%R:%TUZ:%afghi:%lprtux", NULL),
     BUILTIN("dirs", 0, bin_dirs, 0, -1, 0, "clpv", NULL),
     BUILTIN("disable", 0, bin_enable, 0, -1, BIN_DISABLE, "afmr", NULL),
     BUILTIN("disown", 0, bin_fg, 0, -1, BIN_DISOWN, NULL, NULL),
-    BUILTIN("echo", BINF_PRINTOPTS | BINF_ECHOPTS, bin_print, 0, -1, BIN_ECHO, "neE", "-"),
+    BUILTIN("echo", BINF_PRINTOPTS | BINF_SKIPINVALID, bin_print, 0, -1, BIN_ECHO, "neE", "-"),
     BUILTIN("emulate", 0, bin_emulate, 1, 1, 0, "LR", NULL),
     BUILTIN("enable", 0, bin_enable, 0, -1, BIN_ENABLE, "afmr", NULL),
     BUILTIN("eval", BINF_PSPECIAL, bin_eval, 0, -1, BIN_EVAL, NULL, NULL),
@@ -99,10 +99,10 @@
     BUILTIN("patdebug", 0, bin_patdebug, 1, -1, 0, "p", NULL),
 #endif
 
-    BUILTIN("popd", 0, bin_cd, 0, 2, BIN_POPD, NULL, NULL),
+    BUILTIN("popd", 0, bin_cd, 0, 1, BIN_POPD, NULL, NULL),
     BUILTIN("print", BINF_PRINTOPTS, bin_print, 0, -1, BIN_PRINT, "RDPbnrsf:lzNu:pioOcm-", NULL),
     BUILTIN("printf", 0, bin_print, 1, -1, BIN_PRINTF, NULL, NULL),
-    BUILTIN("pushd", 0, bin_cd, 0, 2, BIN_PUSHD, NULL, NULL),
+    BUILTIN("pushd", BINF_SKIPINVALID | BINF_SKIPNUM | BINF_SKIPDASH | BINF_DASHDASHVALID, bin_cd, 0, 2, BIN_PUSHD, "sPL", NULL),
     BUILTIN("pushln", BINF_PRINTOPTS, bin_print, 0, -1, BIN_PRINT, NULL, "-nz"),
     BUILTIN("pwd", 0, bin_pwd, 0, 0, 0, "rLP", NULL),
     BUILTIN("r", 0, bin_fc, 0, -1, BIN_R, "nrl", NULL),
@@ -238,10 +238,8 @@
 int
 execbuiltin(LinkList args, Builtin bn)
 {
-    LinkNode n;
-    char *arg, *pp, *name, *optstr;
-    char *oxarg, *xarg = NULL;
-    int flags, sense, argc = 0, execop, xtr = isset(XTRACE), lxarg = 0;
+    char *pp, *name, *optstr;
+    int flags, sense, argc, execop, xtr = isset(XTRACE);
     struct options ops;
 
     /* initialise options structure */
@@ -252,8 +250,6 @@
     /* initialize some local variables */
     name = (char *) ugetnode(args);
 
-    arg = (char *) ugetnode(args);
-
     if (!bn->handlerfunc) {
 	zwarnnam(name, "autoload failed", NULL, 0);
 	deletebuiltin(bn->nam);
@@ -263,141 +259,150 @@
     flags = bn->flags;
     optstr = bn->optstr;
 
-    /* Sort out the options. */
-    if ((flags & BINF_ECHOPTS) && isset(BSDECHO))
-	ops.ind['E'] = 1;
-    if (optstr)
-	/* while arguments look like options ... */
-	while (arg &&
-	       ((sense = (*arg == '-')) ||
-		((flags & BINF_PLUSOPTS) && *arg == '+')) &&
-	       ((flags & BINF_PLUSOPTS) || !atoi(arg))) {
-	    /* unrecognised options to echo etc. are not really options */
-	    if (flags & BINF_ECHOPTS) {
-		char *p = arg;
-		while (*++p && strchr(optstr, (int) *p));
-		if (*p)
+    /* Set up the argument list. */
+    /* count the arguments */
+    argc = countlinknodes(args);
+
+    {
+	/*
+	 * Keep all arguments, including options, in an array.
+	 * We don't actually need the option part of the argument
+	 * after option processing, but it makes XTRACE output
+	 * much simpler.
+	 */
+	VARARR(char *, argarr, argc + 1);
+	char **argv;
+
+	/*
+	 * Get the actual arguments, into argv.  Remember argarr
+	 * may be an array declaration, depending on the compiler.
+	 */
+	argv = argarr;
+	while ((*argv++ = (char *)ugetnode(args)));
+	argv = argarr;
+
+	/* Sort out the options. */
+	if (optstr) {
+	    char *arg = *argv;
+	    /* while arguments look like options ... */
+	    while (arg &&
+		   /* Must begin with - or maybe + */
+		   ((sense = (*arg == '-')) ||
+		    ((flags & BINF_PLUSOPTS) && *arg == '+'))) {
+		/* For cd and friends, numbers cannot be options. */
+		if ((flags & BINF_SKIPNUM) && idigit(arg[1]))
+		    break;
+		/* For cd and friends, a single dash is not an option. */
+		if ((flags & BINF_SKIPDASH) && !arg[1])
+		    break;
+		if ((flags & BINF_DASHDASHVALID) && !strcmp(arg, "--")) {
+		    /*
+		     * Need to skip this before checking whether this is
+		     * really an option.
+		     */
+		    argv++;
 		    break;
-	    }
-	    /* save the options in xarg, for execution tracing */
-	    if (xtr) {
-		if (xarg) {
-		    int l = strlen(arg) + lxarg + 1;
-
-		    oxarg = zhalloc(l + 1);
-		    strcpy(oxarg, xarg);
-		    oxarg[lxarg] = ' ';
-		    strcpy(oxarg + lxarg + 1, arg);
-		    xarg = oxarg;
-		    lxarg = l + 1;
-		} else {
-		    xarg = dupstring(arg);
-		    lxarg = strlen(xarg);
 		}
-	    }
-	    /* handle -- or - (ops.ind['-']), and +
-	     * (ops.ind['-'] and ops.ind['+']) */
-	    if (arg[1] == '-')
-		arg++;
-	    if (!arg[1]) {
-		ops.ind['-'] = 1;
-		if (!sense)
-		    ops.ind['+'] = 1;
-	    }
-	    /* save options in ops, as long as they are in bn->optstr */
-	    execop = -1;
-	    while (*++arg) {
-		char *optptr;
-		if ((optptr = strchr(optstr, execop = (int)*arg))) {
-		    ops.ind[(int)*arg] = (sense) ? 1 : 2;
-		    if (optptr[1] == ':') {
-			char *argptr = NULL;
-			if (optptr[2] == ':') {
-			    if (arg[1])
-				argptr = arg+1;
-			    /* Optional argument in same word*/
-			} else if (optptr[2] == '%') {
-			    /* Optional numeric argument in same
-			     * or next word. */
-			    if (arg[1] && idigit(arg[1]))
-				argptr = arg+1;
-			    else if (firstnode(args) &&
-				     idigit(*(char *)peekfirst(args)))
-				argptr = arg = (char *)ugetnode(args);
-			} else {
-			    /* Mandatory argument */
-			    if (arg[1])
-				argptr = arg+1;
-			    else if ((arg = (char *)ugetnode(args)))
-				argptr = arg;
-			    else {
-				zwarnnam(name, "argument expected: -%c", NULL,
-					 execop);
-				return 1;
+		/* unrecognised options to echo etc. are not really options */
+		if (flags & BINF_SKIPINVALID) {
+		    char *p = arg;
+		    if (optstr)
+			while (*++p && strchr(optstr, (int) *p));
+		    else
+			p++;
+		    if (*p)
+			break;
+		}
+		/* handle -- or - (ops.ind['-']), and +
+		 * (ops.ind['-'] and ops.ind['+']) */
+		if (arg[1] == '-')
+		    arg++;
+		if (!arg[1]) {
+		    ops.ind['-'] = 1;
+		    if (!sense)
+			ops.ind['+'] = 1;
+		}
+		/* save options in ops, as long as they are in bn->optstr */
+		while (*++arg) {
+		    char *optptr;
+		    if ((optptr = strchr(optstr, execop = (int)*arg))) {
+			ops.ind[(int)*arg] = (sense) ? 1 : 2;
+			if (optptr[1] == ':') {
+			    char *argptr = NULL;
+			    if (optptr[2] == ':') {
+				if (arg[1])
+				    argptr = arg+1;
+				/* Optional argument in same word*/
+			    } else if (optptr[2] == '%') {
+				/* Optional numeric argument in same
+				 * or next word. */
+				if (arg[1] && idigit(arg[1]))
+				    argptr = arg+1;
+				else if (argv[1] && idigit(*argv[1]))
+				    argptr = arg = *++argv;
+			    } else {
+				/* Mandatory argument */
+				if (arg[1])
+				    argptr = arg+1;
+				else if ((arg = *++argv))
+				    argptr = arg;
+				else {
+				    zwarnnam(name, "argument expected: -%c",
+					     NULL, execop);
+				    return 1;
+				}
 			    }
-			}
-			if (argptr) {
-			    if (new_optarg(&ops)) {
-				zwarnnam(name, 
-					 "too many option arguments", NULL, 0);
-				return 1;
+			    if (argptr) {
+				if (new_optarg(&ops)) {
+				    zwarnnam(name, 
+					     "too many option arguments",
+					     NULL, 0);
+				    return 1;
+				}
+				ops.ind[execop] |= ops.argscount << 2;
+				ops.args[ops.argscount-1] = argptr;
+				while (arg[1])
+				    arg++;
 			    }
-			    ops.ind[execop] |= ops.argscount << 2;
-			    ops.args[ops.argscount-1] = argptr;
-			    while (arg[1])
-				arg++;
 			}
-		    }
-		} else
+		    } else
+			break;
+		}
+		/* The above loop may have exited on an invalid option.  (We  *
+		 * assume that any option requiring metafication is invalid.) */
+		if (*arg) {
+		    if(*arg == Meta)
+			*++arg ^= 32;
+		    zwarn("bad option: -%c", NULL, *arg);
+		    return 1;
+		}
+		arg = *++argv;
+		/* for the "print" builtin, the options after -R are treated as
+		   options to "echo" */
+		if ((flags & BINF_PRINTOPTS) && ops.ind['R'] &&
+		    !ops.ind['f']) {
+		    optstr = "ne";
+		    flags |= BINF_SKIPINVALID;
+		}
+		/* the option -- indicates the end of the options */
+		if (ops.ind['-'])
 		    break;
 	    }
-	    /* The above loop may have exited on an invalid option.  (We  *
-	     * assume that any option requiring metafication is invalid.) */
-	    if (*arg) {
-		if(*arg == Meta)
-		    *++arg ^= 32;
-		zwarn("bad option: -%c", NULL, *arg);
-		return 1;
-	    }
-	    arg = (char *) ugetnode(args);
-	    /* for the "print" builtin, the options after -R are treated as
-	       options to "echo" */
-	    if ((flags & BINF_PRINTOPTS) && ops.ind['R'] && !ops.ind['f']) {
-		optstr = "ne";
-		flags |= BINF_ECHOPTS;
-	    }
-	    /* the option -- indicates the end of the options */
-	    if (ops.ind['-'])
-		break;
 	}
-    /* handle built-in options, for overloaded handler functions */
-    if ((pp = bn->defopts)) {
-	while (*pp) {
-	    /* only if not already set */
-	    if (!ops.ind[(int)*pp])
-		ops.ind[(int)*pp] = 1;
-	    pp++;
+
+	/* handle built-in options, for overloaded handler functions */
+	if ((pp = bn->defopts)) {
+	    while (*pp) {
+		/* only if not already set */
+		if (!ops.ind[(int)*pp])
+		    ops.ind[(int)*pp] = 1;
+		pp++;
+	    }
 	}
-    }
 
-    /* Set up the argument list. */
-    if (arg) {
-	/* count the arguments */
-	argc = 1;
-	n = firstnode(args);
-	while (n)
-	    argc++, incnode(n);
-    }
-    {
-	VARARR(char *, argarr, (argc + 1));
-	char **argv, **oargv;
+	/* Fix the argument count by subtracting option arguments */
+	argc -= argv - argarr;
 
-	/* Get the actual arguments, into argv.  Oargv saves the *
-	 * beginning of the array for later reference.           */
-	oargv = argv = argarr;
-	if ((*argv++ = arg))
-	    while ((*argv++ = (char *)ugetnode(args)));
-	argv = oargv;
 	if (errflag) {
 	    errflag = 0;
 	    return 1;
@@ -412,15 +417,13 @@
 
 	/* display execution trace information, if required */
 	if (xtr) {
+	    /* Use full argument list including options for trace output */
+	    char **fullargv = argarr;
 	    printprompt4();
 	    fprintf(xtrerr, "%s", name);
-	    if (xarg) {
-	        fputc(' ', xtrerr);
-	        quotedzputs(xarg, xtrerr);
-	    }
-	    while (*oargv) {
+	    while (*fullargv) {
 	        fputc(' ', xtrerr);
-	        quotedzputs(*oargv++, xtrerr);
+	        quotedzputs(*fullargv++, xtrerr);
 	    }
 	    fputc('\n', xtrerr);
 	    fflush(xtrerr);
@@ -762,27 +765,11 @@
     }
     doprintdir = (doprintdir == -1);
 
-    for (; *argv && **argv == '-'; argv++) {
-	char *s = *argv + 1;
-
-	do {
-	    switch (*s) {
-	    case 's':
-	    case 'P':
-	    case 'L':
-		break;
-	    default:
-		goto brk;
-	    }
-	} while (*++s);
-	for (s = *argv; *++s; ops->ind[STOUC(*s)] = 1);
-    }
- brk:
     chasinglinks = OPT_ISSET(ops,'P') || 
 	(isset(CHASELINKS) && !OPT_ISSET(ops,'L'));
     queue_signals();
     zpushnode(dirstack, ztrdup(pwd));
-    if (!(dir = cd_get_dest(nam, argv, ops, func))) {
+    if (!(dir = cd_get_dest(nam, argv, OPT_ISSET(ops,'s'), func))) {
 	zsfree(getlinknode(dirstack));
 	unqueue_signals();
 	return 1;
@@ -812,7 +799,7 @@
 
 /**/
 static LinkNode
-cd_get_dest(char *nam, char **argv, Options ops, int func)
+cd_get_dest(char *nam, char **argv, int hard, int func)
 {
     LinkNode dir = NULL;
     LinkNode target;
@@ -882,7 +869,7 @@
     if (!dir) {
 	dir = firstnode(dirstack);
     }
-    if (!(dest = cd_do_chdir(nam, getdata(dir), OPT_ISSET(ops,'s')))) {
+    if (!(dest = cd_do_chdir(nam, getdata(dir), hard))) {
 	if (!target)
 	    zsfree(getlinknode(dirstack));
 	if (func == BIN_POPD)
@@ -3026,6 +3013,8 @@
     
     if (func == BIN_PRINTF)
 	fmt = *args++;
+    else if (func == BIN_ECHO && isset(BSDECHO))
+	ops->ind['E'] = 1;
     else if (OPT_HASARG(ops,'f'))
 	fmt = OPT_ARG(ops,'f');
     if (fmt)
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.41
diff -u -r1.41 zsh.h
--- Src/zsh.h	27 Aug 2002 21:10:34 -0000	1.41
+++ Src/zsh.h	2 Sep 2002 12:36:15 -0000
@@ -1002,15 +1002,19 @@
 #define BINF_PLUSOPTS		(1<<1)	/* +xyz legal */
 #define BINF_PRINTOPTS		(1<<2)
 #define BINF_ADDED		(1<<3)	/* is in the builtins hash table */
-#define BINF_ECHOPTS		(1<<4)
-#define BINF_MAGICEQUALS	(1<<5)  /* needs automatic MAGIC_EQUAL_SUBST substitution */
-#define BINF_PREFIX		(1<<6)
-#define BINF_DASH		(1<<7)
-#define BINF_BUILTIN		(1<<8)
-#define BINF_COMMAND		(1<<9)
-#define BINF_EXEC		(1<<10)
-#define BINF_NOGLOB		(1<<11)
-#define BINF_PSPECIAL		(1<<12)
+#define BINF_MAGICEQUALS	(1<<4)  /* needs automatic MAGIC_EQUAL_SUBST substitution */
+#define BINF_PREFIX		(1<<5)
+#define BINF_DASH		(1<<6)
+#define BINF_BUILTIN		(1<<7)
+#define BINF_COMMAND		(1<<8)
+#define BINF_EXEC		(1<<9)
+#define BINF_NOGLOB		(1<<10)
+#define BINF_PSPECIAL		(1<<11)
+/* Builtin option handling */
+#define BINF_SKIPINVALID	(1<<12)	/* Treat invalid option as argument */
+#define BINF_SKIPNUM		(1<<13) /* Treat `-NUM' as argument */
+#define BINF_SKIPDASH		(1<<14) /* Treat `-' as argument (maybe `+') */
+#define BINF_DASHDASHVALID	(1<<15) /* Handle `--' evenf if SKIPINVALD */
 
 struct module {
     char *nam;

-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************