215268 - "Diff" link comes up even if PatchReader is not installed

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[John Keiser (jkeiser)]

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.

Comment 1

[John Keiser (jkeiser)]

Attachment: Patch

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 2

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 3

[John Keiser (jkeiser)]

Attachment: Patch v1.1

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.

Comment 4

[John Keiser (jkeiser)]

Attachment: Patch v1.2

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 5

[Myk Melez [:myk] [@mykmelez]]

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

Comment 6

[John Keiser (jkeiser)]

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