Fix some bugs in variable pattern substitution (e.g. $(VAR:A=B)),

reported by Markus Mauhart <qwe123@chello.at>.  One was a simple typo; to
fix the other we call patsubst_expand() for all instances of variable
substitution, even when there is no '%'.  We used to call subst_expand()
with a special flag set in the latter case, but it didn't work properly
in all situations.  Easier to just use patsubst_expand() since that's
what it is.

9 files changed, 147 insertions, 103 deletions

diff --git a/ChangeLog b/ChangeLog
index 759c4e4..9b2f30d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2004-09-20  Paul D. Smith  <psmith@gnu.org>
+
+	* expand.c (variable_expand_string): Modify to invoke
+	patsubst_expand() instead of subst_expand(); the latter didn't
+	handle suffix patterns correctly.
+	* function.c (subst_expand): Remove the SUFFIX_ONLY parameter; it
+	was used only from variable_expand_string() and is no longer used
+	there.
+	(func_subst): Ditto, on call to subst_expand().
+	(patsubst_expand): Require the percent pointers to point to the
+	character after the %, not to the % itself.
+	* read.c (record_files): New call criteria for patsubst_expand().
+	* variable.h: Remove SUFFIX_ONLY from subst_expand() prototype.
+	This is to fix a bug reported by Markus Mauhart <qwe123@chello.at>.
+
+2004-09-19  Paul D. Smith  <psmith@gnu.org>
+
+	* function.c (subst_expand): Fix a check in by_word: look for a
+	previous blank if we're beyond the beginning of the string, not
+	the beginning of the word.
+	Bugs reported by Markus Mauhart <qwe123@chello.at>.
+
 2004-05-16  Paul D. Smith  <psmith@gnu.org>
 
 	* remake.c (update_goal_chain): Change the argument specifying
diff --git a/expand.c b/expand.c
index 2c8b4b6..4440192 100644
--- a/expand.c
+++ b/expand.c
@@ -304,51 +304,49 @@ variable_expand_string (char *line, char *string, long length)
 		    if (v == 0)
 		      warn_undefined (beg, colon - beg);
 
