Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode

[Peter Rosin]

Stefano Lattarini skrev 2012-02-03 13:32:
> On 02/03/2012 10:07 AM, Peter Rosin wrote:
>> Stefano Lattarini skrev 2012-02-03 09:35:

*snip*

>> I'd rather do that in a follow-up, like below, so that the real change
>> in the above patch isn't hidden in the churn.
>>
> Sounds sensible; agreed.
> 
>> Ok?
>>
> Not exactly, there are several problems with the patch.  See below.
> 
>> No sanity checks included, but that seems way overkill to me...
>>
> OK, no problem.
> 
>> --- a/lib/depcomp
>> +++ b/lib/depcomp
>> @@ -1,7 +1,7 @@
>>  #! /bin/sh
>>  # depcomp - compile a program generating dependencies as side-effects
>>  
>> -scriptversion=2012-02-03.07; # UTC
>> +scriptversion=2012-02-03.08; # UTC
>>  
>>  # Copyright (C) 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2009, 2010,
>>  # 2011, 2012 Free Software Foundation, Inc.
>> @@ -57,6 +57,12 @@ EOF
>>      ;;
>>  esac
>>  
>> +# A tabulation character.
>> +tab='       '
>> +# A newline character.
>> +nl='
>> +'
>> +
>>  if test -z "$depmode" || test -z "$source" || test -z "$object"; then
>>    echo "depcomp: Variables source, object and depmode must be set" 1>&2
>>    exit 1
>> @@ -162,8 +168,7 @@ gcc)
>>  ## typically no way to rebuild the header).  We avoid this by adding
>>  ## dummy dependencies for each header file.  Too bad gcc doesn't do
>>  ## this for us directly.
>> -  tr ' ' '
>> -' < "$tmpdepfile" |
>> +  tr ' ' $nl < "$tmpdepfile" |
>>
> Missing quote around "$nl".  The result will be an error from 'tr',
> and an empty output passed down the pipe (d'oh!).  Several similar
> instances in the rest of the patch.

*snip* further evidence of <something>

Man, what a brain-fart, how embarrassing!

Thanks for the review, it's highly appropriate this time.

In my defense, the testsuite didn't catch it either (no new FAILs anyway).
But in the closing argument, the prosecution brings up that I didn't even
check a single outcome of the autoconf test "checking dependency style
of..."  Big sigh!

Regarding changing the 's/.*['$tab' ]//' pattern into "s/.*[$tab ]//"
I'm a bit reluctant to have * and ? characters inside " quotes as
that, on occasion, have resulted in bogus filename expansions.  And
those bugs are not fun to chase.  So, I went with 's/.*['"$tab"' ]//'
instead.

Maybe depmod.tap should be replaced/rewritten with "compilers" that
simulate the different depmods?  I could tinker with that a bit...

Anyway, let's take this one more round since it is obvious that I can't
be 100% trusted today and on top of that have been reading through this
enough to not see any bugs in it even if clearly marked as such...

So, ok to push "depcomp: recognize tabs as whitespace in the dashmstdout mode"
and the below patch to the msvc branch?

Cheers,
Peter



>From 8d94820bf3e3cfdd3e26f4631a1c7358411d7151 Mon Sep 17 00:00:00 2001
From: Peter Rosin <address@hidden>
Date: Fri, 3 Feb 2012 13:49:12 +0100
Subject: [PATCH] depcomp: try to prevent whitespace regressions

Suggested by Stefano Lattarini.

* lib/depcomp: Add $tab and $nl variables and use them
throughout.
---
 lib/depcomp |   42 ++++++++++++++++++++----------------------
 1 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/lib/depcomp b/lib/depcomp
index f2681b9..c4ab8b2 100755
--- a/lib/depcomp
+++ b/lib/depcomp
@@ -1,7 +1,7 @@
 #! /bin/sh
 # depcomp - compile a program generating dependencies as side-effects
 
-scriptversion=2012-02-03.07; # UTC
+scriptversion=2012-02-03.12; # UTC
 
 # Copyright (C) 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2009, 2010,
 # 2011, 2012 Free Software Foundation, Inc.
@@ -57,6 +57,12 @@ EOF
     ;;
 esac
 
+# A tabulation character.
+tab='  '
+# A newline character.
+nl='
+'
+
 if test -z "$depmode" || test -z "$source" || test -z "$object"; then
   echo "depcomp: Variables source, object and depmode must be set" 1>&2
   exit 1
@@ -162,8 +168,7 @@ gcc)
 ## typically no way to rebuild the header).  We avoid this by adding
 ## dummy dependencies for each header file.  Too bad gcc doesn't do
 ## this for us directly.
-  tr ' ' '
-' < "$tmpdepfile" |
+  tr ' ' "$nl" < "$tmpdepfile" |
 ## Some versions of gcc put a space before the `:'.  On the theory
 ## that the space means something, we add a space to the output as
 ## well.  hp depmode also adds that space, but also prefixes the VPATH
@@ -205,16 +210,13 @@ sgi)
     # IRIX 6.2 sed, 8192 in IRIX 6.5).  We also remove comment lines;
     # the IRIX cc adds comments like `#:fec' to the end of the
     # dependency line.
