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

[Pádraig Brady]

On 01/26/2013 05: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.
> 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.
>
> > +  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, so it would be safer to probe for
> and use a backslash safe echo here -- 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"
>
> (assuming gnulib bootstrap sets nl='<literal newline>' somewhere)
>
>
> > +      || die "insert_if_absent $file $str: failed"
> > +  fi

Thanks for the review and tips Gary.
While I noticed some of those I didn't fix because
those styles/issues were already used (elsewhere) in bootstrap.
Addressing them are probably best for a subsequent patch.

> 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/

>        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\$/ ?

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 }

>        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.

>          || func_permissions_error "$file"
>      done
> }

thanks,
Pádraig.