[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] git-version-gen: include command name in one more diagnostic

From: Jim Meyering
Subject: Re: [PATCH] git-version-gen: include command name in one more diagnostic
Date: Mon, 03 Jan 2011 16:17:04 +0100

Bruce Korb wrote:

> Hi Jim,
> On 01/03/11 02:56, Jim Meyering wrote:
>> @@ -85,14 +85,14 @@ v=
>>  # then try "git describe", then default.
>>  if test -f $tarball_version_file
>>  then
>> -    v=`cat $tarball_version_file` || exit 1
>> +    v=`cat $tarball_version_file` || v=
>>      case $v in
>>      *$nl*) v= ;; # reject multi-line output
>>      [0-9]*) ;;
>>      *) v= ;;
>>      esac
>>      test -z "$v" \
>> -    && echo "$0: WARNING: $tarball_version_file seems to be damaged" 1>&2
>> +    && echo "$0: WARNING: $tarball_version_file is missing or damaged" 1>&2
>>  fi
> Please notice that before "cat" runs, the file is tested for existence.
> When "cat" does run, any error message goes to stderr and is thus
> separated from the ``echo "$0: WARNING:..." stuff.  I put them together
> into a single well-ordered output.  Also, "is missing" is not possible.

Hi Bruce,

I now see that your patch did not discard stderr, as I said.
Rather, it combined stderr with the potentially mangled output from
a failing "cat" command, and then printed that potentially tainted
output as a suffix of your new diagnostic.  If the offending file
is a block of NUL bytes or contains some binary garbage, it's better
not to print its contents as part of a diagnostic: e.g., if it contains
a \r byte before the first newline, it's will appear to erase the
diagnostic in some cases.  Then there may be terminal escape codes...

Also, while it's not likely that the file is missing at that point,
it is possible, since there is a race condition.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]