This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 554878 - eats patch content, context, headers, comments when using the diff viewer
eats patch content, context, headers, comments when using the diff viewer
Status: NEW
Product: Bugzilla
Classification: Community
Component: Attachments/Requests (Show other bugs)
3.6
All Linux
low Severity medium (vote)
: ---
: ---
Assigned To: PnT DevOps Devs
tools-bugs
: Reopened
: 562218 698594 708014 1209721 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-12 16:33 EST by Bill Nottingham
Modified: 2016-06-29 22:43 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-12 20:40:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Bill Nottingham 2010-01-12 16:33:27 EST
Description of problem:

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

The comments/context at the top of the patch is important to keep around.

Version-Release number of selected component (if applicable):

bz-current

How reproducible:

100%
Comment 1 Ville Skyttä 2010-03-12 13:58:11 EST
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.
Comment 2 Jeff Fearn 2012-05-30 00:35:57 EDT
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.
Comment 3 Simon Green 2012-05-30 09:21:38 EDT
This was fixed in Bugzilla 4.2
Comment 4 Ville Skyttä 2012-05-30 10:22:45 EDT
(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.
Comment 5 Simon Green 2012-06-19 23:02:40 EDT
*** Bug 562218 has been marked as a duplicate of this bug. ***
Comment 6 Simon Green 2012-06-20 20:43:13 EDT
*** Bug 708014 has been marked as a duplicate of this bug. ***
Comment 7 Simon Green 2012-06-20 20:43:26 EDT
*** Bug 698594 has been marked as a duplicate of this bug. ***
Comment 8 Jeff Fearn 2012-06-20 23:53:33 EDT
These bugs have been flagged as still relevant and are being reset to default values for PM consideration.
Comment 9 Matt Tyson 2012-10-26 00:00:32 EDT
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.
Comment 10 Matt Tyson 2012-10-28 19:42:05 EDT
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.
Comment 11 Ville Skyttä 2012-10-29 10:44:38 EDT
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)
Comment 12 Matt Tyson 2012-10-29 19:35:07 EDT
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.
Comment 13 Ville Skyttä 2012-10-30 03:20:49 EDT
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.
Comment 14 Matt Tyson 2012-11-05 19:22:14 EST
(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.
Comment 15 Richard W.M. Jones 2015-02-06 05:09:53 EST
Reopening because the bug is real and still not fixed.
Comment 16 Richard W.M. Jones 2015-02-06 05:12:41 EST
Another example of a broken patch:
https://bugzilla.redhat.com/show_bug.cgi?id=1189894#c4
Comment 17 Jeff Fearn 2015-02-09 21:11:04 EST
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.
Comment 18 Jason McDonald 2015-04-08 02:28:40 EDT
As hinted in Comment 1, a diff of diffs doesn't display correctly as reported in Bug 1209721.
Comment 19 Jason McDonald 2015-04-08 02:29:23 EDT
*** Bug 1209721 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.