bzr-handle-patch doesn't like patch files containing "diff" lines before each patch

Bug #502076 reported by Michael Bienia on 2010-01-01
32
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Martin Pool
Bazaar GTK+ Frontends
Medium
Unassigned
bzr (Fedora)
Won't Fix
Medium
bzr (Ubuntu)
Undecided
Unassigned

Bug Description

Binary package hint: bzr-gtk

As bzr-handle-patch registers itself for .patch files, Firefox offers to use it when I click on a .patch as can be found on the Debian PTS (links to the patches on http://patches.ubuntu.com). As I regularly use these patches to check all current Ubuntu delta when sponsoring merges, bzr-handle-patch isn't really helpful (as the standard application) as it claims broken patch files and crashes:

Malformed patch header. No orig name
'diff -pruN 1.2.4-1/debian/changelog 1.2.4-1ubuntu1/debian/changelog\n'

The patch itself looks as following:

diff -pruN 1.2.4-1/debian/changelog 1.2.4-1ubuntu1/debian/changelog
--- 1.2.4-1/debian/changelog 2009-06-25 21:20:52.000000000 +0100
+++ 1.2.4-1ubuntu1/debian/changelog 2009-06-25 21:15:43.000000000 +0100
@@ -1,3 +1,11 @@
[...]

If I remove the offending "diff" lines from the whole patch, bzr-handle-patch can open it then.
As creating patch files with "diff -ruN" (or even "diff -pruN") is pretty common, bzr-handle-patch should cope with them (if it registers itself for those files).

Related branches

abrt 1.0.3 detected a crash.

How to reproduce: Try to open the patch file from https://mozilla-plugin.forge.funambol.org/servlets/TrackerDownload/remcurreport/true/template/ViewAttachment.vm/attachid/453/filename/0001-fix-preferences.patch and you will get the attached crash.
Attached file: backtrace
cmdline: /usr/bin/python /usr/bin/bzr-handle-patch /home/fm/0001-fix-preferences.patch
component: bzr-gtk
executable: /usr/bin/bzr-handle-patch
kernel: 2.6.31.9-174.fc12.i686.PAE
package: bzr-gtk-0.97.0-2.fc12
uuid: 4b0c976f

Created attachment 386005
File: backtrace

This has been filed upstream before. Putting upstream bug# in. However, this looks like a bug in bzr code rather than bzr-gtk. Looking further.

Toshio Kuratomi (toshio) wrote :

from the traceback, this looks to be a bug in bzrlib. I'm not sure how to retarget this bug there, though:

Summary: TB4b0c976f patches.py:76:get_patch_names:MalformedPatchHeader: Malformed patch header. No orig name
'diff --git a/funambol-mozilla-plugin/extension/chrome/content/funambol/funambol-preferences-linux.xul b/funambol-mozilla-plugin/extension/chrome/content/funambol/funambol-preferences-linux.xul\n'

Traceback (most recent call last):
  File "/usr/bin/bzr-handle-patch", line 28, in <module>
    controller = DiffController(path, lines)
  File "/usr/lib/python2.6/site-packages/bzrlib/plugins/gtk/diff.py", line 530, in __init__
    self.initialize_window(window)
  File "/usr/lib/python2.6/site-packages/bzrlib/plugins/gtk/diff.py", line 534, in initialize_window
    window.diff.set_diff_text_sections(self.get_diff_sections())
  File "/usr/lib/python2.6/site-packages/bzrlib/plugins/gtk/diff.py", line 317, in set_diff_text_sections
    for oldname, newname, patch in sections:
  File "/usr/lib/python2.6/site-packages/bzrlib/plugins/gtk/diff.py", line 539, in get_diff_sections
    for patch in parse_patches(self.patch):
  File "/usr/lib/python2.6/site-packages/bzrlib/patches.py", line 401, in parse_patches
    return [parse_patch(f.__iter__()) for f in iter_file_patch(iter_lines)]
  File "/usr/lib/python2.6/site-packages/bzrlib/patches.py", line 345, in parse_patch
    (orig_name, mod_name) = get_patch_names(iter_lines)
  File "/usr/lib/python2.6/site-packages/bzrlib/patches.py", line 76, in get_patch_names
    raise MalformedPatchHeader("No orig name", line)
MalformedPatchHeader: Malformed patch header. No orig name
'diff --git a/funambol-mozilla-plugin/extension/chrome/content/funambol/funambol-preferences-linux.xul b/funambol-mozilla-plugin/extension/chrome/content/funambol/funambol-preferences-linux.xul\n'

Local variables in innermost frame:
iter_lines: <generator object iter_lines_handle_nl at 0x8bdfdc4>
line: diff --git a/funambol-mozilla-plugin/extension/chrome/content/funambol/funambol-preferences-linux.xul b/funambol-mozilla-plugin/extension/chrome/content/funambol/funambol-preferences-linux.xul

match: None

Downstream bug report: https://bugzilla.redhat.com/show_bug.cgi?id=557573

Toshio Kuratomi (toshio) wrote :

Here's my fix for the problem. The patch handling code in bzrlib is not allowing "junk" before and after the individual patches in the file. This patch makes that code more lenient.

This needs to be applied to bzr, not bzr-gtk. If someone knows how to retarget this bug that would help out.

I'm able to fix this bug with a patch to bzr's patch handling to make it more lenient. (Currently, that patch handling code doesn't accept "junk" before and after the individual patches within a file.). Will attach a patch.

hno, do you know if the patches.py code is meant to handle any patch file? If so, this is appropriate. If it's really only meant to handle bzr-generated patches, then bzr-gtk is misusing the bzrlib code.

Created attachment 386120
Patch to make bzr's patch handling more lenient

Gordon Tyler (doxxx) on 2010-01-22
affects: bzr-gtk (Ubuntu) → bzr (Ubuntu)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gordon Tyler wrote:
> ** Also affects: bzr
> Importance: Undecided
> Status: New
>

I think we would be okay with a flag that allows patches to be 'dirty',
but I think we also would like a way to force them to be pristine.

I don't remember exactly where handle-patch is used, but there are some
places (I believe) where we emit the diff and then parse it later, and
we'll want to make sure they match exactly.

I didn't look at the patch, but it would be nice if the decision to be
lenient could be turned on/off.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktZuGIACgkQJdeBCYSNAAPCGwCeOAYDgZUq9aNEBsK594BKNEV5
05IAnRenOnyMe9+pNXkIo87GVlg3AWL+
=V+lD
-----END PGP SIGNATURE-----

Toshio Kuratomi (toshio) wrote :

New patch against lp:bzr (looks the same for 2.0.x) that gives parse_patches() and the necessary functions that it calls an allow_dirty parameter. allow_dirty turns on the new behaviour of allowing junk at the beginning and end of individual patches within the patch file.

The default is to use the current behaviour. I'll have a patch for bzr-gtk (diff.py and bzr-handle-patch) to set allow_dirty next.

Looking at the code in iter_file_patch() I note one thing. The current algorithm allows some non-diff output anywhere if it has special characters in front. You might want to change that as it accepts patches like this:

"""
=== Hey bro! I'm pretendin' to be a dumb patch
*** That takes advantage of iter_file_patch()'s
# algorithm to parse ...
--- bzrlib/patches.py>--2009-11-03 15:45:56 +0000
+++ bzrlib/patches.py>--2010-01-23 17:57:32 +0000
@@ -250,7 +250,13 @@
         return shift

-def iter_hunks(iter_lines):
===
=== ... something that isn't even a valid patch!
# Yep, I can drop text beginning with any of the allowed special chars
# in the patch itself!
===
===
+def iter_hunks(iter_lines, allow_dirty=False):
+ '''
+ :arg iter_lines: iterable of lines to parse for hunks
+ :kwarg allow_dirty: If True, when we encounter something that is not
+ a hunk header when we're looking for one, assume the rest of the lines
+ are not part of the patch (comments or other junk). Default False
+ '''
     hunk = None
     for line in iter_lines:
         if line == "\n":
"""

Toshio Kuratomi (toshio) wrote :

Patch to bzr-gtk that uses the allow_dirty parameter to bzr to handle patches with comments and other text than bzr puts in.

Toshio,

Thanks very much for the patches. Would you mind putting up merge
proposals for these <https://help.launchpad.net/Code/Review> because
we can track and review them better than patches on a branch?

--
Martin <http://launchpad.net/~mbp/>

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

Toshio Kuratomi (toshio) wrote :

Two branches proposed for merge.

BTW, this seems like a better page for learning how to submit a branch for merge from a bug report:
https://help.launchpad.net/Code/BugAndBlueprintLinks

it follows the workflow of someone who is starting from fixing an issue in a bug report and links out to the pages needed for explaining how to download and upload a branch from launchpad. The only thing it needs, in fact, is a link to the Merging page that you posted :-(

Martin Pool (mbp) on 2010-02-15
Changed in bzr:
status: New → In Progress
importance: Undecided → Medium
tags: added: patch
tags: added: patch-needswork
removed: patch
Martin Pool (mbp) on 2010-02-25
Changed in bzr:
assignee: nobody → Martin Pool (mbp)

How to reproduce
-----
1. open the following url in firefox:
http://www.splitted-desktop.com/~gbeauchesne/libva/patches/
2. click on the link 033_g45_add_rgba_subpic.patch
3. open the file with Baazar (crash is reproducible in 100%)

Martin Pool (mbp) wrote :

given Toshio's patch to bzr itself I'm going to mark that closed, and open a new one against bzr-gtk; please reopen if I'm wrong

Changed in bzr:
status: In Progress → Fix Released
Changed in bzr-gtk:
status: New → In Progress

My patch to bzr to fix this is in bzr-2.2b1. We could safely backport it to current bzr packages. Also need a patch to bzr-gtk to turn this functionality on. I have a patch for that linked to the upstream bug report but upstream needs it cconditionalized on having a new enough bzr.

New fix for bzr-gtk submitted upstream: https://code.launchpad.net/~toshio/bzr-gtk/handle-patch-dirty/+merge/23330

If accepted, we'll likely want to backport both the bzr and bzr-gtk fixes back to F-12 and F-13.

David Futcher (bobbo) on 2010-06-08
tags: added: patch-forwarded-upstream
removed: patch-needswork
Jelmer Vernooij (jelmer) on 2010-07-21
Changed in bzr-gtk:
status: In Progress → Fix Released
importance: Undecided → Medium

Package: bzr-gtk-0.97.0-4.fc12
Architecture: i686
OS Release: Fedora release 12 (Constantine)

How to reproduce
-----
1. Add patch as an attachment in the thunderbird program
2. Double click the patch to see its content
3.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package bzr - 2.2.0-1

---------------
bzr (2.2.0-1) unstable; urgency=low

  * New upstream release.
   + Adds support for setting timestamps to originating revisions.
     Closes: #473450
   + Removes remaining string exception. Closes: #585193, LP: #586926
   + Add C extension to work around Python issue 1628205. LP: #583941,
     Closes: #577110
   + Avoids showing progress bars when --quiet is used. Closes: #542105,
     LP: #320035
   + No longer creates ~/.bazaar as root when run under sudo. LP: #376388
   + 'bzr commit' now supports -p as alternative for --show-diff. LP: #571467
   + 'bzr add' no longer adds .THIS/.BASE/.THEIRS files unless
     explicitly requested. LP: #322767
   + When parsing patch files, Bazaar now supports diff lines before each
     patch. LP: #502076
   + WorkingTrees now no longer requires using signal.signal, so can
     be used in a threaded environment. LP: #521989
   + An assertion error is no longer triggered when pushing to a pre-1.6
     Bazaar server. LP: #528041
  * Bump standards version to 3.9.1.

bzr (2.2.0~b4-1) experimental; urgency=low

  * New upstream beta.
  * Bump standards version to 3.9.0.
  * Drop build dependency on zlib.

bzr (2.2.0~b2-1) experimental; urgency=low

  * New upstream release.
  * Recommend python-launchpadlib. Closes: #568937

bzr (2.1.2-1.1) unstable; urgency=low

  * Non-maintainer upload.
  * Fix access via http_proxy. Closes: #588430
 -- Jelmer Vernooij <email address hidden> Sat, 07 Aug 2010 00:54:52 +0200

Changed in bzr (Ubuntu):
status: New → Fix Released

Package: bzr-gtk-0.98.0-1.fc13
Architecture: i686
OS Release: Fedora release 13 (Goddard)

Comment
-----
Just tried to open a C source file.

This message is a reminder that Fedora 12 is nearing its end of life.
Approximately 30 (thirty) days from now Fedora will stop maintaining
and issuing updates for Fedora 12. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as WONTFIX if it remains open with a Fedora
'version' of '12'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version prior to Fedora 12's end of life.

Bug Reporter: Thank you for reporting this issue and we are sorry that
we may not be able to fix it before Fedora 12 is end of life. If you
would still like to see this bug fixed and are able to reproduce it
against a later version of Fedora please change the 'version' of this
bug to the applicable version. If you are unable to change the version,
please add a comment here and someone will do it for you.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

The process we are following is described here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping

Fedora 12 changed to end-of-life (EOL) status on 2010-12-02. Fedora 12 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version.

Thank you for reporting this bug and we are sorry it could not be fixed.

Changed in bzr (Fedora):
importance: Unknown → Medium
status: Unknown → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.