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

[Stefano Lattarini]

On 02/03/2012 10:07 AM, Peter Rosin wrote:
> Stefano Lattarini skrev 2012-02-03 09:35:
>> Hi Peter.
>>
>> On 02/03/2012 08:58 AM, Peter Rosin wrote:
>>> Commit Release-1-7-2b-2-gf03ceab "Cope with DOS filenames in
>>> dependencies." inadvertently converted tabs into spaces.
>>>
>>> * lib/depcomp (dashmstdout): Add a tab character to all sets
>>> matching whitespace.
>>> ---
>>>  lib/depcomp |    6 +++---
>>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> Ok for msvc?
>>>
>> Almost...  to avoid similar regressions in the future, I think we
>> could introduce a new '$tab' variable and use that instead...  and
>> then, for extra safety, we might even add a sanity check like:
>>
>>   case "$tab" in *\ *) fatal "\$tab is not a TAB";; esac
>>
>> early in the script, to ensure $tab is not messed up by editors or
>> plain old carelessness.
>>
>> WDYT?
> 
> 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.

>  ## 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
> @@ -264,7 +266,7 @@ aix)
>      # `$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 [].
>
This comment is now redundant, and IMO could be safely deleted.

> -    sed -e 's,^.*\.[a-z]*:[   ]*,,' -e 's,$,:,' < "$tmpdepfile" >> "$depfile"
> +    sed -e 's,^.*\.[a-z]*:['$tab' ]*,,' -e 's,$,:,' < "$tmpdepfile" >> 
> "$depfile"
>
This will break the first argument to '-e' in half, and cause an error in sed
like "sed: -e expression #1, char 15: unterminated `s' command".  I'd suggest
to do something like this instead:

  sed -e "s,^.*\\.[a-z]*:[$tab ]*,," -e 's,$,:,' < "$tmpdepfile" >> "$depfile"

Several similar instances in the rest of the patch.

>    else
>      # The sourcefile does not contain any dependencies, so just
>      # store a dummy comment line, to avoid errors with the Makefile
> @@ -408,7 +410,7 @@ tru64)
>     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 +445,11 @@ msvc7)
>    p
>  }' | $cygpath_u | sort -u | sed -n '
>  s/ /\\ /g
> -s/\(.*\)/    \1 \\/p
> +s/\(.*\)/'$tab'\1 \\/p
>
This too will cause unwanted breakup of the argument to 'sed -n'; you want
something like this instead:

  s/\(.*\)/'"$tab"'\1 \\/p

More similar instances in the rest of the patch.

Regards,
  Stefano