From 7c98b643b8ea35319eb20b806c5d61556be43dda Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 14:12:01 +0800 Subject: [PATCH 1/3] Refactor post recieve handle function --- routers/private/hook_post_receive.go | 160 ++++++++++++++------------- 1 file changed, 84 insertions(+), 76 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index adc435b42cbd..c7f961e85910 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -37,14 +37,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { ownerName := ctx.Params(":owner") repoName := ctx.Params(":repo") - - // defer getting the repository at this point - as we should only retrieve it if we're going to call update - var ( - repo *repo_model.Repository - gitRepo *git.Repository - ) - defer gitRepo.Close() // it's safe to call Close on a nil pointer - + var repo *repo_model.Repository updates := make([]*repo_module.PushUpdateOptions, 0, len(opts.OldCommitIDs)) wasEmpty := false @@ -88,68 +81,16 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } if repo != nil && len(updates) > 0 { - branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates)) - for _, update := range updates { - if !update.RefFullName.IsBranch() { - continue - } - if repo == nil { - repo = loadRepository(ctx, ownerName, repoName) - if ctx.Written() { - return - } - wasEmpty = repo.IsEmpty - } - - 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) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to add deleted branch: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } - } else { - branchesToSync = append(branchesToSync, update) - - // TODO: should we return the error and return the error when pushing? Currently it will log the error and not prevent the pushing - pull_service.UpdatePullsRefs(ctx, repo, update) - } - } - if len(branchesToSync) > 0 { - var err error - gitRepo, err = gitrepo.OpenRepository(ctx, repo) - if err != nil { - log.Error("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } - - var ( - branchNames = make([]string, 0, len(branchesToSync)) - commitIDs = make([]string, 0, len(branchesToSync)) - ) - for _, update := range branchesToSync { - branchNames = append(branchNames, update.RefFullName.BranchName()) - commitIDs = append(commitIDs, update.NewCommitID) - } - - if err := repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, branchNames, commitIDs, gitRepo.GetCommit); err != nil { - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", ownerName, repoName, err), - }) - return - } + syncBranches(ctx, updates, repo, opts.UserID) + if ctx.Written() { + return } if err := repo_service.PushUpdates(updates); err != nil { - log.Error("Failed to Update: %s/%s Total Updates: %d", ownerName, repoName, len(updates)) + log.Error("Failed to Update: %s/%s Total Updates: %d, Error: %v", ownerName, repoName, len(updates), err) for i, update := range updates { log.Error("Failed to Update: %s/%s Update: %d/%d: Branch: %s", ownerName, repoName, i, len(updates), update.RefFullName.BranchName()) } - log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err) ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ Err: fmt.Sprintf("Failed to Update: %s/%s Error: %v", ownerName, repoName, err), @@ -158,6 +99,76 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } + handlePushOptions(ctx, opts, repo, ownerName, repoName) + if ctx.Written() { + return + } + + results := generateCompareLinks(ctx, opts, ownerName, repoName, wasEmpty) + if ctx.Written() { + return + } + + ctx.JSON(http.StatusOK, private.HookPostReceiveResult{ + Results: results, + RepoWasEmpty: wasEmpty, + }) +} + +func syncBranches(ctx *gitea_context.PrivateContext, updates []*repo_module.PushUpdateOptions, repo *repo_model.Repository, pusherID int64) { + branchesToSync := make([]*repo_module.PushUpdateOptions, 0, len(updates)) + for _, update := range updates { + if !update.RefFullName.IsBranch() { + continue + } + + 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", repo.OwnerName, repo.Name, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to add deleted branch: %s/%s Error: %v", repo.OwnerName, repo.Name, err), + }) + return + } + } else { + branchesToSync = append(branchesToSync, update) + + // TODO: should we return the error and return the error when pushing? Currently it will log the error and not prevent the pushing + pull_service.UpdatePullsRefs(ctx, repo, update) + } + } + if len(branchesToSync) == 0 { + return + } + + gitRepo, err := gitrepo.OpenRepository(ctx, repo) + if err != nil { + log.Error("Failed to open repository: %s/%s Error: %v", repo.OwnerName, repo.Name, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to open repository: %s/%s Error: %v", repo.OwnerName, repo.Name, err), + }) + return + } + defer gitRepo.Close() + + var ( + branchNames = make([]string, 0, len(branchesToSync)) + commitIDs = make([]string, 0, len(branchesToSync)) + ) + for _, update := range branchesToSync { + branchNames = append(branchNames, update.RefFullName.BranchName()) + commitIDs = append(commitIDs, update.NewCommitID) + } + + if err := repo_service.SyncBranchesToDB(ctx, repo.ID, pusherID, branchNames, commitIDs, gitRepo.GetCommit); err != nil { + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to sync branch to DB in repository: %s/%s Error: %v", repo.OwnerName, repo.Name, err), + }) + return + } +} + +func handlePushOptions(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository, ownerName, repoName string) { isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate) // Handle Push Options @@ -169,7 +180,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { // Error handled in loadRepository return } - wasEmpty = repo.IsEmpty } pusher, err := user_model.GetUserByID(ctx, opts.UserID) @@ -217,11 +227,11 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } } +} +func generateCompareLinks(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, wasEmpty bool) []private.HookPostReceiveBranchResult { results := make([]private.HookPostReceiveBranchResult, 0, len(opts.OldCommitIDs)) - - // We have to reload the repo in case its state is changed above - repo = nil + var repo *repo_model.Repository var baseRepo *repo_model.Repository // Now handle the pull request notification trailers @@ -235,7 +245,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { if repo == nil { repo = loadRepository(ctx, ownerName, repoName) if ctx.Written() { - return + return nil } baseRepo = repo @@ -247,7 +257,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { Err: fmt.Sprintf("Failed to get Base Repository of Forked repository: %-v Error: %v", repo, err), RepoWasEmpty: wasEmpty, }) - return + return nil } if repo.BaseRepo.AllowsPulls(ctx) { baseRepo = repo.BaseRepo @@ -259,7 +269,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { ctx.JSON(http.StatusOK, private.HookPostReceiveResult{ RepoWasEmpty: wasEmpty, }) - return + return nil } } @@ -279,7 +289,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { "Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err), RepoWasEmpty: wasEmpty, }) - return + return nil } if pr == nil { @@ -302,8 +312,6 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } } - ctx.JSON(http.StatusOK, private.HookPostReceiveResult{ - Results: results, - RepoWasEmpty: wasEmpty, - }) + + return results } From 6c44844222cf575a7a47dc27e695dfb3575e7ad1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 15:33:46 +0800 Subject: [PATCH 2/3] some improvements --- routers/private/hook_post_receive.go | 119 ++++++++++++++------------- 1 file changed, 60 insertions(+), 59 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index c7f961e85910..be9e0fc44eee 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -104,7 +104,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { return } - results := generateCompareLinks(ctx, opts, ownerName, repoName, wasEmpty) + results := generateBranchResults(ctx, opts, repo, ownerName, repoName, wasEmpty) if ctx.Written() { return } @@ -229,9 +229,8 @@ func handlePushOptions(ctx *gitea_context.PrivateContext, opts *private.HookOpti } } -func generateCompareLinks(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, wasEmpty bool) []private.HookPostReceiveBranchResult { +func generateBranchResults(ctx *gitea_context.PrivateContext, opts *private.HookOptions, repo *repo_model.Repository, ownerName, repoName string, wasEmpty bool) []private.HookPostReceiveBranchResult { results := make([]private.HookPostReceiveBranchResult, 0, len(opts.OldCommitIDs)) - var repo *repo_model.Repository var baseRepo *repo_model.Repository // Now handle the pull request notification trailers @@ -240,76 +239,78 @@ func generateCompareLinks(ctx *gitea_context.PrivateContext, opts *private.HookO newCommitID := opts.NewCommitIDs[i] // If we've pushed a branch (and not deleted it) - if !git.IsEmptyCommitID(newCommitID) && refFullName.IsBranch() { - // First ensure we have the repository loaded, we're allowed pulls requests and we can get the base repo - if repo == nil { - repo = loadRepository(ctx, ownerName, repoName) - if ctx.Written() { - return nil - } + if !refFullName.IsBranch() || git.IsEmptyCommitID(newCommitID) { + continue + } - baseRepo = repo - - if repo.IsFork { - if err := repo.GetBaseRepo(ctx); err != nil { - log.Error("Failed to get Base Repository of Forked repository: %-v Error: %v", repo, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf("Failed to get Base Repository of Forked repository: %-v Error: %v", repo, err), - RepoWasEmpty: wasEmpty, - }) - return nil - } - if repo.BaseRepo.AllowsPulls(ctx) { - baseRepo = repo.BaseRepo - } - } + // First ensure we have the repository loaded, we're allowed pulls requests and we can get the base repo + if repo == nil { + repo = loadRepository(ctx, ownerName, repoName) + if ctx.Written() { + return nil + } - if !baseRepo.AllowsPulls(ctx) { - // We can stop there's no need to go any further - ctx.JSON(http.StatusOK, private.HookPostReceiveResult{ + baseRepo = repo + + if repo.IsFork { + if err := repo.GetBaseRepo(ctx); err != nil { + log.Error("Failed to get Base Repository of Forked repository: %-v Error: %v", repo, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf("Failed to get Base Repository of Forked repository: %-v Error: %v", repo, err), RepoWasEmpty: wasEmpty, }) return nil } + if repo.BaseRepo.AllowsPulls(ctx) { + baseRepo = repo.BaseRepo + } } - branch := refFullName.BranchName() - - // If our branch is the default branch of an unforked repo - there's no PR to create or refer to - if !repo.IsFork && branch == baseRepo.DefaultBranch { - results = append(results, private.HookPostReceiveBranchResult{}) - continue - } - - pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, baseRepo.ID, branch, baseRepo.DefaultBranch, issues_model.PullRequestFlowGithub) - if err != nil && !issues_model.IsErrPullRequestNotExist(err) { - log.Error("Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: fmt.Sprintf( - "Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err), + if !baseRepo.AllowsPulls(ctx) { + // We can stop there's no need to go any further + ctx.JSON(http.StatusOK, private.HookPostReceiveResult{ RepoWasEmpty: wasEmpty, }) return nil } + } - if pr == nil { - if repo.IsFork { - branch = fmt.Sprintf("%s:%s", repo.OwnerName, branch) - } - results = append(results, private.HookPostReceiveBranchResult{ - Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), - Create: true, - Branch: branch, - URL: fmt.Sprintf("%s/compare/%s...%s", baseRepo.HTMLURL(), util.PathEscapeSegments(baseRepo.DefaultBranch), util.PathEscapeSegments(branch)), - }) - } else { - results = append(results, private.HookPostReceiveBranchResult{ - Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), - Create: false, - Branch: branch, - URL: fmt.Sprintf("%s/pulls/%d", baseRepo.HTMLURL(), pr.Index), - }) + branch := refFullName.BranchName() + + // If our branch is the default branch of an unforked repo - there's no PR to create or refer to + if !repo.IsFork && branch == baseRepo.DefaultBranch { + results = append(results, private.HookPostReceiveBranchResult{}) + continue + } + + pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, baseRepo.ID, branch, baseRepo.DefaultBranch, issues_model.PullRequestFlowGithub) + if err != nil && !issues_model.IsErrPullRequestNotExist(err) { + log.Error("Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ + Err: fmt.Sprintf( + "Failed to get active PR in: %-v Branch: %s to: %-v Branch: %s Error: %v", repo, branch, baseRepo, baseRepo.DefaultBranch, err), + RepoWasEmpty: wasEmpty, + }) + return nil + } + + if pr == nil { + if repo.IsFork { + branch = fmt.Sprintf("%s:%s", repo.OwnerName, branch) } + results = append(results, private.HookPostReceiveBranchResult{ + Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), + Create: true, + Branch: branch, + URL: fmt.Sprintf("%s/compare/%s...%s", baseRepo.HTMLURL(), util.PathEscapeSegments(baseRepo.DefaultBranch), util.PathEscapeSegments(branch)), + }) + } else { + results = append(results, private.HookPostReceiveBranchResult{ + Message: setting.Git.PullRequestPushMessage && baseRepo.AllowsPulls(ctx), + Create: false, + Branch: branch, + URL: fmt.Sprintf("%s/pulls/%d", baseRepo.HTMLURL(), pr.Index), + }) } } From ff65f3cb50076a125df95e7a4f2253bb19265363 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 1 May 2024 15:46:36 +0800 Subject: [PATCH 3/3] Add some comments for post receive --- routers/private/hook_post_receive.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index be9e0fc44eee..ec3d3fbf5c81 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -41,6 +41,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { updates := make([]*repo_module.PushUpdateOptions, 0, len(opts.OldCommitIDs)) wasEmpty := false + // generate updates and put the master/main branch first for i := range opts.OldCommitIDs { refFullName := opts.RefFullNames[i] @@ -80,12 +81,22 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } + // sync branches to the database, if failed return error to keep branches consistent between disk and database if repo != nil && len(updates) > 0 { syncBranches(ctx, updates, repo, opts.UserID) if ctx.Written() { return } + } + + // Handle possible Push Options + handlePushOptions(ctx, opts, repo, ownerName, repoName) + if ctx.Written() { + return + } + // push updates to a queue so some notificactions can be handled async + if len(updates) > 0 { if err := repo_service.PushUpdates(updates); err != nil { log.Error("Failed to Update: %s/%s Total Updates: %d, Error: %v", ownerName, repoName, len(updates), err) for i, update := range updates { @@ -99,11 +110,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } } - handlePushOptions(ctx, opts, repo, ownerName, repoName) - if ctx.Written() { - return - } - + // generate branch results for end user. i.e. Displaying a link to create a PR results := generateBranchResults(ctx, opts, repo, ownerName, repoName, wasEmpty) if ctx.Written() { return