Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 738069 - Unified patch display sometimes shows garbled patches
Summary: Unified patch display sometimes shows garbled patches
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Bugzilla
Classification: Community
Component: Attachments/Requests
Version: devel
Hardware: Unspecified
OS: Unspecified
low
low
Target Milestone: ---
Assignee: Jeff Fearn 🐞
QA Contact: tools-bugs
URL:
Whiteboard:
: 554878 1514087 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-09-13 19:24 UTC by Peter Jones
Modified: 2017-11-16 23:25 UTC (History)
10 users (show)

Fixed In Version: 5.0.3.rh12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-10-28 06:04:52 UTC
Embargoed:

Attachments (Terms of Use)

Description Peter Jones 2011-09-13 19:24:33 UTC 
Description of problem:

If a bug has a patch attached, the patch will render differently if I click on the patch in the Attachments box than if I click on the comment referencing the attachment. On the latter, it takes me to a rendered side-by side view. When I click on "unified raw diff" there, the result is a totally mangled patch.

For instance, bug 658387 has a patch attached.  The Attachements box takes me to https://bugzilla.redhat.com/attachment.cgi?id=463663, which correctly renders the patch as:

------------------- BEGIN
From 9b0b7f4645fa44840b81e09bfc4833a70431047a Mon Sep 17 00:00:00 2001
From: W. Michael Petullo <mike>
Date: Tue, 30 Nov 2010 02:35:46 -0600
Subject: [PATCH] Read HYPERVISOR and HYPERVISOR_ARGS from /etc/sysconfig/kernel and set mbkernel and mbargs
 Signed-off-by: W. Michael Petullo <mike>

---
 new-kernel-pkg |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/new-kernel-pkg b/new-kernel-pkg
index 5e04a50..4ac1d81 100755
--- a/new-kernel-pkg
+++ b/new-kernel-pkg
@@ -89,8 +89,8 @@ moddep=""
 verbose=""
 makedefault=""
 package=""
-mbkernel=""
-mbargs=""
+mbkernel="$HYPERVISOR"
+mbargs="$HYPERVISOR_ARGS"
 adddracutargs=""
 addplymouthinitrd=""
 
-- 
1.7.3.2
------------------- END

But if I click on the attachment in the comment and then click "Raw Unified", it takes me to https://bugzilla.redhat.com/attachment.cgi?id=463663&action=diff&context=patch&collapsed=&headers=1&format=raw , which renders the patch as:

------------------- BEGIN
@@ -, +, @@ 
 Signed-off-by: W. Michael Petullo <mike>
 new-kernel-pkg |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
--- a/new-kernel-pkg	
+++ a/new-kernel-pkg	
@@ -89,8 +89,8 @@ moddep=""
 verbose=""
 makedefault=""
 package=""
-mbkernel=""
-mbargs=""
+mbkernel="$HYPERVISOR"
+mbargs="$HYPERVISOR_ARGS"
 adddracutargs=""
 addplymouthinitrd=""
 
------------------- END

Which is completely gibberish.  The result is worse if the patch touches more than one file.
Comment 1 Simon Green 2011-09-19 02:14:16 UTC 
Did a test on Mozilla's landfill bugzilla 3.6 (https://landfill.bugzilla.org/bugzilla-3.6-branch/attachment.cgi?id=1833&action=diff), and it was broken, and a test on landfill bugzilla 4.0 (https://landfill.bugzilla.org/bugzilla-4.0-branch/attachment.cgi?id=2042&action=diff) and it worked fine.

Since there is an acceptable work around, we won't fix thus until we upgrade to Bugzilla 4.0 (scheduled for early 2012)
Comment 2 Adam Jackson 2013-11-21 14:37:54 UTC 
Reopening, this may be fixed upstream but it appears to be broken here.  Consider this bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1030853

and this attachment on it:

https://bugzilla.redhat.com/attachment.cgi?id=825827&action=diff

Clicking 'raw unified' on the above attachment view page gives this URL:

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

And this content:

======
@@ -, +, @@ 
---
 gst/gstpluginloader.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
--- a/gst/gstpluginloader.c	
+++ a/gst/gstpluginloader.c	
@@ -996,27 +996,31 @@ exchange_packets (GstPluginLoader * l)
         l->tx_buf_write - l->tx_buf_read);
 
     if (!l->rx_done) {
-      if (gst_poll_fd_has_error (l->fdset, &l->fd_r) ||
-          gst_poll_fd_has_closed (l->fdset, &l->fd_r)) {
-        GST_LOG ("read fd %d closed/errored", l->fd_r.fd);
+      if (gst_poll_fd_has_error (l->fdset, &l->fd_r)) {
+        GST_LOG ("read fd %d errored", l->fd_r.fd);
         goto fail_and_cleanup;
       }
 
       if (gst_poll_fd_can_read (l->fdset, &l->fd_r)) {
         if (!read_one (l))
           goto fail_and_cleanup;
+      } else if (gst_poll_fd_has_closed (l->fdset, &l->fd_r)) {
+        GST_LOG ("read fd %d closed", l->fd_r.fd);
+        goto fail_and_cleanup;
       }
     }
 
     if (l->tx_buf_read < l->tx_buf_write) {
-      if (gst_poll_fd_has_error (l->fdset, &l->fd_w) ||
-          gst_poll_fd_has_closed (l->fdset, &l->fd_r)) {
-        GST_ERROR ("write fd %d closed/errored", l->fd_w.fd);
+      if (gst_poll_fd_has_error (l->fdset, &l->fd_w)) {
+        GST_ERROR ("write fd %d errored", l->fd_w.fd);
         goto fail_and_cleanup;
       }
       if (gst_poll_fd_can_write (l->fdset, &l->fd_w)) {
         if (!write_one (l))
           goto fail_and_cleanup;
+      } else if (gst_poll_fd_has_closed (l->fdset, &l->fd_w)) {
+        GST_LOG ("write fd %d closed", l->fd_w.fd);
+        goto fail_and_cleanup;
       }
     }
   } while (l->tx_buf_read < l->tx_buf_write);
-- 
======
Comment 3 Simon Green 2013-11-21 23:23:25 UTC 
I'm happy to leave this open. It will be automatically fixed when we migrate to running Bugzilla on RHEL 6 (due to a newer version of the Patch Reader module)

  -- simon
Comment 4 Peter Jones 2014-05-30 20:47:31 UTC 
(In reply to Simon Green from comment #1)
> Did a test on Mozilla's landfill bugzilla 3.6
> (https://landfill.bugzilla.org/bugzilla-3.6-branch/attachment.
> cgi?id=1833&action=diff), and it was broken, and a test on landfill bugzilla
> 4.0
> (https://landfill.bugzilla.org/bugzilla-4.0-branch/attachment.
> cgi?id=2042&action=diff) and it worked fine.
> 
> Since there is an acceptable work around, we won't fix thus until we upgrade
> to Bugzilla 4.0 (scheduled for early 2012)

Both links there currently display malformed diffs.
Comment 5 Matt Tyson 🤬 2014-06-05 04:03:05 UTC 
This is a bug in the PatchReader perl module.  I've submitted a patch upstream to address this.  The patch is waiting for review.
Comment 6 Jeff Fearn 🐞 2015-06-15 05:11:14 UTC 
https://github.com/tmannerm/PatchReader/pull/2
Comment 7 Mike McCune 2016-03-28 23:11:51 UTC 
This bug was accidentally moved from POST to MODIFIED via an error in automation, please see mmccune with any questions
Comment 8 Jeff Fearn 🐞 2016-10-11 10:11:16 UTC 
Hi Matt, can you give me an idea of how long you think it would take to replace the current diff viewer with https://github.com/rtfpessoa/diff2html ?

Looks like:

https://diff2html.xyz/demo#line-by-line
https://diff2html.xyz/demo#side-by-side
Comment 9 Matt Tyson 🤬 2016-10-19 04:15:14 UTC 
It shouldn't be too hard, and it would be good to send a patch upstream.
Comment 11 Jeff Fearn 🐞 2016-10-19 09:41:32 UTC 
*** Bug 554878 has been marked as a duplicate of this bug. ***
Comment 12 Jeff Fearn 🐞 2016-10-19 09:47:41 UTC 
Note for QE: Please check the URLs for patches from Bug 554878 to ensure they render properly.
Comment 13 Rony Gong 🔥 2016-10-27 06:35:02 UTC 
(In reply to Jeff Fearn from comment #12)
> Note for QE: Please check the URLs for patches from Bug 554878 to ensure
> they render properly.

Since the QE test server didn't dump the attachment data of production, so I just create an attachment to test. Seems it works well now.
https://bz-web.host.qe.eng.pek2.redhat.com/show_bug.cgi?id=554878

@Jeff could you confirm it.
Comment 14 Jeff Fearn 🐞 2016-10-27 06:37:39 UTC 
They look good to me!
Comment 15 Jeff Fearn 🐞 2017-11-16 23:25:36 UTC 
*** Bug 1514087 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.