git.latest: Fix regression with identity file usage

Unit test included

salt/modules/git.py

@@ -6,6 +6,7 @@ from __future__ import absolute_import
 
 # Import python libs
 import copy
+import glob
 import logging
 import os
 import re
@@ -178,6 +179,28 @@ def _format_git_opts(opts):
     return _format_opts(opts)
 
 
+def _find_ssh_exe():
+    '''
+    Windows only: search for git.exe in known locations
+    '''
+    # Known locations for Git's ssh.exe in Windows
+    globmasks = [os.path.join(os.getenv('SystemDrive'), os.sep,
+                              'Program Files*', 'Git', 'usr', 'bin',
+                              'ssh.exe'),
+                 os.path.join(os.getenv('SystemDrive'), os.sep,
+                              'Program Files*', 'Git', 'bin',
+                              'ssh.exe')]
+    for globmask in globmasks:
+        ssh_exe = glob.glob(globmask)
+        if ssh_exe and os.path.isfile(ssh_exe[0]):
+            ret = ssh_exe[0]
+            break
+    else:
+        ret = None
+
+    return ret
+
+
 def _git_run(command, cwd=None, user=None, password=None, identity=None,
              ignore_retcode=False, failhard=True, redirect_stderr=False,
              saltenv='base', **kwargs):
@@ -238,18 +261,12 @@ def _git_run(command, cwd=None, user=None, password=None, identity=None,
             )
             tmp_ssh_wrapper = None
             if salt.utils.is_windows():
-                for suffix in ('', ' (x86)'):
-                    ssh_exe = (
-                        'C:\\Program Files{0}\\Git\\bin\\ssh.exe'
-                        .format(suffix)
-                    )
-                    if os.path.isfile(ssh_exe):
-                        env['GIT_SSH_EXE'] = ssh_exe
-                        break
-                else:
+                ssh_exe = _find_ssh_exe()
+                if ssh_exe is None:
                     raise CommandExecutionError(
                         'Failed to find ssh.exe, unable to use identity file'
                     )
+                env['GIT_SSH_EXE'] = ssh_exe
                 # Use the windows batch file instead of the bourne shell script
                 ssh_id_wrapper += '.bat'
                 env['GIT_SSH'] = ssh_id_wrapper
@@ -290,25 +307,33 @@ def _git_run(command, cwd=None, user=None, password=None, identity=None,
                     redirect_stderr=redirect_stderr,
                     **kwargs)
             finally:
-                # Cleanup the temporary ssh wrapper file
-                try:
-                    __salt__['file.remove'](tmp_ssh_wrapper)
-                    log.debug('Removed ssh wrapper file %s', tmp_ssh_wrapper)
-                except AttributeError:
-                    # No wrapper was used
-                    pass
-                except (SaltInvocationError, CommandExecutionError) as exc:
-                    log.warning('Failed to remove ssh wrapper file %s: %s', tmp_ssh_wrapper, exc)
+                if tmp_ssh_wrapper:
+                    # Cleanup the temporary ssh wrapper file
+                    try:
+                        __salt__['file.remove'](tmp_ssh_wrapper)
+                        log.debug('Removed ssh wrapper file %s', tmp_ssh_wrapper)
+                    except AttributeError:
+                        # No wrapper was used
+                        pass
+                    except (SaltInvocationError, CommandExecutionError) as exc:
+                        log.warning(
+                            'Failed to remove ssh wrapper file %s: %s',
+                            tmp_ssh_wrapper, exc
+                        )
 
-                # Cleanup the temporary identity file
-                try:
-                    __salt__['file.remove'](tmp_identity_file)
-                    log.debug('Removed identity file %s', tmp_identity_file)
-                except AttributeError:
-                    # No identify file was used
-                    pass
-                except (SaltInvocationError, CommandExecutionError) as exc:
-                    log.warning('Failed to remove identity file %s: %s', tmp_identity_file, exc)
+                if tmp_identity_file:
+                    # Cleanup the temporary identity file
+                    try:
+                        __salt__['file.remove'](tmp_identity_file)
+                        log.debug('Removed identity file %s', tmp_identity_file)
+                    except AttributeError:
+                        # No identify file was used
+                        pass
+                    except (SaltInvocationError, CommandExecutionError) as exc:
+                        log.warning(
+                            'Failed to remove identity file %s: %s',
+                            tmp_identity_file, exc
+                        )
 
             # If the command was successful, no need to try additional IDs
             if result['retcode'] == 0:

tests/unit/modules/test_git.py

@@ -14,6 +14,7 @@ import subprocess
 from tests.support.mixins import LoaderModuleMockMixin
 from tests.support.unit import TestCase, skipIf
 from tests.support.mock import (
+    Mock,
     MagicMock,
     patch,
     NO_MOCK,
@@ -164,3 +165,50 @@ class GitTestCase(TestCase, LoaderModuleMockMixin):
                     dict([(x, worktree_ret[x]) for x in WORKTREE_INFO
                           if WORKTREE_INFO[x].get('stale', False)])
                 )
+
+    def test__git_run_tmp_wrapper(self):
+        '''
+        When an identity file is specified, make sure we don't attempt to
+        remove a temp wrapper that wasn't created. Windows doesn't use temp
+        wrappers, and *NIX won't unless no username was specified and the path
+        is not executable.
+        '''
+        file_remove_mock = Mock()
+        mock_true = Mock(return_value=True)
+        mock_false = Mock(return_value=False)
+        cmd_mock = MagicMock(return_value={
+            'retcode': 0,
+            'stdout': '',
+            'stderr': '',
+        })
+        with patch.dict(git_mod.__salt__, {'file.file_exists': mock_true,
+                                           'file.remove': file_remove_mock,
+                                           'cmd.run_all': cmd_mock,
+                                           'ssh.key_is_encrypted': mock_false}):
+
+            # Non-windows
+            with patch('salt.utils.is_windows', mock_false), \
+                    patch.object(git_mod, '_path_is_executable_others',
+                                 mock_true):
+
+                # Command doesn't really matter here since we're mocking
+                git_mod._git_run(
+                    ['git', 'rev-parse', 'HEAD'],
+                    cwd='/some/path',
+                    user=None,
+                    identity='/root/.ssh/id_rsa')
+
+                file_remove_mock.assert_not_called()
+
+            file_remove_mock.reset_mock()
+            with patch('salt.utils.is_windows', mock_true), \
+                    patch.object(git_mod, '_find_ssh_exe',
+                                 MagicMock(return_value=r'C:\Git\ssh.exe')):
+                # Command doesn't really matter here since we're mocking
+                git_mod._git_run(
+                    ['git', 'rev-parse', 'HEAD'],
+                    cwd=r'C:\some\path',
+                    user=None,
+                    identity=r'C:\ssh\id_rsa')
+
+                file_remove_mock.assert_not_called()