From 82ca99db2dd557465c86918af01f52777a69066f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 11 Dec 2024 22:15:47 -0800 Subject: [PATCH 1/5] Merge two functions with the same content --- models/repo/fork.go | 29 +++++++++-------------------- models/repo/fork_test.go | 4 ++-- routers/api/v1/repo/pull.go | 6 +++++- routers/web/repo/compare.go | 18 +++++++++++++++--- routers/web/repo/fork.go | 6 +++++- services/repository/fork.go | 4 ++-- 6 files changed, 38 insertions(+), 29 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index 1c75e86458b2f..73797199628b7 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -21,15 +21,17 @@ func GetRepositoriesByForkID(ctx context.Context, forkID int64) ([]*Repository, } // GetForkedRepo checks if given user has already forked a repository with given ID. -func GetForkedRepo(ctx context.Context, ownerID, repoID int64) *Repository { +func GetForkedRepo(ctx context.Context, ownerID, repoID int64) (*Repository, error) { repo := new(Repository) - has, _ := db.GetEngine(ctx). + has, err := db.GetEngine(ctx). Where("owner_id=? AND fork_id=?", ownerID, repoID). Get(repo) - if has { - return repo + if err != nil { + return nil, err + } else if has { + return nil, ErrRepoNotExist{ID: repoID} } - return nil + return repo, nil } // HasForkedRepo checks if given user has already forked a repository with given ID. @@ -41,19 +43,6 @@ func HasForkedRepo(ctx context.Context, ownerID, repoID int64) bool { return has } -// GetUserFork return user forked repository from this repository, if not forked return nil -func GetUserFork(ctx context.Context, repoID, userID int64) (*Repository, error) { - var forkedRepo Repository - has, err := db.GetEngine(ctx).Where("fork_id = ?", repoID).And("owner_id = ?", userID).Get(&forkedRepo) - if err != nil { - return nil, err - } - if !has { - return nil, nil - } - return &forkedRepo, nil -} - // IncrementRepoForkNum increment repository fork number func IncrementRepoForkNum(ctx context.Context, repoID int64) error { _, err := db.GetEngine(ctx).Exec("UPDATE `repository` SET num_forks=num_forks+1 WHERE id=?", repoID) @@ -87,8 +76,8 @@ func GetForksByUserAndOrgs(ctx context.Context, user *user_model.User, repo *Rep if user == nil { return repoList, nil } - forkedRepo, err := GetUserFork(ctx, repo.ID, user.ID) - if err != nil { + forkedRepo, err := GetForkedRepo(ctx, repo.ID, user.ID) + if err != nil && !IsErrRepoNotExist(err) { return repoList, err } if forkedRepo != nil { diff --git a/models/repo/fork_test.go b/models/repo/fork_test.go index e8dca204cc457..7a15874329e8c 100644 --- a/models/repo/fork_test.go +++ b/models/repo/fork_test.go @@ -20,14 +20,14 @@ func TestGetUserFork(t *testing.T) { repo, err := repo_model.GetRepositoryByID(db.DefaultContext, 10) assert.NoError(t, err) assert.NotNil(t, repo) - repo, err = repo_model.GetUserFork(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetForkedRepo(db.DefaultContext, repo.ID, 13) assert.NoError(t, err) assert.NotNil(t, repo) repo, err = repo_model.GetRepositoryByID(db.DefaultContext, 9) assert.NoError(t, err) assert.NotNil(t, repo) - repo, err = repo_model.GetUserFork(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetForkedRepo(db.DefaultContext, repo.ID, 13) assert.NoError(t, err) assert.Nil(t, repo) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 86b204f51e250..bad078414e68e 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1132,7 +1132,11 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) } // Check if current user has fork of repository or in the same repository. - headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) + headRepo, err := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { + ctx.Error(http.StatusInternalServerError, "GetForkedRepo", err) + return nil, nil, nil, "", "" + } if headRepo == nil && !isSameRepo { err := baseRepo.GetBaseRepo(ctx) if err != nil { diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 278974bec3ecf..a500dd6f1e466 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -356,7 +356,11 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { // "OwnForkRepo" var ownForkRepo *repo_model.Repository if ctx.Doer != nil && baseRepo.OwnerID != ctx.Doer.ID { - repo := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, baseRepo.ID) + repo, err := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, baseRepo.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { + ctx.ServerError("GetForkedRepo", err) + return nil + } if repo != nil { ownForkRepo = repo ctx.Data["OwnForkRepo"] = ownForkRepo @@ -380,13 +384,21 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { // 5. If the headOwner has a fork of the baseRepo - use that if !has { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) + ci.HeadRepo, err = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { + ctx.ServerError("GetForkedRepo", err) + return nil + } has = ci.HeadRepo != nil } // 6. If the baseRepo is a fork and the headUser has a fork of that use that if !has && baseRepo.IsFork { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) + ci.HeadRepo, err = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { + ctx.ServerError("GetForkedRepo", err) + return nil + } has = ci.HeadRepo != nil } diff --git a/routers/web/repo/fork.go b/routers/web/repo/fork.go index 27e42a8f98e44..c0d823c079c25 100644 --- a/routers/web/repo/fork.go +++ b/routers/web/repo/fork.go @@ -166,7 +166,11 @@ func ForkPost(ctx *context.Context) { ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_has_same_repo"), tplFork, &form) return } - repo := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) + repo, err := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { + ctx.ServerError("GetForkedRepo", err) + return + } if repo != nil { ctx.Redirect(ctxUser.HomeLink() + "/" + url.PathEscape(repo.Name)) return diff --git a/services/repository/fork.go b/services/repository/fork.go index bc4fdf85627b0..26057266c5f18 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -71,8 +71,8 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } } - forkedRepo, err := repo_model.GetUserFork(ctx, opts.BaseRepo.ID, owner.ID) - if err != nil { + forkedRepo, err := repo_model.GetForkedRepo(ctx, opts.BaseRepo.ID, owner.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { return nil, err } if forkedRepo != nil { From 8a71bd9f6a749f6a5b0e5ea8686cbd340da90a81 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 14:48:00 +0800 Subject: [PATCH 2/5] Update models/repo/fork.go Co-authored-by: Zettat123 --- models/repo/fork.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index 73797199628b7..b69c539ccce03 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -28,7 +28,7 @@ func GetForkedRepo(ctx context.Context, ownerID, repoID int64) (*Repository, err Get(repo) if err != nil { return nil, err - } else if has { + } else if !has { return nil, ErrRepoNotExist{ID: repoID} } return repo, nil From 9632e6fbc06cd0e5fa9f5cadbd82d9608025bbe5 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 12:00:25 -0800 Subject: [PATCH 3/5] Fix bug --- models/repo/fork.go | 2 +- models/repo/fork_test.go | 6 +++--- services/repository/fork.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index 73797199628b7..72dd8a77e55bd 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -76,7 +76,7 @@ func GetForksByUserAndOrgs(ctx context.Context, user *user_model.User, repo *Rep if user == nil { return repoList, nil } - forkedRepo, err := GetForkedRepo(ctx, repo.ID, user.ID) + forkedRepo, err := GetForkedRepo(ctx, user.ID, repo.ID) if err != nil && !IsErrRepoNotExist(err) { return repoList, err } diff --git a/models/repo/fork_test.go b/models/repo/fork_test.go index 7a15874329e8c..10b6b7e26183b 100644 --- a/models/repo/fork_test.go +++ b/models/repo/fork_test.go @@ -13,21 +13,21 @@ import ( "github.com/stretchr/testify/assert" ) -func TestGetUserFork(t *testing.T) { +func Test_GetForkedRepo(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) // User13 has repo 11 forked from repo10 repo, err := repo_model.GetRepositoryByID(db.DefaultContext, 10) assert.NoError(t, err) assert.NotNil(t, repo) - repo, err = repo_model.GetForkedRepo(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetForkedRepo(db.DefaultContext, 13, repo.ID) assert.NoError(t, err) assert.NotNil(t, repo) repo, err = repo_model.GetRepositoryByID(db.DefaultContext, 9) assert.NoError(t, err) assert.NotNil(t, repo) - repo, err = repo_model.GetForkedRepo(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetForkedRepo(db.DefaultContext, 13, repo.ID) assert.NoError(t, err) assert.Nil(t, repo) } diff --git a/services/repository/fork.go b/services/repository/fork.go index 26057266c5f18..ea0690da26081 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -71,7 +71,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } } - forkedRepo, err := repo_model.GetForkedRepo(ctx, opts.BaseRepo.ID, owner.ID) + forkedRepo, err := repo_model.GetForkedRepo(ctx, owner.ID, opts.BaseRepo.ID) if err != nil && !repo_model.IsErrRepoNotExist(err) { return nil, err } From bb7c4464b8067b58c3c76aa875a6329614802dff Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 16 Dec 2024 16:23:29 -0800 Subject: [PATCH 4/5] Fix bug --- routers/api/v1/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index fc690bee022b1..5c136d8d96931 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1133,7 +1133,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) headRepo, err := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) if err != nil && !repo_model.IsErrRepoNotExist(err) { ctx.Error(http.StatusInternalServerError, "GetForkedRepo", err) - return nil, nil, nil, "", "" + return nil, nil } if headRepo == nil && !isSameRepo { err = baseRepo.GetBaseRepo(ctx) From 57a106e48535e4fd64cbc57c20b116ee943e997f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 27 Dec 2024 21:09:29 -0800 Subject: [PATCH 5/5] Fix test --- models/repo/fork_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/repo/fork_test.go b/models/repo/fork_test.go index 10b6b7e26183b..4004c977a2224 100644 --- a/models/repo/fork_test.go +++ b/models/repo/fork_test.go @@ -28,6 +28,7 @@ func Test_GetForkedRepo(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, repo) repo, err = repo_model.GetForkedRepo(db.DefaultContext, 13, repo.ID) - assert.NoError(t, err) + assert.Error(t, err) + assert.True(t, repo_model.IsErrRepoNotExist(err)) assert.Nil(t, repo) }