Bug 16008

Hi Mark,
Actually it's a ... feature :)
You are not supposed to add a file and remove it in the same patch set.
It should be a new check actually, but at the moment I would prefer to keep the warning.

Mark,
The patch works but it's not the proper way to handle this error.
Other checks just return 0 (and not 1) if the file does not exist.
It does not make sense to test this failure in the check_forbidden_patterns subroutine.

You will need to update the koha-qa repo and apply 16104 first to make the tests pass.

Mark, any chances to see you testing this patch?

(In reply to Jonathan Druart from comment #7)
> Mark, any chances to see you testing this patch?

I'll let QA test this harder, but I have tested for the newer git manipulation test which you added (nice simplification) and forbidden patterns (since I hit that with 16236 last night).

Spelling, tt_valid, and valid_template failures should still be confirmed.

Created attachment 50136 [details] [review]
[SIGNED-OFF] 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

NOTE: ran this test plan, and checked forbidden patterns.
      need confirmation on spelling, tt_valid, and valid_template.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Thanks Mark!

Too bad not to see another QAer taking a look at this one.
I am going to push it anyway.

(In reply to Jonathan Druart from comment #11)
> Too bad not to see another QAer taking a look at this one.
> I am going to push it anyway.

No objection at all for doing so.
If there would be an error, you will hear :)
Fetched latest commit now.. ;)

Now tested again with Bug 15839
Error is gone, but I see a few 0's and 1's (debug stuff?)

 OK     C4/Review.pm
   OK     critic
   OK     forbidden patterns
                0
   OK     git manipulation
   OK     pod
   SKIP   spelling
   OK     valid
                0

 FAIL   Koha/Review.pm
   OK     critic
   OK     forbidden patterns
                1

(In reply to Marcel de Rooy from comment #13)
> Now tested again with Bug 15839
> Error is gone, but I see a few 0's and 1's (debug stuff?)
> 
>  OK     C4/Review.pm
>    OK     critic
>    OK     forbidden patterns
>                 0
>    OK     git manipulation
>    OK     pod
>    SKIP   spelling
>    OK     valid
>                 0
> 
>  FAIL   Koha/Review.pm
>    OK     critic
>    OK     forbidden patterns
>                 1

Yes I have seen it too, but I think it already existed prior to this patch.

Marcel,
  commit 687b5cd790bbb0378a91d1896bcd947bbe76bec1
    Display the errors if it's a valid error
Will fix the 0 and 1