Re: RFC: [email protected]

"Tony R. Ambardar" <tonya-1KtHxNhk3+Ww5LPnMra/[email protected]> Thu, 18 Sep 2003 13:29:12 -0700 (PDT)

Newsgroups	gmane.comp.gnu.mingw.patches
Message-ID	<[email protected]>

Hi Don,

With a little break and fresh eyes I finally had a chance to look over our
patches. They're functionally equivalent down to the CreateProcess calls,
though mine will sometimes be conservative and generate redundant
backslashes in the CreateProcess command-line.

So I've merged things with a eye to making it as easy as possible for the
GNU Make maintainer to accept the patch as is, and also make it easily
understandable. Please apply the patch to a fresh sub_proc.c and check it
out. I've also re-run my same tests successfully. I think it's much
cleaner and more clear than the original code.

Main changes from the original code and our first patches are:

1. Improved and cleaned up the commentary. Some was a bit misleading.

2. In the mem alloc loop you moved the switch " case down and relied on
fall-through to set *enclose_in_quotes_i, but I left it where it is and
explicitly set *enclose_in_quotes_i. I think this is clearer because there
are really two very different reasons why we're quoting arguments: one
case is for preserving whitespace, and the other is to work around the
weird parsing of arguments containing "s done by MSVCRT.

3. In the character copying loop only, use of the backslash_count variable
and multiple while loops is needlessly complex and results in a "spread
out" logic which can be error-prone. The equivalent logic of "running"
backslash count can be done in just one of the else cases. This lets us
remove all use of backslash_count and the while loop, and also get rid of
one #if/#endif in the closing " code. The resulting logic is much clearer
and more localized.


Take care,
Tony Ambardar

make-mingw-joint.patch (text/plain, 3.3 KB)

--- mingw32-make-3.80.0-3/w32/subproc/sub_proc.c.orig	Sun Aug 31 17:58:42 2003
+++ mingw32-make-3.80.0-3/w32/subproc/sub_proc.c	Wed Sep 17 19:18:35 2003
@@ -968,15 +968,25 @@
 			switch (*p) {
 			case '\"':
 				/*
-				 * We have to insert a backslash for each "
-				 * and each \ that precedes the ".
+				 * We may have to insert a backslash or extra "
+				 * for each " in the argument. Allocate memory
+				 * for both cases.
 				 */
-				bytes_required += (backslash_count + 1);
-				backslash_count = 0;
+				bytes_required++;
+				backslash_count++;
+				/* 
+				 * We wrap the argument in quotes to prevent errors
+				 * due to the bizarre parsing of " in MSVCRT.
+				 */
+				*enclose_in_quotes_i = 1;
 				break;
 
 #if !defined(HAVE_MKS_SHELL) && !defined(HAVE_CYGWIN_SHELL)
 			case '\\':
+				/*
+				 * Backslashes may need to be escaped themselves
+				 * so allocate an extra one.
+				 */
 				backslash_count++;
 				break;
 #endif
@@ -994,16 +1004,17 @@
 	 */
 			case ' ':
 			case '\t':
+				/* Arguments with whitespace need to
+				 * be quoted in order to parse as one. */
 				*enclose_in_quotes_i = 1;
 				/* fall through */
 
 			default:
-				backslash_count = 0;
 				break;
 			}
 
 			/*
-			 * Add one for each character in argv[i].
+			 * Allocate one for each character in argv[i].
 			 */
 			bytes_required++;
 
@@ -1012,9 +1023,9 @@
 
 		if (*enclose_in_quotes_i) {
 			/*
-			 * Add one for each enclosing ",
-			 * and one for each \ that precedes the
-			 * closing ".
+			 * Allocate one for each enclosing ",
+			 * and one for each extra \ required
+			 * within the argument.
 			 */
 			bytes_required += (backslash_count + 2);
 		}
@@ -1027,7 +1038,7 @@
 	}
 
 	/*
-	 * Add one for the terminating NULL.
+	 * Allocate one for the terminating NULL.
 	 */
 	bytes_required++;
 
@@ -1061,35 +1072,36 @@
 
 	while(*argvi) {
 		char* p = *argvi;
-		unsigned int backslash_count = 0;
 
 		if (*enclose_in_quotes_i) {
+			/*
+			 * Add the opening ".
+			 */
 			*(command_line_i++) = '\"';
 		}
 
 		while(*p) {
 			if (*p == '\"') {
 				if (cygwin_mode && have_sh) { /* HAVE_CYGWIN_SHELL */
-					/* instead of a \", cygwin likes "" */
+					/*
+					 * Instead of a \", cygwin likes "".
+					 */
 					*(command_line_i++) = '\"';
 				} else {
 
-				/*
-				 * We have to insert a backslash for the "
-				 * and each \ that precedes the ".
-				 */
-				backslash_count++;
-
-				while(backslash_count) {
+					/*
+					 * We have to insert a backslash for the ".
+					 */
 					*(command_line_i++) = '\\';
-					backslash_count--;
-				};
 				}
 #if !defined(HAVE_MKS_SHELL) && !defined(HAVE_CYGWIN_SHELL)
 			} else if (*p == '\\') {
-				backslash_count++;
-			} else {
-				backslash_count = 0;
+				/*
+				 * An original \ needs to be escaped if
+				 * the argument is to be quoted.
+				 */
+				if (*enclose_in_quotes_i)
+					*(command_line_i++) = '\\';
 #endif
 			}
 
@@ -1100,15 +1112,9 @@
 		}
 
 		if (*enclose_in_quotes_i) {
-#if !defined(HAVE_MKS_SHELL) && !defined(HAVE_CYGWIN_SHELL)
 			/*
-			 * Add one \ for each \ that precedes the
-			 * closing ".
+			 * Add the closing ".
 			 */
-			while(backslash_count--) {
-				*(command_line_i++) = '\\';
-			};
-#endif
 			*(command_line_i++) = '\"';
 		}