Redirect open PRs when the target branch is deleted through merging a PR #28539

tulzke · 2023-12-19T19:16:51Z

Added a change the destination branch for those pull requests whose destination branch was deleted as a result of merge.
PR's HEAD changes to HEAD of the merged PR.

Resolves #27062

… as a result of the PR merge

denyskon · 2023-12-19T20:14:04Z

Looks like a duplicate of #18478 to me, but with a slightly simpler implementation

tulzke · 2023-12-27T11:08:10Z

@denyskon , thx for reply.
Yes, it looks like it.
But the PR you are referring to does not look active. He is almost two years old, and he is still not in master.
But me and our team really needs this feature.
What needs to be done so that we finally see this feature in the next minor version?

denyskon · 2023-12-27T11:11:30Z

This implementation looks clean. I will try to test it out during the next few days :)
I think however that it counts as a new feature, so according to our policy it would not be released before Gitea 1.22

denyskon

According to my tests, this feature seems to work great, and the implementation is a lot simpler than in the linked PR 👍

routers/web/repo/pull.go

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>

tulzke · 2024-01-02T19:06:26Z

Hello @denyskon. Thanks for the review.
I re-read my code and realized that assigning ctx.Repo.Repository to prTohead.Issue.Repo is not valid in all cases.
I added a condition to check that both issues have the same RepoId. If this is not the case, then Repo is loaded from the database.

I apologize for making changes after the review.

delvh · 2024-01-03T11:16:41Z

routers/web/repo/pull.go

+		pullRequestsToHead, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, pr.HeadRepoID, pr.HeadBranch)
+		if err != nil {
+			ctx.ServerError("GetUnmergedPullRequestsByBaseInfo", err)
+			return
+		}
+
+		for _, prToHead := range pullRequestsToHead {
+			if prToHead.BaseRepoID != pr.BaseRepoID {
+				continue
+			}
+
+			if err := prToHead.LoadIssue(ctx); err != nil {
+				ctx.ServerError(fmt.Sprintf("LoadIssueForPullRequest[%d]", prToHead.ID), err)
+				return
+			}
+
+			if prToHead.Issue.RepoID == pr.Issue.RepoID {
+				prToHead.Issue.Repo = pr.Issue.Repo
+			} else {
+				if err := prToHead.Issue.LoadRepo(ctx); err != nil {
+					ctx.ServerError(fmt.Sprintf("LoadRepoForIssue[%d]", prToHead.IssueID), err)
+					return
+				}
+			}
+
+			if err := pull_service.ChangeTargetBranch(ctx, prToHead, ctx.Doer, pr.BaseBranch); err != nil {
+				ctx.ServerError(fmt.Sprintf("ChangeTargetBranch[%d]", prToHead.ID), err)
+				return
+			}
+		}


Could we please extract this code into a new subfunction?
Something like redirectOpenPulls or so?

I think it's a good idea. This will allow you to use redirection in the API, as @kvaster noted.

delvh · 2024-01-03T11:31:32Z

Also, I'm currently wondering how edge cases will behave.
Do we run into any sort of security issue with this implementation, where someone can suddenly see something they are not supposed to see?
So, assume the following example:
We have

public repo A
fork B of A
private repo C

The owner of A has been given collaboration permissions on C.
A opens a PR targeting C and checks delete branch on merge.
Similarly, B opens a PR targeting the base branch of A.
As PR is merged.
So, is Bs PR now targeting C, a repo it doesn't even know of?
Am I missing something, or isn't this state possible, and it is a low priority security issue?

denyskon · 2024-01-03T14:00:05Z

@delvh According to

A fork of a private repository remains private. So how should A in your case become public? 🤔

kvaster · 2024-01-03T14:39:01Z

I think that this PR is almost the same as mine (#18478), but it covers even less when mine:

This PR covers only web/repo/pull, but it do nothing with api - routers/api/v1/repo/pull and it should, I think.

And also both PRs do not cover following case:

Repo A.
Repo B is a fork of repo A
PR1 is a PR from B to A
PR2 is a PR from B to PR branch in B

What should happen in case PR1 is merged and deleted ?

PR2 should be closed
PR2 should be retarget to PR1's branch, but in repo B ?
PR2 should be retarget to PR's target - i.e. to repo A ? But in this case we should also MOVE somehow PR2 to repo A...

kvaster · 2024-01-03T14:46:09Z

So, I think, that we should:

cover all entry points for merging PR's
think of exact behaviour when base PR is targeting another repo

kvaster · 2024-01-03T17:51:02Z

@denyskon , thx for reply. Yes, it looks like it. But the PR you are referring to does not look active. He is almost two years old, and he is still not in master. But me and our team really needs this feature. What needs to be done so that we finally see this feature in the next minor version?

The PR was complete from my point of view. Probably nobody cared about it and I had no time/wish to push this topic. I was just updating PR from time to time. Also I had no time (no experiance in go) to write unit tests for it.

denyskon · 2024-01-03T18:04:16Z

@kvaster

To me, it doesn't matter which PR is merged in the end. As it already has two approvals, it would maybe happen faster here, but I'm also open to reviewing the other PR if needed.

I'd say we need the functionality for the API, which can easily be provided by the function that delvh suggested to add. As for the example you brought up, I don't think we should retarget these PRs as it would basically mean that PRs end up in a repo they maybe weren't intended to, without the author even being notified about it.

(cc @tulzke)

kvaster · 2024-01-03T18:35:04Z

@denyskon, to me it always metters if someone is spending his time to help.

I already wrote my concerns about current status of this PR:

Not all entry points for merging PR's are covered.
Behaviour is unpredictable in case base PR is targeting another repo.
pull_service.ChangeTargetBranch error is not checked and merge can fail even if it's not an error

kvaster · 2024-01-03T18:44:32Z

This is not a competition. I really want this feature to be merged aswell. I' ve reopened my PR in case somebody will want to make a review.

#28686

tulzke · 2024-01-04T08:55:53Z

Behaviour is unpredictable in case base PR is targeting another repo.
You are wrong, the behavior is quite predictable. If a redirect has not been made for the PR, then it will be closed as it is now.

I don't think redirecting PR to other repositories is a good idea.

pull_service.ChangeTargetBranch error is not checked and merge can fail even if it's not an error

The error is being checked. Why did you decide that this was not the case?

tulzke · 2024-01-04T08:58:24Z

@delvh in your case PR will be closed. The current behavior remained in place. If the PR has not been redirected to another branch, it will be closed.

kvaster · 2024-01-04T09:11:11Z

@tulzke, seems I've missed - you're really checking if base repo is the same.

For error checking - ChangeTargetBranch emits also following errors which are not errors:

issues_model.IsErrIssueIsClosed(err)
models.IsErrPullRequestHasMerged(err)
issues_model.IsErrPullRequestAlreadyExists(err)

Also deleteBranch is called in reouter/web/repo/pull in two places:

MergePullRequest
CleanUpPullRequest

both should be covered

Also routers/api/v1/repo/pull have method MergePullRequest - this one also should be covered.

As I said before - I really don't know why we should go through this once again.

…ranch. Added function call to api/v1

tulzke · 2024-01-09T10:28:55Z

In the end, I don't care which PR is merged. I just want to see this feature in the next minor release.

I took the logic out into a separate function, and also added a call to the api.
Of the three voiced errors, it makes sense to check only one of them: issues_model.IsErrPullRequestAlreadyExists(err).

It doesn't make sense to check these two errors, because we only work with open PRS:

issues_model.IsErrIssueIsClosed(err)
models.IsErrPullRequestHasMerged(err)

kvaster · 2024-01-09T10:41:47Z

@tulzke, my thoughts about issues_model.IsErrIssueIsClosed(err) and models.IsErrPullRequestHasMerged(err) were only about what should happen if someone will merge child PR concurrently.

Also what do you think about routers/api/v1/repo/pull and CleanUpPullRequest method ?

tulzke · 2024-01-09T11:02:25Z

@kvaster

routers/api/v1/repo/pull

Yes, i added redirect to MergePullRequest.

CleanUpPullRequest

I think you're right.
I hadn't thought about this method before, because I always deleted the branch during the merge.
In this case, the implementation almost completely copies yours, except for the presence of a feature-flag.

@denyskon @delvh @a1012112796 Can we review @kvaster's proposed PR? It looks like everything is ready there.

denyskon · 2024-01-14T12:26:27Z

Okay, I think we can agree on closing this in favor of #28686....

tulzke · 2024-01-14T14:33:40Z

Okay, I think we can agree on closing this in favor of #28686....

Good news, thanks

Added PR redirection to a new branch if the target branch was deleted…

6da6ae0

… as a result of the PR merge

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 19, 2023

pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 19, 2023

denyskon added the giteabot/update-branch Hint for the bot that it should update a PR with the latest state on main label Dec 30, 2023

Merge branch 'main' into feature/retarget

2bcdbd2

GiteaBot removed the giteabot/update-branch Hint for the bot that it should update a PR with the latest state on main label Dec 30, 2023

denyskon approved these changes Dec 30, 2023

View reviewed changes

routers/web/repo/pull.go Outdated Show resolved Hide resolved

routers/web/repo/pull.go Outdated Show resolved Hide resolved

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 Dec 30, 2023

tulzke and others added 3 commits January 2, 2024 21:35

Update routers/web/repo/pull.go

f1ec56c

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>

Update routers/web/repo/pull.go

19ffb87

Co-authored-by: Denys Konovalov <kontakt@denyskon.de>

Added check of repoID. If repoID is different, LoadRepo is used

ce88869

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 2, 2024

a1012112796 approved these changes Jan 3, 2024

View reviewed changes

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 Jan 3, 2024

denyskon self-requested a review January 3, 2024 08:31

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Jan 3, 2024

denyskon added the giteabot/update-branch Hint for the bot that it should update a PR with the latest state on main label Jan 3, 2024

Merge branch 'main' into feature/retarget

17bbdd2

GiteaBot removed the giteabot/update-branch Hint for the bot that it should update a PR with the latest state on main label Jan 3, 2024

denyskon approved these changes Jan 3, 2024

View reviewed changes

This was referenced Jan 3, 2024

Retarget Pulls Based on Pulls on Merge #18478

Closed

Allow cascade PRs #18408

Closed

GiteaBot added 2 commits January 3, 2024 18:43

Merge branch 'main' into feature/retarget

e40af8a

Merge branch 'main' into feature/retarget

e3cb347

delvh reviewed Jan 3, 2024

View reviewed changes

delvh changed the title ~~Added PR redirection to a new branch if the target branch was deleted~~ Redirect open PRs when the target branch is deleted through merging a PR Jan 3, 2024

Merge branch 'main' into feature/retarget

8ab7efc

denyskon removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 3, 2024

6543 and others added 2 commits January 4, 2024 18:41

Merge branch 'main' into feature/retarget

ed7dbe3

The logic has been moved to the new function RedirectOpenPullsToBaseB…

9e4e6ab

…ranch. Added function call to api/v1

github-actions bot added the modifies/api This PR adds API routes or modifies them label Jan 9, 2024

denyskon closed this Jan 14, 2024

GiteaBot removed this from the 1.22.0 milestone Jan 14, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redirect open PRs when the target branch is deleted through merging a PR #28539

Redirect open PRs when the target branch is deleted through merging a PR #28539

tulzke commented Dec 19, 2023 •

edited

Loading

denyskon commented Dec 19, 2023

tulzke commented Dec 27, 2023

denyskon commented Dec 27, 2023

denyskon left a comment

tulzke commented Jan 2, 2024

delvh Jan 3, 2024

tulzke Jan 4, 2024

delvh commented Jan 3, 2024 •

edited

Loading

denyskon commented Jan 3, 2024

kvaster commented Jan 3, 2024 •

edited

Loading

kvaster commented Jan 3, 2024

kvaster commented Jan 3, 2024

denyskon commented Jan 3, 2024 •

edited

Loading

kvaster commented Jan 3, 2024

kvaster commented Jan 3, 2024

tulzke commented Jan 4, 2024

tulzke commented Jan 4, 2024

kvaster commented Jan 4, 2024

tulzke commented Jan 9, 2024

kvaster commented Jan 9, 2024

tulzke commented Jan 9, 2024 •

edited

Loading

denyskon commented Jan 14, 2024

tulzke commented Jan 14, 2024

Redirect open PRs when the target branch is deleted through merging a PR #28539

Redirect open PRs when the target branch is deleted through merging a PR #28539

Conversation

tulzke commented Dec 19, 2023 • edited Loading

denyskon commented Dec 19, 2023

tulzke commented Dec 27, 2023

denyskon commented Dec 27, 2023

denyskon left a comment

Choose a reason for hiding this comment

tulzke commented Jan 2, 2024

delvh Jan 3, 2024

Choose a reason for hiding this comment

tulzke Jan 4, 2024

Choose a reason for hiding this comment

delvh commented Jan 3, 2024 • edited Loading

denyskon commented Jan 3, 2024

kvaster commented Jan 3, 2024 • edited Loading

kvaster commented Jan 3, 2024

kvaster commented Jan 3, 2024

denyskon commented Jan 3, 2024 • edited Loading

kvaster commented Jan 3, 2024

kvaster commented Jan 3, 2024

tulzke commented Jan 4, 2024

tulzke commented Jan 4, 2024

kvaster commented Jan 4, 2024

tulzke commented Jan 9, 2024

kvaster commented Jan 9, 2024

tulzke commented Jan 9, 2024 • edited Loading

denyskon commented Jan 14, 2024

tulzke commented Jan 14, 2024

tulzke commented Dec 19, 2023 •

edited

Loading

delvh commented Jan 3, 2024 •

edited

Loading

kvaster commented Jan 3, 2024 •

edited

Loading

denyskon commented Jan 3, 2024 •

edited

Loading

tulzke commented Jan 9, 2024 •

edited

Loading