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

[Jim Meyering]

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.