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

Fix Edit links if version is referenced by annotated tag #3302

Merged
merged 2 commits into from Dec 21, 2017

Conversation

@techtonik
Copy link
Contributor

@techtonik techtonik commented Nov 23, 2017

A better version of #3238 that fixes #1637, #1820 etc. thanks to responses from git ML.

https://public-inbox.org/git/CAPkN8x+MELCnttE+xptKzYXsYPWqbiE59LABrwNBhFroayc+wQ@mail.gmail.com/

Annotated tags in Git are special. GitHub is unable to find commit corresponding to tag hash, because annotated tags are pointing to tag meta data and need to be dereferenced first.

Fixes Edit links when pointed tag is annotated. GitHub is unable to find
commit corresponding to tag hash, because annotated tags are pointing
to tag meta data and need be dereferenced first.

https://public-inbox.org/git/CAPkN8x+MELCnttE+xptKzYXsYPWqbiE59LABrwNBhFroayc+wQ@mail.gmail.com/
Copy link
Member

@humitos humitos left a comment

I'm not 100% sure to understand how for-each-ref works but I did a simple test with the https://github.com/poliastro repository and I found that none of the hash are valid in the command we are executing

[humitos@julia:poliastro|master]$ for hash in `git show-ref --tags | awk '{ print $1 }'`; do echo https://github.com/poliastro/poliastro/blob/$hash/README.rst; done

https://github.com/poliastro/poliastro/blob/f8fa59b5834478719170ba2056d5c3f049349a36/README.rst
https://github.com/poliastro/poliastro/blob/d7b21e90b77393606f51d3f177b1c96a548be02f/README.rst
https://github.com/poliastro/poliastro/blob/ef405c2e63de4087c3f2c48cd6d38273d74c9380/README.rst
https://github.com/poliastro/poliastro/blob/91f76e251226ac8855801a59026421b72605ddce/README.rst
https://github.com/poliastro/poliastro/blob/778803d441c2b9ca9016793ca19587c00f3dbd3e/README.rst
https://github.com/poliastro/poliastro/blob/a6b3ed03e2c43fd0a9f3baa6feaca0b2d7b9d0df/README.rst
https://github.com/poliastro/poliastro/blob/53d8708e5d433d5f87b0073878053e0109cc9cc4/README.rst
https://github.com/poliastro/poliastro/blob/172cebad14ba30920202603394186774bb16372e/README.rst
https://github.com/poliastro/poliastro/blob/96163241473efd8696402d542da09c2bb6d4a5bf/README.rst
https://github.com/poliastro/poliastro/blob/d31071a614857326b687900951b8607c49f2d8c0/README.rst
https://github.com/poliastro/poliastro/blob/042c09e7fc4d15275ade47cd9d4beb4d15bd0c8c/README.rst
https://github.com/poliastro/poliastro/blob/e764748048f9d6be25e1ef44b014d7dd72a5f291/README.rst
https://github.com/poliastro/poliastro/blob/e670b8c60bd2655c911330edba3a75fec0cd09a4/README.rst
https://github.com/poliastro/poliastro/blob/bebf81a3aad9b516f9d008588dd08db027ed94db/README.rst
https://github.com/poliastro/poliastro/blob/1a5b2113747676b6484ca4f677423ebc39a4b852/README.rst
https://github.com/poliastro/poliastro/blob/19e2f6d341efcafe5f633cbf348ebc34d05d413e/README.rst
https://github.com/poliastro/poliastro/blob/0f43d85221d42e08948bec798d00e1d9b1741c6e/README.rst
https://github.com/poliastro/poliastro/blob/ac701e30fc354494fc226c72f2bf0f3addb62d17/README.rst
https://github.com/poliastro/poliastro/blob/cdda68241b49ebd2aaa7bc489d97e5c6d0104f6f/README.rst

On the other hand, with the command proposed in the PR, it works properly.

[humitos@julia:poliastro|master]$ for hash in `git for-each-ref --format="%(if)%(*objectname)%(then)%(*objectname)%(else)%(objectname)%(end) %(refname)" refs/tags | awk '{ print $1 }'`; do echo https://github.com/poliastro/poliastro/blob/$hash/README.rst; done

https://github.com/poliastro/poliastro/blob/cf0274439e04abc164254ed92d2bf9cc17c53dce/README.rst
https://github.com/poliastro/poliastro/blob/53111940f2c0dc44c729724c8a5cb19815a75448/README.rst
https://github.com/poliastro/poliastro/blob/f1ac1904cb48074d2dd503399001d7dc9438472b/README.rst
https://github.com/poliastro/poliastro/blob/91f76e251226ac8855801a59026421b72605ddce/README.rst
https://github.com/poliastro/poliastro/blob/39668fae67cdd40f63513b7b5d161ee18e90064b/README.rst
https://github.com/poliastro/poliastro/blob/0b9044e3f75ebe048f271f83e8054c2cc880607b/README.rst
https://github.com/poliastro/poliastro/blob/4c2b72283ba9674c9d223207ef9d5dbf6a7778ef/README.rst
https://github.com/poliastro/poliastro/blob/16b4b6d496fdddfea816fab6cc0ed8062180d189/README.rst
https://github.com/poliastro/poliastro/blob/d888e8045011403dc30c59c1d5f376198f7bf257/README.rst
https://github.com/poliastro/poliastro/blob/72f3b62bab1432948b94d3fbfb38132d7d1065a0/README.rst
https://github.com/poliastro/poliastro/blob/c7b914953fe58a37f2b52274eca8210fe6eec225/README.rst
https://github.com/poliastro/poliastro/blob/7f9b8208e64d6604638eedb25e27345a77e65032/README.rst
https://github.com/poliastro/poliastro/blob/d08c9baf2d8d12577075d64d172f8f4103591d4c/README.rst
https://github.com/poliastro/poliastro/blob/564b1d19a9a2fe0f7306d833638bea839d05fbdd/README.rst
https://github.com/poliastro/poliastro/blob/d56e4718dd31aa651e6ed036b1607abe16aa94f0/README.rst
https://github.com/poliastro/poliastro/blob/3d05513e6a78987fb10e9b9d2dd29933cb5c3939/README.rst
https://github.com/poliastro/poliastro/blob/93d229833dc1b9838ab167e791d655da103a0c9f/README.rst
https://github.com/poliastro/poliastro/blob/72c4cf841c77bb74d1dead34a28415e42610d51c/README.rst
https://github.com/poliastro/poliastro/blob/44dbc6e26f2d0a6b395a910c39c9e8131c8946b2/README.rst

Loading

@humitos
Copy link
Member

@humitos humitos commented Dec 21, 2017

@ericholscher this is another PR that I think it's ready for merge and it's not too big. Can you take a look at this one?

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 21, 2017

I'd like to see a test that shows what this is fixing? I don't understand it otherwise really.

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 21, 2017

Specifically, a test in our code, so that we know it doesn't break in the future :)

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 21, 2017

I guess we don't have much for tests on the VCS code, so will go ahead and merge this.

Loading

@ericholscher ericholscher merged commit 1b3404b into readthedocs:master Dec 21, 2017
1 check passed
Loading
@humitos
Copy link
Member

@humitos humitos commented Dec 21, 2017

I think we introduce a bug here after merging the PR:

[21/Dec/2017 14:21:39] readthedocs.vcs_support.utils:99[15182]: INFO Lock (poliastro): Releasing
[21/Dec/2017 14:21:39] readthedocs.doc_builder.environments:344[15182]: ERROR (Build) [poliastro:latest] need more than 1 value to unpack
Traceback (most recent call last):
  File "/home/humitos/rtfd/code/readthedocs.org/readthedocs/projects/tasks.py", line 209, in run_setup
    self.setup_vcs()
  File "/home/humitos/rtfd/code/readthedocs.org/readthedocs/projects/tasks.py", line 339, in setup_vcs
    update_imported_docs(self.version.pk)
  File "/home/humitos/.pyenv/versions/2.7.14/envs/rtfd/lib/python2.7/site-packages/celery/local.py", line 191, in __call__
    return self._get_current_object()(*a, **kw)
  File "/home/humitos/.pyenv/versions/2.7.14/envs/rtfd/lib/python2.7/site-packages/celery/app/task.py", line 380, in __call__
    return self.run(*args, **kwargs)
  File "/home/humitos/rtfd/code/readthedocs.org/readthedocs/projects/tasks.py", line 619, in update_imported_docs
    } for v in version_repo.tags
  File "/home/humitos/rtfd/code/readthedocs.org/readthedocs/vcs_support/backends/git.py", line 95, in tags
    return self.parse_tags(stdout)
  File "/home/humitos/rtfd/code/readthedocs.org/readthedocs/vcs_support/backends/git.py", line 121, in parse_tags
    commit_hash, name = row
ValueError: need more than 1 value to unpack

This is what I get from inside the RTD code:

(Pdb++) self.run('git', 'for-each-ref', '--format="%(if)%(*objectname)%(then)%(*objectname)%(else)%(objectname)%(end) %(refname)"', 'refs/tags')
[21/Dec/2017 14:31:26] readthedocs.vcs_support.base:62[15749]: INFO VCS[poliastro:latest]: git for-each-ref --format="%(if)%(*objectname)%(then)%(*objectname)%(else)%(objectname)%(end) %(refname)" refs/tags
[21/Dec/2017 14:31:26] readthedocs.vcs_support.base:70[15749]: INFO VCS[poliastro:latest]: "cf0274439e04abc164254ed92d2bf9cc17c53dce refs/tags/v0.1.0"
"53111940f2c0dc44c729724c8a5cb19815a75448 refs/tags/v0.1.1"
"f1ac1904cb48074d2dd503399001d7dc9438472b refs/tags/v0.1.2"
"91f76e251226ac8855801a59026421b72605ddce refs/tags/v0.2.0"
"39668fae67cdd40f63513b7b5d161ee18e90064b refs/tags/v0.2.1"
"0b9044e3f75ebe048f271f83e8054c2cc880607b refs/tags/v0.3.0"
"4c2b72283ba9674c9d223207ef9d5dbf6a7778ef refs/tags/v0.3.0rc1"
"16b4b6d496fdddfea816fab6cc0ed8062180d189 refs/tags/v0.3.1"
"d888e8045011403dc30c59c1d5f376198f7bf257 refs/tags/v0.3.2"
"72f3b62bab1432948b94d3fbfb38132d7d1065a0 refs/tags/v0.3.3"
"c7b914953fe58a37f2b52274eca8210fe6eec225 refs/tags/v0.4.0"
"7f9b8208e64d6604638eedb25e27345a77e65032 refs/tags/v0.4.1"
"d08c9baf2d8d12577075d64d172f8f4103591d4c refs/tags/v0.4.2"
"564b1d19a9a2fe0f7306d833638bea839d05fbdd refs/tags/v0.4.3"
"d56e4718dd31aa651e6ed036b1607abe16aa94f0 refs/tags/v0.5.0"
"3d05513e6a78987fb10e9b9d2dd29933cb5c3939 refs/tags/v0.6.0"
"93d229833dc1b9838ab167e791d655da103a0c9f refs/tags/v0.7.0"
"72c4cf841c77bb74d1dead34a28415e42610d51c refs/tags/v0.7.0b1"
"44dbc6e26f2d0a6b395a910c39c9e8131c8946b2 refs/tags/v0.8.0"

(0, u'"cf0274439e04abc164254ed92d2bf9cc17c53dce refs/tags/v0.1.0"\n"53111940f2c0dc44c729724c8a5cb19815a75448 refs/tags/v0.1.1"\n"f1ac1904cb48074d2dd503399001d7dc9438472b refs/tags/v0.1.2"\n"91f76e251226ac8855801a59026421b72605ddce refs/tags/v0.2.0"\n"39668fae67cdd40f63513b7b5d161ee18e90064b refs/tags/v0.2.1"\n"0b9044e3f75ebe048f271f83e8054c2cc880607b refs/tags/v0.3.0"\n"4c2b72283ba9674c9d223207ef9d5dbf6a7778ef refs/tags/v0.3.0rc1"\n"16b4b6d496fdddfea816fab6cc0ed8062180d189 refs/tags/v0.3.1"\n"d888e8045011403dc30c59c1d5f376198f7bf257 refs/tags/v0.3.2"\n"72f3b62bab1432948b94d3fbfb38132d7d1065a0 refs/tags/v0.3.3"\n"c7b914953fe58a37f2b52274eca8210fe6eec225 refs/tags/v0.4.0"\n"7f9b8208e64d6604638eedb25e27345a77e65032 refs/tags/v0.4.1"\n"d08c9baf2d8d12577075d64d172f8f4103591d4c refs/tags/v0.4.2"\n"564b1d19a9a2fe0f7306d833638bea839d05fbdd refs/tags/v0.4.3"\n"d56e4718dd31aa651e6ed036b1607abe16aa94f0 refs/tags/v0.5.0"\n"3d05513e6a78987fb10e9b9d2dd29933cb5c3939 refs/tags/v0.6.0"\n"93d229833dc1b9838ab167e791d655da103a0c9f refs/tags/v0.7.0"\n"72c4cf841c77bb74d1dead34a28415e42610d51c refs/tags/v0.7.0b1"\n"44dbc6e26f2d0a6b395a910c39c9e8131c8946b2 refs/tags/v0.8.0"\n', u'')
(Pdb++)

and this is from my own computer:

[humitos@julia:latest|(8685e25...)]$ git for-each-ref --format="%(if)%(*objectname)%(then)%(*objectname)%(else)%(objectname)%(end) %(refname)" refs/tags
cf0274439e04abc164254ed92d2bf9cc17c53dce refs/tags/v0.1.0
53111940f2c0dc44c729724c8a5cb19815a75448 refs/tags/v0.1.1
f1ac1904cb48074d2dd503399001d7dc9438472b refs/tags/v0.1.2
91f76e251226ac8855801a59026421b72605ddce refs/tags/v0.2.0
39668fae67cdd40f63513b7b5d161ee18e90064b refs/tags/v0.2.1
0b9044e3f75ebe048f271f83e8054c2cc880607b refs/tags/v0.3.0
4c2b72283ba9674c9d223207ef9d5dbf6a7778ef refs/tags/v0.3.0rc1
16b4b6d496fdddfea816fab6cc0ed8062180d189 refs/tags/v0.3.1
d888e8045011403dc30c59c1d5f376198f7bf257 refs/tags/v0.3.2
72f3b62bab1432948b94d3fbfb38132d7d1065a0 refs/tags/v0.3.3
c7b914953fe58a37f2b52274eca8210fe6eec225 refs/tags/v0.4.0
7f9b8208e64d6604638eedb25e27345a77e65032 refs/tags/v0.4.1
d08c9baf2d8d12577075d64d172f8f4103591d4c refs/tags/v0.4.2
564b1d19a9a2fe0f7306d833638bea839d05fbdd refs/tags/v0.4.3
d56e4718dd31aa651e6ed036b1607abe16aa94f0 refs/tags/v0.5.0
3d05513e6a78987fb10e9b9d2dd29933cb5c3939 refs/tags/v0.6.0
93d229833dc1b9838ab167e791d655da103a0c9f refs/tags/v0.7.0
72c4cf841c77bb74d1dead34a28415e42610d51c refs/tags/v0.7.0b1
44dbc6e26f2d0a6b395a910c39c9e8131c8946b2 refs/tags/v0.8.0
[humitos@julia:latest|(8685e25...)]$ 

the difference is that from inside RTD the output comes with " at the begining and the end of the line, so it's not well split by the csv reader.

Loading

@humitos
Copy link
Member

@humitos humitos commented Dec 21, 2017

Just propose a hotfix at #3437.

Loading

ericholscher added a commit that referenced this issue Dec 22, 2017
This reverts commit 1b3404b, reversing
changes made to 26102e0.
@humitos humitos mentioned this pull request Dec 29, 2017
@magopian
Copy link

@magopian magopian commented Feb 14, 2018

Just for people coming here and thinking this PR fixed the issue: it was reverted, and is currently not fixed yet (see #3441)

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants