Bug 554878

It is not only the context/headers/comments that are lost in some cases but bits of actual patch content as well.  This is reported against 3.6 but I'm currently (I believe) using 3.4.6 and it happens with it too.

Compare:
https://bugzilla.redhat.com/attachment.cgi?id=399519

With:
https://bugzilla.redhat.com/attachment.cgi?id=399519&action=diff
https://bugzilla.redhat.com/attachment.cgi?id=399519&action=diff&context=patch&collapsed=&headers=1&format=raw

The actual patch contains these two lines:

+-            s/^.*\(U\|msg_u\)sage.*{\(.*\)}.*$/\1/p" \
++            s/^.*\(U\|msg_u\)sage.*{\(.*\)}.*$/\2/p" \

Only the first of these is shown by the diff viewer, the second is missing.

As part of the recent Bugzilla 2.4 upgrade the Bugzilla team are cleaning up bugs opened against old versions of Bugzilla. This bug has been flagged as an old bug and will be CLOSED WONTFIX in 7 days time.

If you believe this bug is an issue in the latest Bugzilla version please comment on this bug within 7 days. Doing so will ensure this bug is not closed automatically.

Thanks, the Bugzilla team.

This was fixed in Bugzilla 4.2

(In reply to comment #3)
> This was fixed in Bugzilla 4.2

It is not fixed, the test case described in comment 1 still fails - the mentioned second line of patch content is still missing.

*** Bug 562218 has been marked as a duplicate of this bug. ***

*** Bug 708014 has been marked as a duplicate of this bug. ***

*** Bug 698594 has been marked as a duplicate of this bug. ***

These bugs have been flagged as still relevant and are being reset to default values for PM consideration.

This looks like a bug in the PatchReader library.  It appears it's not designed to cope with nested patches, or anything that has patch-like syntax scattered about where it would not normally be expected in a standard patch.

As stated in comment 9, this is a bug in the PatchReader code.

This is not a bug in bugzilla and should be filed upstream with the PatchReader maintainers.

What's the version of PatchReader running at bugzilla.redhat.com? I'm asking because PatchReader 0.9.6's change log shows a couple of things changed that could affect this, and it'd be good to verify this before spending time on trying to create upstream PatchReader reproducers:

0.9.6 4/11/2011
  Add tests to instantiate all modules and verify parsing of patches
  Fix POD syntax
  Fix corruption due to patched lines having a + or - as the first character
  Fix FixPatchRoot w/DOS eol patches (RT bug #35362, regression from v0.9.3)
  Attic file support for FixPatchRoot (RT bug #40467)
  Multiple file patch support (RT bug #47625)

You are correct.  PatchReader 0.9.6 fixes this problem.

I've made a test case for comment 1, and that line is not dropped in 0.9.6, but it is dropped in 0.9.5.

As part of the next release we will upgrade PatchReader.

The problem reported in the bug description will still exist, as the email header and Git output is not valid in a patch file.  This information should be put in the attachment description.

I believe the mail header and git stuff as well as just about anything is ok to have before the unified diff indicators/headers in patch files. From patch(1):

| patch tries to skip any leading garbage, apply the diff, and then skip any
| trailing garbage.  Thus you could feed an  article  or  message containing
| a  diff listing to patch, and it should work.

Using leading text this way is certainly a common practice and definitely useful there for example for typical git workflows (git am). My understanding is that  the case being discussed in this bug is exactly the "message containing a diff listing" one.

BTW, PatchReader is listed as an optional module "for pretty HTML view of patches" in Bugzilla upstream installation docs - FWIW my opinion is that if it can break/confuse things this way it'd be better to disable it altogether.

(In reply to comment #13)
> I believe the mail header and git stuff as well as just about anything is ok
> to have before the unified diff indicators/headers in patch files. From
> patch(1):
> 
> | patch tries to skip any leading garbage, apply the diff, and then skip any
> | trailing garbage.  Thus you could feed an  article  or  message containing
> | a  diff listing to patch, and it should work.
> 
> Using leading text this way is certainly a common practice and definitely
> useful there for example for typical git workflows (git am). My
> understanding is that  the case being discussed in this bug is exactly the
> "message containing a diff listing" one.

Unfortunately PatchReader does not support this.
If you feel this is a bug in PatchReader, then please submit a bug against PatchReader in CPAN's bug tracker.

Reopening because the bug is real and still not fixed.

Another example of a broken patch:
https://bugzilla.redhat.com/show_bug.cgi?id=1189894#c4

Bugzilla does not corrupt patches, some of the views of them are incorrect.

e.g. the raw view shows the full patch https://bugzilla.redhat.com/attachment.cgi?id=988596

The module used to show the unified view does not support git syntax. The upstream bug tracker, where RFEs should be lodged, is https://rt.cpan.org/Public/Dist/Display.html?Name=PatchReader

Until the upstream module is enhanced do not use git diff syntax if you want to use the unified diff view.

IMO what we should do is ensure that BZ refuses to display patches via the enhanced views if the format cannot be fully supported.

As hinted in Comment 1, a diff of diffs doesn't display correctly as reported in Bug 1209721.

*** Bug 1209721 has been marked as a duplicate of this bug. ***

Might be worth investigating https://foswiki.org/Extensions/DiffPlugin to see if it does what we need and is easy to turn in to a Bugzilla extension.

*** Bug 1384801 has been marked as a duplicate of this bug. ***

Perhaps the “raw unified diff” link could be renamed to “raw diff”, and point to the raw attachment download?  This way, the faulty reformatting would no longer be exposed (except for existing hyperlinks).

Closing as a dupe as Bug 738069 replaces the patch viewer with https://diff2html.xyz

*** This bug has been marked as a duplicate of bug 738069 ***