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 git hooks prompt #26258

Closed

Conversation

CaiCandong
Copy link
Member

close #26129

Before:

After:

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 1, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 1, 2023
@CaiCandong
Copy link
Member Author

There is another bug here that can lead to the generation of dirty data: since Git operations rely on the creation of database PRs, when a Git operation fails, it's unable to roll back the changes in the database, resulting in the generation of dirty data.

Anyone have a good idea how to fix this bug?

  • Screenshot
    gif
  • Related code
    if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil {
    return err
    }
    for _, assigneeID := range assigneeIDs {
    if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID); err != nil {
    return err
    }
    }
    pr.Issue = pull
    pull.PullRequest = pr
    // Now - even if the request context has been cancelled as the PR has been created
    // in the db and there is no way to cancel that transaction we have to proceed - therefore
    // create new context and work from there
    prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index))
    defer finished()
    if pr.Flow == issues_model.PullRequestFlowGithub {
    err = PushToBaseRepo(prCtx, pr)
    } else {
    err = UpdateRef(prCtx, pr)

@CaiCandong CaiCandong marked this pull request as ready for review August 3, 2023 06:47
@CaiCandong
Copy link
Member Author

Anyone have a good idea how to fix this bug?

The bug will be fixed #26259

@delvh delvh added the type/bug label Aug 5, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 5, 2023
@wxiaoguang
Copy link
Contributor

I do not think the UI is right.

image

@CaiCandong
Copy link
Member Author

I do not think the UI is right.

The prompt message is from a user-set hook, which isn't very pretty, but giving a prompt is a little friendlier than the previous 500.

@wxiaoguang
Copy link
Contributor

But there are full of HTML tags. Are these tags from the git hook?

@CaiCandong
Copy link
Member Author

But there are full of HTML tags. Are these tags from the git hook?

HTML tags are not from a git hook, now I get what you're saying.

routers/web/repo/pull.go Outdated Show resolved Hide resolved
silverwind pushed a commit that referenced this pull request Aug 10, 2023
Fix #26129
Replace #26258 

This PR will introduce a transaction on creating pull request so that if
some step failed, it will rollback totally. And there will be no dirty
pull request exist.

---------

Co-authored-by: Giteabot <teabot@gitea.io>
@CaiCandong
Copy link
Member Author

I haven't figured out how this Flash works.,why I only get network errors with the following function?@zeripath

ctx.Flash.Error(flashError, true)

@yp05327
Copy link
Contributor

yp05327 commented Aug 10, 2023

I think we can not use Flash error here, we should use JSONError and handling the msg by js.
But we are using tippy to show this error msg which can not render html code correctly,
and we need do more changes as the comment said here:

// TODO: use a better toast UI instead of the tippy. If the form height is large, the tippy position is not good

@silverwind
Copy link
Member

The toast module may be more suitable, but it currently also escapes HTML for security reason. If we are sure there is nothing user-supplied in these messages, we could add an option to disable HTML escaping. Same is also possible for Tippy, but generally I'm wary because the screenshot above does show it contains user data.

https://github.com/go-gitea/gitea/blob/main/web_src/js/modules/toast.js

lunny added a commit to lunny/gitea that referenced this pull request Aug 10, 2023
…ea#26259)

Fix go-gitea#26129
Replace go-gitea#26258

This PR will introduce a transaction on creating pull request so that if
some step failed, it will rollback totally. And there will be no dirty
pull request exist.

---------

Co-authored-by: Giteabot <teabot@gitea.io>
@yp05327
Copy link
Contributor

yp05327 commented Aug 17, 2023

Maybe we can remove Details in the error message?
Then using toast will not cause security problems.

ps: Did #26259 fully fix #26129? #26129 is closed now.

@CaiCandong CaiCandong closed this Aug 18, 2023
@CaiCandong CaiCandong deleted the fix-git-hooks-pre-receive-502-error branch September 4, 2023 12:40
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Git Hooks (pre-receive) with fail check drops Gitea to 502 Error
6 participants