Return 404 when deleting non-existent branch through api

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
-func AddDeletedBranch(ctx context.Context, repoID int64, branchName string, deletedByID int64) error {
+// MarkBranchAsDeleted marks branch as deleted
+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.AddDeletedBranch(t.Context(), repo.ID, "branch2", int64(1)))
+	assert.NoError(t, git_model.MarkBranchAsDeleted(t.Context(), repo.ID, firstBranch.Name, firstBranch.DeletedByID))
+	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,7 +106,7 @@ 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)
 					ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
 						Err: fmt.Sprintf("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err),

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

@@ -574,7 +574,9 @@ func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchNam
 }
 
 // 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 {
+	var branchCommit *git.Commit
+	branchExistsInGit := false
 	err := repo.MustNotBeArchived()
 	if err != nil {
 		return err
@@ -584,46 +586,51 @@ 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
+	if err := db.WithTx(ctx, func(ctx context.Context) error {
+		branchExistsInDB, err := git_model.IsBranchExist(ctx, repo.ID, branchName)
+		if err != nil {
+			return fmt.Errorf("GetBranch: %vc", err)
+		}
 
-	branchCommit, err := gitRepo.GetBranchCommit(branchName)
-	if err != nil && !errors.Is(err, util.ErrNotExist) {
-		return err
-	}
+		branchCommit, err = gitRepo.GetBranchCommit(branchName)
+		if err != nil && !errors.Is(err, util.ErrNotExist) {
+			return err
+		}
+		branchExistsInGit = branchCommit != nil
 
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
-		if !notExist {
-			if err := git_model.AddDeletedBranch(ctx, repo.ID, branchName, doer.ID); err != nil {
+		if !branchExistsInDB {
+			if branchExistsInGit {
+				err := gitrepo.DeleteBranch(ctx, repo, branchName, true)
+				return err
+			} else {
+				return git.ErrBranchNotExist{
+					Name: branchName,
+				}
+			}
+		} else {
+			if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, branchName, doer.ID); err != nil {
 				return err
 			}
-		}
 
-		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 branchExistsInGit {
+				return gitrepo.DeleteBranch(ctx, repo, branchName, true)
+			} else {
+				return nil
 			}
 		}
-		if branchCommit == nil {
-			return nil
-		}
-
-		return gitrepo.DeleteBranch(ctx, repo, branchName, true)
 	}); err != nil {
 		return err
 	}
 
-	if branchCommit == nil {
-		return nil
+	// Don't return error below this since the deletion has succeeded
+	if branchExistsInGit {
+		deleteBranchSuccessPostProcess(doer, repo, branchName, branchCommit)
 	}
+	return nil
+}
 
+func deleteBranchSuccessPostProcess(doer *user_model.User, repo *repo_model.Repository, branchName string, branchCommit *git.Commit) {
 	// Don't return error below this
-
 	objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName)
 	if err := PushUpdate(
 		&repo_module.PushUpdateOptions{
@@ -637,8 +644,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 +891,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,7 @@ func TestAPIBranchProtection(t *testing.T) {
 	// Test branch deletion
 	testAPIDeleteBranch(t, "master", http.StatusForbidden)
 	testAPIDeleteBranch(t, "branch2", http.StatusNoContent)
+	testAPIDeleteBranch(t, "branch2", http.StatusNotFound)
 }
 
 func TestAPICreateBranchWithSyncBranches(t *testing.T) {