From 0e0daa8afe5c940ee33cddb0cd657e76bf1e2620 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 2 Mar 2026 11:08:16 -0800 Subject: [PATCH] Delete non-exist branch should return 404 (#36694) Fix #36682 --------- Co-authored-by: wxiaoguang --- models/git/branch.go | 6 +- models/git/branch_test.go | 4 +- routers/api/v1/repo/branch.go | 2 +- routers/private/hook_post_receive.go | 6 +- routers/web/repo/branch.go | 2 +- services/repository/branch.go | 89 ++++++++++++++--------- tests/integration/actions_trigger_test.go | 2 +- tests/integration/api_branch_test.go | 2 + 8 files changed, 68 insertions(+), 45 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index 30d517a06d..698c43f147 100644 --- a/models/git/branch.go +++ b/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 diff --git a/models/git/branch_test.go b/models/git/branch_test.go index b9b761fd5f..3832df9350 100644 --- a/models/git/branch_test.go +++ b/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) diff --git a/routers/api/v1/repo/branch.go b/routers/api/v1/repo/branch.go index 82fd68bdec..eaa8afb1e1 100644 --- a/routers/api/v1/repo/branch.go +++ b/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) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index e8bef7d6c1..b595a95b23 100644 --- a/routers/private/hook_post_receive.go +++ b/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 { - log.Error("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err) + if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil { + 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 } diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index f563035600..1d6961f6fc 100644 --- a/routers/web/repo/branch.go +++ b/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) diff --git a/services/repository/branch.go b/services/repository/branch.go index b3310b2e68..87aa3bbc33 100644 --- a/services/repository/branch.go +++ b/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 { - if !notExist { - if err := git_model.AddDeletedBranch(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 branchCommit == nil { - return nil - } - - return gitrepo.DeleteBranch(ctx, repo, branchName, true) - }); err != nil { + branchExisted, err := db.WithTx2(ctx, func(ctx context.Context) (bool, error) { + return deleteBranchInternal(ctx, doer, repo, branchName, branchCommit) + }) + if err != nil { return err } - if branchCommit == nil { - return nil + if !branchExisted { + 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 } diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 5ad574bebc..e63e1b26a4 100644 --- a/tests/integration/actions_trigger_test.go +++ b/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", diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 1163c1e8c9..f5f33dacf0 100644 --- a/tests/integration/api_branch_test.go +++ b/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) {