Refactor the fork service slightly to take ForkRepoOptions (#16744)

* Refactor the fork service slightly to take ForkRepoOptions

This reduces the number of places we need to change if we want to add other
options during fork time.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

* Fix integrations and tests after ForkRepository refactor

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

* Update OldRepo -> BaseRepo

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

* gofmt pass

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

integrations/pull_update_test.go

@@ -60,7 +60,11 @@ func createOutdatedPR(t *testing.T, actor, forkOrg *models.User) *models.PullReq
 	assert.NoError(t, err)
 	assert.NotEmpty(t, baseRepo)
 
-	headRepo, err := repo_module.ForkRepository(actor, forkOrg, baseRepo, "repo-pr-update", "desc")
+	headRepo, err := repo_module.ForkRepository(actor, forkOrg, models.ForkRepoOptions{
+		BaseRepo:    baseRepo,
+		Name:        "repo-pr-update",
+		Description: "desc",
+	})
 	assert.NoError(t, err)
 	assert.NotEmpty(t, headRepo)
 

models/repo.go

@@ -1004,6 +1004,13 @@ type CreateRepoOptions struct {
 	MirrorInterval string
 }
 
+// ForkRepoOptions contains the fork repository options
+type ForkRepoOptions struct {
+	BaseRepo    *Repository
+	Name        string
+	Description string
+}
+
 // GetRepoInitFile returns repository init files
 func GetRepoInitFile(tp, name string) ([]byte, error) {
 	cleanedName := strings.TrimLeft(path.Clean("/"+name), "/")

modules/repository/fork.go

@@ -16,15 +16,15 @@ import (
 )
 
 // ForkRepository forks a repository
-func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name, desc string) (_ *models.Repository, err error) {
+func ForkRepository(doer, owner *models.User, opts models.ForkRepoOptions) (_ *models.Repository, err error) {
-	forkedRepo, err := oldRepo.GetUserFork(owner.ID)
+	forkedRepo, err := opts.BaseRepo.GetUserFork(owner.ID)
 	if err != nil {
 		return nil, err
 	}
 	if forkedRepo != nil {
 		return nil, models.ErrForkAlreadyExist{
 			Uname:    owner.Name,
-			RepoName: oldRepo.FullName(),
+			RepoName: opts.BaseRepo.FullName(),
 			ForkName: forkedRepo.FullName(),
 		}
 	}
@@ -33,17 +33,17 @@ func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name,
 		OwnerID:       owner.ID,
 		Owner:         owner,
 		OwnerName:     owner.Name,
-		Name:          name,
+		Name:          opts.Name,
-		LowerName:     strings.ToLower(name),
+		LowerName:     strings.ToLower(opts.Name),
-		Description:   desc,
+		Description:   opts.Description,
-		DefaultBranch: oldRepo.DefaultBranch,
+		DefaultBranch: opts.BaseRepo.DefaultBranch,
-		IsPrivate:     oldRepo.IsPrivate || oldRepo.Owner.Visibility == structs.VisibleTypePrivate,
+		IsPrivate:     opts.BaseRepo.IsPrivate || opts.BaseRepo.Owner.Visibility == structs.VisibleTypePrivate,
-		IsEmpty:       oldRepo.IsEmpty,
+		IsEmpty:       opts.BaseRepo.IsEmpty,
 		IsFork:        true,
-		ForkID:        oldRepo.ID,
+		ForkID:        opts.BaseRepo.ID,
 	}
 
-	oldRepoPath := oldRepo.RepoPath()
+	oldRepoPath := opts.BaseRepo.RepoPath()
 
 	err = models.WithTx(func(ctx models.DBContext) error {
 		if err = models.CreateRepository(ctx, doer, owner, repo, false); err != nil {
@@ -59,13 +59,13 @@ func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name,
 			}
 		}
 
-		if err = models.IncrementRepoForkNum(ctx, oldRepo.ID); err != nil {
+		if err = models.IncrementRepoForkNum(ctx, opts.BaseRepo.ID); err != nil {
 			rollbackRemoveFn()
 			return err
 		}
 
 		// copy lfs files failure should not be ignored
-		if err := models.CopyLFS(ctx, repo, oldRepo); err != nil {
+		if err := models.CopyLFS(ctx, repo, opts.BaseRepo); err != nil {
 			rollbackRemoveFn()
 			return err
 		}
@@ -73,9 +73,9 @@ func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name,
 		repoPath := models.RepoPath(owner.Name, repo.Name)
 		if stdout, err := git.NewCommand(
 			"clone", "--bare", oldRepoPath, repoPath).
-			SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", oldRepo.FullName(), repo.FullName())).
+			SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", opts.BaseRepo.FullName(), repo.FullName())).
 			RunInDirTimeout(10*time.Minute, ""); err != nil {
-			log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, oldRepo, stdout, err)
+			log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, opts.BaseRepo, stdout, err)
 			rollbackRemoveFn()
 			return fmt.Errorf("git clone: %v", err)
 		}
@@ -103,7 +103,7 @@ func ForkRepository(doer, owner *models.User, oldRepo *models.Repository, name,
 	if err = repo.UpdateSize(ctx); err != nil {
 		log.Error("Failed to update size for repository: %v", err)
 	}
-	if err := models.CopyLanguageStat(oldRepo, repo); err != nil {
+	if err := models.CopyLanguageStat(opts.BaseRepo, repo); err != nil {
 		log.Error("Copy language stat from oldRepo failed")
 	}
 

modules/repository/fork_test.go

@@ -18,7 +18,11 @@ func TestForkRepository(t *testing.T) {
 	user := models.AssertExistsAndLoadBean(t, &models.User{ID: 13}).(*models.User)
 	repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 10}).(*models.Repository)
 
-	fork, err := ForkRepository(user, user, repo, "test", "test")
+	fork, err := ForkRepository(user, user, models.ForkRepoOptions{
+		BaseRepo:    repo,
+		Name:        "test",
+		Description: "test",
+	})
 	assert.Nil(t, fork)
 	assert.Error(t, err)
 	assert.True(t, models.IsErrForkAlreadyExist(err))

routers/api/v1/repo/fork.go

@@ -123,7 +123,11 @@ func CreateFork(ctx *context.APIContext) {
 		forker = org
 	}
 
-	fork, err := repo_service.ForkRepository(ctx.User, forker, repo, repo.Name, repo.Description)
+	fork, err := repo_service.ForkRepository(ctx.User, forker, models.ForkRepoOptions{
+		BaseRepo:    repo,
+		Name:        repo.Name,
+		Description: repo.Description,
+	})
 	if err != nil {
 		ctx.Error(http.StatusInternalServerError, "ForkRepository", err)
 		return

routers/web/repo/pull.go

@@ -225,7 +225,11 @@ func ForkPost(ctx *context.Context) {
 		}
 	}
 
-	repo, err := repo_service.ForkRepository(ctx.User, ctxUser, forkRepo, form.RepoName, form.Description)
+	repo, err := repo_service.ForkRepository(ctx.User, ctxUser, models.ForkRepoOptions{
+		BaseRepo:    forkRepo,
+		Name:        form.RepoName,
+		Description: form.Description,
+	})
 	if err != nil {
 		ctx.Data["Err_RepoName"] = true
 		switch {

services/repository/repository.go

@@ -47,13 +47,13 @@ func DeleteUnadoptedRepository(doer, owner *models.User, name string) error {
 }
 
 // ForkRepository forks a repository
-func ForkRepository(doer, u *models.User, oldRepo *models.Repository, name, desc string) (*models.Repository, error) {
+func ForkRepository(doer, u *models.User, opts models.ForkRepoOptions) (*models.Repository, error) {
-	repo, err := repo_module.ForkRepository(doer, u, oldRepo, name, desc)
+	repo, err := repo_module.ForkRepository(doer, u, opts)
 	if err != nil {
 		return nil, err
 	}
 
-	notification.NotifyForkRepository(doer, oldRepo, repo)
+	notification.NotifyForkRepository(doer, opts.BaseRepo, repo)
 
 	return repo, nil
 }