2.0.0rc2: error: Invalid value for commit message

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

Oscar Fuentes wrote:
> Public bug reported:
>
> Committing from Emacs on Windows XP produced a traceback complaining
> about the log message. The problem was the same either with utf-8 and
> windows-1252 encodings for the the log message.
>
> Committing to: D:/dev/idb-llvm/lp0/
> modified var.h
> modified var.cpp
> modified backend.cpp
> modified llvm.cpp
> aborting commit write group: ValueError("Invalid value for commit message: u'llvm-cmds branch: Nueva variable\\r\\n`defstaticDestructorDestroyer`. Necesaria porque la expresi\\xf3n\\r\\n`Commands::DataDestroyer<DefstaticDestructor>` se resolv\\xeda a una\\r\\naddress diferente en el plugin LLVM cuando se compilaba en mingw,\\r\\nproduciendo crashes en test cases que invocaban destructores de\\r\\ndefstatics.\\r\\n'",)
> bzr: ERROR: exceptions.ValueError: Invalid value for commit message: u'llvm-cmds branch: Nueva variable\r\n`defstaticDestructorDestroyer`. Necesaria porque la expresi\xf3n\r\n`Commands::DataDestroyer<DefstaticDestructor>` se resolv\xeda a una\r\naddress diferente en el plugin LLVM cuando se compilaba en mingw,\r\nproduciendo crashes en test cases que invocaban destructores de\r\ndefstatics.\r\n'
>
> Traceback (most recent call last):
> File "bzrlib\commands.pyo", line 842, in exception_to_return_code
> File "bzrlib\commands.pyo", line 1037, in run_bzr
> File "bzrlib\commands.pyo", line 654, in run_argv_aliases
> File "bzrlib\builtins.pyo", line 3051, in run
> File "bzrlib\decorators.pyo", line 192, in write_locked
> File "bzrlib\workingtree_4.pyo", line 197, in commit
> File "bzrlib\decorators.pyo", line 192, in write_locked
> File "bzrlib\mutabletree.pyo", line 228, in commit
> File "bzrlib\commit.pyo", line 375, in commit
> File "bzrlib\repository.pyo", line 163, in commit
> File "bzrlib\repository.pyo", line 146, in _validate_unicode_text
> ValueError: Invalid value for commit message: u'llvm-cmds branch: Nueva variable\r\n`defstaticDestructorDestroyer`. Necesaria porque la expresi\xf3n\r\n`Commands::DataDestroyer<DefstaticDestructor>` se resolv\xeda a una\r\naddress diferente en el plugin LLVM cuando se compilaba en mingw,\r\nproduciendo crashes en test cases que invocaban destructores de\r\ndefstatics.\r\n'
>

^- My guess is that the '\r\n' is being caught and rejected. We prefer
to have regular "\n" line endings. I'm not sure what code is passing
these in that isn't sanitizing them. Are you using a special emacs plugin?

John
=:->

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

iEYEARECAAYFAkq3BM8ACgkQJdeBCYSNAAPRfACghUWCj4TVd99OqPYwLYBtk6g4
/xcAn2YsE/2JT1gwzHug7tALbtGS26UD
=LMjG
-----END PGP SIGNATURE-----

I'm using VC on emacs 23.1.50.1. The \r\n line endings on the traceback persist even when I set the buffer encoding to utf-8-unix or windos1252-unix.

\r\n is the line ending native to Windows. Usually, text editors and IDEs use it. It would be a good thing for bzr to accept log messages with embedded \r\n on Windows.

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

Oscar Fuentes wrote:
> I'm using VC on emacs 23.1.50.1. The \r\n line endings on the traceback
> persist even when I set the buffer encoding to utf-8-unix or
> windos1252-unix.
>
> \r\n is the line ending native to Windows. Usually, text editors and
> IDEs use it. It would be a good thing for bzr to accept log messages
> with embedded \r\n on Windows.
>

My question is in how it gets introduced into the system.

If you are doing "bzr commit -F filename.log" we open that file in text
mode, so it will automatically translate \r\n => \n.

If you are doing a bare "bzr commit" and we are launching emacs as the
editor, then we use:
        f = file(msgfilename, 'rU')

Which *also* translates the line endings.

Now potentially if you were using:

 "bzr commit -m "argument\r\nwith\r\nnewlines"
we may not be translating them from the command line.

Alternatively, if the emacs interface is manually calling
"tree.commit()" it might be passing things in differently.

I'm trying to track down what path these text is being inserted, so that
we can sort out why they aren't getting translated like they do with all
the other paths.

John
=:->

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

iEYEARECAAYFAkq3lDAACgkQJdeBCYSNAAOMhgCfS1iEOApwunaQ69D54NTjyRJr
XpsAn24Wt/Q2iZE8mLFGzr9D9wDV1USt
=O3r5
-----END PGP SIGNATURE-----

John - its '-m' in the arguments:
arguments: ['c:\\apps\\Bazaar\\bzr.exe', 'commit', '-m', 'llvm-cmds branch: Nueva variable\r\n`defstaticDestructorDestroyer`. Necesaria porque la expresi\xf3n\r\n`Commands::DataDestroyer<DefstaticDestructor>` se resolv\xeda a una\r\naddress diferente en el plugin LLVM cuando se compilaba en mingw,\r\nproduciendo crashes en test cases que invocaban destructores de\r\ndefstatics.\r\n', 'backend.cpp', 'llvm.cpp', 'var.cpp', 'var.h']

So you can see it has \r\n in the -m 'foo' string.

As Robert says, the exception is thrown when -m is used to pass the commit message. With -F, using the very same commit message (including /r/n) there is no problem.

So I guess the solution is to translate the line endings for -m too.

Any idea about how to fix this? If bzr does not allow commit messages with \r\n line endings internally, maybe they should be removed on the `commit' method of commit.py. Sadly my python knowledge is practically inexistent, nor I have the slightest idea about how to execute bzr from source, but if you agree that this is the rigth thing, I could make an attempt at creating a patch.

This bug prevents using bzr with Emacs on Windows.

IMO, the proper place to fix message line-endings is in builtins.py cmd_commit.run. Maybe as part of arguments parsing, i.e. to provide custom function in Option('message',...) instead of unicode.

The fix can be trivial, but the test for it can be problematic.

Here is simple patch that fixes this problem for you:

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2009-10-29 05:49:32 +0000
+++ bzrlib/builtins.py 2009-11-03 16:40:01 +0000
@@ -3038,6 +3038,9 @@
             elif my_message and file:
                 raise errors.BzrCommandError(
                     "please specify either --message or --file")
+ elif my_message and sys.platform == 'win32':
+ # sanitize message on Windows (see bug #433779)
+ my_message = my_message.replace('\r\n', '\n')
             if file:
                 my_message = codecs.open(file, 'rt',
                                          osutils.get_user_encoding()).read()

The fix that is landing in bzr.dev for 2.1.0b3 is viable to be merged into 2.0.3 (was built on the 2.0 series). We just have to decide if this is really a pure 'bugfix' or a feature enhancement.

I'm using bzr-2.1.1 from the bzr PPA, on Ubuntu 9.10. My understanding is that this version should include the fix for this bug. However, when using "commit -F", if the supplied file contains DOS newlines, bzr aborts the commit with the error:

aborting commit write group: ValueError("Invalid value for commit message: u'a\\r\\nnewline\\r\\n\\r\\n'",)
bzr: ERROR: exceptions.ValueError: Invalid value for commit message: u'a\r\nnewline\r\n\r\n'

This is slightly different than the original described bug, in that the commit message is in a file, not passed on the command line. However, it sounds from the comments like all the commit message input sources are supposed to be filtered.

Crash file for my comment (#11)

Apparently we decided not to land this in the 2.0 series (at least looking at the code, the '.replace()' is not present even in 2.0.3)

I believe we had a different fix for 'bzr commit -F' which was that we open the file in text mode, rather than in binary mode. However, if you are on Linux, text mode reads \r\n without changing them. (So writing DOS mode lines on linux will try to add them as DOS lines.)