The "Diff" link shows up on cvs tip Bugzilla with the checkin of Patch Viewer (bug 174942), even if you have not installed the PatchReader package. Subsequently, clicking on that link will cause compile failures. Bugzilla needs to check whether PatchReader exists before putting the "Diff" link next to a patch. Note that PatchIterator has been renamed PatchReader for better marketability, so this patch will include the appropriate changes.

This patch fixes the problem, and renames PatchIterator to PatchReader globally. PatchReader can be found at http://www.cpan.org/authors/id/J/JK/JKEISER/PatchReader-0.9.tar.gz .

Comment on attachment 129290 [details] [diff] [review] Patch >Index: attachment.cgi >- require PatchIterator::DiffPrinter::raw; >- $last_iter->sends_data_to(new PatchIterator::DiffPrinter::raw()); >+ require PatchReader::DiffPrinter::raw; >+ $last_iter->sends_data_to(new PatchReader::DiffPrinter::raw()); Nit: Now that you call it Patch*Reader* it's a little strange for the variables to continue being called "iter" and "last_iter". Not illogical, just a little strange; I can see future Bugzilla programmers wondering why they were named that way. >@@ -570,23 +570,23 @@ >- new PatchIterator::AddCVSContext($::FORM{'context'}, >+ new PatchReader::AddCVSContext($::FORM{'context'}, > Param('cvsroot_get'))); Nit: fix indentation on succeeding lines. Otherwise, looks good, and r=myk since all comments are nits, but I wonder if it makes sense to make the patchviewerinstalled test stricter. After all, if the installation has installed PatchReader but not configured the "cvsroot" param the patch viewer will fail as well, right?

Variables changed from $iter to $reader. The patchviewerinstalled variable generally encompasses all it needs to--no parameters are necessary for patch viewer to run at a basic level. Well, with this patch anyway--when I checked for cvsroot I found one place (Raw Unified Diff generation) where it was assuming cvsroot was there, so I fixed that.

Since I was waiting anyway, I decided to change checksetup.pl as well, to check for PatchReader and warn if it's not there. It now warns if interdiff is not there, as well, to give the user a chance at install time to install it.

Comment on attachment 130086 [details] [diff] [review] Patch v1.2 Nit: In the past we've recommended people use the CPAN module to get modules. This seems like better form, especially since the CPAN module utilizes mirror servers, while your text directs everyone to search.cpan.org. Otherwise looks good. r=myk

Fix checked in. The CPAN issue will be resolved once I have registered the namespace officialy, I think.