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

Projects
None yet
5 participants
@techtonik
Contributor

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.

Fix Edit links if version is referenced by annotated tag
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/
@humitos

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

@humitos

This comment has been minimized.

Member

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?

@ericholscher

This comment has been minimized.

Member

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.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Dec 21, 2017

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

@ericholscher

This comment has been minimized.

Member

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.

@ericholscher ericholscher merged commit 1b3404b into rtfd:master Dec 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@humitos

This comment has been minimized.

Member

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.

@humitos

This comment has been minimized.

Member

humitos commented Dec 21, 2017

Just propose a hotfix at #3437.

ericholscher added a commit that referenced this pull request Dec 22, 2017

Revert "Merge pull request #3302 from techtonik/patch-2"
This reverts commit 1b3404b, reversing
changes made to 26102e0.

@humitos humitos referenced this pull request Dec 29, 2017

Merged

Fix git #3441

@magopian

This comment has been minimized.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment