git.latest fails on non-fast-forward when a fast-forward is possible

[tdenny]

Description of Issue/Question

My git.latest state is failing with
Repository would be updated to 08d6ca9, but this is not a fast-forward merge. Set 'force_reset' to True to force this update.
However, this merge is a fast-forward merge. ie. using native git allows fast-forward.

I'm fairly certain this is a new issue in 2016.3.2 and related to the improvements in #34869.

Setup

The state failing:

pull-latest-source:
  git.latest:
    - name: git@bitbucket.org:myrepo.git
    - target: /srv/repos/myrepo
    - identity: /root/.ssh/deployment-key
    - force_fetch: True
    - require:
      - file: deployment-key

Steps to Reproduce Issue

Set up state above, execute once to create local repo, and execute again after a new (fast-forward merge-able) commit to the remote repo.

[INFO    ] Running state [git@bitbucket.org:myrepo.git] at time 18:50:00.331401
[INFO    ] Executing state git.latest for git@bitbucket.org:myrepo.git
[INFO    ] Checking remote revision for git@bitbucket.org:myrepo.git
[INFO    ] Attempting git authentication using identity file /root/.ssh/deployment-key
[INFO    ] Executing command ['git', 'ls-remote', 'git@bitbucket.org:myrepo.git'] in directory '/root'
[INFO    ] Executing command ['git', 'for-each-ref', '--format', '%(refname:short)', 'refs/heads/'] in directory '/srv/repos/myrepo'
[INFO    ] Executing command ['git', 'for-each-ref', '--format', '%(refname:short)', 'refs/tags/'] in directory '/srv/repos/myrepo'
[INFO    ] Checking local revision for /srv/repos/myrepo
[INFO    ] Executing command ['git', 'rev-parse', 'HEAD'] in directory '/srv/repos/myrepo'
[INFO    ] Checking local branch for /srv/repos/myrepo
[INFO    ] Executing command ['git', 'rev-parse', '--abbrev-ref', 'HEAD'] in directory '/srv/repos/myrepo'
[INFO    ] Executing command ['git', 'remote', '--verbose'] in directory '/srv/repos/myrepo'
[INFO    ] Executing command ['git', 'diff', 'HEAD'] in directory '/srv/repos/myrepo'
[INFO    ] Executing command ['git', 'rev-parse', '08d6ca93efe97c0647000222e28710700e008068^{commit}'] in directory '/srv/repos/myrepo'
[INFO    ] Executing command ['git', 'merge-base', '--is-ancestor', 'a169d0122073f679e46bd7a66cda8e1c95df4a18', '08d6ca93efe97c0647000222e28710700e008068'] in directory '/srv/repos/myrepo'
[ERROR   ] Repository would be updated to 08d6ca9, but this is not a fast-forward merge. Set 'force_reset' to True to force this update.
[INFO    ] Completed state [git@bitbucket.org:myrepo.git] at time 18:50:02.602905 duration_in_ms=2271.504

Versions Report

Salt Version:
           Salt: 2016.3.2

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.7
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.5 (default, Nov 20 2015, 02:00:19)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.2.1511 Core
        machine: x86_64
        release: 3.10.0-327.10.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.2.1511 Core

Git Version:
        1.8.3.1

[tdenny]

I think that git merge-base call (https://github.com/saltstack/salt/blob/v2016.3.2/salt/states/git.py#L877) is returning false because we haven't fetched from the remote yet.
Manually executing git fetch on the local repo, then re-salting works.

Moreover, executing salt-call git.merge_base /srv/repos/myrepo/ '[a169d01,08d6ca9]' is_ancestor=True gives

[INFO    ] Executing command ['git', '--version'] in directory '/root'
[INFO    ] Executing command ['git', 'merge-base', '--is-ancestor', 'a169d01', '08d6ca9'] in directory '/srv/repos/myrepo/'
[ERROR   ] Command '['git', 'merge-base', '--is-ancestor', 'a169d01', '08d6ca9']' failed with return code: 128
[ERROR   ] stderr: fatal: Not a valid object name 08d6ca9
[ERROR   ] retcode: 128
local:
    False

[Ch3LL]

ping @terminalmage can you give any further insight into this issue.

I attempted to replicate this issue using:

pull-latest-source:
  git.latest:
    - name: git@github.com:Ch3LL/Ch3LLScripts.git
    - target: /tmp/git/
    - identity: /home/ch3ll/.ssh/id_rsa
    - force_fetch: True

1. ran sls initially
2. edited a file in repo
3. re-ran sls file
4. edited the same file/line
5. re-ran sls file and it works each time and i'm seeing in the logs:

2016-08-05 13:32:43,947 [salt.loaded.int.module.cmdmod][DEBUG   ][2797] stdout: Updating 5e28003..0807786
Fast-forward

I'm not sure if my test case is incorrect or if @tdenny 's use case is different from mine. Thanks

[terminalmage]

I was able to reproduce. I'll get a fix in for this later this evening.

[terminalmage]

OK, I decided to write an integration test first, to ensure that it failed in the right way before I get my fix added. Moving on to the fix now.

[terminalmage]

Fixed in #35249, integration test included to keep this from regressing in the future. Man, I'm really sorry about the inconvenience. 😦

[tdenny]

Thanks for the help! Much appreciated.

[terminalmage]

No problem, thanks for bringing this to my attention. The kicker is that I wrote integration tests for the change that I made a few weeks ago, only to have that pull request cause this bug because I didn't have a test case for it. At least now I have a test case though. 😄