shelve raises exception

[Stuart Bishop]

15:10:31~/lp/sql-dag $ bzr shelve
bzr: ERROR: bzrlib.patches.PatchSyntax: No mod name

Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line 626, in run_bzr_catch_errors
    return run_bzr(argv)
  File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line 588, in run_bzr
    ret = run(*run_argv)
  File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line 292, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/__init__.py", line 248, in run
    s.shelve(source, all, message, no_color)
  File "/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/shelf.py", line 203, in shelve
    patches = patch_source.readpatches()
  File "/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/patchsource.py", line 14, in readpatches
    return patches.parse_patches(self.readlines())
  File "/usr/lib/python2.4/site-packages/bzrlib/patches.py", line 355, in parse_patches
    return [parse_patch(f.__iter__()) for f in iter_file_patch(iter_lines)]
  File "/usr/lib/python2.4/site-packages/bzrlib/patches.py", line 310, in parse_patch
    (orig_name, mod_name) = get_patch_names(iter_lines)
  File "/usr/lib/python2.4/site-packages/bzrlib/patches.py", line 69, in get_patch_names
    raise PatchSyntax("No mod name")
PatchSyntax: No mod name

bzr 0.13.0 on python 2.4.4.candidate.1 (linux2)
arguments: ['/usr/bin/bzr', 'shelve']

** please send this report to <email address hidden>

[Aaron Bentley (abentley)]

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

Stuart Bishop wrote:
> Public bug reported:
>
> 15:10:31~/lp/sql-dag $ bzr shelve
> bzr: ERROR: bzrlib.patches.PatchSyntax: No mod name

This is quite odd. It's an error parsing a unified diff emitted by bzr.

> File "/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/patchsource.py", line 14, in readpatches
> return patches.parse_patches(self.readlines())

Can you intercept the patch here, and forward it to me?

Or alternatively, provide steps-to-reproduce.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFFgA/00F+nu1YWqI0RAl8dAJ42E9oFAfzgEvDvIZHb98I4AwHSzgCfbjDY
ne8flUmy/X5R7f5bTTVl9es=
=49zK
-----END PGP SIGNATURE-----

[Stuart Bishop (stub)]

I think the trigger is changing lines starting with '--' (found as comments
in .sql scripts). Here is a test case:

11:08:33~/tmp $ mkdir foo
11:08:34~/tmp $ cd foo/
11:08:35~/tmp/foo $ bzr init
11:08:53~/tmp/foo $ vi foo.txt
11:09:39~/tmp/foo $ cat foo.txt
This is jibberish

-- This is an SQL comment
-- CREATE FUNCTION foo

This is more jibberish
11:09:44~/tmp/foo $ bzr add foo.txt
added foo.txt
11:09:51~/tmp/foo $ bzr commit -m 'Whatever'
added foo.txt
Committed revision 1.
11:10:03~/tmp/foo $ vi foo.txt
11:10:22~/tmp/foo $ bzr diff
=== modified file 'foo.txt'
--- foo.txt 2006-12-14 04:09:54 +0000
+++ foo.txt 2006-12-14 04:10:22 +0000
@@ -1,6 +1,8 @@
 This is jibberish

 -- This is an SQL comment
--- CREATE FUNCTION foo
+CREATE FUNCTION foo AS
+$$
+$$;

 This is more jibberish

11:10:24~/tmp/foo $ bzr shelve
bzr: ERROR: bzrlib.patches.PatchSyntax: No mod name

Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line 626, in
run_bzr_catch_errors
    return run_bzr(argv)
  File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line 588, in
run_bzr
    ret = run(*run_argv)
  File "/usr/lib/python2.4/site-packages/bzrlib/commands.py", line 292, in
run_argv_aliases
    return self.run(**all_cmd_args)
  File
"/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/__init__.py", line
248, in run
    s.shelve(source, all, message, no_color)
  File "/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/shelf.py",
line 203, in shelve
    patches = patch_source.readpatches()
  File
"/usr/lib/python2.4/site-packages/bzrlib/plugins/bzrtools/patchsource.py",
line 14, in readpatches
    return patches.parse_patches(self.readlines())
  File "/usr/lib/python2.4/site-packages/bzrlib/patches.py", line 355, in
parse_patches
    return [parse_patch(f.__iter__()) for f in iter_file_patch(iter_lines)]
  File "/usr/lib/python2.4/site-packages/bzrlib/patches.py", line 310, in
parse_patch
    (orig_name, mod_name) = get_patch_names(iter_lines)
  File "/usr/lib/python2.4/site-packages/bzrlib/patches.py", line 69, in
get_patch_names
    raise PatchSyntax("No mod name")
PatchSyntax: No mod name

bzr 0.13.0 on python 2.4.4.candidate.1 (linux2)
arguments: ['/usr/bin/bzr', 'shelve']

** please send this report to <email address hidden>
11:10:27~/tmp/foo $

