From 2c02c2eb47bf320701f181b39c3e1e76dcaf21a3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 21 May 2024 14:48:19 +0800 Subject: [PATCH 01/14] Cleanup resources when create/adopt/generate repository failed --- services/repository/adopt.go | 49 ++++++++++++++++++++++----------- services/repository/create.go | 31 +++++++++++++-------- services/repository/generate.go | 1 + services/repository/template.go | 29 +++++++++++++++---- 4 files changed, 77 insertions(+), 33 deletions(-) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 3d6fe71a09191..dde796b916343 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" + system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" @@ -48,28 +49,28 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR IsPrivate: opts.IsPrivate, IsFsckEnabled: !opts.IsMirror, CloseIssuesViaCommitInAnyBranch: setting.Repository.DefaultCloseIssuesViaCommitsInAnyBranch, - Status: opts.Status, + Status: repo_model.RepositoryBeingMigrated, IsEmpty: !opts.AutoInit, } - if err := db.WithTx(ctx, func(ctx context.Context) error { - repoPath := repo_model.RepoPath(u.Name, repo.Name) - isExist, err := util.IsExist(repoPath) - if err != nil { - log.Error("Unable to check if %s exists. Error: %v", repoPath, err) - return err - } - if !isExist { - return repo_model.ErrRepoNotExist{ - OwnerName: u.Name, - Name: repo.Name, - } + repoPath := repo_model.RepoPath(u.Name, repo.Name) + isExist, err := util.IsExist(repoPath) + if err != nil { + log.Error("Unable to check if %s exists. Error: %v", repoPath, err) + return nil, err + } + if !isExist { + return nil, repo_model.ErrRepoNotExist{ + OwnerName: u.Name, + Name: repo.Name, } + } - if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, true, false); err != nil { - return err - } + if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, true, false); err != nil { + return nil, err + } + if err := db.WithTx(ctx, func(ctx context.Context) error { // Re-fetch the repository from database before updating it (else it would // override changes that were done earlier with sql) if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil { @@ -97,8 +98,24 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) return fmt.Errorf("CreateRepository(git update-server-info): %w", err) } + + // update repository status + repo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return fmt.Errorf("UpdateRepositoryCols: %w", err) + } + return nil }); err != nil { + if repo != nil { + if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when adopt repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } + } + } return nil, err } diff --git a/services/repository/create.go b/services/repository/create.go index 971793bcc6e35..7b8dafc886437 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -14,6 +14,7 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" + system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" @@ -240,13 +241,13 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt ObjectFormatName: opts.ObjectFormatName, } - var rollbackRepo *repo_model.Repository + needsUpdateStatus := opts.Status == 0 - if err := db.WithTx(ctx, func(ctx context.Context) error { - if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, false, false); err != nil { - return err - } + if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, false, false); err != nil { + return nil, err + } + if err := db.WithTx(ctx, func(ctx context.Context) error { // No need for init mirror. if opts.IsMirror { return nil @@ -285,8 +286,6 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt // Initialize Issue Labels if selected if len(opts.IssueLabels) > 0 { if err = repo_module.InitializeLabels(ctx, repo.ID, opts.IssueLabels, false); err != nil { - rollbackRepo = repo - rollbackRepo.OwnerID = u.ID return fmt.Errorf("InitializeLabels: %w", err) } } @@ -299,15 +298,25 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt SetDescription(fmt.Sprintf("CreateRepository(git update-server-info): %s", repoPath)). RunStdString(&git.RunOpts{Dir: repoPath}); err != nil { log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) - rollbackRepo = repo - rollbackRepo.OwnerID = u.ID return fmt.Errorf("CreateRepository(git update-server-info): %w", err) } + + if needsUpdateStatus { + repo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return fmt.Errorf("UpdateRepositoryCols: %w", err) + } + } + return nil }); err != nil { - if rollbackRepo != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, rollbackRepo.ID); errDelete != nil { + if repo != nil { + if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } } } diff --git a/services/repository/generate.go b/services/repository/generate.go index 9b09e271ab41a..941e22a1c143f 100644 --- a/services/repository/generate.go +++ b/services/repository/generate.go @@ -335,6 +335,7 @@ func generateRepository(ctx context.Context, doer, owner *user_model.User, templ TemplateID: templateRepo.ID, TrustModel: templateRepo.TrustModel, ObjectFormatName: templateRepo.ObjectFormatName, + Status: repo_model.RepositoryBeingMigrated, } if err = repo_module.CreateRepositoryByExample(ctx, doer, owner, generateRepo, false, false); err != nil { diff --git a/services/repository/template.go b/services/repository/template.go index 36a680c8e2009..1d250fffd7126 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -5,12 +5,15 @@ package repository import ( "context" + "fmt" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" + system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" notify_service "code.gitea.io/gitea/services/notify" ) @@ -69,13 +72,12 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } } - var generateRepo *repo_model.Repository - if err = db.WithTx(ctx, func(ctx context.Context) error { - generateRepo, err = generateRepository(ctx, doer, owner, templateRepo, opts) - if err != nil { - return err - } + generateRepo, err := generateRepository(ctx, doer, owner, templateRepo, opts) + if err != nil { + return nil, err + } + if err = db.WithTx(ctx, func(ctx context.Context) error { // Git Content if opts.GitContent && !templateRepo.IsEmpty { if err = GenerateGitContent(ctx, templateRepo, generateRepo); err != nil { @@ -124,8 +126,23 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } } + // update repository status to be ready + generateRepo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, generateRepo, "status"); err != nil { + return fmt.Errorf("UpdateRepositoryCols: %w", err) + } + return nil }); err != nil { + if generateRepo != nil { + if errDelete := DeleteRepositoryDirectly(ctx, doer, generateRepo.ID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when generate repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } + } + } return nil, err } From d66b9059a2648bf224af5c669692a864344abbf9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 27 May 2024 14:37:19 +0800 Subject: [PATCH 02/14] Add transaction for CreateRepoByExample --- services/repository/adopt.go | 5 ++++- services/repository/create.go | 6 ++++-- services/repository/generate.go | 18 +++++++++++++++++- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index dde796b916343..ed3e4f22ccf93 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -66,7 +66,10 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR } } - if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, true, false); err != nil { + // create the repository database operations first + if err := db.WithTx(ctx, func(ctx context.Context) error { + return repo_module.CreateRepositoryByExample(ctx, doer, u, repo, true, false) + }); err != nil { return nil, err } diff --git a/services/repository/create.go b/services/repository/create.go index 7b8dafc886437..1d840d3a25e87 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -241,9 +241,11 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt ObjectFormatName: opts.ObjectFormatName, } - needsUpdateStatus := opts.Status == 0 + needsUpdateStatus := opts.Status != repo_model.RepositoryReady - if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, false, false); err != nil { + if err := db.WithTx(ctx, func(ctx context.Context) error { + return repo_module.CreateRepositoryByExample(ctx, doer, u, repo, false, false) + }); err != nil { return nil, err } diff --git a/services/repository/generate.go b/services/repository/generate.go index 941e22a1c143f..4029415cd220b 100644 --- a/services/repository/generate.go +++ b/services/repository/generate.go @@ -15,8 +15,10 @@ import ( "strings" "time" + "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" + system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" @@ -338,12 +340,26 @@ func generateRepository(ctx context.Context, doer, owner *user_model.User, templ Status: repo_model.RepositoryBeingMigrated, } - if err = repo_module.CreateRepositoryByExample(ctx, doer, owner, generateRepo, false, false); err != nil { + if err := db.WithTx(ctx, func(ctx context.Context) error { + return repo_module.CreateRepositoryByExample(ctx, doer, owner, generateRepo, false, false) + }); err != nil { return nil, err } repoPath := generateRepo.RepoPath() isExist, err := util.IsExist(repoPath) + defer func() { + if err != nil { + if errDelete := DeleteRepositoryDirectly(ctx, doer, generateRepo.ID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when generate repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } + } + } + }() + if err != nil { log.Error("Unable to check if %s exists. Error: %v", repoPath, err) return nil, err From 60c85e7d16cb8db1f9e0e706198251c605d22c33 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 26 Mar 2025 14:01:06 -0700 Subject: [PATCH 03/14] Some improvements --- models/git/branch.go | 5 ++ models/repo/release.go | 5 ++ services/repository/adopt.go | 90 ++++++++++--------- services/repository/create.go | 18 +++- services/repository/delete.go | 21 +++-- services/repository/fork.go | 135 ++++++++++++---------------- services/repository/generate.go | 69 --------------- services/repository/template.go | 151 +++++++++++++++++++++----------- 8 files changed, 238 insertions(+), 256 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index d1caa35947b40..5cc036ac2bdf1 100644 --- a/models/git/branch.go +++ b/models/git/branch.go @@ -223,6 +223,11 @@ func GetDeletedBranchByID(ctx context.Context, repoID, branchID int64) (*Branch, return &branch, nil } +func DeleteRepoBranches(ctx context.Context, repoID int64) error { + _, err := db.GetEngine(ctx).Where("repo_id=?", repoID).Delete(new(Branch)) + return err +} + func DeleteBranches(ctx context.Context, repoID, doerID int64, branchIDs []int64) error { return db.WithTx(ctx, func(ctx context.Context) error { branches := make([]*Branch, 0, len(branchIDs)) diff --git a/models/repo/release.go b/models/repo/release.go index 1c2e4a48e399d..663d310bc027d 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -558,3 +558,8 @@ func FindTagsByCommitIDs(ctx context.Context, repoID int64, commitIDs ...string) } return res, nil } + +func DeleteRepoReleases(ctx context.Context, repoID int64) error { + _, err := db.GetEngine(ctx).Where("repo_id = ?", repoID).Delete(new(Release)) + return err +} diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 21fed30c228ed..9fc2e9ab5705b 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -28,6 +28,18 @@ import ( "github.com/gobwas/glob" ) +func deleteFailedAdoptRepository(ctx context.Context, repoID int64) error { + return db.WithTx(ctx, func(ctx context.Context) error { + if err := deleteDBRepository(ctx, repoID); err != nil { + return fmt.Errorf("deleteDBRepository: %w", err) + } + if err := git_model.DeleteRepoBranches(ctx, repoID); err != nil { + return fmt.Errorf("deleteRepoBranches: %w", err) + } + return repo_model.DeleteRepoReleases(ctx, repoID) + }) +} + // AdoptRepository adopts pre-existing repository files for the user/organization. func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateRepoOptions) (*repo_model.Repository, error) { if !doer.IsAdmin && !u.CanCreateRepo() { @@ -52,64 +64,50 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR IsEmpty: !opts.AutoInit, } - isExist, err := gitrepo.IsRepositoryExist(ctx, repo) + // 1 - create the repository database operations first + err := db.WithTx(ctx, func(ctx context.Context) error { + return CreateRepositoryInDB(ctx, doer, u, repo, true, false) + }) if err != nil { - log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) return nil, err } - if !isExist { - return nil, repo_model.ErrRepoNotExist{ - OwnerName: u.Name, - Name: repo.Name, + + // last - clean up if something goes wrong + defer func() { + if err != nil { + if errDel := deleteFailedAdoptRepository(ctx, repo.ID); errDel != nil { + log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel) + } } - } + }() - // create the repository database operations first - if err := db.WithTx(ctx, func(ctx context.Context) error { - return CreateRepositoryByExample(ctx, doer, u, repo, true, false) - }); err != nil { - return nil, err + // Re-fetch the repository from database before updating it (else it would + // override changes that were done earlier with sql) + if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil { + return nil, fmt.Errorf("getRepositoryByID: %w", err) } - if err := db.WithTx(ctx, func(ctx context.Context) error { - // Re-fetch the repository from database before updating it (else it would - // override changes that were done earlier with sql) - if repo, err = repo_model.GetRepositoryByID(ctx, repo.ID); err != nil { - return fmt.Errorf("getRepositoryByID: %w", err) - } - return nil - }); err != nil { - return nil, err + // 3 - adopt the repository from disk + if err := adoptRepository(ctx, repo, opts.DefaultBranch); err != nil { + return nil, fmt.Errorf("adoptRepository: %w", err) } - if err := func() error { - if err := adoptRepository(ctx, repo, opts.DefaultBranch); err != nil { - return fmt.Errorf("adoptRepository: %w", err) - } - - if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { - return fmt.Errorf("checkDaemonExportOK: %w", err) - } - - if stdout, _, err := git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { - log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) - return fmt.Errorf("CreateRepository(git update-server-info): %w", err) - } + if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + return nil, fmt.Errorf("checkDaemonExportOK: %w", err) + } - // update repository status - repo.Status = repo_model.RepositoryReady - if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { - return fmt.Errorf("UpdateRepositoryCols: %w", err) - } + if stdout, _, err := git.NewCommand("update-server-info"). + RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { + log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) + return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err) + } - return nil - }(); err != nil { - if errDel := DeleteRepository(ctx, doer, repo, false /* no notify */); errDel != nil { - log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel) - } - return nil, err + // 4 - update repository status + repo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) } + notify_service.AdoptRepository(ctx, doer, u, repo) return repo, nil diff --git a/services/repository/create.go b/services/repository/create.go index 543351e8cc25f..4d12baa8cd5d1 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -248,7 +248,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt needsUpdateStatus := opts.Status != repo_model.RepositoryReady if err := db.WithTx(ctx, func(ctx context.Context) error { - return CreateRepositoryByExample(ctx, doer, u, repo, false, false) + return CreateRepositoryInDB(ctx, doer, u, repo, false, false) }); err != nil { return nil, err } @@ -344,8 +344,8 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt return repo, nil } -// CreateRepositoryByExample creates a repository for the user/organization. -func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt, isFork bool) (err error) { +// CreateRepositoryInDB creates a repository for the user/organization. +func CreateRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt, isFork bool) (err error) { if err = repo_model.IsUsableRepoName(repo.Name); err != nil { return err } @@ -365,7 +365,17 @@ func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, re log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) return err } - if !overwriteOrAdopt && isExist { + if overwriteOrAdopt != isExist { + if overwriteOrAdopt { + // repo should exist but doesn't - We have two or three options. + log.Error("Files do not exist in %s and we are not going to adopt or delete.", repo.FullName()) + return repo_model.ErrRepoNotExist{ + OwnerName: u.Name, + Name: repo.Name, + } + } + + // repo already exists - We have two or three options. log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) return repo_model.ErrRepoFilesAlreadyExist{ Uname: u.Name, diff --git a/services/repository/delete.go b/services/repository/delete.go index ff74a20817a7c..cf960af8cf748 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -32,6 +32,19 @@ import ( "xorm.io/builder" ) +func deleteDBRepository(ctx context.Context, repoID int64) error { + if cnt, err := db.GetEngine(ctx).ID(repoID).Delete(&repo_model.Repository{}); err != nil { + return err + } else if cnt != 1 { + return repo_model.ErrRepoNotExist{ + ID: repoID, + OwnerName: "", + Name: "", + } + } + return nil +} + // DeleteRepository deletes a repository for a user or organization. // make sure if you call this func to close open sessions (sqlite will otherwise get a deadlock) func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID int64, ignoreOrgTeams ...bool) error { @@ -82,14 +95,8 @@ func DeleteRepositoryDirectly(ctx context.Context, doer *user_model.User, repoID } needRewriteKeysFile := deleted > 0 - if cnt, err := sess.ID(repoID).Delete(&repo_model.Repository{}); err != nil { + if err := deleteDBRepository(ctx, repoID); err != nil { return err - } else if cnt != 1 { - return repo_model.ErrRepoNotExist{ - ID: repoID, - OwnerName: "", - Name: "", - } } if org != nil && org.IsOrganization() { diff --git a/services/repository/fork.go b/services/repository/fork.go index 5b1ba7a418232..e2956a8e96a29 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -100,95 +100,76 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork IsFork: true, ForkID: opts.BaseRepo.ID, ObjectFormatName: opts.BaseRepo.ObjectFormatName, + Status: repo_model.RepositoryBeingMigrated, } - oldRepoPath := opts.BaseRepo.RepoPath() - - needsRollback := false - rollbackFn := func() { - if !needsRollback { - return - } - - if exists, _ := gitrepo.IsRepositoryExist(ctx, repo); !exists { - return - } - - // As the transaction will be failed and hence database changes will be destroyed we only need - // to delete the related repository on the filesystem - if errDelete := gitrepo.DeleteRepository(ctx, repo); errDelete != nil { - log.Error("Failed to remove fork repo") - } - } - - needsRollbackInPanic := true - defer func() { - panicErr := recover() - if panicErr == nil { - return - } - - if needsRollbackInPanic { - rollbackFn() - } - panic(panicErr) - }() - - err = db.WithTx(ctx, func(txCtx context.Context) error { - if err = CreateRepositoryByExample(txCtx, doer, owner, repo, false, true); err != nil { + // 1 - Create the repository in the database + err = db.WithTx(ctx, func(ctx context.Context) error { + if err = CreateRepositoryInDB(ctx, doer, owner, repo, false, true); err != nil { return err } - - if err = repo_model.IncrementRepoForkNum(txCtx, opts.BaseRepo.ID); err != nil { + if err = repo_model.IncrementRepoForkNum(ctx, opts.BaseRepo.ID); err != nil { return err } // copy lfs files failure should not be ignored - if err = git_model.CopyLFS(txCtx, repo, opts.BaseRepo); err != nil { - return err - } - - needsRollback = true + return git_model.CopyLFS(ctx, repo, opts.BaseRepo) + }) + if err != nil { + return nil, err + } - cloneCmd := git.NewCommand("clone", "--bare") - if opts.SingleBranch != "" { - cloneCmd.AddArguments("--single-branch", "--branch").AddDynamicArguments(opts.SingleBranch) - } - if stdout, _, err := cloneCmd.AddDynamicArguments(oldRepoPath, repo.RepoPath()). - RunStdBytes(txCtx, &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) + // last - clean up if something goes wrong + defer func() { + if err != nil { + if errDelete := gitrepo.DeleteRepository(ctx, repo); errDelete != nil { + log.Error("Failed to remove fork repo") + } } + }() - if err := repo_module.CheckDaemonExportOK(txCtx, repo); err != nil { - return fmt.Errorf("checkDaemonExportOK: %w", err) - } + // 2 - Clone the repository + cloneCmd := git.NewCommand("clone", "--bare") + if opts.SingleBranch != "" { + cloneCmd.AddArguments("--single-branch", "--branch").AddDynamicArguments(opts.SingleBranch) + } + if stdout, _, err := cloneCmd.AddDynamicArguments(opts.BaseRepo.RepoPath(), repo.RepoPath()). + RunStdBytes(ctx, &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 nil, fmt.Errorf("git clone: %w", err) + } - if stdout, _, err := git.NewCommand("update-server-info"). - RunStdString(txCtx, &git.RunOpts{Dir: repo.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) - } + // 3 - Update the repository + if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + return nil, fmt.Errorf("checkDaemonExportOK: %w", err) + } - if err = gitrepo.CreateDelegateHooks(ctx, repo); err != nil { - return fmt.Errorf("createDelegateHooks: %w", err) - } + if stdout, _, err := git.NewCommand("update-server-info"). + RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { + log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, stdout, err) + return nil, fmt.Errorf("git update-server-info: %w", err) + } - gitRepo, err := gitrepo.OpenRepository(txCtx, repo) - if err != nil { - return fmt.Errorf("OpenRepository: %w", err) - } - defer gitRepo.Close() + // 4 - Create hooks + if err = gitrepo.CreateDelegateHooks(ctx, repo); err != nil { + return nil, fmt.Errorf("createDelegateHooks: %w", err) + } - _, err = repo_module.SyncRepoBranchesWithRepo(txCtx, repo, gitRepo, doer.ID) - return err - }) - needsRollbackInPanic = false + // 5 - Sync the repository branches and tags + gitRepo, err := gitrepo.OpenRepository(ctx, repo) if err != nil { - rollbackFn() - return nil, err + return nil, fmt.Errorf("OpenRepository: %w", err) } + defer gitRepo.Close() + if _, err = repo_module.SyncRepoBranchesWithRepo(ctx, repo, gitRepo, doer.ID); err != nil { + return nil, fmt.Errorf("SyncRepoBranchesWithRepo: %w", err) + } + if err := repo_module.SyncReleasesWithTags(ctx, repo, gitRepo); err != nil { + return nil, fmt.Errorf("Sync releases from git tags failed: %v", err) + } + + // 6 - Update the repository // 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) @@ -200,14 +181,10 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, err } - gitRepo, err := gitrepo.OpenRepository(ctx, repo) - if err != nil { - log.Error("Open created git repository failed: %v", err) - } else { - defer gitRepo.Close() - if err := repo_module.SyncReleasesWithTags(ctx, repo, gitRepo); err != nil { - log.Error("Sync releases from git tags failed: %v", err) - } + // 7 - update repository status to be ready + repo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) } notify_service.ForkRepository(ctx, doer, opts.BaseRepo, repo) diff --git a/services/repository/generate.go b/services/repository/generate.go index 188fa0d823cb4..1c4d3b7b6373c 100644 --- a/services/repository/generate.go +++ b/services/repository/generate.go @@ -15,11 +15,8 @@ import ( "strings" "time" - "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" - system_model "code.gitea.io/gitea/models/system" - user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" @@ -330,72 +327,6 @@ func (gro GenerateRepoOptions) IsValid() bool { gro.IssueLabels || gro.ProtectedBranch // or other items as they are added } -// generateRepository generates a repository from a template -func generateRepository(ctx context.Context, doer, owner *user_model.User, templateRepo *repo_model.Repository, opts GenerateRepoOptions) (_ *repo_model.Repository, err error) { - generateRepo := &repo_model.Repository{ - OwnerID: owner.ID, - Owner: owner, - OwnerName: owner.Name, - Name: opts.Name, - LowerName: strings.ToLower(opts.Name), - Description: opts.Description, - DefaultBranch: opts.DefaultBranch, - IsPrivate: opts.Private, - IsEmpty: !opts.GitContent || templateRepo.IsEmpty, - IsFsckEnabled: templateRepo.IsFsckEnabled, - TemplateID: templateRepo.ID, - TrustModel: templateRepo.TrustModel, - ObjectFormatName: templateRepo.ObjectFormatName, - Status: repo_model.RepositoryBeingMigrated, - } - - if err := db.WithTx(ctx, func(ctx context.Context) error { - return CreateRepositoryByExample(ctx, doer, owner, generateRepo, false, false) - }); err != nil { - return nil, err - } - - isExist, err := gitrepo.IsRepositoryExist(ctx, generateRepo) - defer func() { - if err != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, generateRepo.ID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) - // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when generate repository: %v", errDelete); err != nil { - log.Error("CreateRepositoryNotice: %v", err) - } - } - } - }() - - if err != nil { - log.Error("Unable to check if %s exists. Error: %v", generateRepo.FullName(), err) - return nil, err - } - if isExist { - return nil, repo_model.ErrRepoFilesAlreadyExist{ - Uname: generateRepo.OwnerName, - Name: generateRepo.Name, - } - } - - if err = repo_module.CheckInitRepository(ctx, generateRepo); err != nil { - return generateRepo, err - } - - if err = repo_module.CheckDaemonExportOK(ctx, generateRepo); err != nil { - return generateRepo, fmt.Errorf("checkDaemonExportOK: %w", err) - } - - if stdout, _, err := git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: generateRepo.RepoPath()}); err != nil { - log.Error("GenerateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", generateRepo, stdout, err) - return generateRepo, fmt.Errorf("error in GenerateRepository(git update-server-info): %w", err) - } - - return generateRepo, nil -} - var fileNameSanitizeRegexp = regexp.MustCompile(`(?i)\.\.|[<>:\"/\\|?*\x{0000}-\x{001F}]|^(con|prn|aux|nul|com\d|lpt\d)$`) // Sanitize user input to valid OS filenames diff --git a/services/repository/template.go b/services/repository/template.go index 1d250fffd7126..bc44abfd8473a 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -6,6 +6,7 @@ package repository import ( "context" "fmt" + "strings" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" @@ -13,7 +14,10 @@ import ( repo_model "code.gitea.io/gitea/models/repo" system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" + repo_module "code.gitea.io/gitea/modules/repository" notify_service "code.gitea.io/gitea/services/notify" ) @@ -72,78 +76,123 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } } - generateRepo, err := generateRepository(ctx, doer, owner, templateRepo, opts) + generateRepo := &repo_model.Repository{ + OwnerID: owner.ID, + Owner: owner, + OwnerName: owner.Name, + Name: opts.Name, + LowerName: strings.ToLower(opts.Name), + Description: opts.Description, + DefaultBranch: opts.DefaultBranch, + IsPrivate: opts.Private, + IsEmpty: !opts.GitContent || templateRepo.IsEmpty, + IsFsckEnabled: templateRepo.IsFsckEnabled, + TemplateID: templateRepo.ID, + TrustModel: templateRepo.TrustModel, + ObjectFormatName: templateRepo.ObjectFormatName, + Status: repo_model.RepositoryBeingMigrated, + } + + // 1 - check whether the repository with the same storage exists + isExist, err := gitrepo.IsRepositoryExist(ctx, generateRepo) if err != nil { + log.Error("Unable to check if %s exists. Error: %v", generateRepo.FullName(), err) return nil, err } - - if err = db.WithTx(ctx, func(ctx context.Context) error { - // Git Content - if opts.GitContent && !templateRepo.IsEmpty { - if err = GenerateGitContent(ctx, templateRepo, generateRepo); err != nil { - return err - } + if isExist { + return nil, repo_model.ErrRepoFilesAlreadyExist{ + Uname: generateRepo.OwnerName, + Name: generateRepo.Name, } + } + + // 2 - Create the repository in the database + if err := db.WithTx(ctx, func(ctx context.Context) error { + return CreateRepositoryInDB(ctx, doer, owner, generateRepo, false, false) + }); err != nil { + return nil, err + } - // Topics - if opts.Topics { - if err = repo_model.GenerateTopics(ctx, templateRepo, generateRepo); err != nil { - return err + // last - clean up the repository if something goes wrong + defer func() { + if err != nil { + if errDelete := DeleteRepositoryDirectly(ctx, doer, generateRepo.ID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when generate repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } } } + }() - // Git Hooks - if opts.GitHooks { - if err = GenerateGitHooks(ctx, templateRepo, generateRepo); err != nil { - return err - } + // 3 - Generate the git repository in storage + if err = repo_module.CheckInitRepository(ctx, generateRepo); err != nil { + return nil, err + } + + if err = repo_module.CheckDaemonExportOK(ctx, generateRepo); err != nil { + return nil, fmt.Errorf("checkDaemonExportOK: %w", err) + } + + if stdout, _, err := git.NewCommand("update-server-info"). + RunStdString(ctx, &git.RunOpts{Dir: generateRepo.RepoPath()}); err != nil { + log.Error("GenerateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", generateRepo, stdout, err) + return nil, fmt.Errorf("error in GenerateRepository(git update-server-info): %w", err) + } + + // Git Content + if opts.GitContent && !templateRepo.IsEmpty { + if err = GenerateGitContent(ctx, templateRepo, generateRepo); err != nil { + return nil, err } + } - // Webhooks - if opts.Webhooks { - if err = GenerateWebhooks(ctx, templateRepo, generateRepo); err != nil { - return err - } + // Topics + if opts.Topics { + if err = repo_model.GenerateTopics(ctx, templateRepo, generateRepo); err != nil { + return nil, err } + } - // Avatar - if opts.Avatar && len(templateRepo.Avatar) > 0 { - if err = generateAvatar(ctx, templateRepo, generateRepo); err != nil { - return err - } + // Git Hooks + if opts.GitHooks { + if err = GenerateGitHooks(ctx, templateRepo, generateRepo); err != nil { + return nil, err } + } - // Issue Labels - if opts.IssueLabels { - if err = GenerateIssueLabels(ctx, templateRepo, generateRepo); err != nil { - return err - } + // Webhooks + if opts.Webhooks { + if err = GenerateWebhooks(ctx, templateRepo, generateRepo); err != nil { + return nil, err } + } - if opts.ProtectedBranch { - if err = GenerateProtectedBranch(ctx, templateRepo, generateRepo); err != nil { - return err - } + // Avatar + if opts.Avatar && len(templateRepo.Avatar) > 0 { + if err = generateAvatar(ctx, templateRepo, generateRepo); err != nil { + return nil, err } + } - // update repository status to be ready - generateRepo.Status = repo_model.RepositoryReady - if err = repo_model.UpdateRepositoryCols(ctx, generateRepo, "status"); err != nil { - return fmt.Errorf("UpdateRepositoryCols: %w", err) + // Issue Labels + if opts.IssueLabels { + if err = GenerateIssueLabels(ctx, templateRepo, generateRepo); err != nil { + return nil, err } + } - return nil - }); err != nil { - if generateRepo != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, generateRepo.ID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) - // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when generate repository: %v", errDelete); err != nil { - log.Error("CreateRepositoryNotice: %v", err) - } - } + if opts.ProtectedBranch { + if err = GenerateProtectedBranch(ctx, templateRepo, generateRepo); err != nil { + return nil, err } - return nil, err + } + + // 4 - update repository status to be ready + generateRepo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, generateRepo, "status"); err != nil { + return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) } notify_service.CreateRepository(ctx, doer, owner, generateRepo) From ec587178556bad0dc1949b338b7324e5ca589570 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 26 Mar 2025 14:07:47 -0700 Subject: [PATCH 04/14] Fix bug --- services/repository/fork.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/repository/fork.go b/services/repository/fork.go index e2956a8e96a29..5f9db8f3eaad2 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -122,8 +122,8 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork // last - clean up if something goes wrong defer func() { if err != nil { - if errDelete := gitrepo.DeleteRepository(ctx, repo); errDelete != nil { - log.Error("Failed to remove fork repo") + if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { + log.Error("Failed to remove fork repo: %v", errDelete) } } }() From 1d13cc37fd374fa177a5c86d3b139fa3ba8e8255 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 26 Mar 2025 19:28:11 -0700 Subject: [PATCH 05/14] Add tests --- services/repository/adopt.go | 8 +++-- services/repository/adopt_test.go | 26 +++++++++++++- services/repository/create_test.go | 57 ++++++++++++++++++++++++++++++ services/repository/fork.go | 24 ++++++++----- services/repository/fork_test.go | 43 ++++++++++++++++++++++ tests/integration/repo_test.go | 49 +++++++++++++++++++++++++ 6 files changed, 194 insertions(+), 13 deletions(-) create mode 100644 services/repository/create_test.go diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 9fc2e9ab5705b..f19110acb2b28 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -73,6 +73,7 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR } // last - clean up if something goes wrong + // WARNING: Don't override all later err with local variables defer func() { if err != nil { if errDel := deleteFailedAdoptRepository(ctx, repo.ID); errDel != nil { @@ -88,15 +89,16 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR } // 3 - adopt the repository from disk - if err := adoptRepository(ctx, repo, opts.DefaultBranch); err != nil { + if err = adoptRepository(ctx, repo, opts.DefaultBranch); err != nil { return nil, fmt.Errorf("adoptRepository: %w", err) } - if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil { return nil, fmt.Errorf("checkDaemonExportOK: %w", err) } - if stdout, _, err := git.NewCommand("update-server-info"). + var stdout string + if stdout, _, err = git.NewCommand("update-server-info"). RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err) diff --git a/services/repository/adopt_test.go b/services/repository/adopt_test.go index 294185ea1f224..bece098f8daaa 100644 --- a/services/repository/adopt_test.go +++ b/services/repository/adopt_test.go @@ -4,16 +4,19 @@ package repository import ( + "context" "os" "path" "path/filepath" "testing" + "time" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" "github.com/stretchr/testify/assert" ) @@ -89,10 +92,31 @@ func TestListUnadoptedRepositories_ListOptions(t *testing.T) { func TestAdoptRepository(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) + + // a successful adopt assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, "user2", "repo1.git"), filepath.Join(setting.RepoRootPath, "user2", "test-adopt.git"))) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - _, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"}) + adoptedRepo, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"}) assert.NoError(t, err) repoTestAdopt := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "test-adopt"}) assert.Equal(t, "sha1", repoTestAdopt.ObjectFormatName) + + // just delete the adopted repo's db records + err = deleteFailedAdoptRepository(db.DefaultContext, adoptedRepo.ID) + assert.NoError(t, err) + + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"}) + + // a failed adopt + ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Microsecond) + defer cancel() + adoptedRepo, err = AdoptRepository(ctx, user2, user2, CreateRepoOptions{Name: "test-adopt"}) + assert.Error(t, err) + assert.Nil(t, adoptedRepo) + + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"}) + + exist, err := util.IsExist(repo_model.RepoPath("user2", "test-adopt")) + assert.NoError(t, err) + assert.True(t, exist) // the repository should be still in the disk } diff --git a/services/repository/create_test.go b/services/repository/create_test.go new file mode 100644 index 0000000000000..9322b57909237 --- /dev/null +++ b/services/repository/create_test.go @@ -0,0 +1,57 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repository + +import ( + "context" + "testing" + "time" + + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/util" + + "github.com/stretchr/testify/assert" +) + +func TestCreateRepositoryDirectly(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // a successful generate from template + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + createdRepo, err := CreateRepositoryDirectly(git.DefaultContext, user2, user2, CreateRepoOptions{ + Name: "created-repo", + }) + assert.NoError(t, err) + assert.NotNil(t, createdRepo) + + exist, err := util.IsExist(repo_model.RepoPath(user2.Name, createdRepo.Name)) + assert.NoError(t, err) + assert.True(t, exist) + + unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: createdRepo.Name}) + + err = DeleteRepositoryDirectly(db.DefaultContext, user2, createdRepo.ID) + assert.NoError(t, err) + + // a failed generate because a timeout + ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) + defer cancel() + createdRepo2, err := CreateRepositoryDirectly(ctx, user2, user2, CreateRepoOptions{ + Name: "created-repo", + }) + assert.Nil(t, createdRepo2) + assert.Error(t, err) + + // assert the cleanup is successful + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: createdRepo.Name}) + + exist, err = util.IsExist(repo_model.RepoPath(user2.Name, createdRepo.Name)) + assert.NoError(t, err) + assert.False(t, exist) +} diff --git a/services/repository/fork.go b/services/repository/fork.go index 5f9db8f3eaad2..f9c4247b98eda 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -120,6 +120,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } // last - clean up if something goes wrong + // WARNING: Don't override all later err with local variables defer func() { if err != nil { if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { @@ -133,20 +134,22 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork if opts.SingleBranch != "" { cloneCmd.AddArguments("--single-branch", "--branch").AddDynamicArguments(opts.SingleBranch) } - if stdout, _, err := cloneCmd.AddDynamicArguments(opts.BaseRepo.RepoPath(), repo.RepoPath()). + var stdout []byte + if stdout, _, err = cloneCmd.AddDynamicArguments(opts.BaseRepo.RepoPath(), repo.RepoPath()). RunStdBytes(ctx, &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 nil, fmt.Errorf("git clone: %w", err) } // 3 - Update the repository - if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil { return nil, fmt.Errorf("checkDaemonExportOK: %w", err) } - if stdout, _, err := git.NewCommand("update-server-info"). + var output string + if output, _, err = git.NewCommand("update-server-info"). RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { - log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, stdout, err) + log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, output, err) return nil, fmt.Errorf("git update-server-info: %w", err) } @@ -156,7 +159,8 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } // 5 - Sync the repository branches and tags - gitRepo, err := gitrepo.OpenRepository(ctx, repo) + var gitRepo *git.Repository + gitRepo, err = gitrepo.OpenRepository(ctx, repo) if err != nil { return nil, fmt.Errorf("OpenRepository: %w", err) } @@ -165,19 +169,21 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork if _, err = repo_module.SyncRepoBranchesWithRepo(ctx, repo, gitRepo, doer.ID); err != nil { return nil, fmt.Errorf("SyncRepoBranchesWithRepo: %w", err) } - if err := repo_module.SyncReleasesWithTags(ctx, repo, gitRepo); err != nil { + if err = repo_module.SyncReleasesWithTags(ctx, repo, gitRepo); err != nil { return nil, fmt.Errorf("Sync releases from git tags failed: %v", err) } // 6 - Update the repository // even if below operations failed, it could be ignored. And they will be retried - if err := repo_module.UpdateRepoSize(ctx, repo); err != nil { + if err = repo_module.UpdateRepoSize(ctx, repo); err != nil { log.Error("Failed to update size for repository: %v", err) + err = nil } - if err := repo_model.CopyLanguageStat(ctx, opts.BaseRepo, repo); err != nil { + if err = repo_model.CopyLanguageStat(ctx, opts.BaseRepo, repo); err != nil { log.Error("Copy language stat from oldRepo failed: %v", err) + err = nil } - if err := repo_model.CopyLicense(ctx, opts.BaseRepo, repo); err != nil { + if err = repo_model.CopyLicense(ctx, opts.BaseRepo, repo); err != nil { return nil, err } diff --git a/services/repository/fork_test.go b/services/repository/fork_test.go index 452798b25b499..c9a5451bc9619 100644 --- a/services/repository/fork_test.go +++ b/services/repository/fork_test.go @@ -4,13 +4,17 @@ package repository import ( + "context" "testing" + "time" + "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" "github.com/stretchr/testify/assert" ) @@ -46,3 +50,42 @@ func TestForkRepository(t *testing.T) { assert.Nil(t, fork2) assert.True(t, repo_model.IsErrReachLimitOfRepo(err)) } + +func TestForkRepositoryCleanup(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // a successful fork + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + + fork, err := ForkRepository(git.DefaultContext, user2, user2, ForkRepoOptions{ + BaseRepo: repo10, + Name: "test", + }) + assert.NoError(t, err) + assert.NotNil(t, fork) + + exist, err := util.IsExist(repo_model.RepoPath(user2.Name, "test")) + assert.NoError(t, err) + assert.True(t, exist) + + err = DeleteRepositoryDirectly(db.DefaultContext, user2, fork.ID) + assert.NoError(t, err) + + // a failed fork because a timeout + ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) + defer cancel() + fork2, err := ForkRepository(ctx, user2, user2, ForkRepoOptions{ + BaseRepo: repo10, + Name: "test", + }) + assert.Nil(t, fork2) + assert.Error(t, err) + + // assert the cleanup is successful + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "test", Name: "test"}) + + exist, err = util.IsExist(repo_model.RepoPath("test", "test")) + assert.NoError(t, err) + assert.False(t, exist) +} diff --git a/tests/integration/repo_test.go b/tests/integration/repo_test.go index a5728ffcbd6e2..e8139fb61fe06 100644 --- a/tests/integration/repo_test.go +++ b/tests/integration/repo_test.go @@ -4,6 +4,7 @@ package integration import ( + "context" "fmt" "net/http" "path" @@ -11,8 +12,15 @@ import ( "testing" "time" + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/util" + repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" "github.com/PuerkitoBio/goquery" @@ -442,3 +450,44 @@ func TestViewCommit(t *testing.T) { resp := MakeRequest(t, req, http.StatusNotFound) assert.True(t, test.IsNormalPageCompleted(resp.Body.String()), "non-existing commit should render 404 page") } + +func TestGenerateRepository(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // a successful generate from template + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + repo44 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 44}) + + generatedRepo, err := repo_service.GenerateRepository(git.DefaultContext, user2, user2, repo44, repo_service.GenerateRepoOptions{ + Name: "generated-from-template-44", + GitContent: true, + }) + assert.NoError(t, err) + assert.NotNil(t, generatedRepo) + + exist, err := util.IsExist(repo_model.RepoPath(user2.Name, generatedRepo.Name)) + assert.NoError(t, err) + assert.True(t, exist) + + unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: generatedRepo.Name}) + + err = repo_service.DeleteRepositoryDirectly(db.DefaultContext, user2, generatedRepo.ID) + assert.NoError(t, err) + + // a failed generate because a timeout + ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) + defer cancel() + generatedRepo2, err := repo_service.GenerateRepository(ctx, user2, user2, repo44, repo_service.GenerateRepoOptions{ + Name: "generated-from-template-44", + GitContent: true, + }) + assert.Nil(t, generatedRepo2) + assert.Error(t, err) + + // assert the cleanup is successful + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: generatedRepo.Name}) + + exist, err = util.IsExist(repo_model.RepoPath(user2.Name, generatedRepo.Name)) + assert.NoError(t, err) + assert.False(t, exist) +} From eb8884cc6b54edf912329a3e1cc335c47a9baaad Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 26 Mar 2025 21:51:28 -0700 Subject: [PATCH 06/14] Fix creating repository --- services/repository/create.go | 144 ++++++++++++++--------------- services/repository/create_test.go | 4 +- 2 files changed, 73 insertions(+), 75 deletions(-) diff --git a/services/repository/create.go b/services/repository/create.go index 4d12baa8cd5d1..78cd0abc7c17e 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -247,98 +247,96 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt needsUpdateStatus := opts.Status != repo_model.RepositoryReady - if err := db.WithTx(ctx, func(ctx context.Context) error { + // 1 - create the repository database operations first + err := db.WithTx(ctx, func(ctx context.Context) error { return CreateRepositoryInDB(ctx, doer, u, repo, false, false) - }); err != nil { + }) + if err != nil { return nil, err } - if err := db.WithTx(ctx, func(ctx context.Context) error { - // No need for init mirror. - if opts.IsMirror { - return nil - } - - isExist, err := gitrepo.IsRepositoryExist(ctx, repo) + // last - clean up if something goes wrong + // WARNING: Don't override all later err with local variables + defer func() { if err != nil { - log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) - return err - } - if isExist { - // repo already exists - We have two or three options. - // 1. We fail stating that the directory exists - // 2. We create the db repository to go with this data and adopt the git repo - // 3. We delete it and start afresh - // - // Previously Gitea would just delete and start afresh - this was naughty. - // So we will now fail and delegate to other functionality to adopt or delete - log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) - return repo_model.ErrRepoFilesAlreadyExist{ - Uname: u.Name, - Name: repo.Name, + if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } } } + }() - if err = initRepository(ctx, doer, repo, opts); err != nil { - if err2 := gitrepo.DeleteRepository(ctx, repo); err2 != nil { - log.Error("initRepository: %v", err) - return fmt.Errorf( - "delete repo directory %s/%s failed(2): %v", u.Name, repo.Name, err2) - } - return fmt.Errorf("initRepository: %w", err) - } + // No need for init mirror. + if opts.IsMirror { + return repo, nil + } - // Initialize Issue Labels if selected - if len(opts.IssueLabels) > 0 { - if err = repo_module.InitializeLabels(ctx, repo.ID, opts.IssueLabels, false); err != nil { - return fmt.Errorf("InitializeLabels: %w", err) - } + var isExist bool + isExist, err = gitrepo.IsRepositoryExist(ctx, repo) + if err != nil { + log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) + return nil, err + } + if isExist { + // repo already exists in disk - We have two or three options. + // 1. We fail stating that the directory exists + // 2. We create the db repository to go with this data and adopt the git repo + // 3. We delete it and start afresh + // + // Previously Gitea would just delete and start afresh - this was naughty. + // So we will now fail and delegate to other functionality to adopt or delete + log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) + return nil, repo_model.ErrRepoFilesAlreadyExist{ + Uname: u.Name, + Name: repo.Name, } + } - if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { - return fmt.Errorf("checkDaemonExportOK: %w", err) - } + if err = initRepository(ctx, doer, repo, opts); err != nil { + return nil, fmt.Errorf("initRepository: %w", err) + } - if stdout, _, err := git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { - log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) - return fmt.Errorf("CreateRepository(git update-server-info): %w", err) + // Initialize Issue Labels if selected + if len(opts.IssueLabels) > 0 { + if err = repo_module.InitializeLabels(ctx, repo.ID, opts.IssueLabels, false); err != nil { + return nil, fmt.Errorf("InitializeLabels: %w", err) } + } - if needsUpdateStatus { - repo.Status = repo_model.RepositoryReady - if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { - return fmt.Errorf("UpdateRepositoryCols: %w", err) - } + if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + return nil, fmt.Errorf("checkDaemonExportOK: %w", err) + } + + var stdout string + if stdout, _, err = git.NewCommand("update-server-info"). + RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { + log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) + return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err) + } + + if needsUpdateStatus { + repo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) } + } - // update licenses - var licenses []string - if len(opts.License) > 0 { - licenses = append(licenses, opts.License) + // update licenses + var licenses []string + if len(opts.License) > 0 { + licenses = append(licenses, opts.License) - stdout, _, err := git.NewCommand("rev-parse", "HEAD").RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}) - if err != nil { - log.Error("CreateRepository(git rev-parse HEAD) in %v: Stdout: %s\nError: %v", repo, stdout, err) - return fmt.Errorf("CreateRepository(git rev-parse HEAD): %w", err) - } - if err := repo_model.UpdateRepoLicenses(ctx, repo, stdout, licenses); err != nil { - return err - } + stdout, _, err = git.NewCommand("rev-parse", "HEAD").RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}) + if err != nil { + log.Error("CreateRepository(git rev-parse HEAD) in %v: Stdout: %s\nError: %v", repo, stdout, err) + return nil, fmt.Errorf("CreateRepository(git rev-parse HEAD): %w", err) } - return nil - }); err != nil { - if repo != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) - // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { - log.Error("CreateRepositoryNotice: %v", err) - } - } + if err = repo_model.UpdateRepoLicenses(ctx, repo, stdout, licenses); err != nil { + return nil, err } - - return nil, err } return repo, nil diff --git a/services/repository/create_test.go b/services/repository/create_test.go index 9322b57909237..143c27ca05361 100644 --- a/services/repository/create_test.go +++ b/services/repository/create_test.go @@ -21,7 +21,7 @@ import ( func TestCreateRepositoryDirectly(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - // a successful generate from template + // a successful creating repository user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) createdRepo, err := CreateRepositoryDirectly(git.DefaultContext, user2, user2, CreateRepoOptions{ @@ -39,7 +39,7 @@ func TestCreateRepositoryDirectly(t *testing.T) { err = DeleteRepositoryDirectly(db.DefaultContext, user2, createdRepo.ID) assert.NoError(t, err) - // a failed generate because a timeout + // a failed creating because a timeout ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) defer cancel() createdRepo2, err := CreateRepositoryDirectly(ctx, user2, user2, CreateRepoOptions{ From a494a699d6de2f588b611bbdbfacc13d41d07add Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 29 Mar 2025 17:12:11 -0700 Subject: [PATCH 07/14] merge some functions --- services/repository/adopt.go | 12 ++------ services/repository/create.go | 49 +++++++++++++++++++-------------- services/repository/fork.go | 19 ++----------- services/repository/migrate.go | 10 ++----- services/repository/template.go | 24 ++-------------- 5 files changed, 39 insertions(+), 75 deletions(-) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index f19110acb2b28..464fcf722b7ef 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -16,7 +16,6 @@ import ( repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/optional" @@ -93,15 +92,8 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR return nil, fmt.Errorf("adoptRepository: %w", err) } - if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil { - return nil, fmt.Errorf("checkDaemonExportOK: %w", err) - } - - var stdout string - if stdout, _, err = git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { - log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) - return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err) + if err = updateGitRepoAfterCreate(ctx, repo); err != nil { + return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } // 4 - update repository status diff --git a/services/repository/create.go b/services/repository/create.go index 78cd0abc7c17e..206fbe8f2556d 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -257,17 +257,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt // last - clean up if something goes wrong // WARNING: Don't override all later err with local variables - defer func() { - if err != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) - // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { - log.Error("CreateRepositoryNotice: %v", err) - } - } - } - }() + defer cleanupRepository(err, doer, repo.ID) // No need for init mirror. if opts.IsMirror { @@ -306,15 +296,8 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt } } - if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil { - return nil, fmt.Errorf("checkDaemonExportOK: %w", err) - } - - var stdout string - if stdout, _, err = git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { - log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) - return nil, fmt.Errorf("CreateRepository(git update-server-info): %w", err) + if err = updateGitRepoAfterCreate(ctx, repo); err != nil { + return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } if needsUpdateStatus { @@ -329,6 +312,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt if len(opts.License) > 0 { licenses = append(licenses, opts.License) + var stdout string stdout, _, err = git.NewCommand("rev-parse", "HEAD").RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}) if err != nil { log.Error("CreateRepository(git rev-parse HEAD) in %v: Stdout: %s\nError: %v", repo, stdout, err) @@ -488,3 +472,28 @@ func CreateRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r return nil } + +func cleanupRepository(err error, doer *user_model.User, repoID int64) { + if err != nil { + if errDelete := DeleteRepositoryDirectly(db.DefaultContext, doer, repoID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) + } + } + } +} + +func updateGitRepoAfterCreate(ctx context.Context, repo *repo_model.Repository) error { + if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { + return fmt.Errorf("checkDaemonExportOK: %w", err) + } + + if stdout, _, err := git.NewCommand("update-server-info"). + RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { + log.Error("CreateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) + return fmt.Errorf("CreateRepository(git update-server-info): %w", err) + } + return nil +} diff --git a/services/repository/fork.go b/services/repository/fork.go index f9c4247b98eda..3d6f507921d1f 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -121,13 +121,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork // last - clean up if something goes wrong // WARNING: Don't override all later err with local variables - defer func() { - if err != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, repo.ID); errDelete != nil { - log.Error("Failed to remove fork repo: %v", errDelete) - } - } - }() + defer cleanupRepository(err, doer, repo.ID) // 2 - Clone the repository cloneCmd := git.NewCommand("clone", "--bare") @@ -142,15 +136,8 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } // 3 - Update the repository - if err = repo_module.CheckDaemonExportOK(ctx, repo); err != nil { - return nil, fmt.Errorf("checkDaemonExportOK: %w", err) - } - - var output string - if output, _, err = git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: repo.RepoPath()}); err != nil { - log.Error("Fork Repository (git update-server-info) failed for %v:\nStdout: %s\nError: %v", repo, output, err) - return nil, fmt.Errorf("git update-server-info: %w", err) + if err = updateGitRepoAfterCreate(ctx, repo); err != nil { + return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } // 4 - Create hooks diff --git a/services/repository/migrate.go b/services/repository/migrate.go index 9a5c6ffb0fd7a..dc09b5eab94ce 100644 --- a/services/repository/migrate.go +++ b/services/repository/migrate.go @@ -118,14 +118,8 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User, repo.Owner = u } - if err := repo_module.CheckDaemonExportOK(ctx, repo); err != nil { - return repo, fmt.Errorf("checkDaemonExportOK: %w", err) - } - - if stdout, _, err := git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: repoPath}); err != nil { - log.Error("MigrateRepositoryGitData(git update-server-info) in %v: Stdout: %s\nError: %v", repo, stdout, err) - return repo, fmt.Errorf("error in MigrateRepositoryGitData(git update-server-info): %w", err) + if err := updateGitRepoAfterCreate(ctx, repo); err != nil { + return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } gitRepo, err := git.OpenRepository(ctx, repoPath) diff --git a/services/repository/template.go b/services/repository/template.go index bc44abfd8473a..64a89439b798c 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -12,9 +12,7 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" - system_model "code.gitea.io/gitea/models/system" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" repo_module "code.gitea.io/gitea/modules/repository" @@ -114,31 +112,15 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } // last - clean up the repository if something goes wrong - defer func() { - if err != nil { - if errDelete := DeleteRepositoryDirectly(ctx, doer, generateRepo.ID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) - // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when generate repository: %v", errDelete); err != nil { - log.Error("CreateRepositoryNotice: %v", err) - } - } - } - }() + defer cleanupRepository(err, doer, generateRepo.ID) // 3 - Generate the git repository in storage if err = repo_module.CheckInitRepository(ctx, generateRepo); err != nil { return nil, err } - if err = repo_module.CheckDaemonExportOK(ctx, generateRepo); err != nil { - return nil, fmt.Errorf("checkDaemonExportOK: %w", err) - } - - if stdout, _, err := git.NewCommand("update-server-info"). - RunStdString(ctx, &git.RunOpts{Dir: generateRepo.RepoPath()}); err != nil { - log.Error("GenerateRepository(git update-server-info) in %v: Stdout: %s\nError: %v", generateRepo, stdout, err) - return nil, fmt.Errorf("error in GenerateRepository(git update-server-info): %w", err) + if err = updateGitRepoAfterCreate(ctx, generateRepo); err != nil { + return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } // Git Content From 0a54603290cdd4476a9dc62d1f1ab691a9646fce Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 29 Mar 2025 19:43:13 -0700 Subject: [PATCH 08/14] Fix bug --- services/repository/create.go | 20 +++++++++++--------- services/repository/fork.go | 6 +++++- services/repository/template.go | 6 +++++- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/services/repository/create.go b/services/repository/create.go index 6b9d3673b831f..2c87ca8ad4634 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -257,7 +257,11 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt // last - clean up if something goes wrong // WARNING: Don't override all later err with local variables - defer cleanupRepository(err, doer, repo.ID) + defer func() { + if err != nil { + cleanupRepository(doer, repo.ID) + } + }() // No need for init mirror. if opts.IsMirror { @@ -474,14 +478,12 @@ func CreateRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r return nil } -func cleanupRepository(err error, doer *user_model.User, repoID int64) { - if err != nil { - if errDelete := DeleteRepositoryDirectly(db.DefaultContext, doer, repoID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) - // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { - log.Error("CreateRepositoryNotice: %v", err) - } +func cleanupRepository(doer *user_model.User, repoID int64) { + if errDelete := DeleteRepositoryDirectly(db.DefaultContext, doer, repoID); errDelete != nil { + log.Error("Rollback deleteRepository: %v", errDelete) + // add system notice + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { + log.Error("CreateRepositoryNotice: %v", err) } } } diff --git a/services/repository/fork.go b/services/repository/fork.go index 3d6f507921d1f..e995b22305b4f 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -121,7 +121,11 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork // last - clean up if something goes wrong // WARNING: Don't override all later err with local variables - defer cleanupRepository(err, doer, repo.ID) + defer func() { + if err != nil { + cleanupRepository(doer, repo.ID) + } + }() // 2 - Clone the repository cloneCmd := git.NewCommand("clone", "--bare") diff --git a/services/repository/template.go b/services/repository/template.go index 64a89439b798c..2dfc88ab9cb2f 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -112,7 +112,11 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } // last - clean up the repository if something goes wrong - defer cleanupRepository(err, doer, generateRepo.ID) + defer func() { + if err != nil { + cleanupRepository(doer, generateRepo.ID) + } + }() // 3 - Generate the git repository in storage if err = repo_module.CheckInitRepository(ctx, generateRepo); err != nil { From 232217253dfbba2f7a3faa7293b514f83c79f547 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 29 Mar 2025 23:42:00 -0700 Subject: [PATCH 09/14] some improvements --- services/repository/adopt.go | 7 ++++--- services/repository/adopt_test.go | 2 +- services/repository/create.go | 1 + services/repository/fork.go | 1 + services/repository/template.go | 1 + 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 464fcf722b7ef..23a9d66eabd12 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -27,8 +27,8 @@ import ( "github.com/gobwas/glob" ) -func deleteFailedAdoptRepository(ctx context.Context, repoID int64) error { - return db.WithTx(ctx, func(ctx context.Context) error { +func deleteFailedAdoptRepository(repoID int64) error { + return db.WithTx(db.DefaultContext, func(ctx context.Context) error { if err := deleteDBRepository(ctx, repoID); err != nil { return fmt.Errorf("deleteDBRepository: %w", err) } @@ -75,7 +75,8 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR // WARNING: Don't override all later err with local variables defer func() { if err != nil { - if errDel := deleteFailedAdoptRepository(ctx, repo.ID); errDel != nil { + // we can not use the ctx because it maybe canceled or timeout + if errDel := deleteFailedAdoptRepository(repo.ID); errDel != nil { log.Error("Failed to delete repository %s that could not be adopted: %v", repo.FullName(), errDel) } } diff --git a/services/repository/adopt_test.go b/services/repository/adopt_test.go index bece098f8daaa..79e70b4f156ed 100644 --- a/services/repository/adopt_test.go +++ b/services/repository/adopt_test.go @@ -102,7 +102,7 @@ func TestAdoptRepository(t *testing.T) { assert.Equal(t, "sha1", repoTestAdopt.ObjectFormatName) // just delete the adopted repo's db records - err = deleteFailedAdoptRepository(db.DefaultContext, adoptedRepo.ID) + err = deleteFailedAdoptRepository(adoptedRepo.ID) assert.NoError(t, err) unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"}) diff --git a/services/repository/create.go b/services/repository/create.go index 2c87ca8ad4634..463f972d347dc 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -259,6 +259,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt // WARNING: Don't override all later err with local variables defer func() { if err != nil { + // we can not use the ctx because it maybe canceled or timeout cleanupRepository(doer, repo.ID) } }() diff --git a/services/repository/fork.go b/services/repository/fork.go index e995b22305b4f..0303fc10c815c 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -123,6 +123,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork // WARNING: Don't override all later err with local variables defer func() { if err != nil { + // we can not use the ctx because it maybe canceled or timeout cleanupRepository(doer, repo.ID) } }() diff --git a/services/repository/template.go b/services/repository/template.go index 2dfc88ab9cb2f..28617b1937318 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -114,6 +114,7 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ // last - clean up the repository if something goes wrong defer func() { if err != nil { + // we can not use the ctx because it maybe canceled or timeout cleanupRepository(doer, generateRepo.ID) } }() From 0142df4009be267b2ca4f1b2d7270d73f933800a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Apr 2025 17:34:55 -0700 Subject: [PATCH 10/14] Refactor to make test easier --- services/repository/adopt.go | 2 +- services/repository/adopt_test.go | 15 +++++++------ services/repository/create.go | 35 ++++++------------------------ services/repository/create_test.go | 12 +++++----- services/repository/fork.go | 18 ++++++++++++++- services/repository/fork_test.go | 12 +++++----- services/repository/template.go | 32 ++++++++++++++------------- tests/integration/repo_test.go | 12 +++++----- 8 files changed, 69 insertions(+), 69 deletions(-) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 23a9d66eabd12..c0ddd49f2a929 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -65,7 +65,7 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR // 1 - create the repository database operations first err := db.WithTx(ctx, func(ctx context.Context) error { - return CreateRepositoryInDB(ctx, doer, u, repo, true, false) + return createRepositoryInDB(ctx, doer, u, repo, false) }) if err != nil { return nil, err diff --git a/services/repository/adopt_test.go b/services/repository/adopt_test.go index 79e70b4f156ed..84287479044d9 100644 --- a/services/repository/adopt_test.go +++ b/services/repository/adopt_test.go @@ -4,12 +4,10 @@ package repository import ( - "context" "os" "path" "path/filepath" "testing" - "time" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" @@ -94,7 +92,8 @@ func TestAdoptRepository(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) // a successful adopt - assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, "user2", "repo1.git"), filepath.Join(setting.RepoRootPath, "user2", "test-adopt.git"))) + destDir := filepath.Join(setting.RepoRootPath, "user2", "test-adopt.git") + assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, "user2", "repo1.git"), destDir)) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) adoptedRepo, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"}) assert.NoError(t, err) @@ -107,10 +106,12 @@ func TestAdoptRepository(t *testing.T) { unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"}) - // a failed adopt - ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Microsecond) - defer cancel() - adoptedRepo, err = AdoptRepository(ctx, user2, user2, CreateRepoOptions{Name: "test-adopt"}) + // a failed adopt because some mock data + // remove the hooks directory and create a file so that we cannot create the hooks successfully + _ = os.RemoveAll(filepath.Join(destDir, "hooks", "update.d")) + assert.NoError(t, os.WriteFile(filepath.Join(destDir, "hooks", "update.d"), []byte("tests"), os.ModePerm)) + + adoptedRepo, err = AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"}) assert.Error(t, err) assert.Nil(t, adoptedRepo) diff --git a/services/repository/create.go b/services/repository/create.go index 463f972d347dc..61b06345e4644 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -249,7 +249,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt // 1 - create the repository database operations first err := db.WithTx(ctx, func(ctx context.Context) error { - return CreateRepositoryInDB(ctx, doer, u, repo, false, false) + return createRepositoryInDB(ctx, doer, u, repo, false) }) if err != nil { return nil, err @@ -284,10 +284,12 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt // Previously Gitea would just delete and start afresh - this was naughty. // So we will now fail and delegate to other functionality to adopt or delete log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) - return nil, repo_model.ErrRepoFilesAlreadyExist{ - Uname: u.Name, + // we need err in defer to cleanupRepository + err = repo_model.ErrRepoFilesAlreadyExist{ + Uname: repo.OwnerName, Name: repo.Name, } + return nil, err } if err = initRepository(ctx, doer, repo, opts); err != nil { @@ -331,8 +333,8 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt return repo, nil } -// CreateRepositoryInDB creates a repository for the user/organization. -func CreateRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt, isFork bool) (err error) { +// createRepositoryInDB creates a repository for the user/organization. +func createRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, isFork bool) (err error) { if err = repo_model.IsUsableRepoName(repo.Name); err != nil { return err } @@ -347,29 +349,6 @@ func CreateRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r } } - isExist, err := gitrepo.IsRepositoryExist(ctx, repo) - if err != nil { - log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) - return err - } - if overwriteOrAdopt != isExist { - if overwriteOrAdopt { - // repo should exist but doesn't - We have two or three options. - log.Error("Files do not exist in %s and we are not going to adopt or delete.", repo.FullName()) - return repo_model.ErrRepoNotExist{ - OwnerName: u.Name, - Name: repo.Name, - } - } - - // repo already exists - We have two or three options. - log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) - return repo_model.ErrRepoFilesAlreadyExist{ - Uname: u.Name, - Name: repo.Name, - } - } - if err = db.Insert(ctx, repo); err != nil { return err } diff --git a/services/repository/create_test.go b/services/repository/create_test.go index 143c27ca05361..9ecf404e8bac5 100644 --- a/services/repository/create_test.go +++ b/services/repository/create_test.go @@ -4,9 +4,8 @@ package repository import ( - "context" + "os" "testing" - "time" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" @@ -39,10 +38,11 @@ func TestCreateRepositoryDirectly(t *testing.T) { err = DeleteRepositoryDirectly(db.DefaultContext, user2, createdRepo.ID) assert.NoError(t, err) - // a failed creating because a timeout - ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) - defer cancel() - createdRepo2, err := CreateRepositoryDirectly(ctx, user2, user2, CreateRepoOptions{ + // a failed creating because some mock data + // create the repository directory so that the creation will fail after database record created. + assert.NoError(t, os.MkdirAll(repo_model.RepoPath(user2.Name, createdRepo.Name), os.ModePerm)) + + createdRepo2, err := CreateRepositoryDirectly(db.DefaultContext, user2, user2, CreateRepoOptions{ Name: "created-repo", }) assert.Nil(t, createdRepo2) diff --git a/services/repository/fork.go b/services/repository/fork.go index 0303fc10c815c..af36f43608438 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -105,7 +105,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork // 1 - Create the repository in the database err = db.WithTx(ctx, func(ctx context.Context) error { - if err = CreateRepositoryInDB(ctx, doer, owner, repo, false, true); err != nil { + if err = createRepositoryInDB(ctx, doer, owner, repo, true); err != nil { return err } if err = repo_model.IncrementRepoForkNum(ctx, opts.BaseRepo.ID); err != nil { @@ -128,6 +128,22 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } }() + var isExist bool + isExist, err = gitrepo.IsRepositoryExist(ctx, repo) + if err != nil { + log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) + return nil, err + } + if isExist { + log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) + // we need err in defer to cleanupRepository + err = repo_model.ErrRepoFilesAlreadyExist{ + Uname: repo.OwnerName, + Name: repo.Name, + } + return nil, err + } + // 2 - Clone the repository cloneCmd := git.NewCommand("clone", "--bare") if opts.SingleBranch != "" { diff --git a/services/repository/fork_test.go b/services/repository/fork_test.go index c9a5451bc9619..b741f95fb231e 100644 --- a/services/repository/fork_test.go +++ b/services/repository/fork_test.go @@ -4,9 +4,8 @@ package repository import ( - "context" + "os" "testing" - "time" "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" @@ -72,10 +71,11 @@ func TestForkRepositoryCleanup(t *testing.T) { err = DeleteRepositoryDirectly(db.DefaultContext, user2, fork.ID) assert.NoError(t, err) - // a failed fork because a timeout - ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) - defer cancel() - fork2, err := ForkRepository(ctx, user2, user2, ForkRepoOptions{ + // a failed creating because some mock data + // create the repository directory so that the creation will fail after database record created. + assert.NoError(t, os.MkdirAll(repo_model.RepoPath(user2.Name, "test"), os.ModePerm)) + + fork2, err := ForkRepository(db.DefaultContext, user2, user2, ForkRepoOptions{ BaseRepo: repo10, Name: "test", }) diff --git a/services/repository/template.go b/services/repository/template.go index 28617b1937318..ca1f91b983e67 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -91,22 +91,9 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ Status: repo_model.RepositoryBeingMigrated, } - // 1 - check whether the repository with the same storage exists - isExist, err := gitrepo.IsRepositoryExist(ctx, generateRepo) - if err != nil { - log.Error("Unable to check if %s exists. Error: %v", generateRepo.FullName(), err) - return nil, err - } - if isExist { - return nil, repo_model.ErrRepoFilesAlreadyExist{ - Uname: generateRepo.OwnerName, - Name: generateRepo.Name, - } - } - - // 2 - Create the repository in the database + // 1 - Create the repository in the database if err := db.WithTx(ctx, func(ctx context.Context) error { - return CreateRepositoryInDB(ctx, doer, owner, generateRepo, false, false) + return createRepositoryInDB(ctx, doer, owner, generateRepo, false) }); err != nil { return nil, err } @@ -119,6 +106,21 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } }() + // 2 - check whether the repository with the same storage exists + isExist, err := gitrepo.IsRepositoryExist(ctx, generateRepo) + if err != nil { + log.Error("Unable to check if %s exists. Error: %v", generateRepo.FullName(), err) + return nil, err + } + if isExist { + // we need err in defer to cleanupRepository + err = repo_model.ErrRepoFilesAlreadyExist{ + Uname: generateRepo.OwnerName, + Name: generateRepo.Name, + } + return nil, err + } + // 3 - Generate the git repository in storage if err = repo_module.CheckInitRepository(ctx, generateRepo); err != nil { return nil, err diff --git a/tests/integration/repo_test.go b/tests/integration/repo_test.go index cc458503daa1c..3da171f48baed 100644 --- a/tests/integration/repo_test.go +++ b/tests/integration/repo_test.go @@ -4,9 +4,9 @@ package integration import ( - "context" "fmt" "net/http" + "os" "path" "strconv" "strings" @@ -502,6 +502,7 @@ func testViewCommit(t *testing.T) { assert.True(t, test.IsNormalPageCompleted(resp.Body.String()), "non-existing commit should render 404 page") } +// TestGenerateRepository the test cannot succeed when moved as a unit test func TestGenerateRepository(t *testing.T) { defer tests.PrepareTestEnv(t)() @@ -525,10 +526,11 @@ func TestGenerateRepository(t *testing.T) { err = repo_service.DeleteRepositoryDirectly(db.DefaultContext, user2, generatedRepo.ID) assert.NoError(t, err) - // a failed generate because a timeout - ctx, cancel := context.WithTimeout(db.DefaultContext, 1*time.Millisecond) - defer cancel() - generatedRepo2, err := repo_service.GenerateRepository(ctx, user2, user2, repo44, repo_service.GenerateRepoOptions{ + // a failed creating because some mock data + // create the repository directory so that the creation will fail after database record created. + assert.NoError(t, os.MkdirAll(repo_model.RepoPath(user2.Name, "generated-from-template-44"), os.ModePerm)) + + generatedRepo2, err := repo_service.GenerateRepository(db.DefaultContext, user2, user2, repo44, repo_service.GenerateRepoOptions{ Name: "generated-from-template-44", GitContent: true, }) From b61f41718e466a66372657d0d7c970efc1e96a57 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Apr 2025 18:50:46 -0700 Subject: [PATCH 11/14] Remove duplicated checks --- modules/repository/init.go | 27 --------------------------- services/repository/create.go | 16 ++++++---------- services/repository/fork.go | 2 +- services/repository/template.go | 11 +++++++---- 4 files changed, 14 insertions(+), 42 deletions(-) diff --git a/modules/repository/init.go b/modules/repository/init.go index ace21254ba0c4..e6331966baf34 100644 --- a/modules/repository/init.go +++ b/modules/repository/init.go @@ -11,11 +11,7 @@ import ( "strings" issues_model "code.gitea.io/gitea/models/issues" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/label" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/options" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" @@ -121,29 +117,6 @@ func LoadRepoConfig() error { return nil } -func CheckInitRepository(ctx context.Context, repo *repo_model.Repository) (err error) { - // Somehow the directory could exist. - isExist, err := gitrepo.IsRepositoryExist(ctx, repo) - if err != nil { - log.Error("Unable to check if %s exists. Error: %v", repo.FullName(), err) - return err - } - if isExist { - return repo_model.ErrRepoFilesAlreadyExist{ - Uname: repo.OwnerName, - Name: repo.Name, - } - } - - // Init git bare new repository. - if err = git.InitRepository(ctx, repo.RepoPath(), true, repo.ObjectFormatName); err != nil { - return fmt.Errorf("git.InitRepository: %w", err) - } else if err = gitrepo.CreateDelegateHooks(ctx, repo); err != nil { - return fmt.Errorf("createDelegateHooks: %w", err) - } - return nil -} - // InitializeLabels adds a label set to a repository using a template func InitializeLabels(ctx context.Context, id int64, labelTemplate string, isOrg bool) error { list, err := LoadTemplateLabelsByDisplayName(labelTemplate) diff --git a/services/repository/create.go b/services/repository/create.go index 61b06345e4644..43771a14d920c 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -141,8 +141,11 @@ func prepareRepoCommit(ctx context.Context, repo *repo_model.Repository, tmpDir // InitRepository initializes README and .gitignore if needed. func initRepository(ctx context.Context, u *user_model.User, repo *repo_model.Repository, opts CreateRepoOptions) (err error) { - if err = repo_module.CheckInitRepository(ctx, repo); err != nil { - return err + // Init git bare new repository. + if err = git.InitRepository(ctx, repo.RepoPath(), true, repo.ObjectFormatName); err != nil { + return fmt.Errorf("git.InitRepository: %w", err) + } else if err = gitrepo.CreateDelegateHooks(ctx, repo); err != nil { + return fmt.Errorf("createDelegateHooks: %w", err) } // Initialize repository according to user's choice. @@ -276,15 +279,8 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt return nil, err } if isExist { - // repo already exists in disk - We have two or three options. - // 1. We fail stating that the directory exists - // 2. We create the db repository to go with this data and adopt the git repo - // 3. We delete it and start afresh - // - // Previously Gitea would just delete and start afresh - this was naughty. - // So we will now fail and delegate to other functionality to adopt or delete log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) - // we need err in defer to cleanupRepository + // Don't return directly, we need err in defer to cleanupRepository err = repo_model.ErrRepoFilesAlreadyExist{ Uname: repo.OwnerName, Name: repo.Name, diff --git a/services/repository/fork.go b/services/repository/fork.go index af36f43608438..2ffcafea6c6df 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -136,7 +136,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } if isExist { log.Error("Files already exist in %s and we are not going to adopt or delete.", repo.FullName()) - // we need err in defer to cleanupRepository + // Don't return directly, we need err in defer to cleanupRepository err = repo_model.ErrRepoFilesAlreadyExist{ Uname: repo.OwnerName, Name: repo.Name, diff --git a/services/repository/template.go b/services/repository/template.go index ca1f91b983e67..612de47e6972c 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -13,9 +13,9 @@ import ( issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" - repo_module "code.gitea.io/gitea/modules/repository" notify_service "code.gitea.io/gitea/services/notify" ) @@ -113,7 +113,7 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ return nil, err } if isExist { - // we need err in defer to cleanupRepository + // Don't return directly, we need err in defer to cleanupRepository err = repo_model.ErrRepoFilesAlreadyExist{ Uname: generateRepo.OwnerName, Name: generateRepo.Name, @@ -122,8 +122,11 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } // 3 - Generate the git repository in storage - if err = repo_module.CheckInitRepository(ctx, generateRepo); err != nil { - return nil, err + // Init git bare new repository. + if err = git.InitRepository(ctx, generateRepo.RepoPath(), true, generateRepo.ObjectFormatName); err != nil { + return nil, fmt.Errorf("git.InitRepository: %w", err) + } else if err = gitrepo.CreateDelegateHooks(ctx, generateRepo); err != nil { + return nil, fmt.Errorf("createDelegateHooks: %w", err) } if err = updateGitRepoAfterCreate(ctx, generateRepo); err != nil { From 8d85006465797b4ae900ae82b438af79856ff78b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Apr 2025 19:36:20 -0700 Subject: [PATCH 12/14] Fix test --- services/repository/adopt_test.go | 4 ++-- services/repository/create.go | 4 ++-- services/repository/fork_test.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/services/repository/adopt_test.go b/services/repository/adopt_test.go index 84287479044d9..733d1a7d09286 100644 --- a/services/repository/adopt_test.go +++ b/services/repository/adopt_test.go @@ -115,9 +115,9 @@ func TestAdoptRepository(t *testing.T) { assert.Error(t, err) assert.Nil(t, adoptedRepo) - unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"}) + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: "test-adopt"}) - exist, err := util.IsExist(repo_model.RepoPath("user2", "test-adopt")) + exist, err := util.IsExist(repo_model.RepoPath(user2.Name, "test-adopt")) assert.NoError(t, err) assert.True(t, exist) // the repository should be still in the disk } diff --git a/services/repository/create.go b/services/repository/create.go index 43771a14d920c..fbd6180f2010d 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -456,9 +456,9 @@ func createRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r func cleanupRepository(doer *user_model.User, repoID int64) { if errDelete := DeleteRepositoryDirectly(db.DefaultContext, doer, repoID); errDelete != nil { - log.Error("Rollback deleteRepository: %v", errDelete) + log.Error("cleanupRepository failed: %v", errDelete) // add system notice - if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when create repository: %v", errDelete); err != nil { + if err := system_model.CreateRepositoryNotice("DeleteRepositoryDirectly failed when cleanup repository: %v", errDelete); err != nil { log.Error("CreateRepositoryNotice: %v", err) } } diff --git a/services/repository/fork_test.go b/services/repository/fork_test.go index b741f95fb231e..9edc0aa39f934 100644 --- a/services/repository/fork_test.go +++ b/services/repository/fork_test.go @@ -83,9 +83,9 @@ func TestForkRepositoryCleanup(t *testing.T) { assert.Error(t, err) // assert the cleanup is successful - unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "test", Name: "test"}) + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: "test"}) - exist, err = util.IsExist(repo_model.RepoPath("test", "test")) + exist, err = util.IsExist(repo_model.RepoPath(user2.Name, "test")) assert.NoError(t, err) assert.False(t, exist) } From 5feb0dda03a9d661f3b1072a0691dc8f17b3cb9f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Apr 2025 21:06:13 -0700 Subject: [PATCH 13/14] Update comments --- services/repository/adopt.go | 3 ++- services/repository/create.go | 22 +++++++++++++--------- services/repository/fork.go | 13 +++++++------ services/repository/template.go | 7 ++++--- 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/services/repository/adopt.go b/services/repository/adopt.go index c0ddd49f2a929..6953970e286d8 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -88,11 +88,12 @@ func AdoptRepository(ctx context.Context, doer, u *user_model.User, opts CreateR return nil, fmt.Errorf("getRepositoryByID: %w", err) } - // 3 - adopt the repository from disk + // 2 - adopt the repository from disk if err = adoptRepository(ctx, repo, opts.DefaultBranch); err != nil { return nil, fmt.Errorf("adoptRepository: %w", err) } + // 3 - Update the git repository if err = updateGitRepoAfterCreate(ctx, repo); err != nil { return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } diff --git a/services/repository/create.go b/services/repository/create.go index fbd6180f2010d..22dfce91f007a 100644 --- a/services/repository/create.go +++ b/services/repository/create.go @@ -272,6 +272,7 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt return repo, nil } + // 2 - check whether the repository with the same storage exists var isExist bool isExist, err = gitrepo.IsRepositoryExist(ctx, repo) if err != nil { @@ -288,29 +289,24 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt return nil, err } + // 3 - init git repository in storage if err = initRepository(ctx, doer, repo, opts); err != nil { return nil, fmt.Errorf("initRepository: %w", err) } - // Initialize Issue Labels if selected + // 4 - Initialize Issue Labels if selected if len(opts.IssueLabels) > 0 { if err = repo_module.InitializeLabels(ctx, repo.ID, opts.IssueLabels, false); err != nil { return nil, fmt.Errorf("InitializeLabels: %w", err) } } + // 5 - Update the git repository if err = updateGitRepoAfterCreate(ctx, repo); err != nil { return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } - if needsUpdateStatus { - repo.Status = repo_model.RepositoryReady - if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { - return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) - } - } - - // update licenses + // 6 - update licenses var licenses []string if len(opts.License) > 0 { licenses = append(licenses, opts.License) @@ -326,6 +322,14 @@ func CreateRepositoryDirectly(ctx context.Context, doer, u *user_model.User, opt } } + // 7 - update repository status to be ready + if needsUpdateStatus { + repo.Status = repo_model.RepositoryReady + if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { + return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) + } + } + return repo, nil } diff --git a/services/repository/fork.go b/services/repository/fork.go index 2ffcafea6c6df..daf69135105a7 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -128,6 +128,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } }() + // 2 - check whether the repository with the same storage exists var isExist bool isExist, err = gitrepo.IsRepositoryExist(ctx, repo) if err != nil { @@ -144,7 +145,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, err } - // 2 - Clone the repository + // 3 - Clone the repository cloneCmd := git.NewCommand("clone", "--bare") if opts.SingleBranch != "" { cloneCmd.AddArguments("--single-branch", "--branch").AddDynamicArguments(opts.SingleBranch) @@ -156,17 +157,17 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, fmt.Errorf("git clone: %w", err) } - // 3 - Update the repository + // 4 - Update the git repository if err = updateGitRepoAfterCreate(ctx, repo); err != nil { return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } - // 4 - Create hooks + // 5 - Create hooks if err = gitrepo.CreateDelegateHooks(ctx, repo); err != nil { return nil, fmt.Errorf("createDelegateHooks: %w", err) } - // 5 - Sync the repository branches and tags + // 6 - Sync the repository branches and tags var gitRepo *git.Repository gitRepo, err = gitrepo.OpenRepository(ctx, repo) if err != nil { @@ -181,7 +182,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, fmt.Errorf("Sync releases from git tags failed: %v", err) } - // 6 - Update the repository + // 7 - Update the repository // 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) @@ -195,7 +196,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork return nil, err } - // 7 - update repository status to be ready + // 8 - update repository status to be ready repo.Status = repo_model.RepositoryReady if err = repo_model.UpdateRepositoryCols(ctx, repo, "status"); err != nil { return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) diff --git a/services/repository/template.go b/services/repository/template.go index 612de47e6972c..b3f34e485892d 100644 --- a/services/repository/template.go +++ b/services/repository/template.go @@ -121,18 +121,19 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ return nil, err } - // 3 - Generate the git repository in storage - // Init git bare new repository. + // 3 -Init git bare new repository. if err = git.InitRepository(ctx, generateRepo.RepoPath(), true, generateRepo.ObjectFormatName); err != nil { return nil, fmt.Errorf("git.InitRepository: %w", err) } else if err = gitrepo.CreateDelegateHooks(ctx, generateRepo); err != nil { return nil, fmt.Errorf("createDelegateHooks: %w", err) } + // 4 - Update the git repository if err = updateGitRepoAfterCreate(ctx, generateRepo); err != nil { return nil, fmt.Errorf("updateGitRepoAfterCreate: %w", err) } + // 5 - generate the repository contents according to the template // Git Content if opts.GitContent && !templateRepo.IsEmpty { if err = GenerateGitContent(ctx, templateRepo, generateRepo); err != nil { @@ -181,7 +182,7 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ } } - // 4 - update repository status to be ready + // 6 - update repository status to be ready generateRepo.Status = repo_model.RepositoryReady if err = repo_model.UpdateRepositoryCols(ctx, generateRepo, "status"); err != nil { return nil, fmt.Errorf("UpdateRepositoryCols: %w", err) From 5a041ccd7f1655c662b40301de2a6507a9ee7cf8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 7 Apr 2025 21:09:35 -0700 Subject: [PATCH 14/14] update test --- services/repository/adopt_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/services/repository/adopt_test.go b/services/repository/adopt_test.go index 733d1a7d09286..6e1dc417b315c 100644 --- a/services/repository/adopt_test.go +++ b/services/repository/adopt_test.go @@ -91,10 +91,12 @@ func TestListUnadoptedRepositories_ListOptions(t *testing.T) { func TestAdoptRepository(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) - // a successful adopt - destDir := filepath.Join(setting.RepoRootPath, "user2", "test-adopt.git") - assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, "user2", "repo1.git"), destDir)) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + // a successful adopt + destDir := filepath.Join(setting.RepoRootPath, user2.Name, "test-adopt.git") + assert.NoError(t, unittest.SyncDirs(filepath.Join(setting.RepoRootPath, user2.Name, "repo1.git"), destDir)) + adoptedRepo, err := AdoptRepository(db.DefaultContext, user2, user2, CreateRepoOptions{Name: "test-adopt"}) assert.NoError(t, err) repoTestAdopt := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{Name: "test-adopt"}) @@ -104,7 +106,7 @@ func TestAdoptRepository(t *testing.T) { err = deleteFailedAdoptRepository(adoptedRepo.ID) assert.NoError(t, err) - unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: "user2", Name: "test-adopt"}) + unittest.AssertNotExistsBean(t, &repo_model.Repository{OwnerName: user2.Name, Name: "test-adopt"}) // a failed adopt because some mock data // remove the hooks directory and create a file so that we cannot create the hooks successfully