Fix unclear `IsRepositoryExist` logic (#24374)

There was only one `IsRepositoryExist` function, it did: `has && isDir`

However it's not right, and it would cause 500 error when creating a new
repository if the dir exists.

Then, it was changed to `has || isDir`, it is still incorrect, it
affects the "adopt repo" logic.

To make the logic clear:

* IsRepositoryModelOrDirExist
* IsRepositoryModelExist
This commit is contained in:
wxiaoguang 2023-04-29 02:14:26 +08:00 committed by GitHub
parent 572af214a7
commit a6450494c3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 20 additions and 16 deletions

View File

@ -726,12 +726,9 @@ func GetRepositoriesMapByIDs(ids []int64) (map[int64]*Repository, error) {
return repos, db.GetEngine(db.DefaultContext).In("id", ids).Find(&repos) return repos, db.GetEngine(db.DefaultContext).In("id", ids).Find(&repos)
} }
// IsRepositoryExist returns true if the repository with given name under user has already existed. // IsRepositoryModelOrDirExist returns true if the repository with given name under user has already existed.
func IsRepositoryExist(ctx context.Context, u *user_model.User, repoName string) (bool, error) { func IsRepositoryModelOrDirExist(ctx context.Context, u *user_model.User, repoName string) (bool, error) {
has, err := db.GetEngine(ctx).Get(&Repository{ has, err := IsRepositoryModelExist(ctx, u, repoName)
OwnerID: u.ID,
LowerName: strings.ToLower(repoName),
})
if err != nil { if err != nil {
return false, err return false, err
} }
@ -739,6 +736,13 @@ func IsRepositoryExist(ctx context.Context, u *user_model.User, repoName string)
return has || isDir, err return has || isDir, err
} }
func IsRepositoryModelExist(ctx context.Context, u *user_model.User, repoName string) (bool, error) {
return db.GetEngine(ctx).Get(&Repository{
OwnerID: u.ID,
LowerName: strings.ToLower(repoName),
})
}
// GetTemplateRepo populates repo.TemplateRepo for a generated repository and // GetTemplateRepo populates repo.TemplateRepo for a generated repository and
// returns an error on failure (NOTE: no error is returned for // returns an error on failure (NOTE: no error is returned for
// non-generated repositories, and TemplateRepo will be left untouched) // non-generated repositories, and TemplateRepo will be left untouched)

View File

@ -116,7 +116,7 @@ func CheckCreateRepository(doer, u *user_model.User, name string, overwriteOrAdo
return err return err
} }
has, err := IsRepositoryExist(db.DefaultContext, u, name) has, err := IsRepositoryModelOrDirExist(db.DefaultContext, u, name)
if err != nil { if err != nil {
return fmt.Errorf("IsRepositoryExist: %w", err) return fmt.Errorf("IsRepositoryExist: %w", err)
} else if has { } else if has {
@ -147,7 +147,7 @@ func ChangeRepositoryName(doer *user_model.User, repo *Repository, newRepoName s
return err return err
} }
has, err := IsRepositoryExist(db.DefaultContext, repo.Owner, newRepoName) has, err := IsRepositoryModelOrDirExist(db.DefaultContext, repo.Owner, newRepoName)
if err != nil { if err != nil {
return fmt.Errorf("IsRepositoryExist: %w", err) return fmt.Errorf("IsRepositoryExist: %w", err)
} else if has { } else if has {

View File

@ -172,7 +172,7 @@ func CreatePendingRepositoryTransfer(ctx context.Context, doer, newOwner *user_m
} }
// Check if new owner has repository with same name. // Check if new owner has repository with same name.
if has, err := repo_model.IsRepositoryExist(ctx, newOwner, repo.Name); err != nil { if has, err := repo_model.IsRepositoryModelExist(ctx, newOwner, repo.Name); err != nil {
return fmt.Errorf("IsRepositoryExist: %w", err) return fmt.Errorf("IsRepositoryExist: %w", err)
} else if has { } else if has {
return repo_model.ErrRepoAlreadyExist{ return repo_model.ErrRepoAlreadyExist{
@ -249,7 +249,7 @@ func TransferOwnership(doer *user_model.User, newOwnerName string, repo *repo_mo
newOwnerName = newOwner.Name // ensure capitalisation matches newOwnerName = newOwner.Name // ensure capitalisation matches
// Check if new owner has repository with same name. // Check if new owner has repository with same name.
if has, err := repo_model.IsRepositoryExist(ctx, newOwner, repo.Name); err != nil { if has, err := repo_model.IsRepositoryModelOrDirExist(ctx, newOwner, repo.Name); err != nil {
return fmt.Errorf("IsRepositoryExist: %w", err) return fmt.Errorf("IsRepositoryExist: %w", err)
} else if has { } else if has {
return repo_model.ErrRepoAlreadyExist{ return repo_model.ErrRepoAlreadyExist{

View File

@ -35,7 +35,7 @@ func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, re
return err return err
} }
has, err := repo_model.IsRepositoryExist(ctx, u, repo.Name) has, err := repo_model.IsRepositoryModelExist(ctx, u, repo.Name)
if err != nil { if err != nil {
return fmt.Errorf("IsRepositoryExist: %w", err) return fmt.Errorf("IsRepositoryExist: %w", err)
} else if has { } else if has {

View File

@ -95,7 +95,7 @@ func AdoptRepository(ctx *context.APIContext) {
} }
// check not a repo // check not a repo
has, err := repo_model.IsRepositoryExist(ctx, ctxUser, repoName) has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, repoName)
if err != nil { if err != nil {
ctx.InternalServerError(err) ctx.InternalServerError(err)
return return
@ -157,7 +157,7 @@ func DeleteUnadoptedRepository(ctx *context.APIContext) {
} }
// check not a repo // check not a repo
has, err := repo_model.IsRepositoryExist(ctx, ctxUser, repoName) has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, repoName)
if err != nil { if err != nil {
ctx.InternalServerError(err) ctx.InternalServerError(err)
return return

View File

@ -133,7 +133,7 @@ func AdoptOrDeleteRepository(ctx *context.Context) {
repoName := dirSplit[1] repoName := dirSplit[1]
// check not a repo // check not a repo
has, err := repo_model.IsRepositoryExist(ctx, ctxUser, repoName) has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, repoName)
if err != nil { if err != nil {
ctx.ServerError("IsRepositoryExist", err) ctx.ServerError("IsRepositoryExist", err)
return return

View File

@ -31,7 +31,7 @@ func AdoptOrDeleteRepository(ctx *context.Context) {
root := user_model.UserPath(ctxUser.LowerName) root := user_model.UserPath(ctxUser.LowerName)
// check not a repo // check not a repo
has, err := repo_model.IsRepositoryExist(ctx, ctxUser, dir) has, err := repo_model.IsRepositoryModelExist(ctx, ctxUser, dir)
if err != nil { if err != nil {
ctx.ServerError("IsRepositoryExist", err) ctx.ServerError("IsRepositoryExist", err)
return return

View File

@ -206,7 +206,7 @@ func DeleteUnadoptedRepository(ctx context.Context, doer, u *user_model.User, re
} }
} }
if exist, err := repo_model.IsRepositoryExist(ctx, u, repoName); err != nil { if exist, err := repo_model.IsRepositoryModelExist(ctx, u, repoName); err != nil {
return err return err
} else if exist { } else if exist {
return repo_model.ErrRepoAlreadyExist{ return repo_model.ErrRepoAlreadyExist{