Re: [PATCH] bootstrap: remove the need for a sorted .gitignore

[Stefano Lattarini]

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 <address@hidden> 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