--
Stuart Bishop <email address hidden> http://www.canonical.com/
Canonical Ltd. http://www.ubuntu.com/

[Michael Ellerman (michael-ellerman)]

Actually this _is_ a bug in bzr, because shelf uses bzrlib.patches, and that's where the bug is.

[Aaron Bentley (abentley)]

The patch itself is ambiguous, so I'm not sure what the right fix is.

[Stuart Bishop (stub)]

Why is the patch ambiguous? There is one line starting with ' --' (leading whitespace indicating this is content and not a marker), and another line starting with '---' (no leading whitespace, this is a line to be removed).

I can generate a similar diff and feed it to patch just fine and it works as expected.

07:58:53~/tmp/tst $ cp blah.sql blah2.sql
07:59:10~/tmp/tst $ cat blah.sql
This is the first line

-- This is a comment
-- CREATE FUNCTION FOO

This is the last line
07:59:12~/tmp/tst $ vi blah.sql
07:59:31~/tmp/tst $ diff blah.sql blah2.sql
4,6c4
< CREATE FUNCTION FOO
< $$
< $$;
---
> -- CREATE FUNCTION FOO
07:59:40~/tmp/tst $ diff blah2.sql blah.sql
4c4,6
< -- CREATE FUNCTION FOO
---
> CREATE FUNCTION FOO
> $$
> $$;
07:59:56~/tmp/tst $ diff -u blah2.sql blah.sql
--- blah2.sql 2006-12-22 07:59:04.000000000 +0700
+++ blah.sql 2006-12-22 07:59:31.000000000 +0700
@@ -1,6 +1,8 @@
 This is the first line

 -- This is a comment
--- CREATE FUNCTION FOO
+CREATE FUNCTION FOO
+$$
+$$;

 This is the last line
08:00:05~/tmp/tst $ diff -u blah2.sql blah.sql > the.patch
08:01:31~/tmp/tst $ patch < the.patch
patching file blah.sql
Reversed (or previously applied) patch detected! Assume -R? [n] n
Apply anyway? [n]
08:01:48~/tmp/tst $ cp blah2.sql blah.sql
08:01:54~/tmp/tst $ patch < the.patch
patching file blah.sql
08:02:02~/tmp/tst $ cat blah.sql
This is the first line

-- This is a comment
CREATE FUNCTION FOO
$$
$$;

This is the last line

[Aaron Bentley (abentley)]

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

Stuart Bishop wrote:
> Why is the patch ambiguous? There is one line starting with ' --'
> (leading whitespace indicating this is content and not a marker), and
> another line starting with '---' (no leading whitespace, this is a line
> to be removed).
>
> I can generate a similar diff and feed it to patch just fine and it
> works as expected.
>
> 07:58:53~/tmp/tst $ cp blah.sql blah2.sql
> 07:59:10~/tmp/tst $ cat blah.sql
> This is the first line
>
> -- This is a comment
> -- CREATE FUNCTION FOO
>
> This is the last line
> 07:59:12~/tmp/tst $ vi blah.sql
> 07:59:31~/tmp/tst $ diff blah.sql blah2.sql

It's specifically ambiguous for unified diffs, so this doesn't say anything.

> 07:59:56~/tmp/tst $ diff -u blah2.sql blah.sql
> --- blah2.sql 2006-12-22 07:59:04.000000000 +0700
> +++ blah.sql 2006-12-22 07:59:31.000000000 +0700
> @@ -1,6 +1,8 @@
> This is the first line
>
> -- This is a comment
> --- CREATE FUNCTION FOO
> +CREATE FUNCTION FOO
> +$$
> +$$;
>
> This is the last line

Here, you aren't removing a line that begins with --, so it's not ambiguous.

THIS is ambiguous:
abentley@lappy:~$ echo "-- tmp 2006-12-21 22:09:26.000000000 -0500" > tmp
abentley@lappy:~$ echo "++ tmp2 2006-10-27 19:16:03.000000000 -0400" >
tmp2
abentley@lappy:~$ diff -u tmp tmp2
- --- tmp 2006-12-21 22:10:22.000000000 -0500
+++ tmp2 2006-12-21 22:11:00.000000000 -0500
@@ -1 +1 @@
- --- tmp 2006-12-21 22:09:26.000000000 -0500
+++ tmp2 2006-10-27 19:16:03.000000000 -0400

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

iD8DBQFFi0060F+nu1YWqI0RAhN4AJoDmdPcBrJ1oC2L3dxJVVJfZNaWKgCeKsXw
W8fAz+IF2b5BNjkFXxgLWWo=
=U7YF
-----END PGP SIGNATURE-----

[Stuart Bishop (stub)]

Aaron's output got munged by email or PGP:

