Broken fileserver.file_list, dir_list for gitpython using python3.x

[vin01]

Description of Issue

gitpython returns bytes with python3.x when traversing the commit tree and it returns string with python2.7. This leads to a failure in fileserver.file_list, fileserver.dir_list for salt and more failures are also possible wherever this functionality is used.

It also leads to errors being generated in logs while using state.show_sls.

Steps to Reproduce Issue

Run salt master with gitpython using python3.x and attempt to salt-run fileserver.file_list

Versions Report

gitpython: 3.0.2
Python: 3.7.4 (default, Jul  9 2019, 18:13:23)

PoC offending code

Offending code block: https://github.com/saltstack/salt/blob/develop/salt/utils/gitfs.py#L1320

Demonstration:

# -*- coding: utf-8 -*-
from git import Repo
import git
import six

repo = Repo('salt-formula')

rr = git.RemoteReference(repo,'refs/remotes/origin/master')
tree = rr.commit.tree

stream = six.StringIO()

for file_blob in tree.traverse():
    if not isinstance(file_blob, git.Blob):
        continue
    file_blob.stream_data(stream)

Traceback (most recent call last):
  File "gitpythontest.py", line 16, in <module>
    file_blob.stream_data(stream)
  File "/home/vagrant/.local/lib/python3.6/site-packages/git/objects/base.py", line 119, in stream_data
    stream_copy(istream, ostream)
  File "/home/vagrant/.local/lib/python3.6/site-packages/git/util.py", line 124, in stream_copy
    destination.write(chunk)
TypeError: string argument expected, got 'bytes'

Possible solution

Instead of assuming string output from gitpython, we could expect bytes and handle it using six.BytesIO fake file object. This is also what I believe gitpython would prefer as @Byron mentions in this gitpython issue This works with python2.7 as well.

If this approach of handling this in Salt itself makes sense, I will be happy to send a PR.

Thanks!

[xeacott]

Thanks for addressing this and submitting an issue. I'll ping @saltstack/team-core to see what the verdict is.

I'll added the appropriate labels to this.

[waynew]

@xeacott Without looking at the code we should probably be wrapping it in a salt.utils.stringutils.to_str. My guess is that it should be a pretty straightforward fix, and my hope is that it shouldn't require much in the way of writing a regression test 🙃

[vin01]

I took a closer look into the existing tests to see why this was not caught in the existing file_list test here

Turns out, for testing repo shutil.copytree is being used which by default follows/resolved symlinks and hence dest_sym from base test dir is not being treated as a symlink but a regular file which is why this case is not encountered in tests which also I think defeats the purpose of creating the symlink in test repo as they do not reflect the actual repo being tested at test-runtime.

So the root cause of this was using fileserver.file_list and other operations on a repo which has symlinks which in my case happens to be the salt-formula :) https://github.com/saltstack-formulas/salt-formula/blob/master/salt/pkgrepo/arch with python3.

Note: shutil.copytree has a different behavior with python3 than python2. It would not copy the metadata in python2 if symlink=True. https://docs.python.org/2/library/shutil.html#shutil.copytree

Python2:

If symlinks is true, symbolic links in the source tree are represented as symbolic links in the new tree, but the metadata of the original links is NOT copied; if false or omitted, the contents and metadata of the linked files are copied to the new tree.

Python3:

If symlinks is true, symbolic links in the source tree are represented as symbolic links in the new tree and the metadata of the original links will be copied as far as the platform allows; if false or omitted, the contents and metadata of the linked files are copied to the new tree.

This should be harmless for this case.

I have created a separate PR to fix the test to copy symlinks as symlinks and it does seem to generate the right result by encountering the bytes error: https://jenkinsci.saltstack.com/job/pr-kitchen-debian9-py3/job/PR-54453/1/console

[vin01]

@waynew It would be great to hear some feedback so that this can be closed some time soon.

[waynew]

I believe those fixes seem reasonable, but the definitely need some reviews.

I don't know if you've seen the new SEP, but we're changing to a single-branch release model. If you could rebase and re-target your PRs to master (just click edit on the PRs), that would be great!

[vin01]

Looks like the automatic reviewer assignment is not functional for master branch. I have opened another PR to avoid the conflict resolution with previous ones. Awaiting review.

[vin01]

This can be closed now as #54900 is merged.