Delete non-exist branch should return 404 (#36694)

Fix #36682

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/git/branch.go

@@ -247,7 +247,7 @@ func DeleteBranches(ctx context.Context, repoID, doerID int64, branchIDs []int64
 			return err
 		}
 		for _, branch := range branches {
-			if err := AddDeletedBranch(ctx, repoID, branch.Name, doerID); err != nil {
+			if err := MarkBranchAsDeleted(ctx, repoID, branch.Name, doerID); err != nil {
 				return err
 			}
 		}
@@ -268,8 +268,8 @@ func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string
 		})
 }
 
-// AddDeletedBranch adds a deleted branch to the database
+// MarkBranchAsDeleted marks branch as deleted
-func AddDeletedBranch(ctx context.Context, repoID int64, branchName string, deletedByID int64) error {
+func MarkBranchAsDeleted(ctx context.Context, repoID int64, branchName string, deletedByID int64) error {
 	branch, err := GetBranch(ctx, repoID, branchName)
 	if err != nil {
 		return err

models/git/branch_test.go

@@ -28,8 +28,8 @@ func TestAddDeletedBranch(t *testing.T) {
 	firstBranch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{ID: 1})
 
 	assert.True(t, firstBranch.IsDeleted)
-	assert.NoError(t, git_model.AddDeletedBranch(t.Context(), repo.ID, firstBranch.Name, firstBranch.DeletedByID))
+	assert.NoError(t, git_model.MarkBranchAsDeleted(t.Context(), repo.ID, firstBranch.Name, firstBranch.DeletedByID))
-	assert.NoError(t, git_model.AddDeletedBranch(t.Context(), repo.ID, "branch2", int64(1)))
+	assert.NoError(t, git_model.MarkBranchAsDeleted(t.Context(), repo.ID, "branch2", int64(1)))
 
 	secondBranch := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo.ID, Name: "branch2"})
 	assert.True(t, secondBranch.IsDeleted)

routers/api/v1/repo/branch.go

@@ -150,7 +150,7 @@ func DeleteBranch(ctx *context.APIContext) {
 		}
 	}
 
