git.detached is always fired even when there are no changes

[Magomogo]

Description of Issue/Question

Using git.detached I expect this state not to be applied when working copy already has desired commit checked out. But it's triggered every time.

Setup

test-detached.sls:

https://github.com/Magomogo/days-interval:
  git.detached:
    - ref: 'v0.1.0'
    - target: /tmp/days-interval

Steps to Reproduce Issue

Execute this state:

$ salt-call state.apply test-detached
local:
----------
          ID: https://github.com/Magomogo/days-interval
    Function: git.detached
      Result: True
     Comment: Commit ID 5289e24094da8109f0e061c38ed601fa20201426 was checked out at /tmp/days-interval
     Started: 05:04:59.808246
    Duration: 1705.775 ms
     Changes:
              ----------
              HEAD:
                  ----------
                  new:
                      e7c9f32981ec807776b4e9d6cd9f59734c8c1e3b
                  old:
                      e7c9f32981ec807776b4e9d6cd9f59734c8c1e3b

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   1.706 s

### Versions Report

$ git --version
git version 2.1.4

$ sudo salt-call --versions-report
Salt Version:
Salt: 2016.11.0

Dependency Versions:
cffi: 0.8.6
cherrypy: Not Installed
dateutil: 2.2
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.7.3
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.4.2
mysql-python: 1.2.3
pycparser: 2.10
pycrypto: 2.6.1
pygit2: Not Installed
Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
python-gnupg: Not Installed
PyYAML: 3.11
PyZMQ: 14.4.0
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.2.1
ZMQ: 4.0.5

System Versions:
dist: debian 8.6
machine: x86_64
release: 3.16.0-4-amd64
system: Linux
version: debian 8.6

[Ch3LL]

@Magomogo thanks for the great example! was easy to replicate.

Here is also a docker container for anyone that wants to replicate with a container:

1. docker run -it -v /home/ch3ll/git/salt/:/testing/ ch3ll/issues:38092 salt-call --local state.sls test -ldebug (where /home/ch3ll/git/salt/ is a local cloned git salt repo)

I also noticed this behavior in 2016.3.4 so seems it has not worked previously and we need to add some logic there to check fi it needs to run.

[davido-bs]

I noticed this this only seems to apply to tags, despite the fact that git.detached can take a tag as a ref argument. We have a dev/prod setup, where we set prod to a tag. It seems like internally it wants to compare the SHA of the tag itself to the SHA of the merge commit that the tag made off of. Here's some debug output:

2017-04-14 16:29:59,763 [salt.utils.lazy  ][DEBUG   ][19470] LazyLoaded git.detached
2017-04-14 16:29:59,764 [salt.state       ][INFO    ][19470] Running state [git@github.com:<GIT_REPO>] at time 16:29:59.764421
2017-04-14 16:29:59,765 [salt.state       ][INFO    ][19470] Executing state git.detached for git@github.com:<GIT_REPO>
2017-04-14 16:29:59,765 [salt.loaded.int.states.git][INFO    ][19470] Checking local revision for <DIRECTORY>
2017-04-14 16:29:59,766 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'rev-parse', 'HEAD'] in directory '<DIRECTORY>'
2017-04-14 16:29:59,779 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: a927c34466027a49500b79df74deb7d0d737f55d
2017-04-14 16:29:59,779 [salt.loaded.int.states.git][INFO    ][19470] Checking local branch for <DIRECTORY>
2017-04-14 16:29:59,779 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'rev-parse', '--abbrev-ref', 'HEAD'] in directory '<DIRECTORY>'
2017-04-14 16:29:59,792 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: HEAD
2017-04-14 16:29:59,793 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'remote', '--verbose'] in directory '<DIRECTORY>'
2017-04-14 16:29:59,806 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: origin git@github.com:<GIT_REPO> (fetch)
origin  git@github.com:<GIT_REPO> (push)
2017-04-14 16:29:59,806 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'fetch', '--force', 'origin', 'refs/heads/*:refs/remotes/origin/*', '+refs/tags/*:refs/tags/*'] in directory '<DIRECTORY>'
2017-04-14 16:29:59,951 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'ls-remote', '<DIRECTORY>'] in directory '/root'
2017-04-14 16:29:59,967 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: a927c34466027a49500b79df74deb7d0d737f55d       HEAD
<VARIOUS_OTHER_BRANCHES>
1f7d83e7d14e458f95de773ec3faaf7dcd5387bd        refs/tags/v1.0
a927c34466027a49500b79df74deb7d0d737f55d        refs/tags/v1.0^{}
2017-04-14 16:29:59,968 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'reset', '--hard', 'HEAD'] in directory '<DIRECTORY>'
2017-04-14 16:30:00,587 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: HEAD is now at a927c34 Merge pull request #17 from <COMPANY>/RC/1
2017-04-14 16:30:00,588 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'checkout', '1f7d83e7d14e458f95de773ec3faaf7dcd5387bd'] in directory '<DIRECTORY>'
2017-04-14 16:30:00,821 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: HEAD is now at a927c34... Merge pull request #17 from <COMPANY>/RC/1
2017-04-14 16:30:00,822 [salt.loaded.int.module.cmdmod][INFO    ][19470] Executing command ['git', 'rev-parse', 'HEAD'] in directory '<DIRECTORY>'
2017-04-14 16:30:00,837 [salt.loaded.int.module.cmdmod][DEBUG   ][19470] stdout: a927c34466027a49500b79df74deb7d0d737f55d
2017-04-14 16:30:00,837 [salt.loaded.int.states.git][INFO    ][19470] Repository was reset to HEAD before checking out ref. Commit ID 1f7d83e7d14e458f95de773ec3faaf7dcd5387bd was checked out at <DIRECTORY>.
2017-04-14 16:30:00,837 [salt.state       ][INFO    ][19470] Made the following changes: 'HEAD' changed from 'a927c34466027a49500b79df74deb7d0d737f55d' to 'a927c34466027a49500b79df74deb7d0d737f55d'

So 1f7d... is the SHA of the tag itself. If I check it out, it sets HEAD to a927..., which is the commit that the tag was taken from:

git checkout 1f7d83e7d14e458f95de773ec3faaf7dcd5387bd
HEAD is now at a927c34... Merge pull request #17 from <COMPANY>/RC/1

[ikr]

Any news on that one? For the moment we have to use the following workaround, which makes things quite a bit slower

- unless: >
    cd {{ repo_dir }} &&
    git fetch origin -p && 
    git ls-remote origin {{ rev }} {{ rev }}^{} | 
    grep -w $(git rev-parse HEAD)

[c-wicklein]

Here's the basis for a a potentially faster workaround similar to what ikr is doing. I haven't given this a ton of thought, but it's worth noting that I'm not fetching from origin, so in practical cases the network piece is cut out, so the check that there's nothing to do is fast.

[me@laptop git-annex]$ git show-ref -d 7.20181211 | grep "$(git rev-parse HEAD)"
426bdbf11360bb0a9fd6a8d4bddafb2e11f49754 refs/tags/7.20181211^{}

For example:

git://git-annex.branchable.com/:
  git.detached:
    - target: /home/me/src/git-annex
    - rev: {{ pillar['git_annex_version'] }}
    - user: me
    - require:
      - file: /home/me/src
    - unless:  cd /home/me/src/git-annex && git show-ref -d {{ pillar['git_annex_version'] }} | grep "$(git rev-parse HEAD)"

[c-wicklein]

Upgraded to 2019.2.1 when I installed CentOS 8 on my laptop, and this is still broken. I have unless like the Lorax for git.detached, so I took a look at fixing this.

