Fail git.latest states with uncommitted changes when force_reset=False (#34869)

* Add git.diff function

* Fail git.latest states with uncommitted changes when force_reset=False

Also, discard these changes when running the state if force_reset=True.

* Add integration test for case where there are uncommitted changes

salt/modules/git.py

@@ -1360,6 +1360,145 @@ def describe(cwd, rev='HEAD', user=None, ignore_retcode=False):
                     ignore_retcode=ignore_retcode)['stdout']
 
 
+def diff(cwd,
+         item1=None,
+         item2=None,
+         opts='',
+         user=None,
+         no_index=False,
+         cached=False,
+         paths=None):
+    '''
+    .. versionadded:: 2015.8.12,2016.3.3
+
+    Interface to `git-diff(1)`_
+
+    cwd
+        The path to the git checkout
+
+    item1 and item2
+        Revision(s) to pass to the ``git diff`` command. One or both of these
+        arguments may be ignored if some of the options below are set to
+        ``True``. When ``cached`` is ``False``, and no revisions are passed
+        to this function, then the current working tree will be compared
+        against the index (i.e. unstaged changes). When two revisions are
+        passed, they will be compared to each other.
+
+    opts
+        Any additional options to add to the command line, in a single string
+
+        .. note::
+            On the Salt CLI, if the opts are preceded with a dash, it is
+            necessary to precede them with ``opts=`` (as in the CLI examples
+            below) to avoid causing errors with Salt's own argument parsing.
+
+    user
+        User under which to run the git command. By default, the command is run
+        by the user under which the minion is running.
+
+    no_index : False
+        When it is necessary to diff two files in the same repo against each
+        other, and not diff two different revisions, set this option to
+        ``True``. If this is left ``False`` in these instances, then a normal
+        ``git diff`` will be performed against the index (i.e. unstaged
+        changes), and files in the ``paths`` option will be used to narrow down
+        the diff output.
+
+        .. note::
+            Requires Git 1.5.1 or newer. Additionally, when set to ``True``,
+            ``item1`` and ``item2`` will be ignored.
+
+    cached : False
+        If ``True``, compare staged changes to ``item1`` (if specified),
+        otherwise compare them to the most recent commit.
+
+        .. note::
+            ``item2`` is ignored if this option is is set to ``True``.
+
+    paths
+        File paths to pass to the ``git diff`` command. Can be passed as a
+        comma-separated list or a Python list.
+
+    .. _`git-diff(1)`: http://git-scm.com/docs/git-diff
+
+
+    CLI Example:
+
+    .. code-block:: bash
+
+        # Perform diff against the index (staging area for next commit)
+        salt myminion git.diff /path/to/repo
+        # Compare staged changes to the most recent commit
+        salt myminion git.diff /path/to/repo cached=True
+        # Compare staged changes to a specific revision
+        salt myminion git.diff /path/to/repo mybranch cached=True
+        # Perform diff against the most recent commit (includes staged changes)
+        salt myminion git.diff /path/to/repo HEAD
+        # Diff two commits
+        salt myminion git.diff /path/to/repo abcdef1 aabbccd
+        # Diff two commits, only showing differences in the specified paths
+        salt myminion git.diff /path/to/repo abcdef1 aabbccd paths=path/to/file1,path/to/file2
+        # Diff two files with one being outside the working tree
+        salt myminion git.diff /path/to/repo no_index=True paths=path/to/file1,/absolute/path/to/file2
+    '''
+    if no_index and cached:
+        raise CommandExecutionError(
+            'The \'no_index\' and \'cached\' options cannot be used together'
+        )
+
+    command = ['git', 'diff']
+    command.extend(_format_opts(opts))
+
+    if paths is not None and not isinstance(paths, (list, tuple)):
+        try:
+            paths = paths.split(',')
+        except AttributeError:
+            paths = str(paths).split(',')
+
+    ignore_retcode = False
+    failhard = True
+
+    if no_index:
+        if _LooseVersion(version(versioninfo=False)) < _LooseVersion('1.5.1'):
+            raise CommandExecutionError(
+                'The \'no_index\' option is only supported in Git 1.5.1 and '
+                'newer'
+            )
+        ignore_retcode = True
+        failhard = False
+        command.append('--no-index')
+        for value in [x for x in (item1, item2) if x]:
+            log.warning(
+                'Revision \'%s\' ignored in git diff, as revisions cannot be '
+                'used when no_index=True', value
+            )
+
+    elif cached:
+        command.append('--cached')
+        if item1:
+            command.append(item1)
+        if item2:
+            log.warning(
+                'Second revision \'%s\' ignored in git diff, at most one '
+                'revision is considered when cached=True', item2
+            )
+
+    else:
+        for value in [x for x in (item1, item2) if x]:
+            command.append(value)
+
+    if paths:
+        command.append('--')
+        command.extend(paths)
+
+    return _git_run(command,
+                    cwd=cwd,
+                    runas=user,
+                    ignore_retcode=ignore_retcode,
+                    failhard=failhard,
+                    redirect_stderr=True)['stdout']
+
+
 def fetch(cwd,
           remote=None,
           force=False,

salt/states/git.py

@@ -177,17 +177,24 @@ def _failed_submodule_update(ret, exc, comments=None):
     return _fail(ret, msg, comments)
 
 
-def _not_fast_forward(ret, pre, post, branch, local_branch, comments):
+def _not_fast_forward(ret, pre, post, branch, local_branch,
+                      local_changes, comments):
+    pre = _short_sha(pre)
+    post = _short_sha(post)
     return _fail(
         ret,
-        'Repository would be updated from {0} to {1}{2}, but this is not a '
+        'Repository would be updated {0}{1}, but {2}. Set \'force_reset\' to '
-        'fast-forward merge. Set \'force_reset\' to True to force this '
+        'True to force this update{3}.'.format(
-        'update.'.format(
+            'from {0} to {1}'.format(pre, post)
-            _short_sha(pre),
+                if local_changes and pre != post
-            _short_sha(post),
+                else 'to {0}'.format(post),
             ' (after checking out local branch \'{0}\')'.format(branch)
                 if _need_branch_change(branch, local_branch)
-                else ''
+                else '',
+            'this is not a fast-forward merge'
+                if not local_changes
+                else 'there are uncommitted changes',
+            ' and discard these changes' if local_changes else ''
         ),
         comments
     )
@@ -700,6 +707,19 @@ def latest(name,
                                               redact_auth=False)
 
             revs_match = _revs_equal(local_rev, remote_rev, remote_rev_type)
+            try:
+                local_changes = bool(
+                    __salt__['git.diff'](target, 'HEAD', user=user)
+                )
+            except CommandExecutionError:
+                # No need to capture the error and log it, the _git_run()
+                # helper in the git execution module will have already logged
+                # the output from the command.
+                log.warning(
+                    'git.latest: Unable to determine if %s has local changes',
+                    target
+                )
+                local_changes = False
 
             if remote_rev_type == 'sha1' \
                     and base_rev is not None \
@@ -789,13 +809,15 @@ def latest(name,
                         elif remote_rev_type == 'sha1':
                             has_remote_rev = True
 
-            if not has_remote_rev:
+            # If has_remote_rev is False, then either the remote rev could not
-                # Either the remote rev could not be found with git
+            # be found with git ls-remote (in which case we won't know more
-                # ls-remote (in which case we won't know more until
+            # until fetching) or we're going to be checking out a new branch
-                # fetching) or we're going to be checking out a new branch
+            # and don't have to worry about fast-forwarding. So, we will set
-                # and don't have to worry about fast-forwarding.
+            # fast_forward to None (to signify uncertainty) unless there are
-                fast_forward = None
+            # local changes, in which case we will set it to False.
-            else:
+            fast_forward = None if not local_changes else False
+
+            if has_remote_rev:
                 # Remote rev already present
                 if (not revs_match and not update_head) \
                         and (branch is None or branch == local_branch):
@@ -809,14 +831,18 @@ def latest(name,
                     )
                     return ret
 
+            # No need to check if this is a fast_forward if we already know
+            # that it won't be (due to local changes).
+            if fast_forward is not False:
                 if base_rev is None:
                     # If we're here, the remote_rev exists in the local
                     # checkout but there is still no HEAD locally. A possible
                     # reason for this is that an empty repository existed there
                     # and a remote was added and fetched, but the repository
                     # was not fast-forwarded. Regardless, going from no HEAD to
-                    # a locally-present rev is considered a fast-forward update.
+                    # a locally-present rev is considered a fast-forward
-                    fast_forward = True
+                    # update, unless there are local changes.
+                    fast_forward = not bool(local_changes)
                 else:
                     fast_forward = __salt__['git.merge_base'](
                         target,
@@ -833,6 +859,7 @@ def latest(name,
                         remote_rev,
                         branch,
                         local_branch,
+                        local_changes,
                         comments)
                 merge_action = 'hard-reset'
             elif fast_forward is True:
@@ -1122,11 +1149,10 @@ def latest(name,
                             remote_rev,
                             branch,
                             local_branch,
+                            local_changes,
                             comments)
 
                 if _need_branch_change(branch, local_branch):
-                    local_changes = __salt__['git.status'](target,
-                                                           user=user)
                     if local_changes and not force_checkout:
                         return _fail(
                             ret,
@@ -1168,6 +1194,9 @@ def latest(name,
                             '\'{0}\' was checked out'.format(checkout_rev)
                         )
 
+                if local_changes:
+                    comments.append('Local changes were discarded')
+
                 if fast_forward is False:
                     __salt__['git.reset'](
                         target,

tests/integration/states/git.py

@@ -150,6 +150,53 @@ class GitTest(integration.ModuleCase, integration.SaltReturnAssertsMixIn):
         finally:
             shutil.rmtree(name, ignore_errors=True)
 
+    def test_latest_with_local_changes(self):
+        '''
+        Ensure that we fail the state when there are local changes and succeed
+        when force_reset is True.
+        '''
+        name = os.path.join(integration.TMP, 'salt_repo')
+        try:
+            # Clone repo
+            ret = self.run_state(
+                'git.latest',
+                name='https://{0}/saltstack/salt-test-repo.git'.format(self.__domain),
+                target=name
+            )
+            self.assertSaltTrueReturn(ret)
+            self.assertTrue(os.path.isdir(os.path.join(name, '.git')))
+
+            # Make change to LICENSE file.
+            with salt.utils.fopen(os.path.join(name, 'LICENSE'), 'a') as fp_:
+                fp_.write('Lorem ipsum dolor blah blah blah....\n')
+
+            # Make sure that we now have uncommitted changes
+            self.assertTrue(self.run_function('git.diff', [name, 'HEAD']))
+
+            # Re-run state with force_reset=False, this should fail
+            ret = self.run_state(
+                'git.latest',
+                name='https://{0}/saltstack/salt-test-repo.git'.format(self.__domain),
+                target=name,
+                force_reset=False
+            )
+            self.assertSaltFalseReturn(ret)
+
+            # Now run the state with force_reset=True, this should succeed
+            ret = self.run_state(
+                'git.latest',
+                name='https://{0}/saltstack/salt-test-repo.git'.format(self.__domain),
+                target=name,
+                force_reset=True
+            )
+            self.assertSaltTrueReturn(ret)
+
+            # Make sure that we no longer have uncommitted changes
+            self.assertFalse(self.run_function('git.diff', [name, 'HEAD']))
+
+        finally:
+            shutil.rmtree(name, ignore_errors=True)
+
     def test_present(self):
         '''
         git.present