From f95fb7de9a41252be76195db48e73d38eb4d691a Mon Sep 17 00:00:00 2001 From: "Ing. Jaroslav Safka" Date: Mon, 23 May 2022 07:59:43 +0200 Subject: [PATCH 1/3] Fix checks in PR for empty commits #19603 * Fixes issue #19603 * fill HeadCommitID in PullRequest * compare real commits ID as check for merging --- routers/web/repo/pull.go | 1 + services/pull/patch.go | 15 +++------------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 6e8f575ad54a5..846f398cf4346 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1210,6 +1210,7 @@ func CompareAndPullRequestPost(ctx *context.Context) { BaseRepo: repo, MergeBase: ci.CompareInfo.MergeBase, Type: issues_model.PullRequestGitea, + HeadCommitID: ci.CompareInfo.HeadCommitID, AllowMaintainerEdit: form.AllowMaintainerEdit, } // FIXME: check error in the case two people send pull request at almost same time, give nice error prompt diff --git a/services/pull/patch.go b/services/pull/patch.go index c7a69501c32f0..8b63e191df4f2 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -284,18 +284,9 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * } if !conflict { - var treeHash string - treeHash, _, err = git.NewCommand(ctx, "write-tree").RunStdString(&git.RunOpts{Dir: tmpBasePath}) - if err != nil { - return false, err - } - treeHash = strings.TrimSpace(treeHash) - baseTree, err := gitRepo.GetTree("base") - if err != nil { - return false, err - } - if treeHash == baseTree.ID.String() { - log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) + log.Info("PullRequest[%d]: Head SHA: %s, Merge base SHA: %s", pr.ID, pr.HeadCommitID, pr.MergeBase) + if pr.HeadCommitID == pr.MergeBase { + log.Debug("PullRequest[%d]: Commits are equal - ignoring", pr.ID) pr.Status = issues_model.PullRequestStatusEmpty } From 4aab46f8710bbe1f36d858da9040a36386dd493a Mon Sep 17 00:00:00 2001 From: "Ing. Jaroslav Safka" Date: Mon, 23 May 2022 09:06:35 +0200 Subject: [PATCH 2/3] Fix integration test TestPullCreate_EmptyChanges.. --- integrations/pull_status_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/integrations/pull_status_test.go b/integrations/pull_status_test.go index a5247f56ec5f3..28e8339c7a62e 100644 --- a/integrations/pull_status_test.go +++ b/integrations/pull_status_test.go @@ -105,7 +105,12 @@ func doAPICreateCommitStatus(ctx APITestContext, commitID string, status api.Com } } -func TestPullCreate_EmptyChangesWithCommits(t *testing.T) { +func TestPullCreate_EmptyChangesWithDifferentCommits(t *testing.T) { + // Merge must continue if commits SHA are different, even if content is same + // Reason: gitflow and merging master back into develop, where is high possiblity, there are no changes + // but just commit saying "Merge branch". And this meta commit can be also tagged, + // so we need to have this meta commit also in develop branch. + onGiteaRun(t, func(t *testing.T, u *url.URL) { session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") @@ -126,6 +131,6 @@ func TestPullCreate_EmptyChangesWithCommits(t *testing.T) { doc := NewHTMLParser(t, resp.Body) text := strings.TrimSpace(doc.doc.Find(".merge-section").Text()) - assert.Contains(t, text, "This branch is equal with the target branch.") + assert.Contains(t, text, "This pull request can be merged automatically.") }) } From 9e6169c6217321e2aa663e272ec890fa3de3eabd Mon Sep 17 00:00:00 2001 From: "Ing. Jaroslav Safka" Date: Thu, 26 May 2022 05:49:16 +0200 Subject: [PATCH 3/3] Add test for same commits in PR * added explaining comment into patch.go * change log.info into log.debug --- integrations/pull_status_test.go | 24 ++++++++++++++++++++++++ services/pull/patch.go | 7 ++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/integrations/pull_status_test.go b/integrations/pull_status_test.go index 28e8339c7a62e..5495486b75899 100644 --- a/integrations/pull_status_test.go +++ b/integrations/pull_status_test.go @@ -134,3 +134,27 @@ func TestPullCreate_EmptyChangesWithDifferentCommits(t *testing.T) { assert.Contains(t, text, "This pull request can be merged automatically.") }) } + +func TestPullCreate_EmptyChangesWithSameCommits(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + session := loginUser(t, "user1") + testRepoFork(t, session, "user2", "repo1", "user1", "repo1") + testCreateBranch(t, session, "user1", "repo1", "branch/master", "status1", http.StatusSeeOther) + + url := path.Join("user1", "repo1", "compare", "master...status1") + req := NewRequestWithValues(t, "POST", url, + map[string]string{ + "_csrf": GetCSRF(t, session, url), + "title": "pull request from status1", + }, + ) + session.MakeRequest(t, req, http.StatusSeeOther) + + req = NewRequest(t, "GET", "/user1/repo1/pulls/1") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + + text := strings.TrimSpace(doc.doc.Find(".merge-section").Text()) + assert.Contains(t, text, "This branch is equal with the target branch.") + }) +} diff --git a/services/pull/patch.go b/services/pull/patch.go index 8b63e191df4f2..ac8548188b08f 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -284,8 +284,13 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * } if !conflict { - log.Info("PullRequest[%d]: Head SHA: %s, Merge base SHA: %s", pr.ID, pr.HeadCommitID, pr.MergeBase) + log.Debug("PullRequest[%d]: Head SHA: %s, Merge base SHA: %s", pr.ID, pr.HeadCommitID, pr.MergeBase) if pr.HeadCommitID == pr.MergeBase { + // Merge must continue if commits SHA are different, even if content is same + // Reason: gitflow and merging master back into develop, where is high possiblity, there are no changes + // but just commit saying "Merge branch". And this meta commit can be also tagged, + // so we need to have this meta commit also in develop branch. + log.Debug("PullRequest[%d]: Commits are equal - ignoring", pr.ID) pr.Status = issues_model.PullRequestStatusEmpty }