On 21 Sep 2010, at 12:07, Ralf Wildenhues wrote: > Hi Gary, Hallo Ralf,
Thanks again for the review.
> * Gary V. Vaughan wrote on Tue, Sep 21, 2010 at 03:05:46AM CEST:
>> Well, it does at least show that the script interacts correctly with
>> an error for make to help catch the case where someone commits a change
>> to the first paragraph of README without a matching edit to the sed
>> expressions in edit-readme-alpha.
>>
>> Here is (an overkill) patch to not exit with an error, so that `make
>> distcheck' can complete.
>>
>> Okay to push?
>
> Well, I don't want to appear overly nitpicky for this issue,
> but if you don't absolutely need to check and skip non-writable files,
> I wouldn't do it, and rather error out in that case.
Well, the thing that is screwing up distcheck is when the distribution
tarball is unpacked into a read-only tree, and edit-readme-alpha wants
to rewrite the file again...
...although, the "don't re-edit when the alpha text is already there"
check later on should catch this already. Although I feel happier with
both checks in place, I'll remove the writeable test if you'd prefer.
>> * libltdl/config/edit-readme-alpha: If README is non-writable
>> assume that it is being run from distcheck, and bail out with
>> a warning
>
> FWIW, exiting 0 is not what I'd call "bail out".
I mean "bail out" as in: don't do the rest of the processing.
>> for file in "$@"; do
>> + # Assume that read-only README indicates that we are running inside
>> + # the latter part of a `make distcheck'.
>> + test -w $file || {
>> + echo "$progname: not editing non-writeable \`$file' (distcheck?)"
>
> If you prefer to, or need to keep the warning+skip, I suggest to
> print warnings on stderr.
Oops. Well caught, thanks.
>> + continue
>> + }
>> +
>> # Make sure the paragraph we are matching has not been edited since
>> # this script was written.
>> - matched=`sed -n -e '/^This is GNU Libtool,/,/^interface.$/p' $file \
>> + matched=`sed -n -e '/^This is GNU Libtool,/,/^interface\.$/p' $file \
>> |wc -l |sed 's|^ *||'`
>> +
>> + # Unless, of course, it was edited by this script already.
>> + test 3 = "$matched" \
>> + || matched=`sed -n -e '/^This is an alpha testing release/,/behind a
>> consistent, portable interface\.$/p' $file \
>> + |wc -l |sed 's|^ *||'`
>
> Indentation is a bit weird here, I'd have expected the | to align one
> after the ` in the line above.
Okay. I was lining it up with previous clause, but it is ugly. I'll fix
that too.
With those addressed, and another successful distcheck, okay to push?
Cheers,
--
Gary V. Vaughan (g...@gnu.org)
PGP.sig
Description: This is a digitally signed message part
