Use sh library to wrap git commands.
ClosedPublic

Authored by mmodell on Nov 1 2017, 10:53 AM.

Details

Reviewers
demon
thcipriani
Group Reviewers
Release-Engineering-Team
Commits
rMSCA5df545014065: Use sh library to wrap git commands.
Patch without arc
git checkout -b D866 && curl -L https://phabricator.wikimedia.org/D866?download=true | git apply
Summary

I think this is cleaner than GitPython. I've only partially converted
to using the sh wrapper but all tests pass (and I've added a couple
of new ones)

This also adds a VERSION check for git version and adds --jobs conditionally.

Test Plan

ran nosetests

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mmodell created this revision.Nov 1 2017, 10:53 AM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptNov 1 2017, 10:53 AM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
mmodell updated this revision to Diff 2285.Nov 1 2017, 11:09 AM

check version before appending --jobs arg

mmodell edited the summary of this revision. (Show Details)Nov 1 2017, 11:10 AM
mmodell updated this revision to Diff 2286.Nov 1 2017, 11:14 AM

Only run git-fat tests when git-fat binary is available

mmodell updated this revision to Diff 2287.Nov 1 2017, 11:51 AM

fix build errors: exclude some stuff from flake8 and use six.moves for importing configparser

mmodell updated this revision to Diff 2288.Nov 1 2017, 12:46 PM

more test coverage.

couple random things from testing this morning.

scap/config.py
25

Can we then remove python-configparser from Depends in debian/control and just use python-six?

scap/git.py
334

if VERSION[0] > 2 and VERSION[1] > 9

mmodell updated this revision to Diff 2289.Nov 1 2017, 4:46 PM
mmodell marked an inline comment as done.

fix version check

scap/config.py
25

I don't know... probably?

demon added inline comments.Nov 1 2017, 4:58 PM
scap/git.py
39

Do we have to use globals? They trigger all kinds of bad feelings. We can avoid the global (and the VERSION constant too, tbh):

try:
    return version.git_version
except AttributeError:
    version_numbers = git('version').split(' ')[2]
    version.git_version = tuple(int(n) for n in version_numbers.split('.')[:4]
                    if n.isdigit())
    return version.git_version
mmodell added inline comments.Nov 1 2017, 6:05 PM
scap/git.py
39

That's not really much better than globals, IMO. Python's globals are module-level not truly global globals...

demon added inline comments.Nov 1 2017, 6:08 PM
scap/git.py
39

Eh, I think it's better but idk. I know explicitly using globals makes pylint whine though ;-)

mmodell updated this revision to Diff 2290.Nov 1 2017, 6:16 PM

don't use globals.

mmodell marked an inline comment as done.Nov 1 2017, 6:16 PM
mmodell added inline comments.
scap/git.py
39

Better?

mmodell marked 3 inline comments as done.Nov 1 2017, 6:17 PM
mmodell marked an inline comment as done.
mmodell retitled this revision from WIP: Use sh library to wrap git commands. to Use sh library to wrap git commands..
demon added inline comments.Nov 1 2017, 6:18 PM
scap/git.py
39

<3

demon added a comment.Nov 3 2017, 1:20 AM

Wait, why are we copying sh in rather than just using the pip-installed one?

mmodell added a comment.EditedNov 3 2017, 10:10 AM
In D866#17277, @demon wrote:

Wait, why are we copying sh in rather than just using the pip-installed one?

Because we don't use pip in prod, we use debian packages and debian has a horribly outdated version of sh. (Also because sh is a single file and they make it easy to do it that way)

Note this would likely also allow us to eliminate cmd.py which is a very limited clone of some of the sh functionality.

demon added a comment.Nov 3 2017, 3:30 PM

Fair enough, I didn't look for a debian package

mmodell updated this revision to Diff 2293.Nov 3 2017, 5:47 PM

check git version when using the --reference arg.

demon accepted this revision.Nov 6 2017, 6:25 PM
This revision is now accepted and ready to land.Nov 6 2017, 6:25 PM
This revision was automatically updated to reflect the committed changes.