-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Skip tags that point to blob objects instead of commits #4442
Conversation
# 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) |
There was a problem hiding this comment.
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.
for tag in repo.tags | ||
] | ||
for tag in repo.tags: | ||
try: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Looks like this can be merged. We can return to tighten the exception handling later |
Maybe this is not the best solution ever since we may be skipping other cases also since I'm catching the
ValueError
exception (too general) but initially we want to skip them, log and continue building.Closes #4440