[libvirt] consistency: push "update" hook vs. "make syntax-check"
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. 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. 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' That command prints the name of each offending file with its trailing blank line count. While it takes well under a second on my system, (admittedly, with a hot cache), it's not well optimized, reading each file into memory and processing it. If it matters, we can come up with a more efficient (yet still portable) way to compare the last two bytes of each file to "\n\n". I went ahead and wrote a nearly-minimal script to do that. Rather than reading/processing all 27MB of sources, this reads just the last 2 bytes of each of the 1048 files, comparing those bytes to "\n\n" and printing the name when there's a match: git ls-files -z \ | xargs -0 perl -le ' foreach my $f (@ARGV) { open F,"<",$f or (warn "failed to open $f: $!\n"), next; my $p = sysseek(F, -2, 2); # seek failure probably means file has < 2 bytes; ignore my $two; defined $p and $p = sysread F,$two,2; close F; # ignore read failure $p && $two eq "\n\n" and (print $f),$fail=1; } END {exit defined $fail ? 1 : 0}' However, counting minor page faults, there's little difference (2193 before, 1976 after), but maximum memory consumption is probably way down. I didn't measure that. With a hot cache, the latter takes .02elapsed, and the former takes .09 seconds. 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.
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.
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.
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'
That command prints the name of each offending file with its trailing blank line count. While it takes well under a second on my system, (admittedly, with a hot cache), it's not well optimized, reading each file into memory and processing it.
Here are two change-sets to implement that. The first just removes all trailing blank line, so I've omitted its boring 100+ diffs.
From 90046bad3bb688f8bfddfedcf3b681cf9b99028d Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 16 Jul 2009 08:25:36 +0200 Subject: [PATCH 1/2] remove all trailing blank lines
by running this command: git ls-files -z | xargs -0 perl -pi -0777 -e 's/\n\n+$/\n/' This is in preparation for a more strict make syntax-check rule that will detect trailing blank lines. --- COPYING.LIB | 2 -- NEWS | 1 - acinclude.m4 | 1 - .../0001-Step-1-of-8-Define-the-public-API.patch | 1 - ...tep-2-of-8-Define-the-internal-driver-API.patch | 1 - ...0003-Step-3-of-8-Implement-the-public-API.patch | 1 - ...ep-4-of-8-Define-the-wire-protocol-format.patch | 1 - ...0005-Step-5-of-8-Implement-the-RPC-client.patch | 1 - ...of-8-Implement-the-server-side-dispatcher.patch | 1 - ...-Step-7-of-8-Implement-the-driver-methods.patch | 1 - .../0008-Step-8-of-8-Add-virsh-support.patch | 1 - docs/devhelp/devhelp.xsl | 2 -- docs/devhelp/html.xsl | 2 -- docs/examples/python/README | 1 - docs/examples/python/dominfo.py | 2 -- docs/library.xen | 1 - docs/news.xsl | 1 - docs/schemas/capability.rng | 1 - docs/testnetdef.xml | 1 - docs/testnetpriv.xml | 1 - examples/domain-events/events-python/event-test.py | 1 - python/libvirt_wrap.h | 1 - qemud/THREADING.txt | 1 - qemud/libvirtd_qemu.aug | 1 - src/logging.c | 1 - src/lxc_conf.h | 1 - src/network_conf.c | 1 - src/network_conf.h | 1 - src/nodeinfo.c | 2 -- src/opennebula/one_client.c | 2 -- src/opennebula/one_client.h | 2 -- src/opennebula/one_conf.c | 1 - src/opennebula/one_conf.h | 1 - src/qemu_conf.c | 1 - src/storage_backend.c | 1 - src/vbox/vbox_XPCOMCGlue.c | 1 - src/vbox/vbox_XPCOMCGlue.h | 1 - src/veth.c | 1 - tests/capabilityschemadata/caps-qemu-kvm.xml | 2 -- tests/capabilityschemadata/caps-test.xml | 2 -- tests/confdata/libvirtd.conf | 2 -- ...ge_serial_3600c0ff000d7a2a5d463ff4902000000.xml | 2 -- tests/nodeinfodata/linux-nodeinfo-1.cpuinfo | 1 - tests/nodeinfodata/linux-nodeinfo-2.cpuinfo | 1 - tests/nodeinfodata/linux-nodeinfo-4.cpuinfo | 1 - tests/nodeinfodata/linux-nodeinfo-5.cpuinfo | 1 - tests/nodeinfodata/linux-nodeinfo-6.cpuinfo | 1 - tests/nodeinfotest.c | 1 - tests/object-locking.ml | 2 -- tests/reconnect.c | 1 - tests/sexpr2xmldata/sexpr2xml-bridge-ipaddr.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-disk-block.sexpr | 1 - .../sexpr2xml-disk-drv-blktap-qcow.sexpr | 1 - .../sexpr2xml-disk-drv-blktap-raw.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-disk-file.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-net-bridged.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-net-routed.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-pci-devs.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-pv-localtime.sexpr | 1 - .../sexpr2xml-pv-vfb-new-vncdisplay.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-pv-vfb-new.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-pv-vfb-orig.sexpr | 1 - tests/sexpr2xmldata/sexpr2xml-pv.sexpr | 1 - tests/sexpr2xmltest.c | 1 - tests/storagepoolschemadata/pool-fs.xml | 1 - tests/storagepoolschemadata/pool-netfs.xml | 1 - tests/storagevolschemadata/vol-logical.xml | 1 - tests/testutilsqemu.h | 1 - tests/testutilsxen.h | 1 - tests/xencapsdata/xen-i686-pae-hvm.cpuinfo | 1 - tests/xencapsdata/xen-i686-pae.cpuinfo | 2 -- tests/xencapsdata/xen-i686.cpuinfo | 1 - tests/xencapstest.c | 1 - tests/xmconfigtest.c | 1 - tests/xml2sexprdata/xml2sexpr-bridge-ipaddr.xml | 1 - .../xml2sexpr-disk-block-shareable.xml | 1 - tests/xml2sexprdata/xml2sexpr-disk-block.xml | 1 - tests/xml2sexprdata/xml2sexpr-disk-drv-blkback.xml | 1 - .../xml2sexpr-disk-drv-blktap-qcow.xml | 1 - .../xml2sexpr-disk-drv-blktap-raw.xml | 1 - tests/xml2sexprdata/xml2sexpr-disk-drv-blktap.xml | 1 - tests/xml2sexprdata/xml2sexpr-disk-drv-loop.xml | 1 - tests/xml2sexprdata/xml2sexpr-disk-file.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-kernel.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-localtime.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-parallel-tcp.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-file.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-null.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-pipe.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-pty.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-stdio.xml | 1 - .../xml2sexpr-fv-serial-tcp-telnet.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-tcp.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-udp.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-serial-unix.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-sound.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-usbmouse.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-usbtablet.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-utc.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv-vncunused.xml | 1 - tests/xml2sexprdata/xml2sexpr-fv.xml | 1 - tests/xml2sexprdata/xml2sexpr-net-bridged.xml | 1 - tests/xml2sexprdata/xml2sexpr-net-e1000.xml | 1 - tests/xml2sexprdata/xml2sexpr-net-routed.xml | 1 - tests/xml2sexprdata/xml2sexpr-pci-devs.xml | 1 - tests/xml2sexprdata/xml2sexpr-pv-bootloader.xml | 1 - tests/xml2sexprdata/xml2sexpr-pv.xml | 1 - tests/xml2sexprtest.c | 1 - 109 files changed, 0 insertions(+), 122 deletions(-) ...
From 9d93638ab94709f5a5f5b48db506e1d0afbe3504 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 16 Jul 2009 09:06:58 +0200 Subject: [PATCH 2/2] make "make syntax-check" consistent with "git diff --check"
This makes "make syntax-check" fail when a version-controlled file contains a trailing blank line. * cfg.mk (sc_prohibit_trailing_blank_lines): New rule. --- cfg.mk | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-) diff --git a/cfg.mk b/cfg.mk index 3b3d57f..0bf935d 100644 --- a/cfg.mk +++ b/cfg.mk @@ -228,6 +228,15 @@ sc_libvirt_unmarked_diagnostics: { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ exit 1; } || : +# Disallow trailing blank lines. +sc_prohibit_trailing_blank_lines: + @$(VC_LIST_EXCEPT) | xargs perl -ln -0777 -e \ + '/\n\n+$$/ and print $$ARGV' > $@-t + @found=0; test -s $@-t && { found=1; cat $@-t 1>&2; \ + echo '$(ME): found trailing blank line(s)' 1>&2; }; \ + rm -f $@-t; \ + test $$found = 0 + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.6.4.rc0.127.g81400
On Thu, Jul 16, 2009 at 09:10:17AM +0200, Jim Meyering wrote:
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.
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.
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'
That command prints the name of each offending file with its trailing blank line count. While it takes well under a second on my system, (admittedly, with a hot cache), it's not well optimized, reading each file into memory and processing it.
Here are two change-sets to implement that. The first just removes all trailing blank line, so I've omitted its boring 100+ diffs.
Fine by me, let's see what others think, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard wrote:
On Thu, Jul 16, 2009 at 09:10:17AM +0200, Jim Meyering wrote: ...
Here are two change-sets to implement that. The first just removes all trailing blank line, so I've omitted its boring 100+ diffs.
Fine by me, let's see what others think,
Dan ACK'd it too, so I've pushed.
On Thu, Jul 16, 2009 at 03:08:58PM +0200, Jim Meyering wrote:
Daniel Veillard wrote:
On Thu, Jul 16, 2009 at 09:10:17AM +0200, Jim Meyering wrote: ...
Here are two change-sets to implement that. The first just removes all trailing blank line, so I've omitted its boring 100+ diffs.
Fine by me, let's see what others think,
Dan ACK'd it too, so I've pushed.
Thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Jul 16, 2009 at 09:10:17AM +0200, Jim Meyering wrote:
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.
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.
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'
That command prints the name of each offending file with its trailing blank line count. While it takes well under a second on my system, (admittedly, with a hot cache), it's not well optimized, reading each file into memory and processing it.
Here are two change-sets to implement that. The first just removes all trailing blank line, so I've omitted its boring 100+ diffs.
From 90046bad3bb688f8bfddfedcf3b681cf9b99028d Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 16 Jul 2009 08:25:36 +0200 Subject: [PATCH 1/2] remove all trailing blank lines
by running this command: git ls-files -z | xargs -0 perl -pi -0777 -e 's/\n\n+$/\n/' This is in preparation for a more strict make syntax-check rule that will detect trailing blank lines.
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Jul 15, 2009 at 11:51:41PM +0200, Jim Meyering wrote:
If it matters, we can come up with a more efficient (yet still portable) way to compare the last two bytes of each file to "\n\n". I went ahead and wrote a nearly-minimal script to do that. Rather than reading/processing all 27MB of sources, this reads just the last 2 bytes of each of the 1048 files, comparing those bytes to "\n\n" and printing the name when there's a match:
I just strace'd the 'tail' program and that does the right thing too # strace tail -c 2 somefile .... open("somefile", O_RDONLY|O_LARGEFILE) = 3 fstat64(3, {st_mode=S_IFREG|0664, st_size=10600, ...}) = 0 _llseek(3, 0, [0], SEEK_CUR) = 0 _llseek(3, 0, [10600], SEEK_END) = 0 _llseek(3, -2, [10598], SEEK_END) = 0 read(3, "l\n"..., 2) = 2
git ls-files -z \ | xargs -0 perl -le ' foreach my $f (@ARGV) { open F,"<",$f or (warn "failed to open $f: $!\n"), next; my $p = sysseek(F, -2, 2); # seek failure probably means file has < 2 bytes; ignore my $two; defined $p and $p = sysread F,$two,2; close F; # ignore read failure $p && $two eq "\n\n" and (print $f),$fail=1; } END {exit defined $fail ? 1 : 0}'
So using 'tail' instead of this perl script would be more readable and efficient. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Wed, Jul 15, 2009 at 11:51:41PM +0200, Jim Meyering wrote:
If it matters, we can come up with a more efficient (yet still portable) way to compare the last two bytes of each file to "\n\n". I went ahead and wrote a nearly-minimal script to do that. Rather than reading/processing all 27MB of sources, this reads just the last 2 bytes of each of the 1048 files, comparing those bytes to "\n\n" and printing the name when there's a match:
I just strace'd the 'tail' program and that does the right thing too
It had better ;-)
# strace tail -c 2 somefile .... open("somefile", O_RDONLY|O_LARGEFILE) = 3 fstat64(3, {st_mode=S_IFREG|0664, st_size=10600, ...}) = 0 _llseek(3, 0, [0], SEEK_CUR) = 0 _llseek(3, 0, [10600], SEEK_END) = 0 _llseek(3, -2, [10598], SEEK_END) = 0 read(3, "l\n"..., 2) = 2
git ls-files -z \ | xargs -0 perl -le ' foreach my $f (@ARGV) { open F,"<",$f or (warn "failed to open $f: $!\n"), next; my $p = sysseek(F, -2, 2); # seek failure probably means file has < 2 bytes; ignore my $two; defined $p and $p = sysread F,$two,2; close F; # ignore read failure $p && $two eq "\n\n" and (print $f),$fail=1; } END {exit defined $fail ? 1 : 0}'
So using 'tail' instead of this perl script would be more readable and efficient.
tail -c2 might be my choice, too, for a small number of files. But in our case, using it would incur the cost of 1000+ fork+execs.
On Thu, Jul 16, 2009 at 03:14:30PM +0200, Jim Meyering wrote:
So using 'tail' instead of this perl script would be more readable and efficient.
tail -c2 might be my choice, too, for a small number of files. But in our case, using it would incur the cost of 1000+ fork+execs.
I was thinking you'd just give it lots of files at once via xargs, though then you have the tedium of parsing its output Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering