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 the handling of closed PR when push commits #23830

Closed

Conversation

sillyguodong
Copy link
Contributor

After #23189 be mergerd, casue some issues, like #23707, #23440.
This PR is mainly to improve the handling of closed PR.

Changes

  1. When pushing a commit, these behaviour might be performed: insert comment, push notification (UI and email), exectue pushToBaseRepo func and trigger action. But not all of these behaviour have to be performed in different cases.
  • The PR is unmerged but open:
    • All the behaviour mentioned above will be performed.
  • The PR is unmerged but closed:
    • not tigger action, not push notification
    • If all the changes of the PR have been merged into the base branch (the head commit of PR already in the base branch of PR), no need to insert comment and execute pushToBaseRepo()
    • If the changes of the PR have not been merged into the base branch, insert comment (if reopen the PR, display in the UI), execute pushToBaseRepo() (pushes commits to the base branch of the base repo)
  1. If user try to reopen a PR of which changes have been merged, the below error message wiil be displayed.
    image

routers/web/repo/issue.go Outdated Show resolved Hide resolved
services/pull/pull.go Outdated Show resolved Hide resolved
services/pull/pull.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 31, 2023
sillyguodong and others added 4 commits March 31, 2023 22:29
Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: Jason Song <i@wolfogre.com>
Co-authored-by: silverwind <me@silverwind.io>
@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 Mar 31, 2023
@techknowlogick techknowlogick added this to the 1.20.0 milestone Mar 31, 2023
}
prHeadCommitID, err := git.GetFullCommitID(ctx, pull.BaseRepo.RepoPath(), prHeadRef)
if err != nil {
ctx.ServerError("Get head commit Id fail", err)
Copy link
Member

Choose a reason for hiding this comment

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

Should we return 500 directly? Maybe just returning a flash with error is better.

defer closer.Close()
ok, err := baseRepo.IsCommitInBranch(prHeadCommitID, pull.BaseBranch)
if err != nil {
ctx.ServerError("", err)
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@sillyguodong sillyguodong changed the title Fix the handing of closed PR when push commits Fix the handling of closed PR when push commits Apr 3, 2023
@sillyguodong
Copy link
Contributor Author

replaced by #24231
so close this

@lunny lunny removed this from the 1.20.0 milestone Apr 20, 2023
lunny pushed a commit that referenced this pull request May 8, 2023
Close #24213 
Replace #23830

#### Cause

- Before, in order to making PR can get latest commit after reopening,
the `ref`(${REPO_PATH}/refs/pull/${PR_INDEX}/head) of evrey closed PR
will be updated when pushing commits to the `head branch` of the closed
PR.

#### Changes

- For closed PR , won't perform these behavior: insert`comment`, push
`notification` (UI and email), exectue
[pushToBaseRepo](https://github.com/go-gitea/gitea/blob/74225033413dc0f2b308bbe069f6d185b551e364/services/pull/pull.go#L409)
function and trigger `action` any more when pushing to the `head branch`
of the closed PR.
- Refresh the reference of the PR when reopening the closed PR (**even
if the head branch has been deleted before**). Make the reference of PR
consistent with the `head branch`.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request May 8, 2023
Close go-gitea#24213 
Replace go-gitea#23830

#### Cause

- Before, in order to making PR can get latest commit after reopening,
the `ref`(${REPO_PATH}/refs/pull/${PR_INDEX}/head) of evrey closed PR
will be updated when pushing commits to the `head branch` of the closed
PR.

#### Changes

- For closed PR , won't perform these behavior: insert`comment`, push
`notification` (UI and email), exectue
[pushToBaseRepo](https://github.com/go-gitea/gitea/blob/74225033413dc0f2b308bbe069f6d185b551e364/services/pull/pull.go#L409)
function and trigger `action` any more when pushing to the `head branch`
of the closed PR.
- Refresh the reference of the PR when reopening the closed PR (**even
if the head branch has been deleted before**). Make the reference of PR
consistent with the `head branch`.
silverwind pushed a commit that referenced this pull request May 8, 2023
Backport #24231 by @sillyguodong

Close #24213 
Replace #23830

#### Cause

- Before, in order to making PR can get latest commit after reopening,
the `ref`(${REPO_PATH}/refs/pull/${PR_INDEX}/head) of evrey closed PR
will be updated when pushing commits to the `head branch` of the closed
PR.

#### Changes

- For closed PR , won't perform these behavior: insert`comment`, push
`notification` (UI and email), exectue
[pushToBaseRepo](https://github.com/go-gitea/gitea/blob/74225033413dc0f2b308bbe069f6d185b551e364/services/pull/pull.go#L409)
function and trigger `action` any more when pushing to the `head branch`
of the closed PR.
- Refresh the reference of the PR when reopening the closed PR (**even
if the head branch has been deleted before**). Make the reference of PR
consistent with the `head branch`.

Co-authored-by: sillyguodong <33891828+sillyguodong@users.noreply.github.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
@sillyguodong sillyguodong deleted the bugfix/pull_request_bugs branch February 29, 2024 03:30
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. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants