Closed Bug 233695 Opened 18 years ago Closed 11 years ago

[PatchReader] Lines beginning with "+" or "-" disappear in new diff function

Categories

(Bugzilla :: Attachments & Requests, defect)

3.0.3
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mikepinkerton, Assigned: wicked)

References

Details

(Whiteboard: [Patch: see comment 5])

Look at the http://bugzilla.mozilla.org/attachment.cgi?id=140802&action=view in
the diff viewer:

http://bugzilla.mozilla.org/attachment.cgi?id=140802&action=diff

If you scroll down to just after the routine "isPantherOrLater" you will see a
function body without a function header. This is because the function header
begins with a "+" symbol (in obj-c, this denotes a static method). You can see
what it should be by looking at the patch itself, which is correct.
This is interesting... John, do you think that this is a bug in PatchReader or
in our Bugzilla code?
Severity: major → normal
*** Bug 338486 has been marked as a duplicate of this bug. ***
QA Contact: mattyt-bugzilla → default-qa
Assignee: myk → ui
Duplicate of this bug: 288038
Summary: Lines beginning with "+" disappear in new diff function → [PatchReader] Lines beginning with "+" or "-" disappear in new diff function
Blocks: bz-patch
No longer blocks: 288038
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070306 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Comment 0 stands, with current b.m.o BugZilla version, whatever it is.

The missing line from the patch is:
{{
++ (nsMacCursor *) createCursor: (enum nsCursor) aCursor
}}
Assignee: ui → john
Component: User Interface → Attachments & Requests
Attachment 190249 [details] [diff] (in bug 288038) fixes this problem.

John, is there any plan to land that patch in Patch Reader?

cl
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Bug still there, after "2008-03-27 bmo upgrade" (tracked by bug 425607).
(That's BugZilla v3.0.3 !?)
Flags: blocking3.0.4?
Whiteboard: [Patch: see comment 5]
Not a blocker. This is a bug in PatchReader itself, not in Bugzilla. And I don't want to implement a workaround in a hurry, especially on a stable branch.
Flags: blocking3.0.4? → blocking3.0.4-
Duplicate of this bug: 430554
Version: unspecified → 3.0.3
(In reply to comment #5)
> Attachment 190249 [details] [diff] (in bug 288038) fixes this problem.

Chris,
can you confirm that that patch does fix this bug (too),
and not (only) bug 303683 ?

See bug 288038 comment 9.

> John, is there any plan to land that patch in Patch Reader?

John, ping ?
FWIW, I filed
n°35362 : PatchReader-0.9.5 bug report: ++/-- line issues
<http://rt.cpan.org/Public/Bug/Display.html?id=35362>
(In reply to comment #9)
> (In reply to comment #5)
> > Attachment 190249 [details] [diff] [details] (in bug 288038) fixes this problem.
> 
> Chris,
> can you confirm that that patch does fix this bug (too),
> and not (only) bug 303683 ?
> 
> See bug 288038 comment 9.

I can't recall if I actually tested it or if I just read it and understood it to be true. Someone with a Bugzilla test install should probably verify that it does indeed fix things as it's supposed to.

cl

Assignee: john → attach-and-request
This is particularly annoying now that the default view for patches is diff mode; I thought attachment 458039 [details] [diff] [review] was corrupt, but in fact it's fine.
bmo has taken local patches to PatchViewer before to fix pain points (e.g. bug 362767); if attachment 190249 [details] [diff] [review] in bug 288038 really does fix this problem, can we take it on bmo, too?

This can affect not just Obj-C code, but also jar manifests in certain cases (e.g. attachment 474602 [details] [diff] [review]).  Particularly as Diff view is now the default view in Bugzilla for most references to patches, this bug is becoming more and more annoying.
This also affects HTML close-comment markers, such as in attachment 493143 [details] [diff] [review].
Fixed in PatchReader 0.9.6.
Assignee: attach-and-request → wicked
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Great. We should require that version, no?
(In reply to comment #16)
> Great. We should require that version, no?

Sure, I already planned to file a bug about this, for 4.2. :)
You need to log in before you can comment on or make changes to this bug.