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

[Gary V. Vaughan]

Hi Padraig,

On 26 Jan 2013, at 21:39, Pádraig Brady <address@hidden> wrote:
> Thanks for the review and tips Gary.

Welcome, and likewise for yours :)

> While I noticed some of those I didn't fix because
> those styles/issues were already used (elsewhere) in bootstrap.

Unfortunately so :(

> Addressing them are probably best for a subsequent patch.

Agreed, though practically speaking, I fear that subsequent patch will
not arrive :(

> On 01/26/2013 05:27 AM, Gary V. Vaughan wrote:
>> Here is the version I've added to libtool bootstrap (note, the arguments
>> are reversed on mine because it supports multiple VCS in the same tree):
>> 
>> # func_insert_if_absent STR FILE...
>> # ---------------------------------
>> # If STR is not already on a line by itself in each FILE, insert it, at the
>> # start.  Entries are inserted at the start of the ignore list to ensure
>> # existing entries starting with ! are not overridden.  Such entries
>> # support whilelisting exceptions after a more generic blacklist pattern.
>> # sorting the new contents of the file and replacing $FILE with the result.
>> func_insert_if_absent ()
>> {
>>     $debug_cmd
>> 
>>     str=$1
>>     shift
>> 
>>     for file
>>     do
>>       test -f "$file" || touch "$file"
>> 
>>       duplicate_entries=`func_gitignore_entries "$file" |sor t |uniq -d`
> 
> s/sor t/sort/

Cut n' paste mangling.

>>       test -n "$duplicate_entries" \
>>           && func_error "duplicate entries in $file: " $duplicate_entries
>> 
>>       func_grep_q "$str" "$file" \
>>           && func_verbose "inserting '$str' into '$file'"
> 
> s/&&/||/ ?
> s/$str/^$str\$/ ?

Agreed on both counts.

> I was wary about using that technique because of
> the possibility of regexp significant characters in $str
> It's probably best to just func_verbose in the { sed condition }

Perhaps.  But the chances of a filename with a regex metachar (that would
change the outcome) are vanishingly small for the sake of a simple verbose
output string, and the alternative code to work around that corner case
significantly uglier, IMHO.

>> 
>>       linesold=`func_gitignore_entries "$file" |wc -l`
>>       linesnew=`$bs_echo "$str" \
>>                 |func_gitignore_entries - "$file" |sort -u |wc -l`
>>       test $linesold -eq $linesnew \
>>         || sed "1i\\$nl$str$nl" "$file" \
> 
> s/sed/sed -i/ ?
> Though you probably can't rely on sed -i so something else would be needed.

Yikes!  How did I miss that?  Thanks for the catch.  Amended to:

    test "$linesold" -eq "$linesnew" \
      || { sed "1i\\$nl$str$nl" "$file" >"$file"T && mv "$file"T "$file"; } \
      || func_permissions_error "$file"

> thanks,
> Pádraig.

Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)