On Sun Jul 15 09:25:30 2007, coke wrote:
> The recently added functionality to not regen MANIFEST.SKIP should be  
> used when determining whether or not to regenerate MANIFEST as well.
> 

Please review the attached.  Modifications to
tools/dev/mk_manifest_and_skip.pl and lib/Parrot/Manifest.pm.  Tso
additional test files in t/manifest/.

In reviewing the patch, I recommend that you first run
tools/dev/mk_manifest_and skip.pl before running t/manifest/*.t.  In
that way, you will be starting from a presumptively correct MANIFEST.

The interface to Parrot::Manifest has changed since last weekend. 
However, that will not matter assuming that all you ever do is call: 
perl tools/dev/mk_manifest_and_skip.pl.  *That* is unchanged.

Thank you very much.

kid51
Index: tools/dev/mk_manifest_and_skip.pl
===================================================================
--- tools/dev/mk_manifest_and_skip.pl   (revision 20000)
+++ tools/dev/mk_manifest_and_skip.pl   (working copy)
@@ -7,14 +7,20 @@
 
 my $script = $0;
 
-my $mani = Parrot::Manifest->new($script);
+my $mani = Parrot::Manifest->new( {
+    script          => $script,
+} );
 
 my $manifest_lines_ref = $mani->prepare_manifest();
-$mani->print_manifest($manifest_lines_ref);
+my $need_for_files = $mani->determine_need_for_manifest($manifest_lines_ref);
+$mani->print_manifest($manifest_lines_ref) if $need_for_files;
 
-my $ignore_ref = $mani->prepare_manifest_skip();
-$mani->print_manifest_skip($ignore_ref);
+my $print_str = $mani->prepare_manifest_skip();
+my $need_for_skip = $mani->determine_need_for_manifest_skip($print_str);
+$mani->print_manifest_skip($print_str) if $need_for_skip;
 
+#################### DOCUMENTATION ####################
+
 =head1 NAME
 
 tools/dev/mk_manifest_and_skip.pl - Recreate MANIFEST and MANIFEST.SKIP
Index: MANIFEST
===================================================================
--- MANIFEST    (revision 20000)
+++ MANIFEST    (working copy)
@@ -1,7 +1,7 @@
 # ex: set ro:
 # $Id$
 #
-# generated by tools/dev/mk_manifest_and_skip.pl Wed Jul 18 18:17:51 2007 UT
+# generated by tools/dev/mk_manifest_and_skip.pl Thu Jul 19 02:43:53 2007 UT
 #
 # See tools/dev/install_files.pl for documentation on the
 # format of this file.
@@ -2931,6 +2931,8 @@
 t/library/test_more.t                                       []
 t/library/yaml_parser_syck.t                                []
 t/manifest/01-basic.t                                       []
+t/manifest/02-regenerate_file.t                             []
+t/manifest/03-regenerate_skip.t                             []
 t/manifest/README                                           []
 t/native_pbc/header.t                                       []
 t/native_pbc/integer.t                                      []
Index: lib/Parrot/Manifest.pm
===================================================================
--- lib/Parrot/Manifest.pm      (revision 20000)
+++ lib/Parrot/Manifest.pm      (working copy)
@@ -3,26 +3,29 @@
 use strict;
 use warnings;
 use Carp;
+use Data::Dumper;
 
 sub new {
     my $class = shift;
-    my $script = shift;
+    my $argsref = shift;
 
     my $self = bless( {}, $class );
 
     my %data = (
-        id          => '$' . 'Id$',
-        time        => scalar gmtime,
-        cmd         => -d '.svn' ? 'svn' : 'svk',
-        script      => $script,
+        id      => '$' . 'Id$',
+        time    => scalar gmtime,
+        cmd     => -d '.svn' ? 'svn' : 'svk',
+        script  => $argsref->{script},
+        file    => $argsref->{file} ? $argsref->{file} : q{MANIFEST},
+        skip    => $argsref->{skip} ? $argsref->{skip} : q{MANIFEST.SKIP},
     );
 
-    my @status_output = qx($data{cmd} status -v);
+    my $status_output_ref = [ qx($data{cmd} status -v) ];
 
     # grab the versioned resources:
     my @versioned_files = ();
     my @dirs = ();
-    my @versioned_output = grep !/^[?D]/, @status_output;
+    my @versioned_output = grep !/^[?D]/, @{ $status_output_ref };
     for my $line (@versioned_output) {
         my @line_info = split( /\s+/, $line );
 
@@ -47,20 +50,36 @@
 
 sub prepare_manifest {
     my $self = shift;
-    my @manifest_lines;
+    my %manifest_lines;
 
     for my $file (@{ $self->{versioned_files} }) {
-        push @manifest_lines, _get_manifest_entry($file);
+        $manifest_lines{$file} = _get_manifest_entry($file);
     }
-    return [EMAIL PROTECTED];
+    return \%manifest_lines;
 }
 
+sub determine_need_for_manifest {
+    my $self = shift;
+    my $proposed_files_ref = shift;
+    if  ( ! -f $self->{file} ) {
+        return 1;
+    } else {
+        my $current_files_ref = $self->_get_current_files();
+        my $different_patterns_count = 0;
+        foreach my $cur (keys %{ $current_files_ref }) {
+            $different_patterns_count++ unless $proposed_files_ref->{$cur};
+        }
+        foreach my $pro (keys %{ $proposed_files_ref }) {
+            $different_patterns_count++ unless $current_files_ref->{$pro};
+        }
+        $different_patterns_count ? return 1 : return;
+    }
+}
+
 sub print_manifest {
     my $self = shift;
     my $manifest_lines_ref = shift;
-    open my $MANIFEST, '>', 'MANIFEST'
-        or croak "Unable to open MANIFEST for writing";
-    print $MANIFEST <<"END_HEADER";
+    my $print_str = <<"END_HEADER";
 # ex: set ro:
 # $self->{id}
 #
@@ -72,8 +91,14 @@
 # has been told about new or deleted files.
 END_HEADER
 
-    print $MANIFEST $_ for ( sort @{ $manifest_lines_ref } );
-    close $MANIFEST or croak "Unable to close MANIFEST after writing";
+    for my $k ( sort keys %{ $manifest_lines_ref } ) {
+        $print_str .= sprintf "%- 59s %s\n", ($k, $manifest_lines_ref->{$k});
+    } 
+    open my $MANIFEST, '>', $self->{file}
+        or croak "Unable to open $self->{file} for writing";
+    print $MANIFEST $print_str;
+    close $MANIFEST or croak "Unable to close $self->{file} after writing";
+    return 1;
 }
 
 sub _get_manifest_entry {
@@ -99,7 +124,7 @@
             : m[^(apps/\w+)/] ? "[$1]"
             :                   '[]';
     }
-    return sprintf( "%- 59s %s\n", $file, $loc );
+    return $loc;
 }
 
 sub _get_special {
@@ -147,6 +172,22 @@
     return \%special;
 }
 
+sub _get_current_files {
+    my $self = shift;
+    my %current_files = ();
+    open my $FILE, "<", $self->{file}
+        or die "Unable to open $self->{file} for reading";
+    while (my $line = <$FILE>) {
+        chomp $line;
+        next if $line =~ /^\s*$/o;
+        next if $line =~ /^#/o;
+        my @els = split /\s+/, $line;
+        $current_files{$els[0]}++;
+    }
+    close $FILE or die "Unable to close $self->{file} after reading";
+    return \%current_files;
+}
+
 sub prepare_manifest_skip {
     my $self = shift;
     my $svnignore = `$self->{cmd} propget svn:ignore @{ $self->{dirs} }`;
@@ -168,11 +209,41 @@
             $ignore{$1} = $2 if $2;
         }
     }
-    return \%ignore;
+    return $self->_compose_print_str( \%ignore );
 }
 
+sub determine_need_for_manifest_skip {
+    my $self = shift;
+    my $print_str = shift;
+    if  ( ! -f $self->{skip} ) {
+        return 1;
+    } else {
+        my $current_skips_ref = $self->_get_current_skips();
+        my $proposed_skips_ref = _get_proposed_skips($print_str);
+        my $different_patterns_count = 0;
+        foreach my $cur (keys %{ $current_skips_ref }) {
+            $different_patterns_count++ unless $proposed_skips_ref->{$cur};
+        }
+        foreach my $pro (keys %{ $proposed_skips_ref }) {
+            $different_patterns_count++ unless $current_skips_ref->{$pro};
+        }
+        $different_patterns_count ? return 1 : return;
+    }
+}
+
 sub print_manifest_skip {
     my $self = shift;
+    my $print_str = shift;
+    open my $MANIFEST_SKIP, '>', $self->{skip}
+        or die "Unable to open $self->{skip} for writing";
+    print $MANIFEST_SKIP $print_str;
+    close $MANIFEST_SKIP
+        or die "Unable to close $self->{skip} after writing";
+    return 1;
+}
+
+sub _compose_print_str {
+    my $self = shift;
     my $ignore_ref = shift;
     my %ignore = %{ $ignore_ref };
     my $print_str = <<"END_HEADER";
@@ -206,37 +277,21 @@
                 : "^$_\$\n^$_/\n";
         }
     }
-    my $current_skips_ref = _get_current_skips();
-    my $proposed_skips_ref = _get_proposed_skips($print_str);
-    my $different_patterns_count = 0;
-    foreach my $cur (keys %{ $current_skips_ref }) {
-        $different_patterns_count++ unless $proposed_skips_ref->{$cur};
-    }
-    foreach my $pro (keys %{ $proposed_skips_ref }) {
-        $different_patterns_count++ unless $current_skips_ref->{$pro};
-    }
-    if ( $different_patterns_count or (! -f 'MANIFEST.SKIP') ) {
-        open my $MANIFEST_SKIP, '>', 'MANIFEST.SKIP'
-            or die "Unable to open MANIFEST.SKIP for writing";
-        print $MANIFEST_SKIP $print_str;
-        close $MANIFEST_SKIP
-            or die "Unable to close MANIFEST.SKIP after writing";
-    }
-    return 1;
+    return $print_str;
 }
 
 sub _get_current_skips {
-    my $sk = q{MANIFEST.SKIP};
-    return {} unless -f $sk;
+    my $self = shift;
     my %current_skips = ();
-    open my $SKIP, "<", $sk or die "Unable to open $sk for reading";
+    open my $SKIP, "<", $self->{skip}
+        or die "Unable to open $self->{skip} for reading";
     while (my $line = <$SKIP>) {
         chomp $line;
         next if $line =~ /^\s*$/o;
         next if $line =~ /^#/o;
         $current_skips{$line}++;
     }
-    close $SKIP or die "Unable to close $sk after reading";
+    close $SKIP or die "Unable to close $self->{skip} after reading";
     return \%current_skips;
 }
 
@@ -267,10 +322,12 @@
     $mani = Parrot::Manifest->new($0);
 
     $manifest_lines_ref = $mani->prepare_manifest();
-    $mani->print_manifest($manifest_lines_ref);
+    $need_for_files = $mani->determine_need_for_manifest($manifest_lines_ref);
+    $mani->print_manifest($manifest_lines_ref) if $need_for_files;
 
-    $ignore_ref = $mani->prepare_manifest_skip();
-    $mani->print_manifest_skip($ignore_ref);
+    $print_str = $mani->prepare_manifest_skip();
+    $need_for_skip = $mani->determine_need_for_manifest_skip($print_str);
+    $mani->print_manifest_skip($print_str) if $need_for_skip;
 
 =head1 SEE ALSO
 
Index: t/manifest/01-basic.t
===================================================================
--- t/manifest/01-basic.t       (revision 20000)
+++ t/manifest/01-basic.t       (working copy)
@@ -6,7 +6,7 @@
 use strict;
 use warnings;
 
-use Test::More tests => 11;
+use Test::More tests => 13;
 use Carp;
 use Cwd;
 use File::Temp qw( tempdir );
@@ -14,43 +14,51 @@
 use_ok('Parrot::Manifest');
 
 my $script = $0;
-my $mani = Parrot::Manifest->new($script);
+my $file = q{MANIFEST};
+my $skip = q{MANIFEST.SKIP};
+
+my $mani = Parrot::Manifest->new( {
+    script          => $script,
+} );
 isa_ok($mani, 'Parrot::Manifest');
 
-ok(scalar(@{ $mani->{dirs} } ),
+ok(scalar( @{ $mani->{dirs} } ),
     "Parrot::Manifest constructor used 'status' command to find at least 1 
directory.");
-ok(scalar(@{ $mani->{versioned_files} } ),
+ok(scalar( @{ $mani->{versioned_files} } ),
     "Parrot::Manifest constructor used 'status' command to find at least 1 
versioned file.");
 
 my $manifest_lines_ref = $mani->prepare_manifest();
-is(ref($manifest_lines_ref), q{ARRAY},
-    "prepare_manifest() returned array ref");
+is(ref($manifest_lines_ref), q{HASH},
+    "prepare_manifest() returned hash ref");
+
 my $cwd = cwd();
 {
     my $tdir = tempdir( CLEANUP => 1 );
     chdir $tdir or
         croak "Unable to change to temporary directory for testing";
-    ok(! -f 'MANIFEST',
-        "No MANIFEST yet in this directory");
+    ok(! -f $file, "No $file yet in this directory");
     $mani->print_manifest($manifest_lines_ref);
-    ok(  -f 'MANIFEST',
-        "MANIFEST has been created in this directory");
+    ok(  -f $file, "$file has been created in this directory");
     chdir $cwd or
         croak "Unable to change back from temporary directory after testing";
 }
 
-my $ignore_ref = $mani->prepare_manifest_skip();
-is(ref($ignore_ref), q{HASH},
-    "prepare_manifest() returned hash ref");
+my $print_str = $mani->prepare_manifest_skip();
+ok($print_str, "prepare_manifest_skip() returned");
+
 {
     my $tdir = tempdir( CLEANUP => 1 );
     chdir $tdir or
         croak "Unable to change to temporary directory for testing";
-    ok(! -f 'MANIFEST.SKIP',
-        "No MANIFEST.SKIP yet in this directory");
-    $mani->print_manifest_skip($ignore_ref);
-    ok(  -f 'MANIFEST.SKIP',
-        "MANIFEST.SKIP has been created in this directory");
+    ok(! -f $skip,
+        "No $skip yet in tempdir");
+    my $need_for_skip = $mani->determine_need_for_manifest_skip($print_str);
+    ok($need_for_skip,
+        "No $skip in tempdir; it must be regenerated");
+    ok( $mani->print_manifest_skip($print_str),
+        "print_manifest_skip() returned true");
+    ok(  -f $skip,
+        "$skip has been created in tempdir");
     chdir $cwd or
         croak "Unable to change back from temporary directory after testing";
 }
Index: t/manifest/02-regenerate_file.t
===================================================================
--- t/manifest/02-regenerate_file.t     (revision 0)
+++ t/manifest/02-regenerate_file.t     (revision 0)
@@ -0,0 +1,116 @@
+#! perl
+# Copyright (C) 2007, The Perl Foundation.
+# $Id$
+# 02-regenerate_file.t
+
+use strict;
+use warnings;
+
+use Test::More tests => 10;
+use Carp;
+use Cwd;
+use File::Copy;
+use File::Temp qw( tempdir );
+use Tie::File;
+use lib ( qw| lib | );
+use_ok('Parrot::Manifest');
+
+my $script = $0;
+my $mani = Parrot::Manifest->new( {
+    script      => $script,
+} );
+isa_ok($mani, 'Parrot::Manifest');
+
+my $cwd = cwd();
+my $f = q{MANIFEST};
+
+my $manifest_lines_ref = $mani->prepare_manifest();
+ok($manifest_lines_ref, "prepare_manifest_skip() returned");
+
+# 1:  Copy the real MANIFEST unaltered to the tempdir.
+# Assuming the real MANIFEST was correct going in to this test, the 
+# absence of any change in it will mean that there will be no need to 
+# regenerate it.
+{
+    my $tdir = tempdir( CLEANUP => 1 );
+    chdir $tdir or
+        croak "Unable to change to temporary directory for testing";
+    copy(qq{$cwd/$f}, qq{$tdir/$f})
+        or croak "Unable to copy $f to tempdir";
+    ok(-f $f, "$f found in tempdir");
+    my $need_for_file =
+        $mani->determine_need_for_manifest($manifest_lines_ref);
+    ok(! $need_for_file, "No need to regenerate $f");
+    chdir $cwd or
+        croak "Unable to change back from temporary directory after testing";
+}
+
+# 2:  Copy the real MANIFEST to the tempdir but mangle it there.
+# The alteration in the copied MANIFEST will be sufficient to require
+# regeneration of MANIFEST.
+{
+    my $tdir = tempdir( CLEANUP => 1 );
+    chdir $tdir or
+        croak "Unable to change to temporary directory for testing";
+    copy(qq{$cwd/$f}, qq{$tdir/$f})
+        or croak "Unable to copy $f to tempdir";
+    ok(-f $f, "$f found in tempdir");
+    my @lines;
+    tie @lines, 'Tie::File', qq{$tdir/$f}
+        or croak "Unable to tie to $f in tempdir";
+    for (1..10) {
+        if ( defined($lines[-1]) ) {
+            pop @lines;
+        }
+    }
+    untie @lines or croak "Unable to untie from $f";
+    my $need_for_file =
+        $mani->determine_need_for_manifest($manifest_lines_ref);
+    ok($need_for_file, "Need to regenerate $f");
+    ok( $mani->print_manifest($manifest_lines_ref),
+        "print_manifest() returned true");
+    ok(  -f $f,
+        "$f has been created in tempdir");
+    chdir $cwd or
+        croak "Unable to change back from temporary directory after testing";
+}
+
+pass("Completed all tests in $0");
+
+
+################### DOCUMENTATION ###################
+
+=head1 NAME
+
+02-regenerate_file.t - test C<Parrot::Manifest> MANIFEST-related methods
+
+=head1 SYNOPSIS
+
+    % prove t/manifest/02-regenerate_file.t
+
+=head1 DESCRIPTION
+
+The files in this directory test the publicly callable methods of
+F<lib/Parrot/Manifest.pm> and packages which inherit from that package.
+
+F<02-regenerate_file.t> tests whether Parrot::Manifest correctly determines
+whether MANIFESTneeds to be regenerated or not.
+
+=head1 AUTHOR
+
+James E Keenan ([EMAIL PROTECTED])
+
+=head1 SEE ALSO
+
+Parrot::Manifest, Parrot::Manifest::Files, Parrot::Manifest::Skip,
+F<tools/dev/mk_manifest_and_skip.pl>.
+
+=cut
+
+# Local Variables:
+#   mode: cperl
+#   cperl-indent-level: 4
+#   fill-column: 100
+# End:
+# vim: expandtab shiftwidth=4:
+

Property changes on: t/manifest/02-regenerate_file.t
___________________________________________________________________
Name: svn:mime-type
   + text/plain
Name: svn:keywords
   + Author Date Id Revision
Name: svn:eol-style
   + native

Index: t/manifest/03-regenerate_skip.t
===================================================================
--- t/manifest/03-regenerate_skip.t     (revision 0)
+++ t/manifest/03-regenerate_skip.t     (revision 0)
@@ -0,0 +1,113 @@
+#! perl
+# Copyright (C) 2007, The Perl Foundation.
+# $Id$
+# 03-regenerate_skip.t
+
+use strict;
+use warnings;
+
+use Test::More tests => 10;
+use Carp;
+use Cwd;
+use File::Copy;
+use File::Temp qw( tempdir );
+use Tie::File;
+use lib ( qw| lib | );
+use_ok('Parrot::Manifest');
+
+my $script = $0;
+my $mani = Parrot::Manifest->new( {
+    script      => $script,
+} );
+isa_ok($mani, 'Parrot::Manifest');
+
+my $cwd = cwd();
+my $sk = q{MANIFEST.SKIP};
+my $print_str = $mani->prepare_manifest_skip();
+ok($print_str, "prepare_manifest_skip() returned");
+
+# 1:  Copy the real MANIFEST.SKIP unaltered to the tempdir.
+# Assuming the real MANIFEST.SKIP was correct going in to this test, the 
+# absence of any change in it will mean that there will be no need to 
+# regenerate it.
+{
+    my $tdir = tempdir( CLEANUP => 1 );
+    chdir $tdir or
+        croak "Unable to change to temporary directory for testing";
+    copy(qq{$cwd/$sk}, qq{$tdir/$sk})
+        or croak "Unable to copy $sk to tempdir";
+    ok(-f $sk, "$sk found in tempdir");
+    my $need_for_skip = $mani->determine_need_for_manifest_skip($print_str);
+    ok(! $need_for_skip, "No need to regenerate $sk");
+    chdir $cwd or
+        croak "Unable to change back from temporary directory after testing";
+}
+
+# 2:  Copy the real MANIFEST.SKIP to the tempdir but mangle it there.
+# The alteration in the copied MANIFEST.SKIP will be sufficient to require
+# regeneration of MANIFEST.SKIP.
+{
+    my $tdir = tempdir( CLEANUP => 1 );
+    chdir $tdir or
+        croak "Unable to change to temporary directory for testing";
+    copy(qq{$cwd/$sk}, qq{$tdir/$sk})
+        or croak "Unable to copy $sk to tempdir";
+    ok(-f $sk, "$sk found in tempdir");
+    my @lines;
+    tie @lines, 'Tie::File', qq{$tdir/$sk}
+        or croak "Unable to tie to $sk in tempdir";
+    for (1..10) {
+        if ( defined($lines[-1]) ) {
+            pop @lines;
+        }
+    }
+    untie @lines or croak "Unable to untie from $sk";
+    my $need_for_skip = $mani->determine_need_for_manifest_skip($print_str);
+    ok($need_for_skip, "Need to regenerate $sk");
+    ok( $mani->print_manifest_skip($print_str),
+        "print_manifest_skip() returned true");
+    ok(  -f $sk,
+        "$sk has been created in tempdir");
+    chdir $cwd or
+        croak "Unable to change back from temporary directory after testing";
+}
+
+pass("Completed all tests in $0");
+
+
+################### DOCUMENTATION ###################
+
+=head1 NAME
+
+03-regenerate_skip.t - test C<Parrot::Manifest> MANIFEST.SKIP-related methods
+
+=head1 SYNOPSIS
+
+    % prove t/manifest/03-regenerate_skip.t
+
+=head1 DESCRIPTION
+
+The files in this directory test the publicly callable methods of
+F<lib/Parrot/Manifest.pm> and packages which inherit from that package.
+
+F<03-regenerate_skip.t> tests whether Parrot::Manifest correctly determines
+whether MANIFEST.SKIP needs to be regenerated or not.
+
+=head1 AUTHOR
+
+James E Keenan ([EMAIL PROTECTED])
+
+=head1 SEE ALSO
+
+Parrot::Manifest, Parrot::Manifest::Files, Parrot::Manifest::Skip,
+F<tools/dev/mk_manifest_and_skip.pl>.
+
+=cut
+
+# Local Variables:
+#   mode: cperl
+#   cperl-indent-level: 4
+#   fill-column: 100
+# End:
+# vim: expandtab shiftwidth=4:
+

Property changes on: t/manifest/03-regenerate_skip.t
___________________________________________________________________
Name: svn:mime-type
   + text/plain
Name: svn:keywords
   + Author Date Id Revision
Name: svn:eol-style
   + native

Reply via email to