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 500 error on repos with no tags #11870

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

mrsdizzie
Copy link
Member

@mrsdizzie mrsdizzie commented Jun 12, 2020

#11846 Introduced feature to show exact tag on commit view. However if a repo has no tags at all git prints out a separate and unhandled error " No names found, cannot describe anything.":

if strings.Contains(err.Error(), "no tag exactly matches") {

Adding --always to the command makes it always use the error in the style of "fatal: no tag exactly matches" even if there are no tags at all.

Fixes #11869
Fixes #11868

 go-gitea#11846 Introduced feature to show exact tag on commit view. However if a repo has no tags at all git prints out a separate and unhandled error " No names found, cannot describe anything."

 Adding --always to the command makes it always use the error in the style of "fatal: no tag exactly matches" even if there are no tags at all.

 Fixes go-gitea#11869
 Fixes go-gitea#11868
@techknowlogick techknowlogick added this to the 1.13.0 milestone Jun 12, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 12, 2020
@silverwind
Copy link
Member

I wonder why we even call GetTagName when no tags exist?

@mrsdizzie
Copy link
Member Author

I believe we don't know if a repo has tags or not in this context and if you can run a single command to both find that out and print the tag if so it's better than running two (one to look for tags in general, one to check commit for tag).

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 12, 2020
@techknowlogick techknowlogick merged commit d729d68 into go-gitea:master Jun 12, 2020
@silverwind
Copy link
Member

silverwind commented Jun 12, 2020

You're right. This does fix the issue by causing git to trigger the other (expected) error:

$ git describe --tags --exact-match --tags HEAD
fatal: No names found, cannot describe anything.
$ git describe --tags --exact-match --tags --always HEAD
fatal: no tag exactly matches '148269db9f5ce7e81c29136a73069ad518dad779'

I think we actually want to ignore all errors printed to stderr for both GetBranchName and GetTagName and only process stdout, that way we're also not dependant on matching error strings.

@mrsdizzie
Copy link
Member Author

Yes I suspect a more robust error handling not relying on a string is better (I know we do this in a lot of places with git).

This was just quick fix to avoid 500 for many commits

@zeripath
Copy link
Contributor

There's no other way to handle git errors I'm afraid. That's why I spent so long to force them to always be in English.

@silverwind
Copy link
Member

silverwind commented Jun 12, 2020

In some cases we do care about the error but in this case we probably don't. If there are no tags, we don't care about why git can't find them. I was thinking about adding a boolean flag ignoreErrors to GetTagName and GetBranchName that would omit passing err if given.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 12, 2020

Thanks for the fix, really annoying

ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
go-gitea#11846 Introduced feature to show exact tag on commit view. However if a repo has no tags at all git prints out a separate and unhandled error " No names found, cannot describe anything."

 Adding --always to the command makes it always use the error in the style of "fatal: no tag exactly matches" even if there are no tags at all.

 Fixes go-gitea#11869
 Fixes go-gitea#11868
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]When view commit detail it returns 500 Listing changes - 500 error
6 participants