diff --git a/cmd/hook.go b/cmd/hook.go index aabf08851b..d92e84b289 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -452,10 +452,17 @@ func hookPrintResults(results []private.HookPostReceiveBranchResult) { fmt.Fprintln(os.Stderr, "") if res.Create { fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", res.Branch) - fmt.Fprintf(os.Stderr, " %s\n", res.URL) - } else { - fmt.Fprint(os.Stderr, "Visit the existing pull request:\n") - fmt.Fprintf(os.Stderr, " %s\n", res.URL) + fmt.Fprintf(os.Stderr, " %s\n", res.CreateURL) + } + if len(res.PullURLS) != 0 { + if len(res.PullURLS) >= 2 { + fmt.Fprint(os.Stderr, "Visit the existing pull requests:\n") + } else { + fmt.Fprint(os.Stderr, "Visit the existing pull request:\n") + } + for _, url := range res.PullURLS { + fmt.Fprintf(os.Stderr, " %s\n", url) + } } fmt.Fprintln(os.Stderr, "") _ = os.Stderr.Sync() diff --git a/models/issues/pull.go b/models/issues/pull.go index 6f65ef3af9..429fddf145 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -632,6 +632,21 @@ func GetUnmergedPullRequest(ctx context.Context, headRepoID, baseRepoID int64, h return pr, nil } +// GetUnmergedPullRequestsAnyTarget returns a pull request that is open and has not been merged +// by given head repo and branch and targeting any other branch on the baseRepo +func GetUnmergedPullRequestsAnyTarget(ctx context.Context, headRepoID, baseRepoID int64, headBranch string, flow PullRequestFlow) (PullRequestList, error) { + var pr PullRequestList + err := db.GetEngine(ctx). + Where("head_repo_id=? AND head_branch=? AND base_repo_id=? AND has_merged=? AND flow = ? AND issue.is_closed=?", + headRepoID, headBranch, baseRepoID, false, flow, false). + Join("INNER", "issue", "issue.id=pull_request.issue_id"). + Find(&pr) + if err != nil { + return nil, err + } + return pr, nil +} + // GetLatestPullRequestByHeadInfo returns the latest pull request (regardless of its status) // by given head information (repo and branch). func GetLatestPullRequestByHeadInfo(ctx context.Context, repoID int64, branch string) (*PullRequest, error) { diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index dd97ed34b2..72ae7f2645 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -172,6 +172,11 @@ func TestGetUnmergedPullRequest(t *testing.T) { require.NoError(t, err) assert.Equal(t, int64(2), pr.ID) + prList, err := issues_model.GetUnmergedPullRequestsAnyTarget(db.DefaultContext, 1, 1, "branch2", issues_model.PullRequestFlowGithub) + require.NoError(t, err) + assert.Len(t, prList, 1) + assert.Equal(t, int64(2), prList[0].ID) + _, err = issues_model.GetUnmergedPullRequest(db.DefaultContext, 1, 9223372036854775807, "branch1", "master", issues_model.PullRequestFlowGithub) require.Error(t, err) assert.True(t, issues_model.IsErrPullRequestNotExist(err)) diff --git a/modules/private/hook.go b/modules/private/hook.go index 89d44119ea..e289f14bba 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -60,10 +60,11 @@ type HookPostReceiveResult struct { // HookPostReceiveBranchResult represents an individual branch result from PostReceive type HookPostReceiveBranchResult struct { - Message bool - Create bool - Branch string - URL string + Message bool + Create bool + Branch string + CreateURL string + PullURLS []string } // HookProcReceiveResult represents an individual result from ProcReceive diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 0acc09ab1b..f7013f14c6 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -231,10 +231,11 @@ func HookPostReceive(ctx *app_context.PrivateContext) { } results = append(results, private.HookPostReceiveBranchResult{ - Message: setting.Git.PullRequestPushMessage && repo.AllowsPulls(ctx), - Create: false, - Branch: "", - URL: fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index), + Message: setting.Git.PullRequestPushMessage && repo.AllowsPulls(ctx), + Create: false, + Branch: "", + CreateURL: "", + PullURLS: []string{fmt.Sprintf("%s/pulls/%d", repo.HTMLURL(), pr.Index)}, }) continue } @@ -281,35 +282,57 @@ func HookPostReceive(ctx *app_context.PrivateContext) { continue } - pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, baseRepo.ID, branch, baseRepo.DefaultBranch, issues_model.PullRequestFlowGithub) - if err != nil && !issues_model.IsErrPullRequestNotExist(err) { - log.Error("Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err) + // Check if there is an existing pull request for this branch. + prList, err := issues_model.GetUnmergedPullRequestsAnyTarget(ctx, repo.ID, baseRepo.ID, branch, issues_model.PullRequestFlowGithub) + if err != nil { + log.Error("Failed to get active PR in: %-v Branch: %s to: %-v Error: %v", repo, branch, baseRepo, err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ Err: fmt.Sprintf( - "Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err), + "Failed to get active PR in: %-v Branch: %s to: %-v Error: %v", repo, branch, baseRepo, err), + RepoWasEmpty: wasEmpty, + }) + return + } + err = prList.LoadRepositories(ctx) + if err != nil { + log.Error("Failed to load repositories for PullRequestList: %s", err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to load repositories for PullRequestList: %s", err), RepoWasEmpty: wasEmpty, }) return } - if pr == nil { - if repo.IsFork { - branch = fmt.Sprintf("%s:%s", repo.OwnerName, branch) - } - results = append(results, private.HookPostReceiveBranchResult{ - Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), - Create: true, - Branch: branch, - URL: fmt.Sprintf("%s/compare/%s...%s", baseRepo.HTMLURL(), util.PathEscapeSegments(baseRepo.DefaultBranch), util.PathEscapeSegments(branch)), - }) - } else { - results = append(results, private.HookPostReceiveBranchResult{ - Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), - Create: false, - Branch: branch, - URL: fmt.Sprintf("%s/pulls/%d", baseRepo.HTMLURL(), pr.Index), - }) + if repo.IsFork { + branch = fmt.Sprintf("%s:%s", repo.OwnerName, branch) } + createURL := fmt.Sprintf("%s/compare/%s...%s", baseRepo.HTMLURL(), util.PathEscapeSegments(baseRepo.DefaultBranch), util.PathEscapeSegments(branch)) + var urls []string + foundDefaultBranch := false + for _, pr := range prList { + var baseBranchDisplay string + if pr.HeadRepoID == pr.BaseRepoID { + // Inside the same repository: just show base branch name + baseBranchDisplay = pr.BaseBranch + } else { + // We are merging this into another repo: display user/repo:branch + baseBranchDisplay = fmt.Sprintf("%s:%s", pr.BaseRepo.FullName(), pr.BaseBranch) + } + urls = append(urls, fmt.Sprintf("%s/pulls/%d merges into %s", baseRepo.HTMLURL(), pr.Index, baseBranchDisplay)) + if pr.BaseBranch == baseRepo.DefaultBranch { + foundDefaultBranch = true + } + } + if foundDefaultBranch { + createURL = "" + } + results = append(results, private.HookPostReceiveBranchResult{ + Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), + Create: !foundDefaultBranch, + Branch: branch, + CreateURL: createURL, + PullURLS: urls, + }) } } ctx.JSON(http.StatusOK, private.HookPostReceiveResult{ diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index 015d1e0ad4..2a68f22f93 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -116,6 +116,8 @@ func testGit(t *testing.T, u *url.URL) { rawTest(t, &httpContext, little, big, littleLFS, bigLFS) mediaTest(t, &httpContext, little, big, littleLFS, bigLFS) + t.Run("PushRemoteMessages", doTestPushMessages(httpContext, u, objectFormat)) + t.Run("PushForkRemoteMessages", doTestForkPushMessages(httpContext, dstPath)) t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "test/head")) t.Run("InternalReferences", doInternalReferences(&httpContext, dstPath)) t.Run("BranchProtect", doBranchProtect(&httpContext, dstPath)) @@ -163,6 +165,8 @@ func testGit(t *testing.T, u *url.URL) { rawTest(t, &sshContext, little, big, littleLFS, bigLFS) mediaTest(t, &sshContext, little, big, littleLFS, bigLFS) + t.Run("PushRemoteMessages", doTestPushMessages(sshContext, u, objectFormat)) + t.Run("PushForkRemoteMessages", doTestForkPushMessages(sshContext, dstPath)) t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "test/head2")) t.Run("InternalReferences", doInternalReferences(&sshContext, dstPath)) t.Run("BranchProtect", doBranchProtect(&sshContext, dstPath)) @@ -1156,3 +1160,108 @@ func doLFSNoAccess(ctx APITestContext, publicKeyID int64, objectFormat git.Objec } } } + +func extractRemoteMessages(stderr string) string { + var remoteMsg string + for line := range strings.SplitSeq(stderr, "\n") { + msg, found := strings.CutPrefix(line, "remote: ") + if found { + remoteMsg += msg + remoteMsg += "\n" + } + } + return remoteMsg +} + +func doTestForkPushMessages(apictx APITestContext, dstPath string) func(*testing.T) { + return func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + doGitCheckoutBranch(dstPath, "-b", "test_msg")(t) + + // Commit/Push on test_msg branch + generateCommitWithNewData(t, littleSize, dstPath, "user2@example.com", "User Two", "testmsg-file") + _, stderr, err := git.NewCommand(git.DefaultContext, "push", "-u", "origin", "test_msg").RunStdString(&git.RunOpts{Dir: dstPath}) // Push + require.NoError(t, err) + + messages := extractRemoteMessages(stderr) + // Remote server should suggest the creation of a pull request to the default branch "master" + require.Contains(t, messages, "Create a new pull request for 'user2:test_msg':") + // But shouldn't show "Visit existing pull requests..." + require.NotContains(t, messages, "Visit") + // Shouldn't contain links to pull requests + require.NotContains(t, messages, "/pulls") + require.NotContains(t, messages, "merges into") + + // Create PR to default branch and push new commit + pr, giterr := doAPICreatePullRequest(apictx, "user2", apictx.Reponame, "master", "test_msg")(t) + require.NoError(t, giterr) + generateCommitWithNewData(t, littleSize, dstPath, "user2@example.com", "User Two", "testmsg-file") + _, stderr, err = git.NewCommand(git.DefaultContext, "push").RunStdString(&git.RunOpts{Dir: dstPath}) + require.NoError(t, err) + messages = extractRemoteMessages(stderr) + // However, a pull request to the base repo doesn't exist + // Because we are a fork, only PRs to the base repo are displayed + require.Contains(t, messages, "Create a new pull request for 'user2:test_msg':") + require.Contains(t, messages, apictx.Reponame+"/compare/master...user2") + require.NotContains(t, messages, "merges into") + require.NotContains(t, messages, pr.HTMLURL) + + // Finally merge the pull request and pull back changes to avoid polluting next tests + baseRepo := pr.Base.Repository + doAPIMergePullRequest(apictx, baseRepo.Owner.UserName, baseRepo.Name, pr.Index)(t) + doGitCheckoutBranch(dstPath, "master")(t) + doGitPull(dstPath)(t) + } +} + +func doTestPushMessages(ctx APITestContext, u *url.URL, objectFormat git.ObjectFormat) func(*testing.T) { + return func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + ctx.Reponame = fmt.Sprintf("repo-test-pushmsg-%s", objectFormat.Name()) + dstPath := t.TempDir() + + // Create/Clone new repo + doAPICreateRepository(ctx, nil, objectFormat)(t) + u.Path = ctx.GitPath() + doGitClone(dstPath, u)(t) + + // Push to master + generateCommitWithNewData(t, littleSize, dstPath, "user2@example.com", "User Two", "testmsg-file") + _, stderr, err := git.NewCommand(git.DefaultContext, "push", "-u", "origin", "master").RunStdString(&git.RunOpts{Dir: dstPath}) // Push + require.NoError(t, err) + messages := extractRemoteMessages(stderr) + // Remote server shouldn't suggest the creation of a pull request: we pushed to the default branch + require.NotContains(t, messages, "Create a new pull request for 'user2:testmsg':") + // ...and shouldn't give links to pull request: there is no PR yet + require.NotContains(t, messages, "Visit the existing") + // Shouldn't contain links to pull requests + require.NotContains(t, messages, "/pulls") + require.NotContains(t, messages, "merges into") + + // Create a branch and push to it + doGitCheckoutBranch(dstPath, "-b", "test_msg")(t) + generateCommitWithNewData(t, littleSize, dstPath, "user2@example.com", "User Two", "testmsg-file") + _, stderr, err = git.NewCommand(git.DefaultContext, "push", "-u", "origin", "test_msg").RunStdString(&git.RunOpts{Dir: dstPath}) + require.NoError(t, err) + messages = extractRemoteMessages(stderr) + // We pushed to a new branch and there is no PR yet: a link to create one should be given + require.Contains(t, messages, ctx.Reponame+"/compare/master...test_msg") + require.NotContains(t, messages, "/pulls") + require.NotContains(t, messages, "merges into") + + // Create PR to default branch and push new commit + pr, giterr := doAPICreatePullRequest(ctx, "user2", ctx.Reponame, "master", "test_msg")(t) + require.NoError(t, giterr) + generateCommitWithNewData(t, littleSize, dstPath, "user2@example.com", "User Two", "testmsg-file") + _, stderr, err = git.NewCommand(git.DefaultContext, "push", "origin", "test_msg").RunStdString(&git.RunOpts{Dir: dstPath}) + require.NoError(t, err) + messages = extractRemoteMessages(stderr) + // A pull request to the base branch already exist + require.NotContains(t, messages, "Create a new pull request") + require.Contains(t, messages, "Visit the existing pull request:") + require.Contains(t, messages, pr.HTMLURL+" merges into master") + + // Delete this test repository + doAPIDeleteRepository(ctx)(t) + } +}