From cfefe2b6c9a07ea0e4cc8e0fa1425e7fc612dad6 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 8 Aug 2024 09:46:38 +0200 Subject: [PATCH] chore(refactor): split repo_service.ForkRepository in two ForkRepository performs two different functions: * The fork itself, if it does not already exist * Updates and notifications after the fork is performed The function is split to reflect that and otherwise unmodified. The two function are given different names to: * clarify which integration tests provides coverage * distinguish it from the notification method by the same name --- routers/api/v1/repo/fork.go | 6 +++--- routers/web/repo/pull.go | 2 +- services/f3/driver/project.go | 2 +- services/repository/fork.go | 18 ++++++++++++++---- services/repository/fork_test.go | 4 ++-- tests/integration/actions_trigger_test.go | 2 +- tests/integration/pull_reopen_test.go | 2 +- tests/integration/pull_review_test.go | 2 +- tests/integration/pull_update_test.go | 2 +- 9 files changed, 25 insertions(+), 15 deletions(-) diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go index 829a977277..97aaffd103 100644 --- a/routers/api/v1/repo/fork.go +++ b/routers/api/v1/repo/fork.go @@ -148,16 +148,16 @@ func CreateFork(ctx *context.APIContext) { name = *form.Name } - fork, err := repo_service.ForkRepository(ctx, ctx.Doer, forker, repo_service.ForkRepoOptions{ + fork, err := repo_service.ForkRepositoryAndUpdates(ctx, ctx.Doer, forker, repo_service.ForkRepoOptions{ BaseRepo: repo, Name: name, Description: repo.Description, }) if err != nil { if errors.Is(err, util.ErrAlreadyExist) || repo_model.IsErrReachLimitOfRepo(err) { - ctx.Error(http.StatusConflict, "ForkRepository", err) + ctx.Error(http.StatusConflict, "ForkRepositoryAndUpdates", err) } else { - ctx.Error(http.StatusInternalServerError, "ForkRepository", err) + ctx.Error(http.StatusInternalServerError, "ForkRepositoryAndUpdates", err) } return } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 5ab139776c..56a46f7f06 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -294,7 +294,7 @@ func ForkPost(ctx *context.Context) { } } - repo, err := repo_service.ForkRepository(ctx, ctx.Doer, ctxUser, repo_service.ForkRepoOptions{ + repo, err := repo_service.ForkRepositoryAndUpdates(ctx, ctx.Doer, ctxUser, repo_service.ForkRepoOptions{ BaseRepo: forkRepo, Name: form.RepoName, Description: form.Description, diff --git a/services/f3/driver/project.go b/services/f3/driver/project.go index d4c3b843aa..b13c352da9 100644 --- a/services/f3/driver/project.go +++ b/services/f3/driver/project.go @@ -155,7 +155,7 @@ func (o *project) Put(ctx context.Context) generic.NodeID { panic(fmt.Errorf("LoadOwner %v %w", o.forgejoProject.BaseRepo, err)) } - repo, err := repo_service.ForkRepository(ctx, doer, owner, repo_service.ForkRepoOptions{ + repo, err := repo_service.ForkRepositoryIfNotExists(ctx, doer, owner, repo_service.ForkRepoOptions{ BaseRepo: o.forgejoProject.BaseRepo, Name: o.forgejoProject.Name, Description: o.forgejoProject.Description, diff --git a/services/repository/fork.go b/services/repository/fork.go index 5346d880f6..0378f7bae6 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -51,8 +51,8 @@ type ForkRepoOptions struct { SingleBranch string } -// ForkRepository forks a repository -func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts ForkRepoOptions) (*repo_model.Repository, error) { +// ForkRepositoryIfNotExists creates a fork of a repository if it does not already exists and fails otherwise +func ForkRepositoryIfNotExists(ctx context.Context, doer, owner *user_model.User, opts ForkRepoOptions) (*repo_model.Repository, error) { // Fork is prohibited, if user has reached maximum limit of repositories if !doer.IsAdmin && !owner.CanForkRepo() { return nil, repo_model.ErrReachLimitOfRepo{ @@ -147,7 +147,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } repoPath := repo_model.RepoPath(owner.Name, repo.Name) if stdout, _, err := cloneCmd.AddDynamicArguments(oldRepoPath, repoPath). - SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", opts.BaseRepo.FullName(), repo.FullName())). + SetDescription(fmt.Sprintf("ForkRepositoryIfNotExists(git clone): %s to %s", opts.BaseRepo.FullName(), repo.FullName())). RunStdBytes(&git.RunOpts{Timeout: 10 * time.Minute}); err != nil { log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, opts.BaseRepo, stdout, err) return fmt.Errorf("git clone: %w", err) @@ -158,7 +158,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } if stdout, _, err := git.NewCommand(txCtx, "update-server-info"). - SetDescription(fmt.Sprintf("ForkRepository(git update-server-info): %s", repo.FullName())). + SetDescription(fmt.Sprintf("ForkRepositoryIfNotExists(git update-server-info): %s", repo.FullName())). RunStdString(&git.RunOpts{Dir: repoPath}); err != nil { log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, stdout, err) return fmt.Errorf("git update-server-info: %w", err) @@ -183,6 +183,16 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, err } + return repo, nil +} + +// ForkRepositoryAndUpdates forks a repository. On success it updates metadata (size, stats, etc.) and send a notification. +func ForkRepositoryAndUpdates(ctx context.Context, doer, owner *user_model.User, opts ForkRepoOptions) (*repo_model.Repository, error) { + repo, err := ForkRepositoryIfNotExists(ctx, doer, owner, opts) + if err != nil { + return nil, err + } + // even if below operations failed, it could be ignored. And they will be retried if err := repo_module.UpdateRepoSize(ctx, repo); err != nil { log.Error("Failed to update size for repository: %v", err) diff --git a/services/repository/fork_test.go b/services/repository/fork_test.go index 4680d99a32..2e1e72aaad 100644 --- a/services/repository/fork_test.go +++ b/services/repository/fork_test.go @@ -23,7 +23,7 @@ func TestForkRepository(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 13}) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) - fork, err := ForkRepository(git.DefaultContext, user, user, ForkRepoOptions{ + fork, err := ForkRepositoryAndUpdates(git.DefaultContext, user, user, ForkRepoOptions{ BaseRepo: repo, Name: "test", Description: "test", @@ -39,7 +39,7 @@ func TestForkRepository(t *testing.T) { setting.Repository.AllowForkWithoutMaximumLimit = false // user has reached maximum limit of repositories user.MaxRepoCreation = 0 - fork2, err := ForkRepository(git.DefaultContext, user, user, ForkRepoOptions{ + fork2, err := ForkRepositoryAndUpdates(git.DefaultContext, user, user, ForkRepoOptions{ BaseRepo: repo, Name: "test", Description: "test", diff --git a/tests/integration/actions_trigger_test.go b/tests/integration/actions_trigger_test.go index 0a00ede806..0f27e204ca 100644 --- a/tests/integration/actions_trigger_test.go +++ b/tests/integration/actions_trigger_test.go @@ -46,7 +46,7 @@ func TestPullRequestTargetEvent(t *testing.T) { defer f() // create the forked repo - forkedRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, org3, repo_service.ForkRepoOptions{ + forkedRepo, err := repo_service.ForkRepositoryAndUpdates(git.DefaultContext, user2, org3, repo_service.ForkRepoOptions{ BaseRepo: baseRepo, Name: "forked-repo-pull-request-target", Description: "test pull-request-target event", diff --git a/tests/integration/pull_reopen_test.go b/tests/integration/pull_reopen_test.go index ff86f80f46..2b7c0c2bf6 100644 --- a/tests/integration/pull_reopen_test.go +++ b/tests/integration/pull_reopen_test.go @@ -70,7 +70,7 @@ func TestPullrequestReopen(t *testing.T) { require.NoError(t, err) // Create an head repository. - headRepo, err := repo_service.ForkRepository(git.DefaultContext, user2, org26, repo_service.ForkRepoOptions{ + headRepo, err := repo_service.ForkRepositoryAndUpdates(git.DefaultContext, user2, org26, repo_service.ForkRepoOptions{ BaseRepo: baseRepo, Name: "reopen-head", }) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 314e614092..3ce1f34844 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -376,7 +376,7 @@ func TestPullView_CodeOwner(t *testing.T) { t.Run("Forked Repo Pull Request", func(t *testing.T) { user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) - forkedRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{ + forkedRepo, err := repo_service.ForkRepositoryAndUpdates(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{ BaseRepo: repo, Name: "test_codeowner_fork", }) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 558c2f794c..003e92c0cf 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -85,7 +85,7 @@ func TestAPIPullUpdateByRebase(t *testing.T) { func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_model.PullRequest { baseRepo, _, _ := CreateDeclarativeRepo(t, actor, "repo-pr-update", nil, nil, nil) - headRepo, err := repo_service.ForkRepository(git.DefaultContext, actor, forkOrg, repo_service.ForkRepoOptions{ + headRepo, err := repo_service.ForkRepositoryAndUpdates(git.DefaultContext, actor, forkOrg, repo_service.ForkRepoOptions{ BaseRepo: baseRepo, Name: "repo-pr-update", Description: "desc",