-	if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName, nil); err != nil {
+	if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
 		switch {
 		case git.IsErrBranchNotExist(err):
 			ctx.APIErrorNotFound(err)

routers/private/hook_post_receive.go

@@ -106,10 +106,10 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 			}
 
 			if update.IsDelRef() {
-				if err := git_model.AddDeletedBranch(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil {
+				if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil {
-					log.Error("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err)
+					log.Error("Failed to mark branch as deleted: %s/%s Error: %v", ownerName, repoName, err)
 					ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
-						Err: fmt.Sprintf("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err),
+						Err: fmt.Sprintf("Failed to mark branch as deleted: %s/%s Error: %v", ownerName, repoName, err),
 					})
 					return
 				}

routers/web/repo/branch.go

@@ -95,7 +95,7 @@ func DeleteBranchPost(ctx *context.Context) {
 	defer jsonRedirectBranches(ctx)
 	branchName := ctx.FormString("name")
 
-	if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName, nil); err != nil {
+	if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName); err != nil {
 		switch {
 		case git.IsErrBranchNotExist(err):
 			log.Debug("DeleteBranch: Can't delete non existing branch '%s'", branchName)

services/repository/branch.go

@@ -573,8 +573,38 @@ func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchNam
 	return nil
 }
 
+func deleteBranchInternal(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branchName string, branchCommit *git.Commit) (branchExisted bool, err error) {
+	activeInDB, err := git_model.IsBranchExist(ctx, repo.ID, branchName)
+	if err != nil {
+		return false, fmt.Errorf("IsBranchExist: %w", err)
+	}
+
+	// process the branch in db
+	if activeInDB {
+		if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, branchName, doer.ID); err != nil {
+			return false, err
+		}
+	}
+
+	// process the branch in git
+	if branchCommit != nil {
+		err := gitrepo.DeleteBranch(ctx, repo, branchName, true)
+		if err != nil {
+			return false, fmt.Errorf("DeleteBranch: %w", err)
+		}
+		// since the branch existed in git, return branchExisted=true
+		branchExisted = true
+	} else {
+		// the branch didn't exist in git, return activeInDB to indicate whether the branch was active in DB,
+		// for consistency with that the user had seen on the web ui or in the branch list API response.
+		branchExisted = activeInDB
+	}
+
+	return branchExisted, nil
+}
+
 // DeleteBranch delete branch
-func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string, pr *issues_model.PullRequest) error {
+func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, gitRepo *git.Repository, branchName string) error {
 	err := repo.MustNotBeArchived()
 	if err != nil {
 		return err
@@ -584,46 +614,31 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
 		return err
 	}
 
-	rawBranch, err := git_model.GetBranch(ctx, repo.ID, branchName)
-	if err != nil && !git_model.IsErrBranchNotExist(err) {
-		return fmt.Errorf("GetBranch: %vc", err)
-	}
-
-	// database branch record not exist or it's a deleted branch
-	notExist := git_model.IsErrBranchNotExist(err) || rawBranch.IsDeleted
-
 	branchCommit, err := gitRepo.GetBranchCommit(branchName)
+	// branchCommit can be nil if the branch doesn't exist in git
 	if err != nil && !errors.Is(err, util.ErrNotExist) {
 		return err
 	}
 
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
+	branchExisted, err := db.WithTx2(ctx, func(ctx context.Context) (bool, error) {
-		if !notExist {
+		return deleteBranchInternal(ctx, doer, repo, branchName, branchCommit)
-			if err := git_model.AddDeletedBranch(ctx, repo.ID, branchName, doer.ID); err != nil {
+	})
-				return err
+	if err != nil {
-			}
-		}
-
-		if pr != nil {
-			if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
-				return fmt.Errorf("DeleteBranch: %v", err)
-			}
-		}
-		if branchCommit == nil {
-			return nil
-		}
-
-		return gitrepo.DeleteBranch(ctx, repo, branchName, true)
-	}); err != nil {
 		return err
 	}
 
-	if branchCommit == nil {
+	if !branchExisted {
-		return nil
+		return git.ErrBranchNotExist{Name: branchName}
 	}
 
-	// Don't return error below this
+	// Don't return error below this since the deletion has succeeded
+	if branchCommit != nil {
+		deleteBranchSuccessPostProcess(doer, repo, branchName, branchCommit)
+	}
+	return nil
+}
 
+func deleteBranchSuccessPostProcess(doer *user_model.User, repo *repo_model.Repository, branchName string, branchCommit *git.Commit) {
 	objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName)
 	if err := PushUpdate(
 		&repo_module.PushUpdateOptions{
@@ -637,8 +652,6 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R
 		}); err != nil {
 		log.Error("PushUpdateOptions: %v", err)
 	}
-
-	return nil
 }
 
 type BranchSyncOptions struct {
@@ -886,9 +899,17 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int
 		return util.ErrorWrapTranslatable(util.ErrUnprocessableContent, "repo.branch.delete_branch_has_new_commits", fullBranchName)
 	}
 
-	err = DeleteBranch(ctx, doer, pr.HeadRepo, gitHeadRepo, pr.HeadBranch, pr)
+	err = DeleteBranch(ctx, doer, pr.HeadRepo, gitHeadRepo, pr.HeadBranch)
 	if errors.Is(err, util.ErrPermissionDenied) || errors.Is(err, util.ErrNotExist) {
 		return errFailedToDelete(err)
 	}
-	return err
+	if err != nil {
+		return err
+	}
+
+	// intentionally ignore the following error, since the branch has already been deleted successfully
+	if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil {
+		log.Error("AddDeletePRBranchComment: %v", err)
+	}
+	return nil
 }

tests/integration/actions_trigger_test.go

@@ -438,7 +438,7 @@ jobs:
 		assert.NotNil(t, run)
 
 		// delete the branch
-		err = repo_service.DeleteBranch(t.Context(), user2, repo, gitRepo, "test-create-branch", nil)
+		err = repo_service.DeleteBranch(t.Context(), user2, repo, gitRepo, "test-create-branch")
 		assert.NoError(t, err)
 		run = unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{
 			Title:      "add workflow",

tests/integration/api_branch_test.go

@@ -402,6 +402,8 @@ func TestAPIBranchProtection(t *testing.T) {
 	// Test branch deletion
 	testAPIDeleteBranch(t, "master", http.StatusForbidden)
 	testAPIDeleteBranch(t, "branch2", http.StatusNoContent)
+	testAPIDeleteBranch(t, "branch2", http.StatusNotFound)        // deleted branch, there is a record in DB with IsDelete=true
+	testAPIDeleteBranch(t, "no-such-branch", http.StatusNotFound) // non-existing branch, not exist in git or DB
 }
 
 func TestAPICreateBranchWithSyncBranches(t *testing.T) {