779862 - shift patchreader to a bugzilla namespace module and fix long standing issues

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement)

Description

[:glob ✱]

until the patchreader module on cpan has been updated to address the various issues we constantly see, we should bring patchreader into the bmo source tree and fix the issues.

i'll dig up the patches i've already provided to upstream, and i'll ensure as i apply them to bmo i'll provide pull requests to the cpan module.

Comment 1

[:glob ✱]

Attachment: patch v1

this is the latest patchreader from github with the following changes:

https://github.com/tmannerm/PatchReader/pull/1

there's also a change to attachment/diff-footer.html.tmpl to display a message when patchreader doesn't recognise a diff (or more likely a file is tagged as a patch, but it isn't).

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 649195 [details] [diff] [review]
patch v1

Review of attachment 649195 [details] [diff] [review]:
-----------------------------------------------------------------

I am not able to get this to work with a set of patches I am trying to interdiff:

https://bugzilla.mozilla.org/attachment.cgi?id=619402&action=edit
https://bugzilla.mozilla.org/attachment.cgi?id=643260&action=edit

I get the error message which is an improvement, but the interdiff currently works on bugzilla.mozilla.org
and will not work with the local copy of PatchReader. it will however work if i revert your
github pull request changes in Bugzilla/PatchReader/Raw.pm and Bugzilla/PatchReader/DiffPrinter/raw.pm. Then 
the interdiff displays as it does on production.

I will dig into this some more later but you may be able to find it before I do.
Note: I am applying this to bmo/4.0 and not 4.2 if that makes a difference.

dkl

Comment 3

[:glob ✱]

Attachment: patch v2

nice catch :)

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 649534 [details] [diff] [review]
patch v2

Review of attachment 649534 [details] [diff] [review]:
-----------------------------------------------------------------

New version works properly for interdiff in my testing. r=dkl

::: Bugzilla/PatchReader/Raw.pm
@@ +175,5 @@
> +
> +sub _init_state {
> +  my $this = shift;
> +  #$this->{SECTION_STATE}{minus_lines} ||= 0;
> +  #$this->{SECTION_STATE}{plus_lines} ||= 0;

Why not just remove this section completely or are you going to need to re-enable it in the future and want to keep it around?

Comment 5

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #4)
> Why not just remove this section completely or are you going to need to
> re-enable it in the future and want to keep it around?

oops, that should be re-enabled.  new patch coming up.

Comment 6

[:glob ✱]

Attachment: patch v3

oops :)

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 649541 [details] [diff] [review]
patch v3

Review of attachment 649541 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ r=dkl

Comment 8

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
added Bugzilla/PatchReader
added Bugzilla/PatchReader.pm
modified Bugzilla/Attachment/PatchReader.pm
added Bugzilla/PatchReader/AddCVSContext.pm
added Bugzilla/PatchReader/Base.pm
added Bugzilla/PatchReader/CVSClient.pm
added Bugzilla/PatchReader/DiffPrinter
added Bugzilla/PatchReader/FilterPatch.pm
added Bugzilla/PatchReader/FixPatchRoot.pm
added Bugzilla/PatchReader/NarrowPatch.pm
added Bugzilla/PatchReader/PatchInfoGrabber.pm
added Bugzilla/PatchReader/Raw.pm
added Bugzilla/PatchReader/DiffPrinter/raw.pm
added Bugzilla/PatchReader/DiffPrinter/template.pm
modified template/en/default/attachment/diff-footer.html.tmpl
Committed revision 8265.

i've only committed this to bmo/4.0 -- hopefully a new version of PatchReader will be out by the time we upgrade to bmo/4.2.