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.

Sign up for GitHub

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

Jump to bottom

Skip tags that point to blob objects instead of commits #4442

Merged
merged 2 commits into from
Aug 27, 2018
Merged

Skip tags that point to blob objects instead of commits #4442

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bfe270d
Skip tags that point to blob objects instead of commits
humitos Jul 27, 2018
3318902
Show full traceback and tag name on log
humitos Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions readthedocs/vcs_support/backends/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,18 @@ def clone(self):

@property
def tags(self):
versions = []
repo = git.Repo(self.working_dir)
versions = [
VCSVersion(self, str(tag.commit), str(tag))
for tag in repo.tags
]
for tag in repo.tags:
try:
Copy link
Member

@safwanrahman safwanrahman Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be rewritten as following

repo = git.Repo(self.working_dir)
versions = []
for tag in repo.tags:
     tag_obj = tag.tag
     commit_obj = tag_obj.object
     tag_name = tag_obj.tag
     versions.append(VCSVersion(self, commit_obj.hexsha, tag_name))

tag.TagObject has the object option which actually points to the commit. It will not raise valueError like the Refs.tag.commit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I found that when I was researching about this but... is this something useful for Read the Docs? I mean, that blob object where the tag is pointing is something we can build?

I considered that and I thought that it will be useless/tricky/not valid for Read the Docs to consider that as a tag (version) that we want to build.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as its a feature of git, we should support it. Many projects may use this feature and tag without commit is also in linux kernel. So from RTD point of view, its a tag and we should not consider whethere the tag is pointed to any commit or blob!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tag can point to a blob, a tree, or even another tag. However, the vast majority of tags across all repos point to commits. A repository that has a non-commit tag is using it for a special purpose (say, it points to a blob of the public key used by the project). With the admittedly limited examples of non-commit tags I've seen none of them could be built as documentation by Read the Docs.

I think it's safe to completely ignore non-commit tags. I would love to see examples of non-commit tags that would somehow be sensible to Read the Docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's safe to completely ignore non-commit tags.

At least until somebody requests it. The fact that nobody has requested it and that non-commit tags caused a repo to fail building on Read the Docs is definitely something to consider as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe if we can support the non commit tags, we should support it. We should not wait till someone open a issue with "My build is not working because my tag does not belong to a commit".

In the solution I provided, we can still support non commit tag as well as commit tag. So why do we exclude the non commit tag?

Copy link
Contributor

@davidfischer davidfischer Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't work. It will just fail later. When you attempt to checkout a non-commit tag in git like we do when we attempt to build documentation, you'll get the error:

fatal: reference is not a tree: TAG-NAME

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I understand! Than I think its better to not have them in Version! Thanks @davidfischer

versions.append(VCSVersion(self, str(tag.commit), str(tag)))
except ValueError as e:
# ValueError: Cannot resolve commit as tag TAGNAME points to a
# blob object - use the `.object` property instead to access it
# This is not a real tag for us, so we skip it
# https://github.com/rtfd/readthedocs.org/issues/4440
log.warning('[Git tag skipped] %s', e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine but you can get the full traceback with exc_info=True if you want.

continue
return versions

@property
Expand Down