Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge two functions with the same content #32805

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 9 additions & 20 deletions models/repo/fork.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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, user.ID, repo.ID)
if err != nil && !IsErrRepoNotExist(err) {
return repoList, err
}
if forkedRepo != nil {
Expand Down
9 changes: 5 additions & 4 deletions models/repo/fork_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,22 @@ 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.GetUserFork(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.GetUserFork(db.DefaultContext, repo.ID, 13)
assert.NoError(t, err)
repo, err = repo_model.GetForkedRepo(db.DefaultContext, 13, repo.ID)
assert.Error(t, err)
assert.True(t, repo_model.IsErrRepoNotExist(err))
assert.Nil(t, repo)
}
6 changes: 5 additions & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -1113,7 +1113,11 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
isSameRepo := ctx.Repo.Owner.ID == headUser.ID

// 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
}
if headRepo == nil && !isSameRepo {
err = baseRepo.GetBaseRepo(ctx)
if err != nil {
Expand Down
18 changes: 15 additions & 3 deletions routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,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
Expand All @@ -381,13 +385,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
}

Expand Down
6 changes: 5 additions & 1 deletion routers/web/repo/fork.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,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
}
Comment on lines +171 to +174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "merge" made code more complex than it should be.

I really dislike (hate) so many "if err { ServerError; return }" in code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any better idea? Just skip the error when getting a fork like before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave to 1.24?

if repo != nil {
ctx.Redirect(ctxUser.HomeLink() + "/" + url.PathEscape(repo.Name))
return
Expand Down
4 changes: 2 additions & 2 deletions services/repository/fork.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, owner.ID, opts.BaseRepo.ID)
if err != nil && !repo_model.IsErrRepoNotExist(err) {
return nil, err
}
if forkedRepo != nil {
Expand Down
Loading