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

[Gary V. Vaughan]

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

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`
      test -n "$duplicate_entries" \
          && func_error "duplicate entries in $file: " $duplicate_entries

      func_grep_q "$str" "$file" \
          && func_verbose "inserting '$str' into '$file'"

      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" \
        || func_permissions_error "$file"
    done
}

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