Salt: git.latest failing unless force_reset: True

[twellspring]

Description of Issue/Question

git.latest is failing with error "Repository would be updated from xxxxxxx to yyyyyyy, but this is not a fast-forward merge. Set 'force_reset' to True to force this update"

Might be related to #27487

Setup

Running this state

deploy_app:
  git.latest:
    - name: "{{ app_data.repo }}"
    - user: {{ app_data.user }}
    - target: {{ app_data.install_dir }}
    - rev: {{ app_data.version }}
    - identity: {{ app_data.home_dir }}/.ssh/deployment_key

and deploying to a target that already exists as a git repo causes this output

          ID: deploy_app
    Function: git.latest
        Name: [email protected]:xxxxxx/xxxxxxxxx.git
      Result: False
     Comment: Repository would be updated from xxxxxxx to yyyyyyy , but this is not a fast-forward merge. Set 'force_reset' to True to force this update.
     Started: 18:19:57.904808
    Duration: 623.057 ms
     Changes:
----------

Steps to Reproduce Issue

(Include debug logs if possible and relevant.)

Can reproduce using various branches. Does not appear to have any relationship to the actual branch being switched from/to, only that it is a different branch than currently exists.

Here is relevant output from sudo salt-call -l debug state.sls deploy-app

[INFO    ] Checking local revision for /home/xxxxx/xxxxxxx
[INFO    ] Executing command ['git', 'rev-parse', 'HEAD'] as user 'web' in directory '/home/xxxxx/xxxxxxx'
[DEBUG   ] stdout: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
[INFO    ] Checking local branch for /home/xxxxx/xxxxxxx
[INFO    ] Executing command ['git', 'rev-parse', '--abbrev-ref', 'HEAD'] as user 'web' in directory '/home/xxxxx/xxxxxxx'
[DEBUG   ] stdout: master
[INFO    ] Executing command ['git', 'remote', '--verbose'] as user 'web' in directory '/home/xxxxx/xxxxxxx'
[DEBUG   ] stdout: origin       [email protected]:xxxxx/xxxxxxx.git (fetch)
origin  [email protected]:xxxxx/xxxxx.git (push)
[INFO    ] Executing command ['git', 'rev-parse', 'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx^{commit}'] in directory '/home/xxxxx/xxxxxxx'
[DEBUG   ] stdout: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
[INFO    ] Executing command ['git', 'rev-parse', 'origin/hotfix/1.6.4'] as user 'web' in directory '/home/xxxxx/xxxxxxx'
[DEBUG   ] stdout: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
[INFO    ] Executing command ['git', 'merge-base', '--is-ancestor', 'de021b0b13f3dfcd2a8b759af03379d9d6e4r233', 'ca4809ee99637181759999e9b1842618c2553rt23'] as user 'web' in directory '/home/xxxxx/xxxxxxx'
[DEBUG   ] retcode: 1
[ERROR   ] Repository would be updated from xxxxxxx to xxxxxxx, but this is not a fast-forward merge. Set 'force_reset' to True to force this update.
[INFO    ] Completed state [[email protected]:riskalyze/pronode.git] at time 18:40:15.662699
...
local:
----------
          ID: deploy_app
    Function: git.latest
        Name: [email protected]:xxxxxxxxxx/xxxxxxxxxx.git
      Result: False
     Comment: Repository would be updated from xxxxxxx to yyyyyyy, but this is not a fast-forward merge. Set 'force_reset' to True to force this update.
     Started: 18:40:15.051599
    Duration: 611.1 ms
     Changes:

Summary for local
------------
Succeeded: 0
Failed:    1

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

minion# dpkg -l | grep salt
ii  salt-common                         2015.8.10+ds-1                   all          shared libraries that salt requires for all packages
ii  salt-minion                         2015.8.10+ds-1                   all          client package for salt, the distributed remote execution system
minion# git --version
git version 1.9.1

master# salt --versions-report
Salt Version:
           Salt: 2015.8.10

Dependency Versions:
         Jinja2: 2.7.2
       M2Crypto: 0.21.1
           Mako: 0.9.1
         PyYAML: 3.10
          PyZMQ: 14.0.1
         Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
           RAET: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.4
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 1.5
          gitdb: 0.5.4
      gitpython: 0.3.2 RC1
          ioflo: Not Installed
        libgit2: Not Installed
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.3.0
   mysql-python: 1.2.3
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
   python-gnupg: 0.3.6
          smmap: 0.8.2
        timelib: Not Installed

System Versions:
           dist: Ubuntu 14.04 trusty
        machine: x86_64
        release: 3.13.0-85-generic
         system: Ubuntu 14.04 trusty
master# git --version
git version 1.9.1

[twellspring]

The above git.latest worked on 2015.5.x. Looked at docs and the only change appears to be the addition of force_fetch. Adding force_fetch: True did not change results.

[terminalmage]

The git.latest state was completely rewritten for 2015.8.0. Some of the behavior has changed.

The reason you're having this issue is because a branch parameter was added to give more control over which local branch is used. This is documented here. When you don't specify the branch parameter, then the state just uses whatever local branch is currently checked out. Therefore, when you switch revisions the update is not a fast-forward, and you must tell it that you approve of the change by setting force_reset: True.

This is expected behavior.

[twellspring]

Read the docs on this. The new branch defaults do not make sense to me. It seems like if branch is not specified that it should create the same local branch as the rev. To me that seems like the behavior people would want most of the time.

Any insight into why the default was set as it is?

Looks like if I add the branch option with the same value as rev that my state will be back to the behavior from 2015.5.

[terminalmage]

It was done this way to improve how git.latest handles cases where tags are checked out. If you set rev to a tag, and Salt checks out a branch with the same name as the tag, this results in an ambiguous ref in the local checkout, which interferes with git rev-parse commands when Salt tries to check the local SHA to see if it differs from the remote SHA.

[twellspring]

Thanks for the insight.

[terminalmage]

No problem. Is there any way that you think we can improve the documentation to make this more clear? I know you said you looked at the docs in the initial post, but you didn't catch this in the branch parameter docs. So if there's something we can do to make the docs better, I'm certainly open to suggestions. As a developer, when I write documentation I'm nearly always doing so from the perspective of someone that already understands how the code works, so it's helpful to get the perspective of people who rely on the docs and who have a different perspective than I.

[h0jeZvgoxFepBQ2C]

I think a hint in the error message should be placed, like: "(please set the branch name if you didnt intend this failure)". i stumbled now above this changes and it was really hard to understand or find out that it is related to a change of git.latest.. this should really be improved, because i thought at first that I somewhat broke my own git repository/history..

Also i had this error without specifying a specific revision!

[terminalmage]

@lichtamberg See #35433, I added a check for when the branch is not set, and the current local branch does not match the desired revision name, and in those cases an additional hint is added to the end of the comment string for the state.

[terminalmage]

I just noticed that you didn't specify a revision, I'll add a check for that as well.

[terminalmage]

OK, I've added a case for when no specific revision is provided.