-    tr ' ' '
-' < "$tmpdepfile" \
+    tr ' ' "$nl" < "$tmpdepfile" \
     | sed -e 's/^.*\.o://' -e 's/#.*$//' -e '/^$/ d' | \
-    tr '
-' ' ' >> "$depfile"
+    tr "$nl" ' ' >> "$depfile"
     echo >> "$depfile"
 
     # The second pass generates a dummy entry for each header file.
-    tr ' ' '
-' < "$tmpdepfile" \
+    tr ' ' "$nl" < "$tmpdepfile" \
    | sed -e 's/^.*\.o://' -e 's/#.*$//' -e '/^$/ d' -e 's/$/:/' \
    >> "$depfile"
   else
@@ -263,8 +265,7 @@ aix)
     # Do two passes, one to just change these to
     # `$object: dependent.h' and one to simply `dependent.h:'.
     sed -e "s,^.*\.[a-z]*:,$object:," < "$tmpdepfile" > "$depfile"
-    # That's a tab and a space in the [].
-    sed -e 's,^.*\.[a-z]*:[     ]*,,' -e 's,$,:,' < "$tmpdepfile" >> "$depfile"
+    sed -e 's,^.*\.[a-z]*:['"$tab"' ]*,,' -e 's,$,:,' < "$tmpdepfile" >> 
"$depfile"
   else
     # The sourcefile does not contain any dependencies, so just
     # store a dummy comment line, to avoid errors with the Makefile
@@ -407,8 +408,7 @@ tru64)
    done
    if test -f "$tmpdepfile"; then
       sed -e "s,^.*\.[a-z]*:,$object:," < "$tmpdepfile" > "$depfile"
-      # That's a tab and a space in the [].
-      sed -e 's,^.*\.[a-z]*:[   ]*,,' -e 's,$,:,' < "$tmpdepfile" >> "$depfile"
+      sed -e 's,^.*\.[a-z]*:['"$tab"' ]*,,' -e 's,$,:,' < "$tmpdepfile" >> 
"$depfile"
    else
       echo "#dummy" > "$depfile"
    fi
@@ -443,11 +443,11 @@ msvc7)
   p
 }' | $cygpath_u | sort -u | sed -n '
 s/ /\\ /g
-s/\(.*\)/      \1 \\/p
+s/\(.*\)/'"$tab"'\1 \\/p
 s/.\(.*\) \\/\1:/
 H
 $ {
-  s/.*/        /
+  s/.*/'"$tab"'/
   G
   p
 }' >> "$depfile"
@@ -502,11 +502,10 @@ dashmstdout)
   # in the target name.  This is to cope with DOS-style filenames:
   # a dependency such as `c:/foo/bar' could be seen as target `c' otherwise.
   "$@" $dashmflag |
-    sed 's:^[   ]*[^:   ][^:][^:]*\:[   ]*:'"$object"'\: :' > "$tmpdepfile"
+    sed 's:^['"$tab"' ]*[^:'"$tab"' ][^:][^:]*\:['"$tab"' ]*:'"$object"'\: :' 
> "$tmpdepfile"
   rm -f "$depfile"
   cat < "$tmpdepfile" > "$depfile"
-  tr ' ' '
-' < "$tmpdepfile" | \
+  tr ' ' "$nl" < "$tmpdepfile" | \
 ## Some versions of the HPUX 10.20 sed can't process this invocation
 ## correctly.  Breaking it into two sed invocations is a workaround.
     sed -e 's/^\\$//' -e '/^$/d' -e '/:$/d' | sed -e 's/$/ :/' >> "$depfile"
@@ -562,8 +561,7 @@ makedepend)
   # makedepend may prepend the VPATH from the source file name to the object.
   # No need to regex-escape $object, excess matching of '.' is harmless.
   sed "s|^.*\($object *:\)|\1|" "$tmpdepfile" > "$depfile"
-  sed '1,2d' "$tmpdepfile" | tr ' ' '
-' | \
+  sed '1,2d' "$tmpdepfile" | tr ' ' "$nl" | \
 ## Some versions of the HPUX 10.20 sed can't process this invocation
 ## correctly.  Breaking it into two sed invocations is a workaround.
     sed -e 's/^\\$//' -e '/^$/d' -e '/:$/d' | sed -e 's/$/ :/' >> "$depfile"
@@ -652,8 +650,8 @@ msvisualcpp)
   sed -n '/^#line [0-9][0-9]* "\([^"]*\)"/ s::\1:p' | $cygpath_u | sort -u > 
"$tmpdepfile"
   rm -f "$depfile"
   echo "$object : \\" > "$depfile"
-  sed < "$tmpdepfile" -n -e 's% %\\ %g' -e '/^\(.*\)$/ s::     \1 \\:p' >> 
"$depfile"
-  echo "       " >> "$depfile"
+  sed < "$tmpdepfile" -n -e 's% %\\ %g' -e '/^\(.*\)$/ s::'"$tab"'\1 \\:p' >> 
"$depfile"
+  echo "$tab" >> "$depfile"
   sed < "$tmpdepfile" -n -e 's% %\\ %g' -e '/^\(.*\)$/ s::\1\::p' >> "$depfile"
   rm -f "$tmpdepfile"
   ;;
-- 
1.7.5.1