[RFC] Fix Bug #220331

[Daniel Watkins]

Hi Audrius,

On Thu, 10 Jul 2008 15:23:50 +0300
Audrius Kažukauskas <neobug at tornado.ktu.lt> wrote:
> I fixed bug #220331.  Could you please take a look at the fix?
Thanks for taking the time to work on this, it's very much
appreciated. :)

> === modified file 'bzrlib/msgeditor.py'
> --- bzrlib/msgeditor.py	2008-04-22 09:58:15 +0000
> +++ bzrlib/msgeditor.py	2008-07-10 12:13:53 +0000
> @@ -56,7 +56,7 @@
>  def _run_editor(filename):
>      """Try to execute an editor to edit the commit message."""
>      for e in _get_editor():
> -        edargs = e.split(' ')
> +        edargs = bzrlib.commands.shlex_split_unicode(e)
>          try:
>              ## mutter("trying editor: %r", (edargs +[filename]))
>              x = call(edargs + [filename])
One thing I would say here is that when Tom Tobin previously attempted
a fix, only used shlex_split_unicode if 'e' was of type 'unicode' (as
seen at http://tinyurl.com/5lefyq).  I don't know if this is necessary,
as encoding stuff makes my head hurt, but I thought you should be aware
of it.

> === modified file 'bzrlib/tests/test_msgeditor.py'
> --- bzrlib/tests/test_msgeditor.py	2008-06-06 13:30:30 +0000
> +++ bzrlib/tests/test_msgeditor.py	2008-07-10 12:13:53 +0000
> @@ -242,6 +242,43 @@
>              else:
>                  os.environ['EDITOR'] = editor
>  
> +    def test__run_editor(self):
> +        # Test if _run_editor can handle path to editor containing
> spaces
> +        bzr_editor = os.environ.get('BZR_EDITOR')
> +        visual = os.environ.get('VISUAL')
> +        editor = os.environ.get('EDITOR')
> +        try:
> +            os.environ['BZR_EDITOR'] = '/tmp/true\ with\ spaces'
> +            os.environ['VISUAL'] = '/usr/bin/touch'
> +            os.environ['EDITOR'] = 'editor'
> +            if os.path.exists('/tmp/true with spaces'):
> +                os.unlink('/tmp/true with spaces')
> +            if os.path.exists('/tmp/foobar'):
> +                os.unlink('/tmp/foobar')
> +            os.symlink('/usr/bin/true', '/tmp/true with spaces')
> +            tmpfile = '/tmp/foobar'
> +            msgeditor._run_editor(tmpfile)
> +            # There shouldn't be any /tmp/foobar
> +            self.assertFalse(os.path.exists('/tmp/foobar'))
The test case which this test is part of subclasses
bzrlib.tests.TestCaseWithTransport.  This has the advantage that any
files you create are separated from other tests, and the rest of the
system.  As such, putting files in the current directory rather than
in /tmp is vastly preferable.  For example,
  os.environ['BZR_EDITOR'] = '/tmp/true\ with\ spaces'
would change to:
  os.environ['BZR_EDITOR'] = 'true\ with\ spaces'

This allows TestCaseWithTransport to clean up after you, as well as
avoiding messing with any user files in /tmp.

> +        finally:
> +            # Restore the environment
> +            if bzr_editor is None:
> +                del os.environ['BZR_EDITOR']
> +            else:
> +                os.environ['BZR_EDITOR'] = bzr_editor
> +            if visual is None:
> +                del os.environ['VISUAL']
> +            else:
> +                os.environ['VISUAL'] = visual
> +            if editor is None:
> +                del os.environ['EDITOR']
> +            else:
> +                os.environ['EDITOR'] = editor
TestCaseWithTransport will also take care of restoring the environment
to its previous state, so you don't need to do this. :)

> +            # Clean-up the mess
> +            os.unlink('/tmp/true with spaces')
> +            if os.path.exists('/tmp/foobar'):
> +                os.unlink('/tmp/foobar')
If there are changed to be within the test directory, then
TestCaseWithTransport will also take care of getting rid of them when
the test is complete.

Once again, thanks for working on this.

Regards,
-- 
Daniel Watkins (Odd_Bloke)