+                    /* If the variable is not empty, perform the
+                       substitution.  */
 		    if (v != 0 && *v->value != '\0')
 		      {
-			char *value = (v->recursive ? recursively_expand (v)
+			char *pattern, *replace, *ppercent, *rpercent;
+			char *value = (v->recursive
+                                       ? recursively_expand (v)
 				       : v->value);
-			char *pattern, *percent;
-			if (free_beg)
-			  {
-			    *subst_end = '\0';
-			    pattern = subst_beg;
-			  }
-			else
-			  {
-			    pattern = (char *) alloca (subst_end - subst_beg
-						       + 1);
-			    bcopy (subst_beg, pattern, subst_end - subst_beg);
-			    pattern[subst_end - subst_beg] = '\0';
-			  }
-			percent = find_percent (pattern);
-			if (percent != 0)
-			  {
-			    char *replace;
-			    if (free_beg)
-			      {
-				*replace_end = '\0';
-				replace = replace_beg;
-			      }
-			    else
-			      {
-				replace = (char *) alloca (replace_end
-							   - replace_beg
-							   + 1);
-				bcopy (replace_beg, replace,
-				       replace_end - replace_beg);
-				replace[replace_end - replace_beg] = '\0';
-			      }
-
-			    o = patsubst_expand (o, value, pattern, replace,
-						 percent, (char *) 0);
-			  }
+
+                        /* Copy the pattern and the replacement.  Add in an
+                           extra % at the beginning to use in case there
+                           isn't one in the pattern.  */
+                        pattern = (char *) alloca (subst_end - subst_beg + 2);
+                        *(pattern++) = '%';
+                        bcopy (subst_beg, pattern, subst_end - subst_beg);
+                        pattern[subst_end - subst_beg] = '\0';
+
+                        replace = (char *) alloca (replace_end
+                                                   - replace_beg + 2);
+                        *(replace++) = '%';
+                        bcopy (replace_beg, replace,
+                               replace_end - replace_beg);
+                        replace[replace_end - replace_beg] = '\0';
+
+                        /* Look for %.  Set the percent pointers properly
+                           based on whether we find one or not.  */
+			ppercent = find_percent (pattern);
+			if (ppercent)
+                          {
+                            ++ppercent;
+                            rpercent = 0;
+                          }
 			else
-			  o = subst_expand (o, value,
-					    pattern, replace_beg,
-					    strlen (pattern),
-					    end - replace_beg,
-					    0, 1);
+                          {
+                            ppercent = pattern;
+                            rpercent = replace;
+                            --pattern;
+                            --replace;
+                          }
+
+                        o = patsubst_expand (o, value, pattern, replace,
+                                             ppercent, rpercent);
+
 			if (v->recursive)
 			  free (value);
 		      }
diff --git a/function.c b/function.c
index bad5258..392ff25 100644
--- a/function.c
+++ b/function.c
@@ -72,19 +72,17 @@ static struct hash_table function_table;
    each occurrence of SUBST with REPLACE. TEXT is null-terminated.  SLEN is
    the length of SUBST and RLEN is the length of REPLACE.  If BY_WORD is
    nonzero, substitutions are done only on matches which are complete
-   whitespace-delimited words.  If SUFFIX_ONLY is nonzero, substitutions are
-   done only at the ends of whitespace-delimited words.  */
+   whitespace-delimited words.  */
 
 char *
 subst_expand (char *o, char *text, char *subst, char *replace,
-              unsigned int slen, unsigned int rlen,
-              int by_word, int suffix_only)
+              unsigned int slen, unsigned int rlen, int by_word)
 {
   char *t = text;
   unsigned int tlen = strlen (text);
   char *p;
 
-  if (slen == 0 && !by_word && !suffix_only)
+  if (slen == 0 && !by_word)
     {
       /* The first occurrence of "" in any string is its end.  */
       o = variable_buffer_output (o, t, tlen);
@@ -95,7 +93,7 @@ subst_expand (char *o, char *text, char *subst, char *replace,
 
   do
     {
-      if ((by_word | suffix_only) && slen == 0)
+      if (by_word && slen == 0)
 	/* When matching by words, the empty string should match
 	   the end of each word, rather than the end of the whole text.  */
 	p = end_of_token (next_token (t));
@@ -116,11 +114,9 @@ subst_expand (char *o, char *text, char *subst, char *replace,
 
       /* If we're substituting only by fully matched words,
 	 or only at the ends of words, check that this case qualifies.  */
-      if ((by_word
-	   && ((p > t && !isblank ((unsigned char)p[-1]))
-	       || (p[slen] != '\0' && !isblank ((unsigned char)p[slen]))))
-	  || (suffix_only
-	      && (p[slen] != '\0' && !isblank ((unsigned char)p[slen]))))
+      if (by_word
+          && ((p > text && !isblank ((unsigned char)p[-1]))
+              || (p[slen] != '\0' && !isblank ((unsigned char)p[slen]))))
 	/* Struck out.  Output the rest of the string that is
 	   no longer to be replaced.  */
 	o = variable_buffer_output (o, subst, slen);
@@ -138,52 +134,65 @@ subst_expand (char *o, char *text, char *subst, char *replace,
 
   return o;
 }
-
+
 
 /* Store into VARIABLE_BUFFER at O the result of scanning TEXT
    and replacing strings matching PATTERN with REPLACE.
    If PATTERN_PERCENT is not nil, PATTERN has already been
    run through find_percent, and PATTERN_PERCENT is the result.
    If REPLACE_PERCENT is not nil, REPLACE has already been
-   run through find_percent, and REPLACE_PERCENT is the result.  */
+   run through find_percent, and REPLACE_PERCENT is the result.
+   Note that we expect PATTERN_PERCENT and REPLACE_PERCENT to point to the
+   character _AFTER_ the %, not to the % itself.
+*/
 
 char *
 patsubst_expand (char *o, char *text, char *pattern, char *replace,
                  char *pattern_percent, char *replace_percent)
 {
   unsigned int pattern_prepercent_len, pattern_postpercent_len;
-  unsigned int replace_prepercent_len, replace_postpercent_len = 0;
+  unsigned int replace_prepercent_len, replace_postpercent_len;
   char *t;
   unsigned int len;
   int doneany = 0;
 
   /* We call find_percent on REPLACE before checking PATTERN so that REPLACE
      will be collapsed before we call subst_expand if PATTERN has no %.  */
-  if (replace_percent == 0)
-    replace_percent = find_percent (replace);
-  if (replace_percent != 0)
+  if (!replace_percent)
+    {
+      replace_percent = find_percent (replace);
+      if (replace_percent)
+        ++replace_percent;
+    }
+
+  /* Record the length of REPLACE before and after the % so we don't have to
+     compute these lengths more than once.  */
+  if (replace_percent)
     {
-      /* Record the length of REPLACE before and after the % so
-	 we don't have to compute these lengths more than once.  */
-      replace_prepercent_len = replace_percent - replace;
-      replace_postpercent_len = strlen (replace_percent + 1);
+      replace_prepercent_len = replace_percent - replace - 1;
+      replace_postpercent_len = strlen (replace_percent);
     }
   else
-    /* We store the length of the replacement
-       so we only need to compute it once.  */
-    replace_prepercent_len = strlen (replace);
+    {
+      replace_prepercent_len = strlen (replace);
+      replace_postpercent_len = 0;
+    }
 
-  if (pattern_percent == 0)
-    pattern_percent = find_percent (pattern);
-  if (pattern_percent == 0)
+  if (!pattern_percent)
+    {
+      pattern_percent = find_percent (pattern);
+      if (pattern_percent)
+        ++pattern_percent;
+    }
+  if (!pattern_percent)
     /* With no % in the pattern, this is just a simple substitution.  */
     return subst_expand (o, text, pattern, replace,
-			 strlen (pattern), strlen (replace), 1, 0);
+			 strlen (pattern), strlen (replace), 1);
 
   /* Record the length of PATTERN before and after the %
      so we don't have to compute it more than once.  */
-  pattern_prepercent_len = pattern_percent - pattern;
-  pattern_postpercent_len = strlen (pattern_percent + 1);
+  pattern_prepercent_len = pattern_percent - pattern - 1;
+  pattern_postpercent_len = strlen (pattern_percent);
 
   while ((t = find_next_token (&text, &len)) != 0)
     {
@@ -196,16 +205,16 @@ patsubst_expand (char *o, char *text, char *pattern, char *replace,
       /* Does the prefix match? */
       if (!fail && pattern_prepercent_len > 0
 	  && (*t != *pattern
-	      || t[pattern_prepercent_len - 1] != pattern_percent[-1]
+	      || t[pattern_prepercent_len - 1] != pattern_percent[-2]
 	      || !strneq (t + 1, pattern + 1, pattern_prepercent_len - 1)))
 	fail = 1;
 
       /* Does the suffix match? */
       if (!fail && pattern_postpercent_len > 0
-	  && (t[len - 1] != pattern_percent[pattern_postpercent_len]
-	      || t[len - pattern_postpercent_len] != pattern_percent[1]
+	  && (t[len - 1] != pattern_percent[pattern_postpercent_len - 1]
+	      || t[len - pattern_postpercent_len] != *pattern_percent
 	      || !strneq (&t[len - pattern_postpercent_len],
-			  &pattern_percent[1], pattern_postpercent_len - 1)))
+			  pattern_percent, pattern_postpercent_len - 1)))
 	fail = 1;
 
       if (fail)
@@ -226,7 +235,7 @@ patsubst_expand (char *o, char *text, char *pattern, char *replace,
 					  len - (pattern_prepercent_len
 						 + pattern_postpercent_len));
 	      /* Output the part of the replacement after the %.  */
-	      o = variable_buffer_output (o, replace_percent + 1,
+	      o = variable_buffer_output (o, replace_percent,
 					  replace_postpercent_len);
 	    }
 	}
@@ -641,7 +650,7 @@ static char *
 func_subst (char *o, char **argv, const char *funcname UNUSED)
 {
   o = subst_expand (o, argv[2], argv[0], argv[1], strlen (argv[0]),
-		    strlen (argv[1]), 0, 0);
+		    strlen (argv[1]), 0);
 
   return o;
 }
diff --git a/job.c b/job.c
index 13431d3..c458893 100644
--- a/job.c
+++ b/job.c
@@ -3099,8 +3099,8 @@ construct_command_argv_internal (char *line, char **restp, char *shell,
   if (new_argv[0] == 0)
     /* Line was empty.  */
     return 0;
-  else
-    return new_argv;
+
+  return new_argv;
 
  slow:;
   /* We must use the shell.  */
diff --git a/read.c b/read.c
index e2ad630..fc69a8e 100644
--- a/read.c
+++ b/read.c
@@ -1861,7 +1861,7 @@ record_files (struct nameseq *filenames, char *pattern, char *pattern_percent,
 		  if (percent == 0)
 		    continue;
 		  o = patsubst_expand (buffer, name, pattern, d->name,
-				       pattern_percent, percent);
+				       pattern_percent+1, percent+1);
                   /* If the name expanded to the empty string, that's
                      illegal.  */
                   if (o == buffer)
@@ -2067,7 +2067,7 @@ record_files (struct nameseq *filenames, char *pattern, char *pattern_percent,
 	      static char *percent = "%";
 	      char *buffer = variable_expand ("");
 	      char *o = patsubst_expand (buffer, name, pattern, percent,
-					 pattern_percent, percent);
+					 pattern_percent+1, percent+1);
 	      f->stem = savestring (buffer, o - buffer);
 	    }
 	}
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 0e84c6b..eec2ef1 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2004-09-20  Paul D. Smith  <psmith@gnu.org>
+
+	* scripts/functions/substitution: Rewrite to use run_make_test()
+	interface, and add test for substitution failures reported by
+	Markus Mauhart <qwe123@chello.at>.
+
 2004-03-22  Paul D. Smith  <psmith@gnu.org>
 
 	* test_driver.pl (run_each_test, toplevel, compare_output): Change
diff --git a/tests/run_make_tests.pl b/tests/run_make_tests.pl
index 8452c6b..991e780 100755
--- a/tests/run_make_tests.pl
+++ b/tests/run_make_tests.pl
@@ -64,6 +64,14 @@ sub run_make_test
     $makefile = &get_tmpfile();
   }
 
+  # If either the makestring or the answer don't end in newlines, add one In
+  # the future should we allow an option to disable this?  For now if you
+  # want to test handling with no newline you have to call the underlying
+  # functions directly.
+
+  $makestring =~ /\n$/s or $makestring .= "\n";
+  $answer =~ /\n$/s     or $answer .= "\n";
+
   # Replace @MAKEFILE@ with the makefile name and @MAKE@ with the path to
   # make in both $makestring and $answer.
 
diff --git a/tests/scripts/functions/substitution b/tests/scripts/functions/substitution
index 9280dbb..0d2f6a2 100644
--- a/tests/scripts/functions/substitution
+++ b/tests/scripts/functions/substitution
@@ -1,32 +1,33 @@
-$description = "The following test creates a makefile to ...";
+#                                                                    -*-perl-*-
 
-$details = "";
-
-open(MAKEFILE,"> $makefile");
+$description = "Test the subst and patsubst functions";
 
-# The Contents of the MAKEFILE ...
+$details = "";
 
-print MAKEFILE "foo := a.o b.o c.o\n"
-              ."bar := \$(foo:.o=.c)\n"
-	      ."bar2:= \$(foo:%.o=%.c)\n"
-              ."bar3:= \$(patsubst %.c,%.o,x.c.c bar.c)\n"
-              ."all:\n"
-              ."\t\@echo \$(bar)\n"
-              ."\t\@echo \$(bar2)\n"
-              ."\t\@echo \$(bar3)\n";
+# Generic patsubst test: test both the function and variable form.
 
-# END of Contents of MAKEFILE
+run_make_test('
+foo := a.o b.o c.o
+bar := $(foo:.o=.c)
+bar2:= $(foo:%.o=%.c)
+bar3:= $(patsubst %.c,%.o,x.c.c bar.c)
+all:;@echo $(bar); echo $(bar2); echo $(bar3)',
+'',
+'a.c b.c c.c
+a.c b.c c.c
+x.c.o bar.o');
 
-close(MAKEFILE);
+# Patsubst without '%'--shouldn't match because the whole word has to match
+# in patsubst.  Based on a bug report by Markus Mauhart <qwe123@chello.at>
 
-&run_make_with_options($makefile,"",&get_logfile);
+run_make_test('all:;@echo $(patsubst Foo,Repl,FooFoo)', '', 'FooFoo');
 
-# Create the answer to what should be produced by this Makefile
-$answer = "a.c b.c c.c\n"
-         ."a.c b.c c.c\n"
-         ."x.c.o bar.o\n";
+# Variable subst where a pattern matches multiple times in a single word.
+# Based on a bug report by Markus Mauhart <qwe123@chello.at>
 
-&compare_output($answer,&get_logfile(1));
+run_make_test('
+A := fooBARfooBARfoo
+all:;@echo $(A:fooBARfoo=REPL)', '', 'fooBARREPL');
 
 1;
 
diff --git a/variable.h b/variable.h
index 613278a..4a23e4a 100644
--- a/variable.h
+++ b/variable.h
@@ -125,7 +125,7 @@ extern void restore_variable_buffer PARAMS ((char *buf, unsigned int len));
 extern int handle_function PARAMS ((char **op, char **stringp));
 extern int pattern_matches PARAMS ((char *pattern, char *percent, char *str));
 extern char *subst_expand PARAMS ((char *o, char *text, char *subst, char *replace,
-		unsigned int slen, unsigned int rlen, int by_word, int suffix_only));
+		unsigned int slen, unsigned int rlen, int by_word));
 extern char *patsubst_expand PARAMS ((char *o, char *text, char *pattern, char *replace,
 		char *pattern_percent, char *replace_percent));