Skip to content

Ignore trivial errors when updating push data #33864

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

Merged
merged 4 commits into from
Mar 14, 2025
Merged

Ignore trivial errors when updating push data #33864

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d4655b0
fix
wxiaoguang Mar 13, 2025
38c48dd
remove useless warning "unknown whitespace behavior"
wxiaoguang Mar 13, 2025
c5e41c2
Merge branch 'main' into fix-push-tag-err
wxiaoguang Mar 14, 2025
fd2d006
Update services/repository/push.go
wxiaoguang Mar 14, 2025
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
36 changes: 20 additions & 16 deletions services/repository/push.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
repo_module "code.gitea.io/gitea/modules/repository"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
issue_service "code.gitea.io/gitea/services/issue"
notify_service "code.gitea.io/gitea/services/notify"
pull_service "code.gitea.io/gitea/services/pull"
Expand Down Expand Up @@ -133,23 +134,26 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
} else { // is new tag
newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
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)
}
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. The "tag" could point to anything. It just happened to point to a blob, and in this case GetCommit considers it as the commit doesn't exist (it is right). I do not see a bad case for this approach.
  2. Why log all errors?

Copy link

Choose a reason for hiding this comment

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

  1. Alright, I think your explanation makes sense.
  2. Do we not need to record errors: there is a tag pointing to a non-existent commit?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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.

} else {
commits := repo_module.NewPushCommits()
commits.HeadCommit = repo_module.CommitToPushCommit(newCommit)
commits.CompareURL = repo.ComposeCompareURL(objectFormat.EmptyObjectID().String(), opts.NewCommitID)

notify_service.PushCommits(
ctx, pusher, repo,
&repo_module.PushUpdateOptions{
RefFullName: opts.RefFullName,
OldCommitID: objectFormat.EmptyObjectID().String(),
NewCommitID: opts.NewCommitID,
}, commits)

addTags = append(addTags, tagName)
notify_service.CreateRef(ctx, pusher, repo, opts.RefFullName, opts.NewCommitID)
}

commits := repo_module.NewPushCommits()
commits.HeadCommit = repo_module.CommitToPushCommit(newCommit)
commits.CompareURL = repo.ComposeCompareURL(objectFormat.EmptyObjectID().String(), opts.NewCommitID)

notify_service.PushCommits(
ctx, pusher, repo,
&repo_module.PushUpdateOptions{
RefFullName: opts.RefFullName,
OldCommitID: objectFormat.EmptyObjectID().String(),
NewCommitID: opts.NewCommitID,
}, commits)

addTags = append(addTags, tagName)
notify_service.CreateRef(ctx, pusher, repo, opts.RefFullName, opts.NewCommitID)
}
} else if opts.RefFullName.IsBranch() {
if pusher == nil || pusher.ID != opts.PusherID {
Expand Down