Throwing in my two cents here ...
On 01/26/2013 06:27 AM, Gary V. Vaughan wrote:
> Hi Bernhard, Padraig,
>
> Sorry for the late review, I didn't notice this until after the push
> notification. Comments interspersed below...
>
> On 21 Jan 2013, at 02:20, Bernhard Voelker <m...@bernhard-voelker.de> wrote:
>> During bootstrap, files may be created which are already included
>> in .gitignore, but the test to add such a file relied on the
>> sort order. Now, it just adds such a new entry and thus only
>> changes the file if the line count would change.
>
> This is much more robust; great idea! I've ported it to my bootstrap
> script rewrite too.
>
>> * bootstrap (insert_if_absent): Instead of comparing the
>> sorted new file with the original, the function compares the line
>> count, and only in case of a difference, the given file is changed.
>> Also ensure that the given ignore file does not already include
>> duplicate entries, as otherwise, the entry count would be innacurate.
>> (sort_patterns): Remove this now redundant function.
>> (gitignore_entries): A new function to return significant entries
>> from .gitignore.
>>
>> Improved-by: Pádraig Brady
>
> Just a nit, but the gitlog-to-changelog compatible tag would be
> Co-authored-by.
>
>> [[snip]]
>> +insert_if_absent() {
>> file=$1
>> str=$2
>> test -f $file || touch $file
>
> $file is referenced unquoted, which won't work if there are spaces in the
> filename or path to the file. Although, this problem is preexisting so not
> the fault of this patch.
>
>> - echo "$str" | sort_patterns - $file | cmp -s - $file > /dev/null \
>> - || { echo "$str" | sort_patterns - $file > $file.bak \
>> - && mv $file.bak $file; } \
>> - || die "insert_sorted_if_absent $file $str: failed"
>> + test -r $file || die "Error: failed to read ignore file: $file"
>> + duplicate_entries=$(gitignore_entries $file | sort | uniq -d)
>
> $(...) is not supported by even some modern systems, including solaris 10.
>
Actually, only by /bin/sh. Solaris 10 has a working Korn shell in
/bin/ksh and a working POSIX shell in /usr/xpg4/bin/sh. Requiring
a developer to use them to run the bootstrap doesn't seem a harsh
requirement to me.
> Better to use `...`, although this problem is rampant in bootstrap too, so
> not the fault of this patch... even so, that's no reason to compound the
> error.
>
I think that, at least in maintainer-specific scripts, we should move
away from old Bourne shell idioms and just require a POSIX shell, as
of *now*. Autoconf-generated configure scripts should follow suite (but
changing Autoconf internals in that direction won't be trivial, so that
will likely take more time).
>> + if [ "$duplicate_entries" ] ; then
>
> Using square brackets instead of calling 'test' directly is fine here
> actually, although it's a good habit for GNU programmers not to do that,
> since it causes problems in Autoconf scripts and snippets copied to a
> configure.ac file.
>
>> + die "Error: Duplicate entries in $file: " $duplicate_entries
>> + fi
>> + linesold=$(gitignore_entries $file | wc -l)
>> + linesnew=$(echo "$str" | gitignore_entries - $file | sort -u | wc -l)
>
> Many echo builtins mangle backslashes,
>
Among them, the one of dash -- which the default shell on Debian these
days. Sigh!
> so it would be safer to probe for
> and use a backslash safe echo here
>
Much better would be to just use printf. Safer, simpler and more portable!
-- although gnulib bootstrap doesn't
> currently check for one, so that too is a separate patch.
>
>> + if [ $linesold != $linesnew ] ; then
>
> When comparing numbers, -ne and -eq are better.
>
>> + { echo "$str" | cat - $file > $file.bak && mv $file.bak $file; } \
>
> Unnecessary forks and tmp files are created here. Better:
>
> sed "1i\\$nl$str$nl" "$file"
>
But this won't change the contents of $file...
> (assuming gnulib bootstrap sets nl='<literal newline>' somewhere)
>
>
>> + || die "insert_if_absent $file $str: failed"
>> + fi
>
> [SNIP]
>
Regards,
Stefano
