Fix checks in PR for empty commits (#20290) (#20352)

Backport #20290

* Fix #19603
* fill HeadCommitID in PullRequest
* compare real commits ID as check for merging


Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Andrew Thornton <[email protected]>

Author: jedi7

integrations/pull_status_test.go

@@ -105,7 +105,11 @@ 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 +130,28 @@ 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.")
+	})
+}
+
+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 already included in the target branch. There is nothing to merge.")
 	})
 }

models/issues/pull.go

@@ -122,6 +122,7 @@ const (
 	PullRequestStatusManuallyMerged
 	PullRequestStatusError
 	PullRequestStatusEmpty
+	PullRequestStatusAncestor
 )
 
 // PullRequestFlow the flow of pull request
@@ -423,6 +424,11 @@ func (pr *PullRequest) IsEmpty() bool {
 	return pr.Status == PullRequestStatusEmpty
 }
 
+// IsAncestor returns true if the Head Commit of this PR is an ancestor of the Base Commit
+func (pr *PullRequest) IsAncestor() bool {
+	return pr.Status == PullRequestStatusAncestor
+}
+
 // SetMerged sets a pull request to merged and closes the corresponding issue
 func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) {
 	if pr.HasMerged {

options/locale/locale_en-US.ini

@@ -1531,7 +1531,8 @@ pulls.remove_prefix = Remove <strong>%s</strong> prefix
 pulls.data_broken = This pull request is broken due to missing fork information.
 pulls.files_conflicted = This pull request has changes conflicting with the target branch.
 pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments."
-pulls.is_empty = "This branch is equal with the target branch."
+pulls.is_ancestor = "This branch is already included in the target branch. There is nothing to merge."
+pulls.is_empty = "The changes on this branch are already on the target branch. This will be an empty commit."
 pulls.required_status_check_failed = Some required checks were not successful.
 pulls.required_status_check_missing = Some required checks are missing.
 pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.

services/pull/check.go

@@ -89,7 +89,7 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
 			return ErrIsWorkInProgress
 		}
 
-		if !pr.CanAutoMerge() {
+		if !pr.CanAutoMerge() && !pr.IsEmpty() {
 			return ErrNotMergableState
 		}
 

services/pull/patch.go

@@ -87,6 +87,14 @@ func TestPatch(pr *issues_model.PullRequest) error {
 		}
 	}
 	pr.MergeBase = strings.TrimSpace(pr.MergeBase)
+	if pr.HeadCommitID, err = gitRepo.GetRefCommitID(git.BranchPrefix + "tracking"); err != nil {
+		return fmt.Errorf("GetBranchCommitID: can't find commit ID for head: %w", err)
+	}
+
+	if pr.HeadCommitID == pr.MergeBase {
+		pr.Status = issues_model.PullRequestStatusAncestor
+		return nil
+	}
 
 	// 2. Check for conflicts
 	if conflicts, err := checkConflicts(ctx, pr, gitRepo, tmpBasePath); err != nil || conflicts || pr.Status == issues_model.PullRequestStatusEmpty {

templates/repo/issue/view_content/pull.tmpl

@@ -195,12 +195,12 @@
 					<i class="icon icon-octicon">{{svg "octicon-sync"}}</i>
 					{{$.i18n.Tr "repo.pulls.is_checking"}}
 				</div>
-			{{else if .Issue.PullRequest.IsEmpty}}
+			{{else if .Issue.PullRequest.IsAncestor}}
 				<div class="item">
 					<i class="icon icon-octicon">{{svg "octicon-alert" 16}}</i>
-					{{$.i18n.Tr "repo.pulls.is_empty"}}
+					{{$.i18n.Tr "repo.pulls.is_ancestor"}}
 				</div>
-			{{else if .Issue.PullRequest.CanAutoMerge}}
+			{{else if or .Issue.PullRequest.CanAutoMerge .Issue.PullRequest.IsEmpty}}
 				{{if .IsBlockedByApprovals}}
 					<div class="item">
 						<i class="icon icon-octicon">{{svg "octicon-x"}}</i>
@@ -282,7 +282,6 @@
 						</div>
 					{{end}}
 				{{end}}
-
 				{{if and (gt .Issue.PullRequest.CommitsBehind 0) (not  .Issue.IsClosed) (not .Issue.PullRequest.IsChecking) (not .IsPullFilesConflicted) (not .IsPullRequestBroken) (not $canAutoMerge)}}
 					<div class="ui divider"></div>
 					<div class="item item-section">
@@ -321,6 +320,14 @@
 						</div>
 					</div>
 				{{end}}
+				{{if .Issue.PullRequest.IsEmpty}}
+					<div class="ui divider"></div>
+
+					<div class="item">
+						<i class="icon icon-octicon">{{svg "octicon-alert" 16}}</i>
+						{{$.i18n.Tr "repo.pulls.is_empty"}}
+					</div>
+				{{end}}
 
 				{{if .AllowMerge}} {{/* user is allowed to merge */}}
 					{{$prUnit := .Repository.MustGetUnit $.UnitTypePullRequests}}
@@ -348,6 +355,7 @@
 
 									'canMergeNow': {{$canMergeNow}},
 									'allOverridableChecksOk': {{not $notAllOverridableChecksOk}},
+									'emptyCommit': {{.Issue.PullRequest.IsEmpty}},
 									'pullHeadCommitID': {{.PullHeadCommitID}},
 									'isPullBranchDeletable': {{.IsPullBranchDeletable}},
 									'defaultDeleteBranchAfterMerge': {{$prUnit.PullRequestsConfig.DefaultDeleteBranchAfterMerge}},

web_src/js/components/PullRequestMergeForm.vue

@@ -48,7 +48,7 @@
 
     <div v-if="!showActionForm" class="df">
       <!-- the merge button -->
-      <div class="ui buttons merge-button" :class="mergeButtonStyleClass" @click="toggleActionForm(true)" >
+      <div class="ui buttons merge-button" :class="[mergeForm.emptyCommit ? 'grey' : mergeForm.allOverridableChecksOk ? 'green' : 'red']" @click="toggleActionForm(true)" >
         <button class="ui button">
           <svg-icon name="octicon-git-merge"/>
           <span class="button-text">