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 b3310b2e68c6e..db9596fd9e018 100644 --- a/services/repository/branch.go +++ b/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 } 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", 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) {