Merge lp:~songofacandy/bzr/user_encoding_utf8 into lp:bzr/2.5

[methane]

Add ``editor_encoding`` configuration to allow writing non-ascii commit message without locale setting.

[Alexander Belchenko (bialix)]

INADA Naoki пишет:
> INADA Naoki has proposed merging lp:~songofacandy/bzr/user_encoding_utf8 into lp:bzr/2.5.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~songofacandy/bzr/user_encoding_utf8/+merge/90039
>
> * osutils.get_user_encoding() fallbacks to utf8 instead of ascii.
> * Add BZR_USER_ENCODING envvar to override user encoding.

The latter sounds great!

I've spotted small typo in your patch though:

+ if old_env is not None:
+ os.envirion['BZR_USER_ENCODING'] = old_env

should be os.environ.

But I think bzrlib.osutils already has special method to set/unset
environment variables, and in the tests you should use it instead of
manual manipulations.

--
All the dude wanted was his rug back

[methane (songofacandy)]

Thank you.
Can I use osutils' method in test for osutils?

[Alexander Belchenko (bialix)]

INADA Naoki пишет:
> Thank you.
> Can I use osutils' method in test for osutils?

Yes, why not?

--
All the dude wanted was his rug back

[Martin Pool (mbp)]

That's really nice, thanks.

It would be nice if this could also check a variable in the global configuration, so that people can set it permanently in bazaar.conf. Some people may find that easier than getting an environment variable set, especially on windows or on remote machines. But, we could come back and do that later. Doing it from config may cause some bootstrapping issues, as bzr needs to know the encoding pretty early on, probably before it loads any configuration.

[Vincent Ladeuil (vila)]

> That's really nice, thanks.
>
> It would be nice if this could also check a variable in the global
> configuration, so that people can set it permanently in bazaar.conf. Some
> people may find that easier than getting an environment variable set,
> especially on windows or on remote machines. But, we could come back and do
> that later.

Even better would be to use only a config variable since env variables are not universal as you point out. In this precise case there (use encoding) I can't think of a use case where an env variable will be more appropriate.

> Doing it from config may cause some bootstrapping issues, as bzr
> needs to know the encoding pretty early on, probably before it loads any
> configuration.

I don't think so, config files are encoded in utf-8 only and cannot be encoded into anything else.

63 + old_env = os.environ.get('BZR_USER_ENCODING')
64 + def cleanup():
65 + osutils.set_or_unset_env('BZR_USER_ENCODING', old_env)
66 + self.addCleanup(cleanup)

As mentioned by bialix, the idiom here should be:

   self.overrideEnv('BZR_USER_ENCODING', <new value>)

But since osutils.get_user cache its result, this won't work reliably you also need:

   self.overrideAttr(osutils, '_cached_user_encoding', None)

So anyway, better switch to config variables.

review: Needs Fixing

[methane (songofacandy)]

OK, I'll change to use global config.
What name is good for the option? "user_encoding" or just "encoding"?

[Martin Packman (gz)]

Treating ascii from the posix locale as utf-8 may be okay, but adding a mechanism outside locale to override this will break things I'm afraid.

This is essentially the argument python-dev were making against allowing any kind of overriding, which is overly conservative, but get_user_encoding is the way bzr finds out what actual encoding the rest of the system is using. On windows for instance, letting someone override this to a different value from what GetACP returns can corrupt the environment block and crash the process. On posix, you'd get unicode decode errors trying to interpret localised error messages, and pass incompatible bytestrings down into libraries and on to other processes.

For your original problem, of wanting to enter a utf-8 commit message in an editor when LANG=C, I think we just need better logic in that particular area. There's little reason not to just try utf-8 first, catch the error, and then try the locale encoding. I don't like that kind of heuristic much, but it will do the right thing more often than the current code.

review: Disapprove

[methane (songofacandy)]

Is there any example of test using GLobalConfig?
set_user_option() changes my bazaar.conf...

[methane (songofacandy)]

> Treating ascii from the posix locale as utf-8 may be okay, but adding a
> mechanism outside locale to override this will break things I'm afraid.

OKey, I've reverted overriding mechanism. Just use utf-8 when posix locale is ascii.

[Vincent Ladeuil (vila)]

> Is there any example of test using GLobalConfig?
> set_user_option() changes my bazaar.conf...

Your test(s) inherited from TestCase instead of TestCaseInTempDir ?

Search for GlobalStack instead of GlobalConfig and use the new config implementation (feedback on whether the doc in doc/developers/configuration.txt is highly welcome).

[lp:~songofacandy/bzr/user_encoding_utf8]

6459. By Patch Queue Manager <email address hidden> on 2012-01-27

(jelmer) Fix the default target name in 'bzr branch' when there is no
 selected colocated branch. (Jelmer Vernooij)

6460. By Patch Queue Manager <email address hidden> on 2012-01-27

(jelmer) Add utility code for opening branches with a specific policy for
 URLs to access. (Jelmer Vernooij)

6461. By Patch Queue Manager <email address hidden> on 2012-01-27

(jelmer) Fix 'bzr send' in a treeless branch. (Jelmer Vernooij)

6462. By Patch Queue Manager <email address hidden> on 2012-01-27

(jelmer) Force lightweight checkouts for colocated branches. (Jelmer
 Vernooij)

6463. By Patch Queue Manager <email address hidden> on 2012-01-27

(jelmer) Support forward slashes in colocated branch names. (Jelmer Vernooij)

6464. By Patch Queue Manager <email address hidden> on 2012-01-27

(gz) Close files more promptly in tests to avoid issues on pypy (Wouter van
 Heyst)

6465. By Patch Queue Manager <email address hidden> on 2012-01-28

(jelmer) Merge colocated branch support back into the 2a format. (Jelmer
 Vernooij)

6466. By Patch Queue Manager <email address hidden> on 2012-01-31

(jelmer) Raise NotBranchError from BzrDir.destroy_branch. (Jelmer Vernooij)

6467. By Patch Queue Manager <email address hidden> on 2012-01-31

(vila) We'll release a sixth beta before 2.5.0 (Vincent Ladeuil)

6468. By Patch Queue Manager <email address hidden> on 2012-01-31

(vila) Provide an ``ssl.ca_certs`` default value for supported platforms.
 (Vincent Ladeuil)

6469. By Patch Queue Manager <email address hidden> on 2012-02-01

(gz) Store the path to .bzr.log as unicode so it can be safely printed by
 `bzr version` (Martin Packman)

6470. By Patch Queue Manager <email address hidden> on 2012-02-02

(gz) Make config.config_dir and related functions consistenly return unicode
 (Martin Packman)

6471. By Patch Queue Manager <email address hidden> on 2012-02-03

(vila) Release bzr-2.5b6 (Vincent Ladeuil)

6472. By Patch Queue Manager <email address hidden> on 2012-02-03

(vila) Open 2.5.0 for bugfixes (second attempt) (Vincent Ladeuil)

6473. By methane on 2012-02-03

Add ``editor_encoding`` configuration to specify encoding to decode commit message from file.

6474. By methane on 2012-02-03

Add tests for editor_encoding.

[methane (songofacandy)]

I've rewote my branch to use special configuration variable and
don't change osutils.get_user_encoding().
I think this is most conservative way.

[Martin Packman (gz)]

I'm still torn on this, we do have a bunch of encoding related bugs in bazaar that have yet to be fixed, but I don't like proliferating global config options for things like file contents where we can get a good default from the OS but may get arbitrary bytes anyway. Commit messages from file are particularly fun because we may put bytes from the files as diffs which again could be whatever.

Unmerged revisions

6474. By methane on 2012-02-03

Add tests for editor_encoding.

6473. By methane on 2012-02-03

Add ``editor_encoding`` configuration to specify encoding to decode commit message from file.

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2012-01-31 15:43:17 +0000
+++ bzrlib/builtins.py	2012-02-03 17:53:19 +0000
@@ -3557,6 +3557,7 @@
         from bzrlib.msgeditor import (
             edit_commit_message_encoded,
             generate_commit_message_template,
+            get_editor_encoding,
             make_commit_message_template_encoded,
             set_commit_message,
         )
@@ -3614,7 +3615,7 @@
             if file:
                 f = open(file)
                 try:
-                    my_message = f.read().decode(osutils.get_user_encoding())
+                    my_message = f.read().decode(get_editor_encoding())
                 finally:
                     f.close()
             elif message is not None:
@@ -3623,8 +3624,7 @@
                 # No message supplied: make one up.
                 # text is the status of the tree
                 text = make_commit_message_template_encoded(tree,
-                        selected_list, diff=show_diff,
-                        output_encoding=osutils.get_user_encoding())
+                        selected_list, diff=show_diff)
                 # start_message is the template generated from hooks
                 # XXX: Warning - looks like hooks return unicode,
                 # make_commit_message_template_encoded returns user encoding.
 
=== modified file 'bzrlib/msgeditor.py'
--- bzrlib/msgeditor.py	2011-12-19 13:23:58 +0000
+++ bzrlib/msgeditor.py	2012-02-03 17:53:19 +0000
@@ -58,6 +58,14 @@
             yield editor, None
 
 
+def get_editor_encoding():
+    """Return a encoding used when editing commit message file with editor."""
+    e = config.GlobalStack().get('editor_encoding')
+    if e is not None:
+        return e
+    return osutils.get_user_encoding()
+
+
 def _run_editor(filename):
     """Try to execute an editor to edit the commit message."""
     for candidate, candidate_source in _get_editor():
@@ -110,10 +118,11 @@
 
     :return:    commit message or None.
     """
-
-    if not start_message is None:
-        start_message = start_message.encode(osutils.get_user_encoding())
-    infotext = infotext.encode(osutils.get_user_encoding(), 'replace')
+    msgencoding = get_editor_encoding()
+    if start_message is not None:
+        start_message = start_message.encode(msgencoding)
+    infotext = infotext.encode(msgencoding, 'replace')
+    # TODO: Add info to tell user which encoding is used.
     return edit_commit_message_encoded(infotext, ignoreline, start_message)
 
 
@@ -139,6 +148,7 @@
     :return:    commit message or None.
     """
     msgfilename = None
+    msgencoding = get_editor_encoding()
     try:
         msgfilename, hasinfo = _create_temp_file_with_commit_template(
                                     infotext, ignoreline, start_message)
@@ -167,7 +177,7 @@
         f = file(msgfilename, 'rU')
         try:
             try:
-                for line in codecs.getreader(osutils.get_user_encoding())(f):
+                for line in codecs.getreader(msgencoding)(f):
                     stripped_line = line.strip()
                     # strip empty line before the log message starts
                     if not started:
@@ -264,7 +274,7 @@
 
 
 def make_commit_message_template_encoded(working_tree, specific_files,
-                                         diff=None, output_encoding='utf-8'):
+                                         diff=None, output_encoding=None):
     """Prepare a template file for a commit into a branch.
 
     Returns an encoded string.
@@ -277,6 +287,8 @@
     from StringIO import StringIO       # must be unicode-safe
     from bzrlib.diff import show_diff_trees
 
+    if output_encoding is None:
+        output_encoding = get_editor_encoding()
     template = make_commit_message_template(working_tree, specific_files)
     template = template.encode(output_encoding, "replace")
 
 
=== modified file 'bzrlib/tests/test_msgeditor.py'
--- bzrlib/tests/test_msgeditor.py	2011-11-16 17:19:13 +0000
+++ bzrlib/tests/test_msgeditor.py	2012-02-03 17:53:19 +0000
@@ -267,6 +267,12 @@
             self.assertEqual(['/usr/bin/editor', 'vi', 'pico', 'nano', 'joe'],
                              editors[4:])
 
+    def test_get_editor_encoding(self):
+        conf = config.GlobalStack()
+        conf.store._load_from_string('[DEFAULT]\neditor_encoding = dummy\n')
+        conf.store.save()
+        encoding = msgeditor.get_editor_encoding()
+        self.assertEqual('dummy', encoding)
 
     def test__run_editor_EACCES(self):
         """If running a configured editor raises EACESS, the user is warned."""
@@ -345,6 +351,18 @@
         self.assertRaises(errors.BadCommitMessageEncoding,
                           msgeditor.edit_commit_message, '')
 
+    def test_configured_encoding_commit_message(self):
+        self.make_fake_editor(u'\u1234\n'.encode('utf-8'))
+        self.overrideEnv('LANG', 'C')
+        conf = config.GlobalStack()
+        conf.store._load_from_string('[DEFAULT]\neditor_encoding = utf-8')
+        conf.store.save()
+
+        working_tree = self.make_uncommitted_tree()
+
+        msg = edit_commit_message_encoded(u'\u1235'.encode("utf-8"))
+        self.assertEqual(u'\u1234\n', msg)
+
     def test_set_commit_message_no_hooks(self):
         commit_obj = commit.Commit()
         self.assertIs(None,