13:09:09~/tmp/tst $ diff -u tmp tmp2
--- tmp 2006-12-22 13:05:15.000000000 +0700
+++ tmp2 2006-12-22 13:05:19.000000000 +0700
@@ -1 +1 @@
--- tmp 2006-12-21 22:09:26.000000000 -0500
+++ tmp2 2006-10-27 19:16:03.000000000 -0400

Again, I don't see what is ambiguous. The first two lines tell us we are dealing with files tmp and tmp2. Then the diff part starts, saying to remove the line starting with '-- tmp' and to replace it with the line starting with '++ tmp2'. As far as I can tell, in the unified diff format what-has-changed-sections, any line not starting with '-', '+' or ' ' would be a syntax error.

[Aaron Bentley (abentley)]

The --- in a recursive diff indicates the beginning of a new file:

$ diff -u old new
diff -u old/a new/a
--- old/a 2006-12-22 11:05:14.000000000 -0500
+++ new/a 2006-12-22 11:05:27.000000000 -0500
@@ -1 +1 @@
-foo
+me
diff -u old/b new/b
--- old/b 2006-12-22 11:05:18.000000000 -0500
+++ new/b 2006-12-22 11:05:30.000000000 -0500
@@ -1 +1 @@
-bar
+you

Note that the "diff -u old/a new/a" line is not a required part of the format.

[Johan Dahlberg (johan-d)]

Hello!

I've created a patch, which I think fixes this problem.
It is done by making iter_file_patch() in bzrlib/patches.py look for contiguous lines beginning with "--- ", "+++ " and "@@ ". I as have understood it, this unambiguously indicates the beginning of a new file in a unified patch.

However, I was thinking that a simpler solution may be possible. When I look at diffs created by "bzr diff", each new file seems to start with a line beginning with "=== ".

Is this line "=== " something which _always_ exists in bzr diffs?
(For unified diffs created with the "diff" command they do not exist.)

Regards,
/Johan Dahlberg

[Aaron Bentley (abentley)]

--- followed by +++ followed by @@ does not unambiguously indicate a new file.

A diff with no context lines that removed a "-253", added "+356", and continued with more changes would look like this:

@@ -216,1 +216,1 @@
---253
+++356
@@ -316,16 +316,32 @@
-more
+More

If you want to do it unambiguously, you have to ensure that you are not in a diff hunk when you encounter the ---.

No, the === is not a reasonable approach, because this library is meant to handle all unified diffs, not just Bazaar's.

[Johan Dahlberg (johan-d)]

I see. However, I think that diffs without context are not really suitable for patching.

Anyway, I think the patch I provided would help in most cases where the shelve command fails, though I agree that a better solution would be nice. This patch will not make the problem worse, will it?
Well, ok. I can work a bit more on this patch. But then I need to know what diff formats the iter_file_patch() function should be able to handle. Can anybody help me with this?

The reason I would like to have at least this "quick fix" is that I am programming in VHDL ("--" style comments) using emacs-vhdl mode and it's file header template. This header has automatic update of a date of last change comment line:
-- Last update: 2007/02/19

This line unfortunately always makes the, otherwise so nice, shelve command fail.

Best regards

[Johan Dahlberg (johan-d)]

Hi again!

How about this improved bug fix? This one counts the original lines in each hunk and ignores any "---" line inside any hunk. It should work with unified diffs without context.

Please let me know if there still is anything strange about it.

Regards

[Aaron Bentley (abentley)]

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

Johan Dahlberg wrote:
> Hi again!
>
> How about this improved bug fix? This one counts the original lines in
> each hunk and ignores any "---" line inside any hunk. It should work
> with unified diffs without context.
>
> Please let me know if there still is anything strange about it.

Using 0 as a sentry value is a bit wacky, because 0 is a valid
orig_range. Here's an example:

$ bzr diff --diff-options='-u0'
=== modified file 'README'
- --- README 2007-01-24 19:42:26 +0000
+++ README 2007-03-02 20:39:03 +0000
@@ -0,0 +1 @@
+++

In this case, a single line containing "++" was added to the diff. But
it seems impossible for the orig_range to be 0 and yet for the patch
hunk to contain a removal.

You're essentially resorted to pre-parsing the patch in order to fix the
bug, which suggests that iter_file_patch is a poor interface-- we should
do "iter_file_hunks" or something, so that we only have to parse hunks
once. But for a bugfix, I think this is acceptable.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF6I100F+nu1YWqI0RAj7aAJ4zXcuPf+9ZKof/o0es866URjZq9QCfe/Fj
bAwYmshTgA5iv+h/yyo7gVA=
=YSL8
-----END PGP SIGNATURE-----

[Johan Dahlberg (johan-d)]

Hello,

As you pointed out, there should be no "-" lines if the hunk contains zero original lines. And "+" lines are not treated in any special way, which should make it safe to ignore the modification line count of the hunk.

I'll submit this patch to BundleBuggy if it seems ok.

Regards,
/Johan Dahlberg