[RFC] Fix Bug #220331

[Audrius Kažukauskas]

(I subscribed to the list, so hopefully this message will go through
without moderation queue and there will be no need to CC me anymore.)

On Fri, 2008-07-11 at 08:54:02 +0100, Daniel Watkins wrote:
> 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. :)

My apologies for being short on details in previous message.  I should
have mentioned that this is the first time for me to work on bazaar
code.  Also I have used bazaar only once or twice, because I am
mercurial user (though only small personal projects).

Right now I am at EuroPython sprints.  This is my first time sprinting
and I happened to choose to work on bzr.  So far it's quite a nice
experience. :-)

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

OK, I will look at this and have some thoughts.

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

There's one question that started bothering me after the commit ─ should
the test work in Windows also?  Because bzr is multi-platform project
and my test assumes Unix-like environment.

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

If I understand correctly, I should get rid of the finally part, yes?

Also what I should do now?  Fix my branch or clone the new one and make
the changes there?  I apologise for asking this, but I am unfamiliar
with the right conventions used in larger projects with lots of
developers (as I mentioned above, I use distributed source control
systems only for my personal stuff).

> Once again, thanks for working on this.

No problem, it was really fun. :-)

-- 
Audrius Kažukauskas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080711/1ee346b3/attachment.pgp