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

Merged
merged 2 commits into from Aug 27, 2018

Conversation

Projects
None yet
5 participants
@humitos
Member

humitos commented Jul 27, 2018

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

Show outdated Hide outdated readthedocs/vcs_support/backends/git.py Outdated
for tag in repo.tags
]
for tag in repo.tags:
try:

This comment has been minimized.

@safwanrahman

safwanrahman Jul 30, 2018

Member

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

@safwanrahman

safwanrahman Jul 30, 2018

Member

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

This comment has been minimized.

@humitos

humitos Jul 30, 2018

Member

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?

@humitos

humitos Jul 30, 2018

Member

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?

This comment has been minimized.

@safwanrahman

safwanrahman Jul 30, 2018

Member

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!

@safwanrahman

safwanrahman Jul 30, 2018

Member

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!

This comment has been minimized.

@davidfischer

davidfischer Jul 30, 2018

Contributor

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.

@davidfischer

davidfischer Jul 30, 2018

Contributor

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.

This comment has been minimized.

@davidfischer

davidfischer Jul 30, 2018

Contributor

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.

@davidfischer

davidfischer Jul 30, 2018

Contributor

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.

This comment has been minimized.

@safwanrahman

safwanrahman Jul 30, 2018

Member

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?

@safwanrahman

safwanrahman Jul 30, 2018

Member

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?

This comment has been minimized.

@davidfischer

davidfischer Jul 30, 2018

Contributor

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
@davidfischer

davidfischer Jul 30, 2018

Contributor

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

This comment has been minimized.

@safwanrahman

safwanrahman Jul 30, 2018

Member

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

@safwanrahman

safwanrahman Jul 30, 2018

Member

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

@stsewd

stsewd approved these changes Aug 24, 2018

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Aug 27, 2018

Contributor

Looks like this can be merged. We can return to tighten the exception handling later

Contributor

agjohnson commented Aug 27, 2018

Looks like this can be merged. We can return to tighten the exception handling later

@agjohnson agjohnson merged commit ff0139d into master Aug 27, 2018

1 check passed

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

@agjohnson agjohnson deleted the humitos/git/skip-non-tags branch Aug 27, 2018

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