[xen-tools-dev] [PATCH v2 00/13] Next bunch of patches
Axel Beckert
abe at deuxchevaux.org
Fri Jul 23 14:58:42 CEST 2010
Hi,
On Sun, Jul 18, 2010 at 10:04:10PM +0200, Stéphane Jourdois wrote:
> Second version of my bunch of patch.
Applied by rebasing.
Some comments though:
On Sun, Jul 18, 2010 at 10:04:22PM +0200, Stéphane Jourdois wrote:
> diff --git a/.mailmap b/.mailmap
> new file mode 100644
> index 0000000..4562e32
> --- /dev/null
> +++ b/.mailmap
> @@ -0,0 +1,6 @@
> +Axel Beckert <abe at deuxchevaux.org>
> +Dmitry Nedospasov <dmitry at nedos.net>
> +Nathan O'Sullivan <nathan at mammoth.com.au>
> +Radu Spineanu <radu at debian.org> radu <none at none>
> +Steve Kemp <steve at steve.org.uk> steve <none at none>
> +Stéphane Jourdois <sjourdois at gmail.com> <stephane at jourdois.fr>
Interesting feature. Didn't know that yet. Unfortunately github
doesn't use it for http://github.com/xtaran/xen-tools/graphs/impact
On Sun, Jul 18, 2010 at 10:04:18PM +0200, Stéphane Jourdois wrote:
> diff --git a/t/programs.t b/t/programs.t
> index ae7126e..9eb1180 100755
> --- a/t/programs.t
> +++ b/t/programs.t
> @@ -12,12 +12,13 @@ use Test::More qw( no_plan );
> #
> # Files that we want to use.
> #
> -my @required = qw( /usr/sbin/debootstrap /bin/ls /bin/dd /bin/mount /bin/cp /bin/tar );
> +my @required = qw( /bin/ls /bin/dd /bin/mount /bin/cp /bin/tar );
>
> #
> # Files that we might wish to use.
> #
> -my @optional = qw( /usr/bin/rpmstrap /usr/sbin/xm /sbin/mkfs.ext3 /sbin/mkfs.xfs/sbin/mkfs.reiserfs );
> +my @optional = qw( /usr/sbin/debootstrap /usr/bin/rpmstrap /usr/sbin/xm
> + /sbin/mkfs.ext3 /sbin/mkfs.xfs/sbin/mkfs.reiserfs );
Thanks for spotting that!
On Sun, Jul 18, 2010 at 10:04:17PM +0200, Stéphane Jourdois wrote:
> Read and test only the shebang, not the whole file, +typos.
Basically a good idea.
> diff --git a/t/shell-syntax.t b/t/shell-syntax.t
> index a53a228..836eb45 100755
> --- a/t/shell-syntax.t
> +++ b/t/shell-syntax.t
> @@ -34,26 +34,28 @@ sub checkFile
[...]
> # See if it is a shell script.
> my $isShell = 0;
>
> - # Read the file.
> + # Read the shebang
> open( INPUT, "<", $file );
> - foreach my $line ( <INPUT> )
> + my $line = <INPUT>;
> + close( INPUT );
> +
> + # Check if it is really a shell file
> + if ( $line =~ /^#! ?\/bin\/(ba)?sh/ )
> {
> - if ( ( $line =~ /\/bin\/sh/ ) ||
> - ( $line =~ /\/bin\/bash/ ) )
> - {
> - $isShell = 1;
> - }
> + $isShell = 1;
> }
> - close( INPUT );
I changed that to only test for /bin/sh -- I don't want to see an
/bin/bash shebang line without reason in the xen-tools code. :-)
And there was nearly none, except in some code to generate some
policy.d scripts. A grep over all files found that, not this test.
I'm thinking about how a test would have to look like to catch code
which generates shell scripts.
On Sun, Jul 18, 2010 at 10:04:16PM +0200, Stéphane Jourdois wrote:
> diff --git a/t/no-tabs.t b/t/no-tabs.t
> index bcc881d..2037fbc 100755
> --- a/t/no-tabs.t
> +++ b/t/no-tabs.t
> @@ -38,6 +38,9 @@ sub checkFile
> # Nor about Makefiles
> return if ( $file =~ /\/Makefile$/ );
>
> + # Nor about temporary files
^^^^^^^^^
> + return if ( $file =~ m{/\.[^/]+$} );
> +
> # Nor about files which start with ./debian/
> return if ( $file =~ /^\.\/debian\// );
>
> diff --git a/t/perl-syntax.t b/t/perl-syntax.t
> index cef0284..0cd9406 100755
> --- a/t/perl-syntax.t
> +++ b/t/perl-syntax.t
> @@ -41,6 +41,12 @@ sub checkFile
> # Nor about Makefiles
> return if ( $file =~ /\/Makefile$/ );
>
> + # Nor about git files
> + return if ( $file =~ /^\.\/\.git\// );
> +
> + # Nor about temporary files
^^^^^^^^^
> + return if ( $file =~ m{/\.[^/]+$} );
> +
> # `modules.sh` is a false positive.
> return if ( $file =~ /modules.sh$/ );
I'd not call dot files "temporary" files. Pushed a fix just now.
On Sun, Jul 18, 2010 at 10:04:15PM +0200, Stéphane Jourdois wrote:
> Pod::Coverages wants to find perl modules in @INC.
On Sun, Jul 18, 2010 at 10:04:20PM +0200, Stéphane Jourdois wrote:
> diff --git a/TODO b/TODO
> index 10e7d0a..b20199d 100644
> --- a/TODO
> +++ b/TODO
> @@ -18,13 +18,6 @@ Minor bugs to fix and features to add before a 4.2 release
> stored in the configuration hash. The only issue is that trailing
> whitespace is missing from the "make_fs_foo" option.
>
> -* Test suite should pass
> -
> - Currently failing:
> -
> - t/pod-coverage.t (fails on Xen::Tools and Xen::Tools::Log despite
> - they do have POD. Strange.)
> -
> * Setup locales in the hooks?
>
> Currently no locales are set and this causes several domU errors
Yay! Now all tests pass for me, too! Thanks a lot! :-)
On Sun, Jul 18, 2010 at 10:04:13PM +0200, Stéphane Jourdois wrote:
> This file is listed in .gitignore, suggesting that it is not necessary
> to track it.
>
> Do not accept this patch if this is not the case !
> ---
> .hgtags | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
> delete mode 100644 .hgtags
I thought quite a while lot about this. Not sure why I put it in
.gitignore while not removing it from the repo. Probably to remove it
in the future and then already having it in .gitignore.
As far as I can see nobody here uses hg to access our git
repositories, so I guess, that future is now. ;-)
On Sun, Jul 18, 2010 at 10:04:12PM +0200, Stéphane Jourdois wrote:
> Do not accept this patch if this file is still used !
> ---
> .release | 34 ----------------------------------
> 1 files changed, 0 insertions(+), 34 deletions(-)
> delete mode 100644 .release
Never noticed that file before. It's not used anywhere in the checked
in code (I suspected the Makefile) and the comment points to a tool
from Steve, so I applied that patch, too.
Kind regards, Axel
--
/~\ Plain Text Ribbon Campaign | Axel Beckert
\ / Say No to HTML in E-Mail and News | abe at deuxchevaux.org (Mail)
X See http://www.asciiribbon.org/ | abe at noone.org (Mail+Jabber)
/ \ I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)
More information about the xen-tools-dev
mailing list