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

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted patch patch v1 (obsolete) — Splinter Review
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).
Attachment #649195 - Flags: review?(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
Attachment #649195 - Flags: review?(dkl) → review-
Posted patch patch v2 (obsolete) — Splinter Review
nice catch :)
Attachment #649195 - Attachment is obsolete: true
Attachment #649534 - Flags: review?(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?
Attachment #649534 - Flags: review?(dkl) → review+
(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.
Posted patch patch v3Splinter Review
oops :)
Attachment #649534 - Attachment is obsolete: true
Attachment #649541 - Flags: review?(dkl)
Comment on attachment 649541 [details] [diff] [review]
patch v3

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

\o/ r=dkl
Attachment #649541 - Flags: review?(dkl) → review+
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Summary: shift patchreader to a bugzilla namespace module → shift patchreader to a bugzilla namespace module and fix long standing issues
You need to log in before you can comment on or make changes to this bug.