[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mod
From: |
Stefano Lattarini |
Subject: |
Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode |
Date: |
Fri, 03 Feb 2012 13:32:04 +0100 |
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
- [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Stefano Lattarini, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode,
Stefano Lattarini <=
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Stefano Lattarini, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Stefano Lattarini, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Jim Meyering, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Eric Blake, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Jim Meyering, 2012/02/03
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/04
- Re: [PATCH] depcomp: recognize tabs as whitespace in the dashmstdout mode, Peter Rosin, 2012/02/08