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

Support git unicode branches #4433

Merged
merged 5 commits into from
Dec 3, 2018
Merged

Support git unicode branches #4433

merged 5 commits into from
Dec 3, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Jul 26, 2018

Use gitpython to get repository branches instead of our custom/hacky csv parser.

With this change, we loose the output from git branch -r command shown to the user under the build output.

These changes need more QA:

NOTE: this PR also changes how the branches are parsed and I'm not 100% sure that removing the origin/ from its name is enough and exactly the same that we were doing before

Ref: #2997 #4052
Fixes: #3060

@humitos humitos added this to the Build stability milestone Jul 26, 2018
@humitos
Copy link
Member Author

humitos commented Jul 26, 2018

I'm not being able to run the tests with unicode chars on branch/tag names in my local instance (test_git_tags also fails). I know that @stsewd worked on this issue and fixed in Travis.

What do I need to do to be able to run these unicode tests in my local instance? (I want to be able to run them all with Py2 and Py3 using tox but also make them work when just running ./manage.py runserver so I can QA on it)

@stsewd
Copy link
Member

stsewd commented Jul 26, 2018

I wasn't able to make this work with manage.py runserver (python2) I think it is because of some environment variables missing (or some additional)

@humitos
Copy link
Member Author

humitos commented Jul 30, 2018

I think we should implement something like #4442 here also as a workaround for now until we found a proper solution, since we are having different problems in production because of this issues of Unicode branches :/

@stsewd
Copy link
Member

stsewd commented Jul 30, 2018

But we aren't close to run rtd with py3 in production?

@ericholscher
Copy link
Member

We're now running py3 in prod, so what is the status of this PR?

@humitos
Copy link
Member Author

humitos commented Nov 6, 2018

@ericholscher Unicode branches/tags are supported in production with the current code.

Although, this branch also migrates the code to parse the output of git branch -r to gitpython which should be better than our hacky CSV parser code.

So, it's still valid for a review if we want to go in that direction.

@ericholscher
Copy link
Member

I think it makes sense to reduce our hacky parsing when possible, especially if a library we're already using does it for us, and is almost certainly better tested and supported.

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #4433 into master will decrease coverage by 0.03%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #4433      +/-   ##
==========================================
- Coverage   76.75%   76.71%   -0.04%     
==========================================
  Files         158      158              
  Lines       10048    10038      -10     
  Branches     1265     1262       -3     
==========================================
- Hits         7712     7701      -11     
  Misses       1995     1995              
- Partials      341      342       +1
Impacted Files Coverage Δ
readthedocs/vcs_support/backends/git.py 81.52% <92.3%> (+0.69%) ⬆️
readthedocs/projects/tasks.py 70.59% <0%> (-0.61%) ⬇️
readthedocs/doc_builder/environments.py 87.06% <0%> (-0.27%) ⬇️

Use `gitpython` to get repository branches instead of our custom/hacky
csv parser.

With this change, we loose the output from `git branch -r` command
shown to the user under the build output.
@humitos
Copy link
Member Author

humitos commented Nov 20, 2018

@stsewd I think that test_fetch_clean_tags_and_branches is not accurate.

I understand that the idea is to check the differences between the upstream_repo and the local_repo but,

  1. local_repo is not a git repository, it's just a directory
  2. the repo.working_dir points to /tmp/tmpah5m49lq/local but the repo.tags uses /tmp/tmppsecoe9j/sample_repo (upstream_repo)
    • if /tmp/tmpah5m49lq/local would be used, it would fail because it's not a real repo
    • if that path is not a real repo, the test doesn't make too much sense: where are these branches/tags coming from?

I don't really understand how it was working before my changes in this PR, but look at the following code:

# This is the original code from master
# with a pdb at test_backend.py:L149
(Pdb) repo
<readthedocs.vcs_support.backends.git.Backend object at 0x7f4483b57978>
(Pdb) repo.working_dir
'/tmp/tmpjlndlpwd/local'
(Pdb) repo.tags
[<VCSVersion: /tmp/tmpi5ht05y4/sample_repo:v01, <VCSVersion: /tmp/tmpi5ht05y4/sample_repo:v02]
(Pdb) repo.branches
[<VCSVersion: /tmp/tmpi5ht05y4/sample_repo:invalidsubmodule, <VCSVersion: /tmp/tmpi5ht05y4/sample_repo:master, <VCSVersion: /tmp/tmpi5ht05y4/sample_repo:newbranch, <VCSVersion: /tmp/tmpi5ht05y4/sample_repo:relativesubmodule, <VCSVersion: /tmp/tmpi5ht05y4/sample_repo:submodule]
(Pdb) self.project.repo
'/tmp/tmpi5ht05y4/sample_repo'
(Pdb) import git
(Pdb) git.Repo(self.project.repo).tags
[<git.TagReference "refs/tags/v01">]
(Pdb) git.Repo(self.project.repo).branches
[<git.Head "refs/heads/invalidsubmodule">, <git.Head "refs/heads/master">, <git.Head "refs/heads/relativesubmodule">, <git.Head "refs/heads/submodule">]
(Pdb) local_repo
'/tmp/tmpjlndlpwd/local'
(Pdb) 

It doesn't make sense to me. The repo is not using the local_repo path at all to get the branches/tags. It's always from upstream_repo and there is somehow a cache that is killing us.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2018

I think you are missing the clone step?

https://github.com/rtfd/readthedocs.org/blob/c37b00995c1bbc5ee51d3552ef176546373bb912/readthedocs/rtd_tests/tests/test_backend.py#L150-L150

there we clone the local repo, I'll pdb to be sure.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2018

I pdb and found that local path is getting a repo

/tmp/tmp0d8n7ki5/local on  master ⌚ 16:49:23
$ g status 
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

Also the remote_repo

/tmp/tmpmfuupwhq/sample_repo on  master! ⌚ 16:50:44
$ g status
On branch master
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	invalidsubmodule/
	relativesubmodule/

nothing added to commit but untracked files present (use "git add" to track)

repo.tags uses local repo (which is the same frmo working_dir)

https://github.com/rtfd/readthedocs.org/blob/c37b00995c1bbc5ee51d3552ef176546373bb912/readthedocs/vcs_support/backends/git.py#L159-L163

@stsewd
Copy link
Member

stsewd commented Nov 20, 2018

Not sure what is going on with gitpython and branches, I'll investigate

@stsewd
Copy link
Member

stsewd commented Nov 20, 2018

Ok, I get it, when cloning the repo, all the branches point to origin, so, technically there aren't local branches, only remote ones. Gitpython list only local branches with repo.branches. Note that previously we were using git branch -r to list all remote branches.

@humitos
Copy link
Member Author

humitos commented Nov 21, 2018

Gitpython list only local branches with repo.branches

Awesome! Thanks for pointing me this out. I was driving crazy and I supposed the test was wrong.

I adapt my git.Backend.branches method to behaves as before. Tests are passing now.

@humitos humitos requested a review from a team November 21, 2018 12:25
@humitos
Copy link
Member Author

humitos commented Nov 28, 2018

This PR is ready to merge after review.

@humitos humitos requested a review from a team November 28, 2018 09:19
@humitos humitos merged commit 4ba947a into master Dec 3, 2018
@humitos humitos deleted the humitos/git/unicode-branches branch December 3, 2018 10:57
@stsewd stsewd mentioned this pull request Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants