Return 404 when deleting non-existent branch through api

[hramrach]

Fixes: #36682
alternative to #36694

still not right

[wxiaoguang]

I can help to edit this PR and complete it, if you don't mind.

[hramrach]

I tried to do nested branching instead of fallthrough. Maybe that makes the logic clearer?

[hramrach]

There is event git_model.IsBranchExist but it's kind of broken, it returns an error when the branch does not exist.

[hramrach]

I can help to edit this PR and complete it, if you don't mind.

Feel free to edit the PR. it's enabled after all.

I have no idea how to improve it at this point.

[Copilot]

Pull request overview

This PR fixes the branch deletion API behavior so that deleting a non-existent branch returns 404 Not Found instead of 204 No Content, aligning the endpoint with expected REST semantics.

Changes:

• Update repository branch deletion logic to detect non-existent branches and return a branch-not-exist error.
• Refactor deleted-branch DB updates by renaming AddDeletedBranch to MarkBranchAsDeleted and updating call sites.
• Add/adjust integration and model tests to cover the new API behavior and updated service signature.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
services/repository/branch.go	Refactors DeleteBranch to return not-found when appropriate; adjusts post-merge deletion flow.
routers/api/v1/repo/branch.go	Uses updated DeleteBranch signature so API can surface 404 on missing branches.
routers/web/repo/branch.go	Updates web branch deletion path to use new DeleteBranch signature.
routers/private/hook_post_receive.go	Switches deleted-branch DB update to MarkBranchAsDeleted during git hook processing.
models/git/branch.go	Renames and redefines deleted-branch DB mutation helper as MarkBranchAsDeleted.
models/git/branch_test.go	Updates model test to use MarkBranchAsDeleted.
tests/integration/api_branch_test.go	Adds regression test asserting 404 when deleting a branch that’s already deleted.
tests/integration/actions_trigger_test.go	Updates service call to match new DeleteBranch signature.

Comments suppressed due to low confidence (2)

routers/private/hook_post_receive.go:114

• MarkBranchAsDeleted returns an error when the branch record doesn't exist in the DB, and this handler treats that as a 500. Since branch rows may be missing (e.g., branches not yet synced, or cleaned up), deleting a ref could incorrectly fail the post-receive hook. Consider treating util.ErrNotExist (or git_model.ErrBranchNotExist) as a no-op here, or changing the DB update helper to upsert a deleted-branch record.

			if update.IsDelRef() {
				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),
					})
					return

models/git/branch.go:276

• MarkBranchAsDeleted currently requires an existing branch row (GetBranch must succeed). Some callers (eg post-receive handling of delete refs) may need this operation to be idempotent even when the branch was never synced to DB. Consider making this function handle missing branches by inserting a deleted record (upsert), or provide a separate helper with that behavior so callers can avoid hard-failing on ErrBranchNotExist.

// 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
	}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

dismiss review

[hramrach]

I think moving AddDeletePRBranchComment to the one caller that needs that and renaming AddDeletedBranch to MarkBranchAsDeleted were good cleanups.

[lunny]

#36694 merged.