From 281bd56e76d190da748d9509e5868ae49c3b800c Mon Sep 17 00:00:00 2001 From: Michal Suchanek Date: Mon, 23 Feb 2026 10:20:02 +0100 Subject: [PATCH 1/8] Return 404 when deleting non-existent branch through api --- services/repository/branch.go | 56 ++++++++++++++++------------ tests/integration/api_branch_test.go | 1 + 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index b3310b2e68c6e..3718073c89ca0 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -575,6 +575,8 @@ 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 { + var branchCommit *git.Commit + branchExistsInGit := false err := repo.MustNotBeArchived() if err != nil { return err @@ -584,41 +586,49 @@ 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 !branchExistsInDB { + if branchExistsInGit { + err := gitrepo.DeleteBranch(ctx, repo, branchName, true) + return err + } else { + return git.ErrBranchNotExist{ + Name: branchName, + } + } + } else { 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 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) + if branchExistsInGit { + return gitrepo.DeleteBranch(ctx, repo, branchName, true) + } else { + return nil + } + } }); err != nil { return err } - if branchCommit == nil { + if !branchExistsInGit { return nil } diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 1163c1e8c9c09..97619964834d8 100644 --- a/tests/integration/api_branch_test.go +++ b/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) { From 9c0f2b3fce90fc2aeb81b36cf8ca77c57642e849 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 2 Mar 2026 23:58:28 +0800 Subject: [PATCH 2/8] fix --- models/git/branch.go | 6 +- models/git/branch_test.go | 4 +- routers/api/v1/repo/branch.go | 2 +- routers/private/hook_post_receive.go | 2 +- routers/web/repo/branch.go | 2 +- services/repository/branch.go | 107 ++++++++++++---------- tests/integration/actions_trigger_test.go | 2 +- 7 files changed, 69 insertions(+), 56 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index 30d517a06daa5..698c43f147242 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 b9b761fd5f1df..3832df9350953 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 82fd68bdececd..eaa8afb1e133c 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 e8bef7d6c14bb..15a93ffa89aaf 100644 --- a/routers/private/hook_post_receive.go +++ b/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), diff --git a/routers/web/repo/branch.go b/routers/web/repo/branch.go index f5630356004f5..1d6961f6fc8f7 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 3718073c89ca0..44fe244f2a397 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -573,10 +573,43 @@ 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) (branchExists bool, err error) { + branchInDB, err := git_model.GetBranch(ctx, repo.ID, branchName) + if err != nil { + return false, fmt.Errorf("GetBranch: %vc", err) + } + + activeInDB := branchInDB != nil && !branchInDB.IsDeleted + if branchCommit != nil { + // if the branch exists in git: + // * if active (not deleted) in DB, we need to mark it as deleted in DB and then delete the branch in git + // * if not active (not in db, or already marked as deleted), only need to delete the branch in git + if activeInDB { + if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, branchName, doer.ID); err != nil { + return false, err + } + } + err := gitrepo.DeleteBranch(ctx, repo, branchName, true) + if err != nil { + return false, fmt.Errorf("DeleteBranch: %v", err) + } + return true, nil + } + + // if the branch does not exist in git + // * if active in DB, we need to mark it as deleted in DB + // * if not active in DB, nothing to do + // in this case, return branchExists=activeInDB to indicate whether the branch exists in DB and is active, for consistency + if activeInDB { + if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, branchName, doer.ID); err != nil { + return false, err + } + } + return activeInDB, 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 { - var branchCommit *git.Commit - branchExistsInGit := false +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 @@ -586,54 +619,33 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R return err } - 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 - } - branchExistsInGit = branchCommit != 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.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) - } - } + branchCommit, err := gitRepo.GetBranchCommit(branchName) + if err != nil && !errors.Is(err, util.ErrNotExist) { + return err + } - if branchExistsInGit { - return gitrepo.DeleteBranch(ctx, repo, branchName, true) - } else { - return nil - } - } - }); err != nil { + var branchExists bool + err = db.WithTx(ctx, func(ctx context.Context) (err error) { + branchExists, err = deleteBranchInternal(ctx, doer, repo, branchName, branchCommit) + return err + }) + if err != nil { return err } - if !branchExistsInGit { - return nil + if !branchExists { + 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) { + // Don't return error below this objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName) if err := PushUpdate( &repo_module.PushUpdateOptions{ @@ -647,8 +659,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 { @@ -896,9 +906,12 @@ 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) } + if err := issues_model.AddDeletePRBranchComment(ctx, doer, pr.BaseRepo, pr.Issue.ID, pr.HeadBranch); err != nil { + log.Error("AddDeletePRBranchComment: %v", err) + } return err } diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 5ad574bebc754..e63e1b26a482c 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", From f3812cd303f2978bbe267bbc52651448f37ce49e Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 3 Mar 2026 00:06:33 +0800 Subject: [PATCH 3/8] fine tune --- services/repository/branch.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 44fe244f2a397..36c9b8764d0b1 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -575,7 +575,8 @@ func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchNam func deleteBranchInternal(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branchName string, branchCommit *git.Commit) (branchExists bool, err error) { branchInDB, err := git_model.GetBranch(ctx, repo.ID, branchName) - if err != nil { + // branchInDB can be nil if the branch doesn't exist in DB + if err != nil && !errors.Is(err, util.ErrNotExist) { return false, fmt.Errorf("GetBranch: %vc", err) } @@ -620,6 +621,7 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R } 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 } From 3acd67c001f71919c2fc786fc89d82e6eb3289b3 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 3 Mar 2026 00:15:18 +0800 Subject: [PATCH 4/8] fine tune --- services/repository/branch.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 36c9b8764d0b1..3fde7094a65a0 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -577,7 +577,7 @@ func deleteBranchInternal(ctx context.Context, doer *user_model.User, repo *repo branchInDB, err := git_model.GetBranch(ctx, repo.ID, branchName) // branchInDB can be nil if the branch doesn't exist in DB if err != nil && !errors.Is(err, util.ErrNotExist) { - return false, fmt.Errorf("GetBranch: %vc", err) + return false, fmt.Errorf("GetBranch: %w", err) } activeInDB := branchInDB != nil && !branchInDB.IsDeleted @@ -912,8 +912,13 @@ func DeleteBranchAfterMerge(ctx context.Context, doer *user_model.User, prID int if errors.Is(err, util.ErrPermissionDenied) || errors.Is(err, util.ErrNotExist) { return errFailedToDelete(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 err + return nil } From 95f67d327792dd0aab6067c2f4d59395a3cbc51d Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 3 Mar 2026 00:17:28 +0800 Subject: [PATCH 5/8] use IsBranchExist --- services/repository/branch.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 3fde7094a65a0..445a85e860ade 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -574,13 +574,11 @@ func CanDeleteBranch(ctx context.Context, repo *repo_model.Repository, branchNam } func deleteBranchInternal(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, branchName string, branchCommit *git.Commit) (branchExists bool, err error) { - branchInDB, err := git_model.GetBranch(ctx, repo.ID, branchName) - // branchInDB can be nil if the branch doesn't exist in DB - if err != nil && !errors.Is(err, util.ErrNotExist) { - return false, fmt.Errorf("GetBranch: %w", err) + activeInDB, err := git_model.IsBranchExist(ctx, repo.ID, branchName) + if err != nil { + return false, fmt.Errorf("IsBranchExist: %w", err) } - activeInDB := branchInDB != nil && !branchInDB.IsDeleted if branchCommit != nil { // if the branch exists in git: // * if active (not deleted) in DB, we need to mark it as deleted in DB and then delete the branch in git From 65b07d71789282f0ca60b2d6c8fbde851e0bc4f7 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 3 Mar 2026 00:23:37 +0800 Subject: [PATCH 6/8] simplify --- services/repository/branch.go | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 445a85e860ade..706306ff5d13e 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -579,32 +579,28 @@ func deleteBranchInternal(ctx context.Context, doer *user_model.User, repo *repo return false, fmt.Errorf("IsBranchExist: %w", err) } - if branchCommit != nil { - // if the branch exists in git: - // * if active (not deleted) in DB, we need to mark it as deleted in DB and then delete the branch in git - // * if not active (not in db, or already marked as deleted), only need to delete the branch in git - if activeInDB { - if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, branchName, doer.ID); err != nil { - return false, 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: %v", err) } - return true, nil + // since the branch exists in git, return branchExists=true + branchExists = true + } else { + // the branch does not 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 the branch list API. + branchExists = activeInDB } - // if the branch does not exist in git - // * if active in DB, we need to mark it as deleted in DB - // * if not active in DB, nothing to do - // in this case, return branchExists=activeInDB to indicate whether the branch exists in DB and is active, for consistency - if activeInDB { - if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, branchName, doer.ID); err != nil { - return false, err - } - } - return activeInDB, nil + return branchExists, nil } // DeleteBranch delete branch From 24bbad2f239d31b85ae3eb4f7f5663dedf8aa348 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 3 Mar 2026 00:26:40 +0800 Subject: [PATCH 7/8] fine tune --- services/repository/branch.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 706306ff5d13e..68636fcf6badc 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -573,7 +573,7 @@ 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) (branchExists bool, err error) { +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) @@ -593,14 +593,14 @@ func deleteBranchInternal(ctx context.Context, doer *user_model.User, repo *repo return false, fmt.Errorf("DeleteBranch: %v", err) } // since the branch exists in git, return branchExists=true - branchExists = true + branchExisted = true } else { // the branch does not 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 the branch list API. - branchExists = activeInDB + branchExisted = activeInDB } - return branchExists, nil + return branchExisted, nil } // DeleteBranch delete branch @@ -620,16 +620,14 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R return err } - var branchExists bool - err = db.WithTx(ctx, func(ctx context.Context) (err error) { - branchExists, err = deleteBranchInternal(ctx, doer, repo, branchName, branchCommit) - return err + branchExisted, err := db.WithTx2(ctx, func(ctx context.Context) (bool, error) { + return deleteBranchInternal(ctx, doer, repo, branchName, branchCommit) }) if err != nil { return err } - if !branchExists { + if !branchExisted { return git.ErrBranchNotExist{Name: branchName} } From f817f07fdb14c8817f1ac51a485a6e0a7eefb03d Mon Sep 17 00:00:00 2001 From: Michal Suchanek Date: Mon, 2 Mar 2026 18:56:32 +0100 Subject: [PATCH 8/8] Remove deleteBranchInternal --- services/repository/branch.go | 79 ++++++++++++++++------------------- 1 file changed, 35 insertions(+), 44 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 68636fcf6badc..db9596fd9e018 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -573,38 +573,10 @@ 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: %v", err) - } - // since the branch exists in git, return branchExists=true - branchExisted = true - } else { - // the branch does not 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 the branch list API. - 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) error { + var branchCommit *git.Commit + branchExistsInGit := false err := repo.MustNotBeArchived() if err != nil { return err @@ -614,25 +586,44 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R return err } - 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 { + branchExistsInDB, err := git_model.IsBranchExist(ctx, repo.ID, branchName) + if err != nil { + return fmt.Errorf("GetBranch: %vc", err) + } - branchExisted, err := db.WithTx2(ctx, func(ctx context.Context) (bool, error) { - return deleteBranchInternal(ctx, doer, repo, branchName, branchCommit) - }) - if err != nil { - return err - } + branchCommit, err = gitRepo.GetBranchCommit(branchName) + if err != nil && !errors.Is(err, util.ErrNotExist) { + return err + } + branchExistsInGit = branchCommit != 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 !branchExisted { - return git.ErrBranchNotExist{Name: branchName} + if branchExistsInGit { + return gitrepo.DeleteBranch(ctx, repo, branchName, true) + } else { + return nil + } + } + }); err != nil { + return err } // Don't return error below this since the deletion has succeeded - if branchCommit != nil { + if branchExistsInGit { deleteBranchSuccessPostProcess(doer, repo, branchName, branchCommit) } return nil