Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify vcs_support backend git by using GitPython #3839

Closed
agjohnson opened this issue Mar 23, 2018 · 5 comments
Closed

Simplify vcs_support backend git by using GitPython #3839

agjohnson opened this issue Mar 23, 2018 · 5 comments
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Milestone

Comments

@agjohnson
Copy link
Contributor

We already started using GitPython for some submodules pieces. We should continue porting some of our hand rolled logic to use GitPython instead.

Pieces of readthedocs.vcs_support.backend.git we should definitely port:

Maybe:

  • tags()
  • branches()

These would be good first targets to port over, as we are executing these commands to get data out of the repository. It's not important that these messages are surfaced to users in build command output.

I'm going to block on getting a feature out that executes these commands in docker first though, as we need to clone and submodule checkout inside the docker container to isolate these calls. I can't quite consider how relying on gitpython for some of these calls, but not all, works with regard to docker vcs checkouts.

@agjohnson agjohnson added Needed: design decision A core team decision is required Status: blocked Issue is blocked on another issue labels Mar 23, 2018
@agjohnson agjohnson added this to the Refactoring milestone Mar 23, 2018
@humitos humitos added the Improvement Minor improvement to code label Mar 23, 2018
@humitos
Copy link
Member

humitos commented Sep 4, 2018

Just want to note here that I'm starting to feel uncomfortable with gitpython because it has several issues around unicode that cause our builds to fail and to debug them, write tests, etc.

I mentioned some of these problem at: #4318 (comment)

I'd like to start thinking for better alternatives. Although, I don't have one in mind yet.

@stsewd
Copy link
Member

stsewd commented Dec 12, 2018

We are only missing git status and git submodule status here

@stsewd
Copy link
Member

stsewd commented Dec 12, 2018

But not sure if we want to replace those, the code looks more complex with git python p:

-        code, _, _ = self.run('git', 'status', record=False)
-        return code == 0
+        try:
+            git.Repo(self.working_dir)
+        except InvalidGitRepositoryError:
+            return False
+        return True
-        code, out, _ = self.run('git', 'submodule', 'status', record=False)
-        return code == 0 and bool(out)
+        repo = git.Repo(self.working_dir)
+        return bool(repo.submodules)

@stsewd
Copy link
Member

stsewd commented Dec 12, 2018

If you think that we are ok with git status and git submodule status, we can close this issue.

@humitos
Copy link
Member

humitos commented Dec 13, 2018

I like the consistency: all git command or all gitpython. A mixture of both it's hard to maintain and to keep in mind. On the other hand here, there are git commands that we can't replace: those with record=True

The code maybe looks longer, but it's more Pythonic, I'd say and we are relying on an external library instead of a local and custom command.

I'm 👍 on migrating them also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code Needed: design decision A core team decision is required
Projects
None yet
Development

No branches or pull requests

3 participants