Honor delete branch on merge repo setting when using merge API

routers/api/v1/repo/pull.go

@@ -25,6 +25,7 @@ import (
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/graceful"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
@@ -938,7 +939,7 @@ func MergePullRequest(ctx *context.APIContext) {
 		} else if errors.Is(err, pull_service.ErrNoPermissionToMerge) {
 			ctx.APIError(http.StatusMethodNotAllowed, "User not allowed to merge PR")
 		} else if errors.Is(err, pull_service.ErrHasMerged) {
-			ctx.APIError(http.StatusMethodNotAllowed, "")
+			ctx.APIError(http.StatusMethodNotAllowed, "The PR is already merged")
 		} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
 			ctx.APIError(http.StatusMethodNotAllowed, "Work in progress PRs cannot be merged")
 		} else if errors.Is(err, pull_service.ErrNotMergeableState) {
@@ -990,7 +991,8 @@ func MergePullRequest(ctx *context.APIContext) {
 	}
 
 	if form.MergeWhenChecksSucceed {
-		scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge)
+		deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)
+		scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge)
 		if err != nil {
 			if pull_model.IsErrAlreadyScheduledToAutoMerge(err) {
 				ctx.APIError(http.StatusConflict, err)
@@ -1035,8 +1037,26 @@ func MergePullRequest(ctx *context.APIContext) {
 	}
 	log.Trace("Pull request merged: %d", pr.ID)
 
+	prUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypePullRequests)
+	if err != nil {
+		ctx.APIErrorInternal(err)
+		return
+	}
+
 	// for agit flow, we should not delete the agit reference after merge
-	if form.DeleteBranchAfterMerge && pr.Flow == issues_model.PullRequestFlowGithub {
+	// FIXME: old code has that comment above. Is that comment valid? What would go wrong if a agit branch is deleted after merge?
+	// * If a agit branch can be deleted after merge, then fix the comment and maybe other related code
+	// * If a agit branch should not be deleted, then we need to fix the logic and add more tests
+	shouldDeleteBranch := pr.Flow == issues_model.PullRequestFlowGithub
+	if form.DeleteBranchAfterMerge != nil {
+		// if the form field is defined, it takes precedence over the repo setting equivalent
+		shouldDeleteBranch = shouldDeleteBranch && *form.DeleteBranchAfterMerge
+	} else {
+		// otherwise, we look at the repo setting to make the determination
+		shouldDeleteBranch = shouldDeleteBranch && prUnit.PullRequestsConfig().DefaultDeleteBranchAfterMerge
+	}
+
+	if shouldDeleteBranch {
 		// check permission even it has been checked in repo_service.DeleteBranch so that we don't need to
 		// do RetargetChildrenOnMerge
 		if err := repo_service.CanDeleteBranch(ctx, pr.HeadRepo, pr.HeadBranch, ctx.Doer); err == nil {

routers/web/repo/pull.go

@@ -32,6 +32,7 @@ import (
 	"code.gitea.io/gitea/modules/graceful"
 	issue_template "code.gitea.io/gitea/modules/issue/template"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/templates"
 	"code.gitea.io/gitea/modules/util"
@@ -1106,7 +1107,8 @@ func MergePullRequest(ctx *context.Context) {
 		// delete all scheduled auto merges
 		_ = pull_model.DeleteScheduledAutoMerge(ctx, pr.ID)
 		// schedule auto merge
-		scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, form.DeleteBranchAfterMerge)
+		deleteBranchAfterMerge := optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false)
+		scheduled, err := automerge.ScheduleAutoMerge(ctx, ctx.Doer, pr, repo_model.MergeStyle(form.Do), message, deleteBranchAfterMerge)
 		if err != nil {
 			ctx.ServerError("ScheduleAutoMerge", err)
 			return
@@ -1192,7 +1194,7 @@ func MergePullRequest(ctx *context.Context) {
 
 	log.Trace("Pull request merged: %d", pr.ID)
 
-	if !form.DeleteBranchAfterMerge {
+	if !optional.FromPtr(form.DeleteBranchAfterMerge).ValueOrDefault(false) {
 		ctx.JSONRedirect(issue.Link())
 		return
 	}

services/forms/repo_form.go

@@ -540,7 +540,7 @@ type MergePullRequestForm struct {
 	HeadCommitID           string `json:"head_commit_id,omitempty"`
 	ForceMerge             bool   `json:"force_merge,omitempty"`
 	MergeWhenChecksSucceed bool   `json:"merge_when_checks_succeed,omitempty"`
-	DeleteBranchAfterMerge bool   `json:"delete_branch_after_merge,omitempty"`
+	DeleteBranchAfterMerge *bool  `json:"delete_branch_after_merge,omitempty"`
 }
 
 // Validate validates the fields

tests/integration/api_issue_config_test.go

@@ -129,7 +129,7 @@ func TestAPIRepoIssueConfigPaths(t *testing.T) {
 				configData, err := yaml.Marshal(configMap)
 				assert.NoError(t, err)
 
-				_, err = createFileInBranch(owner, repo, fullPath, repo.DefaultBranch, string(configData))
+				_, err = createFile(owner, repo, fullPath, string(configData))
 				assert.NoError(t, err)
 
 				issueConfig := getIssueConfig(t, owner.Name, repo.Name)

tests/integration/api_pull_test.go

@@ -17,19 +17,23 @@ import (
 	issues_model "code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/models/perm"
 	repo_model "code.gitea.io/gitea/models/repo"
+	unit_model "code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/services/convert"
 	"code.gitea.io/gitea/services/forms"
 	"code.gitea.io/gitea/services/gitdiff"
 	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
+	repo_service "code.gitea.io/gitea/services/repository"
 	files_service "code.gitea.io/gitea/services/repository/files"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestAPIViewPulls(t *testing.T) {
@@ -186,6 +190,97 @@ func TestAPIMergePullWIP(t *testing.T) {
 	MakeRequest(t, req, http.StatusMethodNotAllowed)
 }
 
+func TestAPIMergePull(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+		apiCtx := NewAPITestContext(t, repo.OwnerName, repo.Name, auth_model.AccessTokenScopeWriteRepository)
+
+		checkBranchExists := func(t *testing.T, branchName string, status int) {
+			req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/branches/%s", owner.Name, repo.Name, branchName)).AddTokenAuth(apiCtx.Token)
+			MakeRequest(t, req, status)
+		}
+
+		createTestBranchPR := func(t *testing.T, branchName string) *api.PullRequest {
+			testCreateFileInBranch(t, owner, repo, createFileInBranchOptions{NewBranch: branchName}, map[string]string{"a-new-file-" + branchName + ".txt": "dummy content"})
+			prDTO, err := doAPICreatePullRequest(apiCtx, repo.OwnerName, repo.Name, repo.DefaultBranch, branchName)(t)
+			require.NoError(t, err)
+			return &prDTO
+		}
+
+		performMerge := func(t *testing.T, prIndex int64, deleteBranchAfterMerge *bool, optExpectedStatus ...int) {
+			req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge", owner.Name, repo.Name, prIndex), &forms.MergePullRequestForm{
+				Do:                     "merge",
+				DeleteBranchAfterMerge: deleteBranchAfterMerge,
+			}).AddTokenAuth(apiCtx.Token)
+			expectedStatus := util.OptionalArg(optExpectedStatus, http.StatusOK)
+			MakeRequest(t, req, expectedStatus)
+		}
+
+		t.Run("Normal", func(t *testing.T) {
+			newBranch := "test-pull-1"
+			prDTO := createTestBranchPR(t, newBranch)
+			performMerge(t, prDTO.Index, nil)
+			checkBranchExists(t, newBranch, http.StatusOK)
+			performMerge(t, prDTO.Index, nil, http.StatusMethodNotAllowed) // make sure we cannot perform a merge on the same PR
+		})
+
+		t.Run("DeleteBranchAfterMergePassedByFormField", func(t *testing.T) {
+			newBranch := "test-pull-2"
+			prDTO := createTestBranchPR(t, newBranch)
+			performMerge(t, prDTO.Index, util.ToPointer(true))
+			checkBranchExists(t, newBranch, http.StatusNotFound)
+		})
+
+		t.Run("DeleteBranchAfterMergePassedByRepoSettings", func(t *testing.T) {
+			newBranch := "test-pull-3"
+			prDTO := createTestBranchPR(t, newBranch)
+
+			// set the default branch after merge setting at the repo level
+			prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests)
+			require.NoError(t, err)
+
+			prConfig := prUnit.PullRequestsConfig()
+			prConfig.DefaultDeleteBranchAfterMerge = true
+
+			var units []repo_model.RepoUnit
+			units = append(units, repo_model.RepoUnit{
+				RepoID: repo.ID,
+				Type:   unit_model.TypePullRequests,
+				Config: prConfig,
+			})
+			require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil))
+
+			performMerge(t, prDTO.Index, nil)
+			checkBranchExists(t, newBranch, http.StatusNotFound)
+		})
+
+		t.Run("DeleteBranchAfterMergeFormFieldIsSetButNotRepoSettings", func(t *testing.T) {
+			newBranch := "test-pull-4"
+			prDTO := createTestBranchPR(t, newBranch)
+
+			// make sure the default branch after merge setting is unset at the repo level
+			prUnit, err := repo.GetUnit(t.Context(), unit_model.TypePullRequests)
+			require.NoError(t, err)
+
+			prConfig := prUnit.PullRequestsConfig()
+			prConfig.DefaultDeleteBranchAfterMerge = false
+
+			var units []repo_model.RepoUnit
+			units = append(units, repo_model.RepoUnit{
+				RepoID: repo.ID,
+				Type:   unit_model.TypePullRequests,
+				Config: prConfig,
+			})
+			require.NoError(t, repo_service.UpdateRepositoryUnits(t.Context(), repo, units, nil))
+
+			// perform merge
+			performMerge(t, prDTO.Index, util.ToPointer(true))
+			checkBranchExists(t, newBranch, http.StatusNotFound)
+		})
+	})
+}
+
 func TestAPICreatePullSuccess(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10})

tests/integration/api_repo_file_create_test.go

@@ -133,7 +133,7 @@ func BenchmarkAPICreateFileSmall(b *testing.B) {
 		b.ResetTimer()
 		for n := 0; b.Loop(); n++ {
 			treePath := fmt.Sprintf("update/file%d.txt", n)
-			_, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath)
+			_, _ = createFile(user2, repo1, treePath)
 		}
 	})
 }
