Bugzilla – Attachment 49318 Details for
Bug 16008
noisy "fatal" warning when new file is moved in another patch
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[COUNTER-PATCH] Bug 16008: Deal with removed files
0001-Bug-16008-Deal-with-removed-files.patch (text/plain), 19.36 KB, created by
Jonathan Druart
on 2016-03-18 14:31:31 UTC
(
hide
)
Description:
[COUNTER-PATCH] Bug 16008: Deal with removed files
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2016-03-18 14:31:31 UTC
Size:
19.36 KB
patch
obsolete
>From a262c6856477bd129101537a4b6224cffb7ccdb9 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Fri, 18 Mar 2016 12:10:07 +0000 >Subject: [PATCH 1/1] Bug 16008: Deal with removed files > >This patch fixes 2 issues: >1/ When a patch just removes files, the $git->log method did not >return the right list of files. The commit which only removed files were >not counted. >2/ If a file is added then deleted in the same patch set, the following >error occurred: >fatal: ambiguous argument 't/db_dependent/RecordProcessor_ViewPolicy.t': >unknown revision or path not in the working tree. >Now it's correctly handled: a new 'git manipulation' check has been >added. > >Note that lot of changes has been made to achieve this goal: >1/ QohA::Files->filter has been removed: it was not needed the >assignement was done in the BUILDER >2/ QohA::Git->log now returns all changes (even for deleted files) >3/ QohA::Report->type has been deleted, it was not used >4/ The report member initialization has been moved to the parent class >(QohA::File). > >Test plan: >% echo "#simple file" > new_file >% echo "#comment" >> mainpage.pl >% git add new_file mainpage.pl; git commit -m'1' >% git rm new_file >% git commit -m'2' >% qa -c 2 -v 2 # should return a warning for new_file >--- > QohA/File.pm | 40 +++++++++++++++++++ > QohA/File/Perl.pm | 11 +----- > QohA/File/Specific/Kohastructure.pm | 12 ++---- > QohA/File/Specific/OpacCss.pm | 12 ++---- > QohA/File/Specific/Sysprefs.pm | 12 ++---- > QohA/File/Template.pm | 10 +---- > QohA/File/XML.pm | 12 ++---- > QohA/File/YAML.pm | 12 ++---- > QohA/Files.pm | 78 +++++++++++-------------------------- > QohA/Git.pm | 14 +++---- > QohA/Report.pm | 3 -- > TODO | 5 --- > koha-qa.pl | 13 ++----- > t/Perl.t | 30 ++++++++++---- > 14 files changed, 112 insertions(+), 152 deletions(-) > >diff --git a/QohA/File.pm b/QohA/File.pm >index 0b828b9..ca7a491 100644 >--- a/QohA/File.pm >+++ b/QohA/File.pm >@@ -5,6 +5,7 @@ use Moo; > use File::Basename; > use Cwd 'abs_path'; > use IPC::Cmd qw( run ); >+use QohA::Report; > > has 'path' => ( > is => 'ro', >@@ -28,6 +29,15 @@ has 'pass' => ( > is => 'rw', > default => sub{0}, > ); >+has 'report' => ( >+ is => 'rw', >+ default => sub { >+ QohA::Report->new(); >+ }, >+); >+has 'git_statuses' => ( >+ is => 'rw', >+); > > sub _build_filename { > my ($self) = @_; >@@ -40,9 +50,23 @@ sub _build_abspath { > return $abs_path; > } > >+sub run_checks { >+ my ($self, $cnt) = @_; >+ >+ my $r; >+ >+ # Check git manipulations >+ $r = $self->check_git_manipulation(); >+ $self->add_to_report('git manipulation', $r); >+ >+ $self->pass($self->pass + 1); >+} >+ > sub check_forbidden_patterns { > my ($self, $cnt, $patterns) = @_; > >+ return 0 unless -e $self->path; >+ > # For the first pass, I don't want to launch any test. > return 1 if $self->pass == 0; > >@@ -115,6 +139,22 @@ sub check_spelling { > : 1; > } > >+sub check_git_manipulation { >+ my ($self) = @_; >+ >+ return 1 if $self->pass == 0; >+ >+ my @errors; >+ >+ if ( $self->{git_statuses} =~ m|A| and $self->{git_statuses} =~ m|D| ) { >+ push @errors, "The file has been added and deleted in the same patchset"; >+ } >+ >+ return @errors >+ ? \@errors >+ : 1; >+} >+ > sub add_to_report { > my ($self, $name, $error) = @_; > $self->report->add( >diff --git a/QohA/File/Perl.pm b/QohA/File/Perl.pm >index b25035a..fedc1c3 100644 >--- a/QohA/File/Perl.pm >+++ b/QohA/File/Perl.pm >@@ -13,19 +13,11 @@ use IPC::Cmd qw[can_run run]; > use Cwd qw( abs_path ); > > use QohA::Git; >-use QohA::Report; > > > our $pc_rc = dirname( abs_path( $INC{'QohA/File/Perl.pm'} ) ) . '/../../perlcriticrc'; > die "Koha's $pc_rc file is missing..." unless ( -e $pc_rc ); > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'perl'} ); >- }, >-); >- > sub run_checks { > my ($self, $cnt) = @_; > >@@ -51,8 +43,7 @@ sub run_checks { > $r = $self->check_spelling(); > $self->SUPER::add_to_report('spelling', $r); > >- >- $self->pass($self->pass + 1); >+ return $self->SUPER::run_checks($cnt); > } > > sub check_critic { >diff --git a/QohA/File/Specific/Kohastructure.pm b/QohA/File/Specific/Kohastructure.pm >index 13c116f..a679077 100644 >--- a/QohA/File/Specific/Kohastructure.pm >+++ b/QohA/File/Specific/Kohastructure.pm >@@ -2,20 +2,14 @@ package QohA::File::Specific::Kohastructure; > > use Modern::Perl; > use Moo; >-use QohA::Report; > extends 'QohA::File'; > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'specific'} ); >- }, >-); >- > sub run_checks { >- my ($self) = @_; >+ my ($self, $cnt) = @_; > my @r = $self->check_charset_collate(); > $self->SUPER::add_to_report('charset_collate', \@r); >+ >+ return $self->SUPER::run_checks($cnt); > } > > sub check_charset_collate { >diff --git a/QohA/File/Specific/OpacCss.pm b/QohA/File/Specific/OpacCss.pm >index 5a31892..ff0234b 100644 >--- a/QohA/File/Specific/OpacCss.pm >+++ b/QohA/File/Specific/OpacCss.pm >@@ -5,20 +5,14 @@ use Moo; > use IPC::Cmd qw[run]; > use File::Basename; > use File::Temp qw[tempfile]; >-use QohA::Report; > extends 'QohA::File'; > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'specific'} ); >- }, >-); >- > sub run_checks { >- my ($self) = @_; >+ my ($self, $cnt) = @_; > my @r = $self->check_css_less_sync(); > $self->SUPER::add_to_report('css_and_less_in_sync', \@r); >+ >+ return $self->SUPER::run_checks($cnt); > } > > sub check_css_less_sync { >diff --git a/QohA/File/Specific/Sysprefs.pm b/QohA/File/Specific/Sysprefs.pm >index 94ad773..72cb54a 100644 >--- a/QohA/File/Specific/Sysprefs.pm >+++ b/QohA/File/Specific/Sysprefs.pm >@@ -2,20 +2,14 @@ package QohA::File::Specific::Sysprefs; > > use Modern::Perl; > use Moo; >-use QohA::Report; > extends 'QohA::File'; > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'specific'} ); >- }, >-); >- > sub run_checks { >- my ($self) = @_; >+ my ($self, $cnt) = @_; > my @r = $self->check_sysprefs_order(); > $self->SUPER::add_to_report('sysprefs_order', \@r); >+ >+ return $self->SUPER::run_checks($cnt); > } > > sub check_sysprefs_order { >diff --git a/QohA/File/Template.pm b/QohA/File/Template.pm >index 15ccf91..480da82 100644 >--- a/QohA/File/Template.pm >+++ b/QohA/File/Template.pm >@@ -8,17 +8,9 @@ extends 'QohA::File'; > use IPC::Cmd qw[can_run run]; > use File::Spec; > >-use QohA::Report; > use C4::TTParser; > use Template; > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'template'} ); >- }, >-); >- > sub run_checks { > my ($self, $cnt) = @_; > my $r; >@@ -38,7 +30,7 @@ sub run_checks { > $r = $self->check_spelling(); > $self->SUPER::add_to_report('spelling', $r); > >- $self->pass($self->pass + 1); >+ return $self->SUPER::run_checks($cnt); > } > > sub check_tt_valid { >diff --git a/QohA/File/XML.pm b/QohA/File/XML.pm >index 4f1dbab..da1ff1a 100644 >--- a/QohA/File/XML.pm >+++ b/QohA/File/XML.pm >@@ -4,20 +4,14 @@ use Modern::Perl; > use Moo; > use XML::LibXML; > >-use QohA::Report; > extends 'QohA::File'; > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'xml'} ); >- }, >-); >- > sub run_checks { >- my ($self) = @_; >+ my ($self, $cnt) = @_; > my @r = $self->check_parse_xml(); > $self->SUPER::add_to_report('xml_valid', \@r); >+ >+ return $self->SUPER::run_checks($cnt); > } > > sub check_parse_xml { >diff --git a/QohA/File/YAML.pm b/QohA/File/YAML.pm >index 41eca2f..91acda2 100644 >--- a/QohA/File/YAML.pm >+++ b/QohA/File/YAML.pm >@@ -4,20 +4,14 @@ use Modern::Perl; > use Moo; > use YAML; > >-use QohA::Report; > extends 'QohA::File'; > >-has 'report' => ( >- is => 'rw', >- default => sub { >- QohA::Report->new( {type => 'yaml'} ); >- }, >-); >- > sub run_checks { >- my ($self) = @_; >+ my ($self, $cnt) = @_; > my @r = $self->check_parse_yaml(); > $self->SUPER::add_to_report('yaml_valid', \@r); >+ >+ return $self->SUPER::run_checks($cnt); > } > > sub check_parse_yaml { >diff --git a/QohA/Files.pm b/QohA/Files.pm >index 71d6839..6da6573 100644 >--- a/QohA/Files.pm >+++ b/QohA/Files.pm >@@ -4,6 +4,7 @@ use Moo; > use Modern::Perl; > use List::MoreUtils qw(uniq); > >+use QohA::File; > use QohA::File::XML; > use QohA::File::Perl; > use QohA::File::Template; >@@ -19,64 +20,31 @@ has 'files' => ( > sub BUILD { > my ( $self, $param ) = @_; > my @files = @{$param->{files}}; >- @files = uniq @files; > $self->files([]); >- for my $file ( @files ) { >- push @{ $self->files }, QohA::File::XML->new(path => $file) >- if $file =~ qr/\.xml$|\.xsl$|\.xslt$/i; >- push @{ $self->files }, QohA::File::Perl->new(path => $file) >- if $file =~ qr/\.pl$|\.pm$|\.t$/i; >- push @{ $self->files }, QohA::File::Template->new(path => $file) >- if $file =~ qr/\.tt$|\.inc$/i; >- push @{ $self->files }, QohA::File::YAML->new(path => $file) >- if $file =~ qr/\.yml$|\.yaml$/i; >- push @{ $self->files }, QohA::File::Specific::Sysprefs->new(path => $file) >- if $file =~ qr/sysprefs\.sql$/; >- push @{ $self->files }, QohA::File::Specific::Kohastructure->new(path => $file) >- if $file =~ qr/kohastructure\.sql$/; >- push @{ $self->files }, QohA::File::Specific::OpacCss->new(path => $file) >- if $file =~ qr/opac\.css$|opac\.less$/; >- } >-} >- >-sub filter { >- my ($self, $params) = @_; >- my $file_types = $params->{extension} // []; >- my $file_names = $params->{name} // []; >- >- die "QohA::Files::filter > Bad call: extension and name params cannot be filled together" >- if scalar( @$file_types ) and scalar( @$file_names ); >- my @wanted_files; >- >- for my $type ( @$file_types ) { >- for my $f ( @{$self->files} ) { >- if ( $type =~ /perl/ ) { >- push @wanted_files, $f >- if ref $f eq 'QohA::File::Perl'; >- } >- elsif ( $type =~ /xml/ ) { >- push @wanted_files, $f >- if ref $f eq 'QohA::File::XML'; >- } >- elsif ( $type =~ /tt/ ) { >- push @wanted_files, $f >- if ref $f eq 'QohA::File::Template'; >- } >- elsif ( $type =~ /yaml/ ) { >- push @wanted_files, $f >- if ref $f eq 'QohA::File::YAML'; >- } >+ for my $f ( @files ) { >+ my $filepath = $f->{filepath}; >+ my $file; >+ if ( $filepath =~ qr/\.xml$|\.xsl$|\.xslt$/i ) { >+ $file = QohA::File::XML->new(path => $filepath); >+ } elsif ( $filepath =~ qr/\.pl$|\.pm$|\.t$/i ) { >+ $file = QohA::File::Perl->new(path => $filepath); >+ } elsif ( $filepath =~ qr/\.tt$|\.inc$/i ) { >+ $file = QohA::File::Template->new(path => $filepath); >+ } elsif ( $filepath =~ qr/\.yml$|\.yaml$/i ) { >+ $file = QohA::File::YAML->new(path => $filepath); >+ } elsif ( $filepath =~ qr/sysprefs\.sql$/ ) { >+ $file = QohA::File::Specific::Sysprefs->new(path => $filepath); >+ } elsif ( $filepath =~ qr/kohastructure\.sql$/ ) { >+ $file = QohA::File::Specific::Kohastructure->new(path => $filepath); >+ } elsif ( $filepath =~ qr/opac\.css$|opac\.less$/ ) { >+ $file = QohA::File::Specific::OpacCss->new(path => $filepath); >+ } elsif ( $f->{statuses} =~ m|A| and $f->{statuses} =~ m|D| ) { >+ $file = QohA::File->new(path => $filepath); > } >+ next unless $file; >+ $file->git_statuses( $f->{statuses} ); >+ push @{ $self->files}, $file; > } >- >- if ( @$file_names ) { >- for my $f ( @{$self->files} ) { >- push @wanted_files, $f >- if grep { $_ eq $f->filename } @$file_names; >- } >- } >- >- return @wanted_files; > } > > 1; >diff --git a/QohA/Git.pm b/QohA/Git.pm >index dc578e1..2890875 100644 >--- a/QohA/Git.pm >+++ b/QohA/Git.pm >@@ -11,24 +11,22 @@ has 'branchname' => ( > ); > > # this sub returns all modified files, for testing on >-# and ignores deleted or moved files > sub log { > my ($self, $cnt) = @_; > >- #skip deleted files.. >- my @r = qx{git log --oneline --numstat --diff-filter='ACMRTUXB' -$cnt}; >- my @r1; >+ my @r = qx{git log --oneline --name-status -$cnt}; >+ my $files = {}; > >- # oops, lets strip out deleted or moved files, from selection > foreach my $rr ( @r ) { > chomp $rr; > my @cols = split '\t', $rr ; > > # ignore lines that are commit shas, not filename >- next if not defined $cols[2]; >- push @r1, $cols[2]; >+ next if not defined $cols[1]; >+ $files->{$cols[1]} .= $cols[0]; > } >- return \@r1 ; >+ >+ return [ map { { filepath => $_, statuses => $files->{$_} } } keys %$files ]; > } > > sub diff_log { >diff --git a/QohA/Report.pm b/QohA/Report.pm >index f2e81ff..3b88eb9 100644 >--- a/QohA/Report.pm >+++ b/QohA/Report.pm >@@ -4,9 +4,6 @@ use Modern::Perl; > use Moo; > use List::Compare; > >-has 'type' => ( >- is => 'ro', >-); > has 'file' => ( > is => 'rw', > ); >diff --git a/TODO b/TODO >index 4e3bf28..e69de29 100644 >--- a/TODO >+++ b/TODO >@@ -1,5 +0,0 @@ >-Bugs: >-- When a patch just removes files, the $git->log method does not return the >-right list of files. We don't want to retrieve deleted files so we don't want >-to add the 'D' filter. Maybe should we add a new test for testing this special >-case. (i.e. bug 10459) >diff --git a/koha-qa.pl b/koha-qa.pl >index 2a3dac9..a7451ac 100755 >--- a/koha-qa.pl >+++ b/koha-qa.pl >@@ -60,21 +60,14 @@ eval { > > print QohA::Git::log_as_string($num_of_commits); > >- my $modified_files = QohA::Files->new( { files => $git->log($num_of_commits) } ); >+ my $log_files = $git->log($num_of_commits); >+ my $modified_files = QohA::Files->new( { files => $log_files } ); > > $git->delete_branch( 'qa-prev-commit' ); > $git->create_and_change_branch( 'qa-prev-commit' ); > $git->reset_hard_prev( $num_of_commits ); > >- my @files = ( >- $modified_files->filter( { extension => [ qw< perl tt xml yaml > ] } ), >- $modified_files->filter( { name => [ qw< sysprefs.sql > ] } ), >- $modified_files->filter( { name => [ qw< kohastructure.sql > ] } ), >- $modified_files->filter( { name => [ qw< opac.css > ] } ), >- $modified_files->filter( { name => [ qw< opac.less > ] } ), >- ); >- >- >+ my @files = @{ $modified_files->files }; > my $i = 1; > say "Processing files before patches"; > for my $f ( @files ) { >diff --git a/t/Perl.t b/t/Perl.t >index 994c4f6..ee41293 100644 >--- a/t/Perl.t >+++ b/t/Perl.t >@@ -38,18 +38,14 @@ eval { > > $CWD = $git_repo; > my $qoha_git = QohA::Git->new(); >- my $modified_files = QohA::Files->new( { files => $qoha_git->log($num_of_commits) } ); >- >+ my $log_files = $qoha_git->log($num_of_commits); >+ my $modified_files = QohA::Files->new( { files => $log_files } ); > > $qoha_git->delete_branch( 'qa-prev-commit_t' ); > $qoha_git->create_and_change_branch( 'qa-prev-commit_t' ); > $qoha_git->reset_hard_prev( $num_of_commits ); > >- my @files = ( >- $modified_files->filter( { extension => [ qw< perl tt xml yaml > ] } ), >- $modified_files->filter( { name => [ qw< sysprefs.sql > ] } ), >- $modified_files->filter( { name => [ qw< kohastructure.sql > ] } ), >- ); >+ my @files = @{ $modified_files->files }; > for my $f ( @files ) { > $f->run_checks(); > } >@@ -114,11 +110,13 @@ eval { > EOL > my $r_v1_expected = <<EOL; > $STATUS_KO i_fail_yaml.yaml >+ $STATUS_OK git manipulation > $STATUS_KO yaml_valid > > $STATUS_KO perl/Authority.pm > $STATUS_OK critic > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -126,6 +124,7 @@ EOL > $STATUS_OK perl/i_dont_fail_spelling.pl > $STATUS_OK critic > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -133,6 +132,7 @@ EOL > $STATUS_KO perl/i_fail_compil.pl > $STATUS_OK critic > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_KO valid >@@ -140,6 +140,7 @@ EOL > $STATUS_KO perl/i_fail_critic.pl > $STATUS_KO critic > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -147,6 +148,7 @@ EOL > $STATUS_KO perl/i_fail_license.pl > $STATUS_OK critic > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -154,6 +156,7 @@ EOL > $STATUS_KO perl/i_fail_patterns.pl > $STATUS_OK critic > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_KO valid >@@ -161,6 +164,7 @@ EOL > $STATUS_KO perl/i_fail_spelling.pl > $STATUS_OK critic > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_KO spelling > $STATUS_OK valid >@@ -168,6 +172,7 @@ EOL > $STATUS_KO perl/i_fail_template_name.pl > $STATUS_OK critic > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -175,6 +180,7 @@ EOL > $STATUS_KO perl/i_fail_unused_declared_variables.pl > $STATUS_KO critic > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -182,6 +188,7 @@ EOL > $STATUS_OK perl/i_m_ok.pl > $STATUS_OK critic > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid >@@ -189,48 +196,57 @@ EOL > $STATUS_KO perl/updatedatabase.pl > $STATUS_OK critic > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK pod > $STATUS_OK spelling > $STATUS_OK valid > > $STATUS_KO sql/kohastructure.sql > $STATUS_KO charset_collate >+ $STATUS_OK git manipulation > > $STATUS_KO sql/sysprefs.sql >+ $STATUS_OK git manipulation > $STATUS_KO sysprefs_order > > $STATUS_KO tmpl/i_fail_patterns.tt > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK spelling > $STATUS_OK tt_valid > $STATUS_OK valid_template > > $STATUS_KO tmpl/i_fail_patterns_theme.tt > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK spelling > $STATUS_OK tt_valid > $STATUS_OK valid_template > > $STATUS_KO tmpl/i_fail_patterns_tt_plugins.tt > $STATUS_KO forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK spelling > $STATUS_OK tt_valid > $STATUS_KO valid_template > > $STATUS_KO tmpl/i_fail_tt_valid.tt > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK spelling > $STATUS_KO tt_valid > $STATUS_OK valid_template > > $STATUS_KO tmpl/i_fail_valid_template.tt > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK spelling > $STATUS_OK tt_valid > $STATUS_KO valid_template > > $STATUS_OK tmpl/i_will_be_correct_tt_valid.tt > $STATUS_OK forbidden patterns >+ $STATUS_OK git manipulation > $STATUS_OK spelling > $STATUS_OK tt_valid > $STATUS_OK valid_template >-- >2.7.0 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=49318">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16008
:
48830
|
48862
|
49318
|
50136