Ignore trivial errors when updating push data #33864
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/M
|
Diff with ignoring spaces: https://github.com/go-gitea/gitea/pull/33864/files?diff=unified&w=1 |
wxiaoguang
force-pushed
the
fix-push-tag-err
branch
from
57b8d63 to
9b0b45b
Compare
wxiaoguang
force-pushed
the
fix-push-tag-err
branch
from
9b0b45b to
d4655b0
Compare
GiteaBot
added
lgtm/need 1
|
By the way, remove another useless warning: |
services/repository/push.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("gitRepo.GetCommit(%s) in %s/%s[%d]: %w", opts.NewCommitID, repo.OwnerName, repo.Name, repo.ID, err) | ||
| // in case there is dirty data, for example, the "github.com/git/git" repository has tags points to non-existing commits | ||
| if !errors.Is(err, util.ErrNotExist) { | ||
| log.Error("Unable to get tag commit: gitRepo.GetCommit(%s) in %s/%s[%d]: %v", opts.NewCommitID, repo.OwnerName, repo.Name, repo.ID, err) | ||
| } |
There was a problem hiding this comment.
Firstly, in this context, git/git does not refer to a tag pointing to a non-existent commit, but rather to a blob. Additionally, why not log all error conditions here?
There was a problem hiding this comment.
- The "tag" could point to anything. It just happened to point to a blob, and in this case
GetCommitconsiders it as the commit doesn't exist (it is right). I do not see a bad case for this approach. - Why log all errors?
There was a problem hiding this comment.
- Alright, I think your explanation makes sense.
- Do we not need to record errors: there is a tag pointing to a non-existent commit?
There was a problem hiding this comment.
Do we not need to record errors: there is a tag pointing to a non-existent commit?
Suppose the error is recorded, then what it would help in real world? Would end user be able to see that error message, or site admin would really regularly check the error message, or site admin could take any action to that error message?
There was a problem hiding this comment.
I think this is generally intended for Gitea administrators. This kind of error message can inform the administrators that a push webhook may be missing here.
GiteaBot
added
lgtm/done
Fix go-gitea#23213 (cherry picked from commit cb6b33c)
Fix #23213
This block is only used for sending notifications, and it is executed after the git repo has been successfully pushed. So we could safely ignore the error.
By the way, remove another useless warning:
unknown whitespace behavior: %q, default to 'show-all'. That parameter is provided by the end user, no need to warn that.