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

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

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.

-----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-----

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":
"""

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/>

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 :-(

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

lp:~toshio/bzr-gtk/handle-patch-fix has become corrupt.

New branch lp:~toshio/bzr-gtk/handle-patch-dirty

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