@@ -149,7 +149,7 @@ func BenchmarkAPICreateFileMedium(b *testing.B) {
 		for n := 0; b.Loop(); n++ {
 			treePath := fmt.Sprintf("update/file%d.txt", n)
 			copy(data, treePath)
-			_, _ = createFileInBranch(user2, repo1, treePath, repo1.DefaultBranch, treePath)
+			_, _ = createFile(user2, repo1, treePath)
 		}
 	})
 }

tests/integration/api_repo_file_helpers.go

@@ -6,26 +6,36 @@ package integration
 import (
 	"context"
 	"strings"
+	"testing"
 
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	files_service "code.gitea.io/gitea/services/repository/files"
+
+	"github.com/stretchr/testify/require"
 )
 
-func createFileInBranch(user *user_model.User, repo *repo_model.Repository, treePath, branchName, content string) (*api.FilesResponse, error) {
+type createFileInBranchOptions struct {
+	OldBranch, NewBranch string
+}
+
+func testCreateFileInBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) *api.FilesResponse {
+	resp, err := createFileInBranch(user, repo, createOpts, files)
+	require.NoError(t, err)
+	return resp
+}
+
+func createFileInBranch(user *user_model.User, repo *repo_model.Repository, createOpts createFileInBranchOptions, files map[string]string) (*api.FilesResponse, error) {
 	ctx := context.TODO()
-	opts := &files_service.ChangeRepoFilesOptions{
-		Files: []*files_service.ChangeRepoFile{
-			{
-				Operation:     "create",
-				TreePath:      treePath,
-				ContentReader: strings.NewReader(content),
-			},
-		},
-		OldBranch: branchName,
-		Author:    nil,
-		Committer: nil,
+	opts := &files_service.ChangeRepoFilesOptions{OldBranch: createOpts.OldBranch, NewBranch: createOpts.NewBranch}
+	for path, content := range files {
+		opts.Files = append(opts.Files, &files_service.ChangeRepoFile{
+			Operation:     "create",
+			TreePath:      path,
+			ContentReader: strings.NewReader(content),
+		})
 	}
 	return files_service.ChangeRepoFiles(ctx, repo, user, opts)
 }
@@ -53,10 +63,12 @@ func createOrReplaceFileInBranch(user *user_model.User, repo *repo_model.Reposit
 		return err
 	}
 
-	_, err = createFileInBranch(user, repo, treePath, branchName, content)
+	_, err = createFileInBranch(user, repo, createFileInBranchOptions{OldBranch: branchName}, map[string]string{treePath: content})
 	return err
 }
 
-func createFile(user *user_model.User, repo *repo_model.Repository, treePath string) (*api.FilesResponse, error) {
-	return createFileInBranch(user, repo, treePath, repo.DefaultBranch, "This is a NEW file")
+// TODO: replace all usages of this function with testCreateFileInBranch or testCreateFile
+func createFile(user *user_model.User, repo *repo_model.Repository, treePath string, optContent ...string) (*api.FilesResponse, error) {
+	content := util.OptionalArg(optContent, "file content "+treePath)
+	return createFileInBranch(user, repo, createFileInBranchOptions{}, map[string]string{treePath: content})
 }