[libvirt] consistency: push "update" hook vs. "make syntax-check"

[Daniel Veillard]

On Wed, Jul 15, 2009 at 11:51:41PM +0200, Jim Meyering wrote:
> Currently the server side hook that runs "git diff --check"
> to prevent pushing a change that adds trailing blanks is
> more strict than our "make syntax-check" hook, since the former
> rejects any change that adds blank lines at the end of a file,
> while "make syntax-check" doesn't complain about that.

  Yes, I have been bitten by this yesterday, having this
raised at push time when everything has been commited is
really inconvenient, especially when pushing someone else
patches.

> The two should be consistent.
> One way is to make "make syntax-check" more strict.
> If we were to do that, we'd have to choose between
> cleaning existing files and exempting them from the new test.
> Cleaning is easy and doesn't impact tests at all, so I prefer it.

  I tend to think extra trailing lines at the end of a file
are not a problem, after all we don't complain for those between
2 functions, and that's semantically equivalent. But we need
to allow any patches passing "make syntax-check", since fixing this
server side is near impossible, I agree that adding the rule at
syntax-check time is the best way to solve the difference.

> Here's what would be involved:
> 
>   - remove 121 trailing newlines from 109 files by running this command:
>     git ls-files -z | xargs -0 perl -pi -0777 -e 's/\n\n+$/\n/'
> 
> Add a rule to cfg.mk so that "make syntax-check" warns about
> any new violations.  It might run something like this:
> 
>     git ls-files -z \
>       | xargs -0 perl -ln -0777 -e '/\n(\n+)$/ and print "$ARGV: ".length $1'

  I would rather fix the current set of files to comply
as this would make the base content consistent with what is
allowed. One simple example is copying a file to create a new
one, this should not lead to problems.

> I'm leaning towards the simplicity of the former, in spite of its cost.
> I'll bet someone can come up with a simple *and* efficient script
> (probably using sed) to list files with one or more trailing blank line.

  Even if not optimal, we already load the full code base in the
cache at "make syntax-check" time, so this will not make a big
difference IMHO, and going for the simpler sounds fine to me.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/