There are two problems here. The first and reported problem is that git.detached can fail to recognize that rev refers to the current revision when rev is a tag. The second problem is that git.detached prematurely queries the remote to make a list of revisions. This makes git.detached slow (and when I'm offline, fail.)

Here's a three-line fix for the first problem as a patch to salt/states/git.py. It's still necessary to query the remote for a list of revisions, but at least the false change isn't reported. I'll have a look at fixing the unneeded remote query next.

--- /usr/lib/python3.6/site-packages/salt/states/git.py.orig	2019-09-12 11:11:55.000000000 -0500
+++ /usr/lib/python3.6/site-packages/salt/states/git.py	2019-10-03 16:04:38.511300892 -0500
@@ -2660,6 +2660,7 @@
         try:
             all_remote_refs = __salt__['git.remote_refs'](
                 target,
+                tags=True,
                 user=user,
                 password=password,
                 identity=identity,
@@ -2670,6 +2671,8 @@
 
             if 'refs/remotes/'+remote+'/'+rev in all_remote_refs:
                 checkout_commit_id = all_remote_refs['refs/remotes/' + remote + '/' + rev]
+            elif 'refs/tags/' + rev + '^{}' in all_remote_refs:
+                checkout_commit_id = all_remote_refs['refs/tags/' + rev + '^{}']
             elif 'refs/tags/' + rev in all_remote_refs:
                 checkout_commit_id = all_remote_refs['refs/tags/' + rev]
             else:

[c-wicklein]

Okay, here's a quick patch for salt/modules/git.py to add local_refs (like remote_refs, but uh local) and for salt/states/git.py which includes my earlier patch but also makes use of local_refs to convert known tags to hashes which can then be treated like any other hash that exists locally. With this patch, I'm able to use tags with git.detached without the unless hack, avoid the false report of changes, and avoid a remote query when rev refers to a hash that exists locally.

---

--- /usr/lib/python3.6/site-packages/salt/modules/git.py.orig	2019-09-12 11:11:55.000000000 -0500
+++ /usr/lib/python3.6/site-packages/salt/modules/git.py	2019-10-03 21:53:32.552901407 -0500
@@ -3823,6 +3823,31 @@
     return all_remotes[remote]
 
 
+def local_refs(cwd,
+               user=None,
+               password=None,
+               ignore_retcode=False,
+               output_encoding=None):
+    '''
+    '''
+    cwd = _expand_path(cwd, user)
+    command = ['git', 'show-ref', '-d']
+    output = _git_run(command,
+                      cwd=cwd,
+                      user=user,
+                      password=password,
+                      ignore_retcode=ignore_retcode,
+                      output_encoding=output_encoding)['stdout']
+    ret = {}
+    for line in salt.utils.itertools.split(output, '\n'):
+        try:
+            sha1_hash, ref_name = line.split(None, 1)
+        except ValueError:
+            continue
+        ret[ref_name] = sha1_hash
+    return ret
+
+
 def remote_refs(url,
                 heads=False,
                 tags=False,

---

--- /usr/lib/python3.6/site-packages/salt/states/git.py.orig	2019-09-12 11:11:55.000000000 -0500
+++ /usr/lib/python3.6/site-packages/salt/states/git.py	2019-10-03 21:50:53.731941531 -0500
@@ -2478,6 +2478,24 @@
                 # The rev is a hash and it exists locally so skip to checkout
                 hash_exists_locally = True
         else:
+            try:
+                local_refs = __salt__['git.local_refs'](
+                    target,
+                    user=user,
+                    password=password,
+                    ignore_retcode=True,
+                    output_encoding=output_encoding)
+                for needle in ('refs/tags/' + rev + '^{}',
+                               'refs/tags/' + rev,):
+                    if needle in local_refs:
+                        hash_exists_locally = True
+                        remote_rev_type = 'hash'
+                        rev = local_refs[needle]
+                        break
+            except CommandExecutionError:
+                hash_exists_locally = False
+
+        if not hash_exists_locally:
             # Check that remote is present and set to correct url
             remotes = __salt__['git.remotes'](target,
                                               user=user,
@@ -2660,6 +2678,7 @@
         try:
             all_remote_refs = __salt__['git.remote_refs'](
                 target,
+                tags=True,
                 user=user,
                 password=password,
                 identity=identity,
@@ -2670,6 +2689,8 @@
 
             if 'refs/remotes/'+remote+'/'+rev in all_remote_refs:
                 checkout_commit_id = all_remote_refs['refs/remotes/' + remote + '/' + rev]
+            elif 'refs/tags/' + rev + '^{}' in all_remote_refs:
+                checkout_commit_id = all_remote_refs['refs/tags/' + rev + '^{}']
             elif 'refs/tags/' + rev in all_remote_refs:
                 checkout_commit_id = all_remote_refs['refs/tags/' + rev]
             else:

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.