Merge lp:~doxxx/bzr/bug939605 into lp:bzr

[Gordon Tyler]

Fixes bug 939605 by updating osutils.find_executable_on_path to additionally search the Windows App Path registry.

The check_availability and invoke functions in mergetools use a small utility function, _get_executable_path, to get the full path to the executable in the mergetool commandline, which skips the PATH search if the path is already absolute.

[Alexander Belchenko (bialix)]

Looks OK for me, though I can't comment on your change to existing test. Maybe second voice from core dev is needed. I can say that I've tested it and it works. Thank you for fixing it so fast.

Also, I'd like to have this patch backported to bzr 2.5.1, because it clearly fixed bug and don't change anything public.

And you need NEWS message, I guess ;-)

review: Approve

[Gordon Tyler (doxxx)]

Darn NEWS...

[Gordon Tyler (doxxx)]

Release notes updated.

[Martin Packman (gz)]

Wouldn't it be better to fold the logic you've added in _get_executable_path into osutils.find_executable_on_path? If we need a look-before-you-leap executable finder, we may as well have a single one that does the right thing rather than needing extra special cases on top. I wouldn't worry about that making the '_on_path' naming a little misleading, it's hardly the biggest problem is osutils.

For the test you could override the PATH envvar for the duration of the test. Just %SystemRoot% should do.

review: Needs Information

[Gordon Tyler (doxxx)]

Since Win32's ShellExecute function searches the App Path, it makes sense to include it in osutils.find_executable_on_path.

That's a good point about the SystemRoot env var. Since '%SystemRoot\system32' is guaranteed to be in the PATH, I can use something like cmd.exe to test that and use the env var to construct the path to test against.

Thanks.

[Gordon Tyler (doxxx)]

Updated according to Martin's suggestions.

[Martin Pool (mbp)]

looks reasonable to me too, and +1 more to putting this in 2.5

review: Approve

[Martin Packman (gz)]

Superseded by proposal targeted at 2.5:

<https://code.launchpad.net/~doxxx/bzr/bug939605-2.5/+merge/94909>

Unmerged revisions

6481. By Gordon Tyler on 2012-02-27

Re-removed unnecessary import.

6480. By Gordon Tyler on 2012-02-27

Improved readablity of check_availability.

6479. By Gordon Tyler on 2012-02-27

Removed unnecessary import.

6478. By Gordon Tyler on 2012-02-27

Updated mergetools._get_executable_path and improved test_invoke_expands_exe_path.

6477. By Gordon Tyler on 2012-02-27

Changed osutils.find_executable_on_path to search using win32utils.get_app_path if the executable is not found using the PATH env var.

6476. By Gordon Tyler on 2012-02-24

Updated release notes.

6475. By Gordon Tyler on 2012-02-24

Added _get_executable_path function to mergetools module, which searches the PATH and App Path registry on win32.
Changed check_availability and invoke to use _get_executable_path.
Updated tests.

=== modified file 'bzrlib/mergetools.py'
--- bzrlib/mergetools.py	2011-12-19 13:23:58 +0000
+++ bzrlib/mergetools.py	2012-02-27 17:50:24 +0000
@@ -48,13 +48,13 @@
     cmd_list = cmdline.split(command_line)
     exe = cmd_list[0]
     if sys.platform == 'win32':
-        if os.path.isabs(exe):
-            base, ext = os.path.splitext(exe)
-            path_ext = [unicode(s.lower())
-                        for s in os.getenv('PATHEXT', '').split(os.pathsep)]
-            return os.path.exists(exe) and ext in path_ext
-        else:
-            return osutils.find_executable_on_path(exe) is not None
+        exe = _get_executable_path(exe)
+        if exe is None:
+            return False
+        base, ext = os.path.splitext(exe)
+        path_ext = [unicode(s.lower())
+                    for s in os.getenv('PATHEXT', '').split(os.pathsep)]
+        return os.path.exists(exe) and ext in path_ext
     else:
         return (os.access(exe, os.X_OK)
                 or osutils.find_executable_on_path(exe) is not None)
@@ -69,6 +69,9 @@
     if invoker is None:
         invoker = subprocess_invoker
     cmd_list = cmdline.split(command_line)
+    exe = _get_executable_path(cmd_list[0])
+    if exe is not None:
+        cmd_list[0] = exe
     args, tmp_file = _subst_filename(cmd_list, filename)
     def cleanup(retcode):
         if tmp_file is not None:
@@ -79,6 +82,12 @@
     return invoker(args[0], args[1:], cleanup)
 
 
+def _get_executable_path(exe):
+    if os.path.isabs(exe):
+        return exe
+    return osutils.find_executable_on_path(exe)
+
+
 def _subst_filename(args, filename):
     subst_names = {
         'base': filename + u'.BASE',

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2012-02-01 12:31:53 +0000
+++ bzrlib/osutils.py	2012-02-27 17:50:24 +0000
@@ -2473,10 +2473,6 @@
     :param name: The base name of the executable.
     :return: The path to the executable found or None.
     """
-    path = os.environ.get('PATH')
-    if path is None:
-        return None
-    path = path.split(os.pathsep)
     if sys.platform == 'win32':
         exts = os.environ.get('PATHEXT', '').split(os.pathsep)
         exts = [ext.lower() for ext in exts]
@@ -2488,11 +2484,18 @@
             exts = [ext]
     else:
         exts = ['']
-    for ext in exts:
-        for d in path:
-            f = os.path.join(d, name) + ext
-            if os.access(f, os.X_OK):
-                return f
+    path = os.environ.get('PATH')
+    if path is not None:
+        path = path.split(os.pathsep)
+        for ext in exts:
+            for d in path:
+                f = os.path.join(d, name) + ext
+                if os.access(f, os.X_OK):
+                    return f
+    if sys.platform == 'win32':
+        app_path = win32utils.get_app_path(name)
+        if app_path != name:
+            return app_path
     return None
 
 

=== modified file 'bzrlib/tests/test_mergetools.py'
--- bzrlib/tests/test_mergetools.py	2011-06-30 21:00:38 +0000
+++ bzrlib/tests/test_mergetools.py	2012-02-27 17:50:24 +0000
@@ -81,11 +81,7 @@
         self.assertTrue(mergetools.check_availability(sys.executable))
 
     def test_exe_on_path(self):
-        if sys.platform == 'win32':
-            exe = 'cmd.exe'
-        else:
-            exe = 'sh'
-        self.assertTrue(mergetools.check_availability(exe))
+        self.assertTrue(mergetools.check_availability('python'))
 
     def test_nonexistent(self):
         self.assertFalse(mergetools.check_availability('DOES NOT EXIST'))
@@ -112,6 +108,19 @@
             ('test.txt.OTHER', 'other stuff'),
         ))
         
+    def test_invoke_expands_exe_path(self):
+        self.overrideEnv('PATH', os.path.dirname(sys.executable))
+        def dummy_invoker(exe, args, cleanup):
+            self._exe = exe
+            self._args = args
+            cleanup(0)
+            return 0
+        command = '%s {result}' % os.path.basename(sys.executable)
+        retcode = mergetools.invoke(command, 'test.txt', dummy_invoker)
+        self.assertEqual(0, retcode)
+        self.assertEqual(sys.executable, self._exe)
+        self.assertEqual(['test.txt'], self._args)
+        
     def test_success(self):
         def dummy_invoker(exe, args, cleanup):
             self._exe = exe

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2012-02-01 12:32:26 +0000
+++ bzrlib/tests/test_osutils.py	2012-02-27 17:50:24 +0000
@@ -2225,6 +2225,14 @@
         self.assertTrue(
             osutils.find_executable_on_path('THIS SHOULD NOT EXIST') is None)
         self.assertTrue(osutils.find_executable_on_path('file.txt') is None)
+        
+    def test_windows_app_path(self):
+        if sys.platform != 'win32':
+            raise tests.TestSkipped('test requires win32')
+        # Override PATH env var so that exe can only be found on App Path
+        self.overrideEnv('PATH', '')
+        # Internt Explorer is always registered in the App Path
+        self.assertTrue(osutils.find_executable_on_path('iexplore') is not None)
 
     def test_other(self):
         if sys.platform == 'win32':

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-02-25 14:13:19 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-02-27 17:50:24 +0000
@@ -47,6 +47,9 @@
 * Fix ``bzr config`` display for ``RegistryOption`` values.
   (Vincent Ladeuil, #930182)
 
+* Fixed merge tool availability checking and invocation to search the
+  Windows App Path registry in addition to the PATH. (Gordon Tyler, #939605)
+
 Documentation
 *************