From 8eb19a5ae19abae15c0666d4ab98906139a7f439 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 9 Dec 2024 15:48:24 -0800 Subject: [PATCH 01/28] Refactor GetDiff/Path functions to let it more flexible --- modules/git/repo_compare.go | 95 ++++++++++++++------------------ modules/git/repo_compare_test.go | 27 ++++++++- services/pull/patch.go | 8 +-- 3 files changed, 71 insertions(+), 59 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 16fcdcf4c8f96..ec8bacb5ec316 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -234,71 +234,73 @@ func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int, } // GetDiffOrPatch generates either diff or formatted patch data between given revisions -func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, patch, binary bool) error { +func (repo *Repository) GetDiffOrPatch(compareString string, w io.Writer, patch, binary bool) error { if patch { - return repo.GetPatch(base, head, w) + return repo.GetPatch(compareString, w) } if binary { - return repo.GetDiffBinary(base, head, w) + return repo.GetDiffBinary(compareString, w) } - return repo.GetDiff(base, head, w) + return repo.GetDiff(compareString, w) +} + +func parseCompareArgs(compareArgs string) (args []string) { + parts := strings.Split(compareArgs, "...") + if len(parts) == 2 { + return []string{compareArgs} + } + parts = strings.Split(compareArgs, "..") + if len(parts) == 2 { + return parts + } + parts = strings.Fields(compareArgs) + if len(parts) == 2 { + return parts + } + + return nil } // GetDiff generates and returns patch data between given revisions, optimized for human readability -func (repo *Repository) GetDiff(base, head string, w io.Writer) error { +func (repo *Repository) GetDiff(compareArgs string, w io.Writer) error { + args := parseCompareArgs(compareArgs) + if len(args) == 0 { + return fmt.Errorf("invalid compareArgs: %s", compareArgs) + } stderr := new(bytes.Buffer) - err := NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(base + "..." + head). + return NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(args...). Run(&RunOpts{ Dir: repo.Path, Stdout: w, Stderr: stderr, }) - if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { - return NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(base, head). - Run(&RunOpts{ - Dir: repo.Path, - Stdout: w, - }) - } - return err } // GetDiffBinary generates and returns patch data between given revisions, including binary diffs. -func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error { - stderr := new(bytes.Buffer) - err := NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(base + "..." + head). - Run(&RunOpts{ - Dir: repo.Path, - Stdout: w, - Stderr: stderr, - }) - if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { - return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(base, head). - Run(&RunOpts{ - Dir: repo.Path, - Stdout: w, - }) +func (repo *Repository) GetDiffBinary(compareArgs string, w io.Writer) error { + args := parseCompareArgs(compareArgs) + if len(args) == 0 { + return fmt.Errorf("invalid compareArgs: %s", compareArgs) } - return err + return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(args...).Run(&RunOpts{ + Dir: repo.Path, + Stdout: w, + }) } // GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply` -func (repo *Repository) GetPatch(base, head string, w io.Writer) error { +func (repo *Repository) GetPatch(compareArgs string, w io.Writer) error { + args := parseCompareArgs(compareArgs) + if len(args) == 0 { + return fmt.Errorf("invalid compareArgs: %s", compareArgs) + } stderr := new(bytes.Buffer) - err := NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(base + "..." + head). + return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(args...). Run(&RunOpts{ Dir: repo.Path, Stdout: w, Stderr: stderr, }) - if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { - return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(base, head). - Run(&RunOpts{ - Dir: repo.Path, - Stdout: w, - }) - } - return err } // GetFilesChangedBetween returns a list of all files that have been changed between the given commits @@ -329,21 +331,6 @@ func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, err return split, err } -// GetDiffFromMergeBase generates and return patch data from merge base to head -func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error { - stderr := new(bytes.Buffer) - err := NewCommand(repo.Ctx, "diff", "-p", "--binary").AddDynamicArguments(base + "..." + head). - Run(&RunOpts{ - Dir: repo.Path, - Stdout: w, - Stderr: stderr, - }) - if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { - return repo.GetDiffBinary(base, head, w) - } - return err -} - // ReadPatchCommit will check if a diff patch exists and return stats func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error) { // Migrated repositories download patches to "pulls" location diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 9983873186720..a5d113b3645f7 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -12,6 +12,31 @@ import ( "github.com/stretchr/testify/assert" ) +func Test_parseCompareArgs(t *testing.T) { + testCases := []struct { + compareString string + expected []string + }{ + { + "master..develop", + []string{"master", "develop"}, + }, + { + "master HEAD", + []string{"master", "HEAD"}, + }, + { + "HEAD...develop", + []string{"HEAD...develop"}, + }, + } + + for _, tc := range testCases { + args := parseCompareArgs(tc.compareString) + assert.Equal(t, tc.expected, args) + } +} + func TestGetFormatPatch(t *testing.T) { bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") clonedPath, err := cloneRepo(t, bareRepo1Path) @@ -28,7 +53,7 @@ func TestGetFormatPatch(t *testing.T) { defer repo.Close() rd := &bytes.Buffer{} - err = repo.GetPatch("8d92fc95^", "8d92fc95", rd) + err = repo.GetPatch("8d92fc95^ 8d92fc95", rd) if err != nil { assert.NoError(t, err) return diff --git a/services/pull/patch.go b/services/pull/patch.go index 0934a86c89a65..192a67739ed84 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -42,9 +42,9 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io } defer closer.Close() - if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch, binary); err != nil { - log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) - return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) + if err := gitRepo.GetDiffOrPatch(pr.MergeBase+".."+pr.GetGitRefName(), w, patch, binary); err != nil { + log.Error("unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) + return fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) } return nil } @@ -355,7 +355,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * _ = util.Remove(tmpPatchFile.Name()) }() - if err := gitRepo.GetDiffBinary(pr.MergeBase, "tracking", tmpPatchFile); err != nil { + if err := gitRepo.GetDiffBinary(pr.MergeBase+"..tracking", tmpPatchFile); err != nil { tmpPatchFile.Close() log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return false, fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) From 48b5bab880fcca12eb1440eef0436fcf7cdecc66 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 10 Dec 2024 13:31:47 -0800 Subject: [PATCH 02/28] fix conflict --- modules/git/repo_compare.go | 11 ----------- services/pull/patch.go | 13 ++++++++++++- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index ec8bacb5ec316..5d7fbe6df64c6 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -233,17 +233,6 @@ func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int, return numFiles, totalAdditions, totalDeletions, err } -// GetDiffOrPatch generates either diff or formatted patch data between given revisions -func (repo *Repository) GetDiffOrPatch(compareString string, w io.Writer, patch, binary bool) error { - if patch { - return repo.GetPatch(compareString, w) - } - if binary { - return repo.GetDiffBinary(compareString, w) - } - return repo.GetDiff(compareString, w) -} - func parseCompareArgs(compareArgs string) (args []string) { parts := strings.Split(compareArgs, "...") if len(parts) == 2 { diff --git a/services/pull/patch.go b/services/pull/patch.go index 192a67739ed84..c5befe39d6d69 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -29,6 +29,17 @@ import ( "github.com/gobwas/glob" ) +// getDiffOrPatch generates either diff or formatted patch data between given revisions +func getDiffOrPatch(repo *git.Repository, compareString string, w io.Writer, patch, binary bool) error { + if patch { + return repo.GetPatch(compareString, w) + } + if binary { + return repo.GetDiffBinary(compareString, w) + } + return repo.GetDiff(compareString, w) +} + // DownloadDiffOrPatch will write the patch for the pr to the writer func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io.Writer, patch, binary bool) error { if err := pr.LoadBaseRepo(ctx); err != nil { @@ -42,7 +53,7 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io } defer closer.Close() - if err := gitRepo.GetDiffOrPatch(pr.MergeBase+".."+pr.GetGitRefName(), w, patch, binary); err != nil { + if err := getDiffOrPatch(gitRepo, pr.MergeBase+"..."+pr.GetGitRefName(), w, patch, binary); err != nil { log.Error("unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) } From 283c030497b455ecfa759d4649f9f8b45158742e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 10 Dec 2024 13:35:09 -0800 Subject: [PATCH 03/28] Fix bug --- services/pull/patch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index c5befe39d6d69..ef7c366f0627b 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -366,7 +366,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * _ = util.Remove(tmpPatchFile.Name()) }() - if err := gitRepo.GetDiffBinary(pr.MergeBase+"..tracking", tmpPatchFile); err != nil { + if err := gitRepo.GetDiffBinary(pr.MergeBase+"...tracking", tmpPatchFile); err != nil { tmpPatchFile.Close() log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return false, fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) From f19b4b79677b90d99173378056d65edb43fc5775 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 10 Dec 2024 21:54:57 -0800 Subject: [PATCH 04/28] Refactor Parsing compare path parameters --- routers/api/v1/repo/compare.go | 93 ++++++++++--- routers/api/v1/repo/pull.go | 191 ++++++------------------- routers/common/compare.go | 194 +++++++++++++++++++++++++- routers/common/compare_test.go | 120 ++++++++++++++++ routers/web/repo/compare.go | 247 +++++++-------------------------- routers/web/repo/pull.go | 4 +- 6 files changed, 481 insertions(+), 368 deletions(-) create mode 100644 routers/common/compare_test.go diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 38e5330b3acb8..6c4d428f22890 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -4,12 +4,18 @@ package repo import ( + "errors" "net/http" - "strings" + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" ) @@ -53,33 +59,84 @@ func CompareDiff(ctx *context.APIContext) { defer gitRepo.Close() } - infoPath := ctx.PathParam("*") - infos := []string{ctx.Repo.Repository.DefaultBranch, ctx.Repo.Repository.DefaultBranch} - if infoPath != "" { - infos = strings.SplitN(infoPath, "...", 2) - if len(infos) != 2 { - if infos = strings.SplitN(infoPath, "..", 2); len(infos) != 2 { - infos = []string{ctx.Repo.Repository.DefaultBranch, infoPath} + pathParam := ctx.PathParam("*") + baseRepo := ctx.Repo.Repository + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) + if err != nil { + switch { + case user_model.IsErrUserNotExist(err): + ctx.NotFound("GetUserByName") + case repo_model.IsErrRepoNotExist(err): + ctx.NotFound("GetRepositoryByOwnerAndName") + case errors.Is(err, util.ErrInvalidArgument): + ctx.NotFound("ParseComparePathParams") + default: + ctx.ServerError("GetRepositoryByOwnerAndName", err) + } + return + } + defer ci.Close() + + // remove the check when we support compare with carets + if ci.CaretTimes > 0 { + ctx.NotFound("Unsupported compare") + return + } + + if !ci.IsSameRepo() { + // user should have permission to read headrepo's codes + permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) + return + } + if !permHead.CanRead(unit.TypeCode) { + if log.IsTrace() { + log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", + ctx.Doer, + ci.HeadRepo, + permHead) } + ctx.NotFound("Can't read headRepo UnitTypeCode") + return } } - _, headGitRepo, ci, _, _ := parseCompareInfo(ctx, api.CreatePullRequestOption{ - Base: infos[0], - Head: infos[1], - }) - if ctx.Written() { + ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() + log.Trace("Repo path: %q, base branch: %q, head branch: %q", ctx.Repo.GitRepo.Path, ci.BaseOriRef, ci.HeadOriRef) + + // Check if current user has fork of repository or in the same repository. + /*headRepo := repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, ctx.Repo.Repository.ID) + if headRepo == nil && !ci.IsSameRepo() { + err := ctx.Repo.Repository.GetBaseRepo(ctx) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) + return nil, nil, nil, "", "" + } + + // Check if baseRepo's base repository is the same as headUser's repository. + if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID { + log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID) + ctx.NotFound("GetBaseRepo") + return nil, nil, nil, "", "" + } + // Assign headRepo so it can be used below. + headRepo = baseRepo.BaseRepo + }*/ + + ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), ci.BaseOriRef, ci.HeadOriRef, false, false) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) return } - defer headGitRepo.Close() verification := ctx.FormString("verification") == "" || ctx.FormBool("verification") files := ctx.FormString("files") == "" || ctx.FormBool("files") - apiCommits := make([]*api.Commit, 0, len(ci.Commits)) + apiCommits := make([]*api.Commit, 0, len(ci.CompareInfo.Commits)) userCache := make(map[string]*user_model.User) - for i := 0; i < len(ci.Commits); i++ { - apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.Commits[i], userCache, + for i := 0; i < len(ci.CompareInfo.Commits); i++ { + apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.CompareInfo.Commits[i], userCache, convert.ToCommitOptions{ Stat: true, Verification: verification, @@ -93,7 +150,7 @@ func CompareDiff(ctx *context.APIContext) { } ctx.JSON(http.StatusOK, &api.Compare{ - TotalCommits: len(ci.Commits), + TotalCommits: len(ci.CompareInfo.Commits), Commits: apiCommits, }) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 86b204f51e250..fe23f89fbc61a 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -28,8 +28,10 @@ import ( "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" + "code.gitea.io/gitea/routers/common" asymkey_service "code.gitea.io/gitea/services/asymkey" "code.gitea.io/gitea/services/automerge" "code.gitea.io/gitea/services/context" @@ -401,14 +403,48 @@ func CreatePullRequest(ctx *context.APIContext) { ) // Get repo/branch information - headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form) - if ctx.Written() { + ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo) + if err != nil { + switch { + case user_model.IsErrUserNotExist(err): + ctx.NotFound("GetUserByName") + case repo_model.IsErrRepoNotExist(err): + ctx.NotFound("GetRepositoryByOwnerAndName") + case errors.Is(err, util.ErrInvalidArgument): + ctx.NotFound("ParseComparePathParams") + default: + ctx.ServerError("GetRepositoryByOwnerAndName", err) + } return } - defer headGitRepo.Close() + defer ci.Close() + + if ci.IsPull() { + ctx.Error(http.StatusUnprocessableEntity, "Bad base or head refs", "Only support branch to branch comparison") + return + } + + if !ci.IsSameRepo() { + // user should have permission to read headrepo's codes + permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) + return + } + if !permHead.CanRead(unit.TypeCode) { + if log.IsTrace() { + log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", + ctx.Doer, + ci.HeadRepo, + permHead) + } + ctx.NotFound("Can't read headRepo UnitTypeCode") + return + } + } // Check if another PR exists with the same targets - existingPr, err := issues_model.GetUnmergedPullRequest(ctx, headRepo.ID, ctx.Repo.Repository.ID, headBranch, baseBranch, issues_model.PullRequestFlowGithub) + existingPr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { ctx.Error(http.StatusInternalServerError, "GetUnmergedPullRequest", err) @@ -484,13 +520,13 @@ func CreatePullRequest(ctx *context.APIContext) { DeadlineUnix: deadlineUnix, } pr := &issues_model.PullRequest{ - HeadRepoID: headRepo.ID, + HeadRepoID: ci.HeadRepo.ID, BaseRepoID: repo.ID, - HeadBranch: headBranch, - BaseBranch: baseBranch, - HeadRepo: headRepo, + HeadBranch: ci.HeadOriRef, + BaseBranch: ci.BaseOriRef, + HeadRepo: ci.HeadRepo, BaseRepo: repo, - MergeBase: compareInfo.MergeBase, + MergeBase: ci.CompareInfo.MergeBase, Type: issues_model.PullRequestGitea, } @@ -1080,143 +1116,6 @@ func MergePullRequest(ctx *context.APIContext) { ctx.Status(http.StatusOK) } -func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) (*repo_model.Repository, *git.Repository, *git.CompareInfo, string, string) { - baseRepo := ctx.Repo.Repository - - // Get compared branches information - // format: ...[:] - // base<-head: master...head:feature - // same repo: master...feature - - // TODO: Validate form first? - - baseBranch := form.Base - - var ( - headUser *user_model.User - headBranch string - isSameRepo bool - err error - ) - - // If there is no head repository, it means pull request between same repository. - headInfos := strings.Split(form.Head, ":") - if len(headInfos) == 1 { - isSameRepo = true - headUser = ctx.Repo.Owner - headBranch = headInfos[0] - } else if len(headInfos) == 2 { - headUser, err = user_model.GetUserByName(ctx, headInfos[0]) - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName") - } else { - ctx.Error(http.StatusInternalServerError, "GetUserByName", err) - } - return nil, nil, nil, "", "" - } - headBranch = headInfos[1] - // The head repository can also point to the same repo - isSameRepo = ctx.Repo.Owner.ID == headUser.ID - } else { - ctx.NotFound() - return nil, nil, nil, "", "" - } - - ctx.Repo.PullRequest.SameRepo = isSameRepo - log.Trace("Repo path: %q, base branch: %q, head branch: %q", ctx.Repo.GitRepo.Path, baseBranch, headBranch) - // Check if base branch is valid. - if !ctx.Repo.GitRepo.IsBranchExist(baseBranch) && !ctx.Repo.GitRepo.IsTagExist(baseBranch) { - ctx.NotFound("BaseNotExist") - return nil, nil, nil, "", "" - } - - // Check if current user has fork of repository or in the same repository. - headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) - if headRepo == nil && !isSameRepo { - err := baseRepo.GetBaseRepo(ctx) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) - return nil, nil, nil, "", "" - } - - // Check if baseRepo's base repository is the same as headUser's repository. - if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID { - log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID) - ctx.NotFound("GetBaseRepo") - return nil, nil, nil, "", "" - } - // Assign headRepo so it can be used below. - headRepo = baseRepo.BaseRepo - } - - var headGitRepo *git.Repository - if isSameRepo { - headRepo = ctx.Repo.Repository - headGitRepo = ctx.Repo.GitRepo - } else { - headGitRepo, err = gitrepo.OpenRepository(ctx, headRepo) - if err != nil { - ctx.Error(http.StatusInternalServerError, "OpenRepository", err) - return nil, nil, nil, "", "" - } - } - - // user should have permission to read baseRepo's codes and pulls, NOT headRepo's - permBase, err := access_model.GetUserRepoPermission(ctx, baseRepo, ctx.Doer) - if err != nil { - headGitRepo.Close() - ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) - return nil, nil, nil, "", "" - } - if !permBase.CanReadIssuesOrPulls(true) || !permBase.CanRead(unit.TypeCode) { - if log.IsTrace() { - log.Trace("Permission Denied: User %-v cannot create/read pull requests or cannot read code in Repo %-v\nUser in baseRepo has Permissions: %-+v", - ctx.Doer, - baseRepo, - permBase) - } - headGitRepo.Close() - ctx.NotFound("Can't read pulls or can't read UnitTypeCode") - return nil, nil, nil, "", "" - } - - // user should have permission to read headrepo's codes - permHead, err := access_model.GetUserRepoPermission(ctx, headRepo, ctx.Doer) - if err != nil { - headGitRepo.Close() - ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) - return nil, nil, nil, "", "" - } - if !permHead.CanRead(unit.TypeCode) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", - ctx.Doer, - headRepo, - permHead) - } - headGitRepo.Close() - ctx.NotFound("Can't read headRepo UnitTypeCode") - return nil, nil, nil, "", "" - } - - // Check if head branch is valid. - if !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) { - headGitRepo.Close() - ctx.NotFound() - return nil, nil, nil, "", "" - } - - compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch, false, false) - if err != nil { - headGitRepo.Close() - ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) - return nil, nil, nil, "", "" - } - - return headRepo, headGitRepo, compareInfo, baseBranch, headBranch -} - // UpdatePullRequest merge PR's baseBranch into headBranch func UpdatePullRequest(ctx *context.APIContext) { // swagger:operation POST /repos/{owner}/{repo}/pulls/{index}/update repository repoUpdatePullRequest diff --git a/routers/common/compare.go b/routers/common/compare.go index 4d1cc2f0d8908..d1b4327a1c7dd 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -4,18 +4,198 @@ package common import ( + "context" + "strings" + + git_model "code.gitea.io/gitea/models/git" 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/util" ) +type CompareRouter struct { + BaseOriRef string + BaseFullRef git.RefName + HeadOwnerName string + HeadRepoName string + HeadOriRef string + HeadFullRef git.RefName + CaretTimes int // ^ times after base ref + DotTimes int // 2(..) or 3(...) +} + +func (cr *CompareRouter) IsSameRepo() bool { + return cr.HeadOwnerName == "" && cr.HeadRepoName == "" +} + +func (cr *CompareRouter) IsSameRef() bool { + return cr.IsSameRepo() && cr.BaseOriRef == cr.HeadOriRef +} + +func (cr *CompareRouter) DirectComparison() bool { + return cr.DotTimes == 2 +} + +func parseBase(base string) (string, int) { + parts := strings.SplitN(base, "^", 2) + if len(parts) == 1 { + return base, 0 + } + return parts[0], len(parts[1]) + 1 +} + +func parseHead(head string) (string, string, string) { + paths := strings.SplitN(head, ":", 2) + if len(paths) == 1 { + return "", "", paths[0] + } + ownerRepo := strings.SplitN(paths[0], "/", 2) + if len(ownerRepo) == 1 { + return "", paths[0], paths[1] + } + return ownerRepo[0], ownerRepo[1], paths[1] +} + +func parseCompareRouter(router string) (*CompareRouter, error) { + var basePart, headPart string + dotTimes := 3 + parts := strings.Split(router, "...") + if len(parts) > 2 { + return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) + } + if len(parts) != 2 { + parts = strings.Split(router, "..") + if len(parts) == 1 { + headOwnerName, headRepoName, headRef := parseHead(router) + return &CompareRouter{ + HeadOriRef: headRef, + HeadOwnerName: headOwnerName, + HeadRepoName: headRepoName, + }, nil + } else if len(parts) > 2 { + return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) + } + dotTimes = 2 + } + basePart, headPart = parts[0], parts[1] + + baseRef, caretTimes := parseBase(basePart) + headOwnerName, headRepoName, headRef := parseHead(headPart) + + return &CompareRouter{ + BaseOriRef: baseRef, + HeadOriRef: headRef, + HeadOwnerName: headOwnerName, + HeadRepoName: headRepoName, + CaretTimes: caretTimes, + DotTimes: dotTimes, + }, nil +} + // CompareInfo represents the collected results from ParseCompareInfo type CompareInfo struct { - HeadUser *user_model.User - HeadRepo *repo_model.Repository - HeadGitRepo *git.Repository - CompareInfo *git.CompareInfo - BaseBranch string - HeadBranch string - DirectComparison bool + *CompareRouter + HeadUser *user_model.User + HeadRepo *repo_model.Repository + HeadGitRepo *git.Repository + CompareInfo *git.CompareInfo + close func() + IsBaseCommit bool + IsHeadCommit bool +} + +// display pull related information or not +func (ci *CompareInfo) IsPull() bool { + return ci.CaretTimes == 0 && !ci.DirectComparison() && + ci.BaseFullRef.IsBranch() && ci.HeadFullRef.IsBranch() +} + +func (ci *CompareInfo) Close() { + if ci.close != nil { + ci.close() + } +} + +func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, oriRef string) (git.RefName, bool, error) { + b, err := git_model.GetBranch(ctx, repoID, oriRef) + if err != nil { + return "", false, err + } + if !b.IsDeleted { + return git.RefNameFromBranch(oriRef), false, nil + } + + rel, err := repo_model.GetRelease(ctx, repoID, oriRef) + if err != nil && !repo_model.IsErrReleaseNotExist(err) { + return "", false, err + } + if rel != nil && rel.Sha1 != "" { + return git.RefNameFromTag(oriRef), false, nil + } + + commitObjectID, err := gitRepo.ConvertToGitID(oriRef) + if err != nil { + return "", false, err + } + return git.RefName(commitObjectID.String()), true, nil +} + +func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { + ci := &CompareInfo{} + var err error + + if pathParam == "" { + ci.HeadOriRef = baseRepo.DefaultBranch + } else { + ci.CompareRouter, err = parseCompareRouter(pathParam) + if err != nil { + return nil, err + } + } + if ci.BaseOriRef == "" { + ci.BaseOriRef = baseRepo.DefaultBranch + } + + if ci.IsSameRepo() { + ci.HeadUser = baseRepo.Owner + ci.HeadRepo = baseRepo + ci.HeadGitRepo = baseGitRepo + } else { + if ci.HeadOwnerName == baseRepo.Owner.Name { + ci.HeadUser = baseRepo.Owner + } else { + ci.HeadUser, err = user_model.GetUserByName(ctx, ci.HeadOwnerName) + if err != nil { + return nil, err + } + } + + ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, ci.HeadOwnerName, ci.HeadRepoName) + if err != nil { + return nil, err + } + ci.HeadRepo.Owner = ci.HeadUser + ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) + if err != nil { + return nil, err + } + ci.close = func() { + if ci.HeadGitRepo != nil { + ci.HeadGitRepo.Close() + } + } + } + + ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef) + if err != nil { + return nil, err + } + + ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) + if err != nil { + return nil, err + } + return ci, nil } diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go new file mode 100644 index 0000000000000..2341f8079280b --- /dev/null +++ b/routers/common/compare_test.go @@ -0,0 +1,120 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCompareRouters(t *testing.T) { + kases := []struct { + router string + compareRouter *CompareRouter + }{ + { + router: "main...develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main..develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 2, + }, + }, + { + router: "main^...develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 1, + DotTimes: 3, + }, + }, + { + router: "main^^^^^...develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 5, + DotTimes: 3, + }, + }, + { + router: "develop", + compareRouter: &CompareRouter{ + HeadOriRef: "develop", + }, + }, + { + router: "lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + }, + }, + { + router: "main...lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main...lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + { + router: "main^...lunny/forked_repo:develop", + compareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + CaretTimes: 1, + }, + }, + { + router: "v1.0...v1.1", + compareRouter: &CompareRouter{ + BaseOriRef: "v1.0", + HeadOriRef: "v1.1", + DotTimes: 3, + }, + }, + { + router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", + compareRouter: &CompareRouter{ + BaseOriRef: "8eb19a5ae19abae15c0666d4ab98906139a7f439", + HeadOriRef: "283c030497b455ecfa759d4649f9f8b45158742e", + DotTimes: 3, + }, + }, + } + for _, kase := range kases { + t.Run(kase.router, func(t *testing.T) { + r, err := parseCompareRouter(kase.router) + assert.NoError(t, err) + assert.EqualValues(t, kase.compareRouter, r) + }) + } +} diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 278974bec3ecf..4279cf9b3a2f2 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -187,12 +187,8 @@ func setCsvCompareContext(ctx *context.Context) { } // ParseCompareInfo parse compare info between two commit for preparing comparing references +// Permission check for base repository's code read should be checked before invoking this function func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { - baseRepo := ctx.Repo.Repository - ci := &common.CompareInfo{} - - fileOnly := ctx.FormBool("file-only") - // Get compared branches information // A full compare url is of the form: // @@ -219,111 +215,51 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { // base<-head: master...head:feature // same repo: master...feature - var ( - isSameRepo bool - infoPath string - err error - ) + fileOnly := ctx.FormBool("file-only") + pathParam := ctx.PathParam("*") + baseRepo := ctx.Repo.Repository - infoPath = ctx.PathParam("*") - var infos []string - if infoPath == "" { - infos = []string{baseRepo.DefaultBranch, baseRepo.DefaultBranch} - } else { - infos = strings.SplitN(infoPath, "...", 2) - if len(infos) != 2 { - if infos = strings.SplitN(infoPath, "..", 2); len(infos) == 2 { - ci.DirectComparison = true - ctx.Data["PageIsComparePull"] = false - } else { - infos = []string{baseRepo.DefaultBranch, infoPath} - } + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) + if err != nil { + switch { + case user_model.IsErrUserNotExist(err): + ctx.NotFound("GetUserByName", nil) + case repo_model.IsErrRepoNotExist(err): + ctx.NotFound("GetRepositoryByOwnerAndName", nil) + case errors.Is(err, util.ErrInvalidArgument): + ctx.NotFound("ParseComparePathParams", nil) + default: + ctx.ServerError("GetRepositoryByOwnerAndName", err) } + return nil } + defer ci.Close() - ctx.Data["BaseName"] = baseRepo.OwnerName - ci.BaseBranch = infos[0] - ctx.Data["BaseBranch"] = ci.BaseBranch - - // If there is no head repository, it means compare between same repository. - headInfos := strings.Split(infos[1], ":") - if len(headInfos) == 1 { - isSameRepo = true - ci.HeadUser = ctx.Repo.Owner - ci.HeadBranch = headInfos[0] - } else if len(headInfos) == 2 { - headInfosSplit := strings.Split(headInfos[0], "/") - if len(headInfosSplit) == 1 { - ci.HeadUser, err = user_model.GetUserByName(ctx, headInfos[0]) - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) - } - return nil - } - ci.HeadBranch = headInfos[1] - isSameRepo = ci.HeadUser.ID == ctx.Repo.Owner.ID - if isSameRepo { - ci.HeadRepo = baseRepo - } - } else { - ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, headInfosSplit[0], headInfosSplit[1]) - if err != nil { - if repo_model.IsErrRepoNotExist(err) { - ctx.NotFound("GetRepositoryByOwnerAndName", nil) - } else { - ctx.ServerError("GetRepositoryByOwnerAndName", err) - } - return nil - } - if err := ci.HeadRepo.LoadOwner(ctx); err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) - } - return nil - } - ci.HeadBranch = headInfos[1] - ci.HeadUser = ci.HeadRepo.Owner - isSameRepo = ci.HeadRepo.ID == ctx.Repo.Repository.ID - } - } else { - ctx.NotFound("CompareAndPullRequest", nil) + // remove the check when we support compare with carets + if ci.CaretTimes > 0 { + ctx.NotFound("Unsupported compare", nil) return nil } - ctx.Data["HeadUser"] = ci.HeadUser - ctx.Data["HeadBranch"] = ci.HeadBranch - ctx.Repo.PullRequest.SameRepo = isSameRepo - - // Check if base branch is valid. - baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(ci.BaseBranch) - baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(ci.BaseBranch) - baseIsTag := ctx.Repo.GitRepo.IsTagExist(ci.BaseBranch) - - if !baseIsCommit && !baseIsBranch && !baseIsTag { - // Check if baseBranch is short sha commit hash - if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(ci.BaseBranch); baseCommit != nil { - ci.BaseBranch = baseCommit.ID.String() - ctx.Data["BaseBranch"] = ci.BaseBranch - baseIsCommit = true - } else if ci.BaseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() { - if isSameRepo { - ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadBranch)) - } else { - ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadBranch)) - } - return nil + + if ci.BaseOriRef == ctx.Repo.GetObjectFormat().EmptyObjectID().String() { + if ci.IsSameRepo() { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadOriRef)) } else { - ctx.NotFound("IsRefExist", nil) - return nil + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadOriRef)) } + return nil } - ctx.Data["BaseIsCommit"] = baseIsCommit - ctx.Data["BaseIsBranch"] = baseIsBranch - ctx.Data["BaseIsTag"] = baseIsTag + + ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) + ctx.Data["BaseName"] = baseRepo.OwnerName + ctx.Data["BaseBranch"] = ci.BaseOriRef + ctx.Data["HeadUser"] = ci.HeadUser + ctx.Data["HeadBranch"] = ci.HeadOriRef + ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() + + ctx.Data["BaseIsCommit"] = ci.IsBaseCommit + ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() + ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() ctx.Data["IsPull"] = true // Now we have the repository that represents the base @@ -391,50 +327,15 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { } // 7. Otherwise if we're not the same repo and haven't found a repo give up - if !isSameRepo && !has { + if !ci.IsSameRepo() && !has { ctx.Data["PageIsComparePull"] = false } - // 8. Finally open the git repo - if isSameRepo { - ci.HeadRepo = ctx.Repo.Repository - ci.HeadGitRepo = ctx.Repo.GitRepo - } else if has { - ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) - if err != nil { - ctx.ServerError("OpenRepository", err) - return nil - } - defer ci.HeadGitRepo.Close() - } else { - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - ctx.Data["HeadRepo"] = ci.HeadRepo ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository - // Now we need to assert that the ctx.Doer has permission to read - // the baseRepo's code and pulls - // (NOT headRepo's) - permBase, err := access_model.GetUserRepoPermission(ctx, baseRepo, ctx.Doer) - if err != nil { - ctx.ServerError("GetUserRepoPermission", err) - return nil - } - if !permBase.CanRead(unit.TypeCode) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in baseRepo has Permissions: %-+v", - ctx.Doer, - baseRepo, - permBase) - } - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - // If we're not merging from the same repo: - if !isSameRepo { + if !ci.IsSameRepo() { // Assert ctx.Doer has permission to read headRepo's codes permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) if err != nil { @@ -501,60 +402,16 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { } } - // Check if head branch is valid. - headIsCommit := ci.HeadGitRepo.IsCommitExist(ci.HeadBranch) - headIsBranch := ci.HeadGitRepo.IsBranchExist(ci.HeadBranch) - headIsTag := ci.HeadGitRepo.IsTagExist(ci.HeadBranch) - if !headIsCommit && !headIsBranch && !headIsTag { - // Check if headBranch is short sha commit hash - if headCommit, _ := ci.HeadGitRepo.GetCommit(ci.HeadBranch); headCommit != nil { - ci.HeadBranch = headCommit.ID.String() - ctx.Data["HeadBranch"] = ci.HeadBranch - headIsCommit = true - } else { - ctx.NotFound("IsRefExist", nil) - return nil - } - } - ctx.Data["HeadIsCommit"] = headIsCommit - ctx.Data["HeadIsBranch"] = headIsBranch - ctx.Data["HeadIsTag"] = headIsTag - - // Treat as pull request if both references are branches - if ctx.Data["PageIsComparePull"] == nil { - ctx.Data["PageIsComparePull"] = headIsBranch && baseIsBranch - } - - if ctx.Data["PageIsComparePull"] == true && !permBase.CanReadIssuesOrPulls(true) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot create/read pull requests in Repo: %-v\nUser in baseRepo has Permissions: %-+v", - ctx.Doer, - baseRepo, - permBase) - } - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - - baseBranchRef := ci.BaseBranch - if baseIsBranch { - baseBranchRef = git.BranchPrefix + ci.BaseBranch - } else if baseIsTag { - baseBranchRef = git.TagPrefix + ci.BaseBranch - } - headBranchRef := ci.HeadBranch - if headIsBranch { - headBranchRef = git.BranchPrefix + ci.HeadBranch - } else if headIsTag { - headBranchRef = git.TagPrefix + ci.HeadBranch - } + ctx.Data["HeadIsCommit"] = ci.IsHeadCommit + ctx.Data["HeadIsBranch"] = ci.HeadFullRef.IsBranch() + ctx.Data["HeadIsTag"] = ci.HeadFullRef.IsTag() - ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef, ci.DirectComparison, fileOnly) + ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(baseRepo.RepoPath(), ci.BaseFullRef.String(), ci.HeadFullRef.String(), ci.DirectComparison(), fileOnly) if err != nil { ctx.ServerError("GetCompareInfo", err) return nil } - if ci.DirectComparison { + if ci.DirectComparison() { ctx.Data["BeforeCommitID"] = ci.CompareInfo.BaseCommitID } else { ctx.Data["BeforeCommitID"] = ci.CompareInfo.MergeBase @@ -582,14 +439,14 @@ func PrepareCompareDiff( ctx.Data["AfterCommitID"] = headCommitID - if (headCommitID == ci.CompareInfo.MergeBase && !ci.DirectComparison) || + if (headCommitID == ci.CompareInfo.MergeBase && !ci.DirectComparison()) || headCommitID == ci.CompareInfo.BaseCommitID { ctx.Data["IsNothingToCompare"] = true if unit, err := repo.GetUnit(ctx, unit.TypePullRequests); err == nil { config := unit.PullRequestsConfig() if !config.AutodetectManualMerge { - allowEmptyPr := !(ci.BaseBranch == ci.HeadBranch && ctx.Repo.Repository.Name == ci.HeadRepo.Name) + allowEmptyPr := !(ci.BaseOriRef == ci.HeadOriRef && ctx.Repo.Repository.Name == ci.HeadRepo.Name) ctx.Data["AllowEmptyPr"] = allowEmptyPr return !allowEmptyPr @@ -601,7 +458,7 @@ func PrepareCompareDiff( } beforeCommitID := ci.CompareInfo.MergeBase - if ci.DirectComparison { + if ci.DirectComparison() { beforeCommitID = ci.CompareInfo.BaseCommitID } @@ -622,7 +479,7 @@ func PrepareCompareDiff( MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, MaxFiles: maxFiles, WhitespaceBehavior: whitespaceBehavior, - DirectComparison: ci.DirectComparison, + DirectComparison: ci.DirectComparison(), FileOnly: fileOnly, }, ctx.FormStrings("files")...) if err != nil { @@ -659,7 +516,7 @@ func PrepareCompareDiff( ctx.Data["content"] = strings.Join(body[1:], "\n") } } else { - title = ci.HeadBranch + title = ci.HeadOriRef } if len(title) > 255 { var trailer string @@ -720,7 +577,7 @@ func CompareDiff(ctx *context.Context) { ctx.Data["DirectComparison"] = ci.DirectComparison ctx.Data["OtherCompareSeparator"] = ".." ctx.Data["CompareSeparator"] = "..." - if ci.DirectComparison { + if ci.DirectComparison() { ctx.Data["CompareSeparator"] = ".." ctx.Data["OtherCompareSeparator"] = "..." } @@ -769,7 +626,7 @@ func CompareDiff(ctx *context.Context) { ctx.Data["HeadTags"] = headTags if ctx.Data["PageIsComparePull"] == true { - pr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadBranch, ci.BaseBranch, issues_model.PullRequestFlowGithub) + pr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { ctx.ServerError("GetUnmergedPullRequest", err) @@ -802,7 +659,7 @@ func CompareDiff(ctx *context.Context) { afterCommitID := ctx.Data["AfterCommitID"].(string) separator := "..." - if ci.DirectComparison { + if ci.DirectComparison() { separator = ".." } ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + separator + base.ShortSha(afterCommitID) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 9694ae845b1b9..fe0469e95ee55 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1315,8 +1315,8 @@ func CompareAndPullRequestPost(ctx *context.Context) { pullRequest := &issues_model.PullRequest{ HeadRepoID: ci.HeadRepo.ID, BaseRepoID: repo.ID, - HeadBranch: ci.HeadBranch, - BaseBranch: ci.BaseBranch, + HeadBranch: ci.HeadOriRef, + BaseBranch: ci.BaseOriRef, HeadRepo: ci.HeadRepo, BaseRepo: repo, MergeBase: ci.CompareInfo.MergeBase, From 4ee5334c109b605e7a9f5d66fac2e271143d829b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 11 Dec 2024 00:23:12 -0800 Subject: [PATCH 05/28] Refactor all compare routers --- models/repo/fork.go | 29 ++---- models/repo/fork_test.go | 4 +- routers/api/v1/repo/compare.go | 19 ---- routers/common/compare.go | 97 +++++++++++++++++-- routers/common/compare_test.go | 18 ++++ routers/web/repo/compare.go | 168 +++------------------------------ routers/web/repo/fork.go | 6 +- services/repository/fork.go | 4 +- 8 files changed, 141 insertions(+), 204 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index 1c75e86458b2f..b69c539ccce03 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/compare.go b/routers/api/v1/repo/compare.go index 6c4d428f22890..56e00db94304f 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -105,25 +105,6 @@ func CompareDiff(ctx *context.APIContext) { ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() log.Trace("Repo path: %q, base branch: %q, head branch: %q", ctx.Repo.GitRepo.Path, ci.BaseOriRef, ci.HeadOriRef) - // Check if current user has fork of repository or in the same repository. - /*headRepo := repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, ctx.Repo.Repository.ID) - if headRepo == nil && !ci.IsSameRepo() { - err := ctx.Repo.Repository.GetBaseRepo(ctx) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) - return nil, nil, nil, "", "" - } - - // Check if baseRepo's base repository is the same as headUser's repository. - if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID { - log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID) - ctx.NotFound("GetBaseRepo") - return nil, nil, nil, "", "" - } - // Assign headRepo so it can be used below. - headRepo = baseRepo.BaseRepo - }*/ - ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), ci.BaseOriRef, ci.HeadOriRef, false, false) if err != nil { ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) diff --git a/routers/common/compare.go b/routers/common/compare.go index d1b4327a1c7dd..0c9d7295fb2d5 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -53,7 +53,7 @@ func parseHead(head string) (string, string, string) { } ownerRepo := strings.SplitN(paths[0], "/", 2) if len(ownerRepo) == 1 { - return "", paths[0], paths[1] + return paths[0], "", paths[1] } return ownerRepo[0], ownerRepo[1], paths[1] } @@ -73,6 +73,7 @@ func parseCompareRouter(router string) (*CompareRouter, error) { HeadOriRef: headRef, HeadOwnerName: headOwnerName, HeadRepoName: headRepoName, + DotTimes: dotTimes, }, nil } else if len(parts) > 2 { return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) @@ -120,10 +121,10 @@ func (ci *CompareInfo) Close() { func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, oriRef string) (git.RefName, bool, error) { b, err := git_model.GetBranch(ctx, repoID, oriRef) - if err != nil { + if err != nil && !git_model.IsErrBranchNotExist(err) { return "", false, err } - if !b.IsDeleted { + if b != nil && !b.IsDeleted { return git.RefNameFromBranch(oriRef), false, nil } @@ -142,6 +143,78 @@ func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, o return git.RefName(commitObjectID.String()), true, nil } +func findHeadRepo(ctx context.Context, baseRepo *repo_model.Repository, headUserID int64) (*repo_model.Repository, error) { + if baseRepo.IsFork { + curRepo := baseRepo + for curRepo.OwnerID != headUserID { // We assume the fork deepth is not too deep. + if err := curRepo.GetBaseRepo(ctx); err != nil { + return nil, err + } + if curRepo.BaseRepo == nil { + return findHeadRepoFromRootBase(ctx, curRepo, headUserID, 3) + } + curRepo = curRepo.BaseRepo + } + return curRepo, nil + } + + return findHeadRepoFromRootBase(ctx, baseRepo, headUserID, 3) +} + +func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Repository, headUserID int64, traverseLevel int) (*repo_model.Repository, error) { + if traverseLevel == 0 { + return nil, nil + } + // test if we are lucky + repo, err := repo_model.GetForkedRepo(ctx, headUserID, baseRepo.ID) + if err == nil { + return repo, nil + } + if !repo_model.IsErrRepoNotExist(err) { + return nil, err + } + + firstLevelForkedRepo, err := repo_model.GetRepositoriesByForkID(ctx, baseRepo.ID) + if err != nil { + return nil, err + } + for _, repo := range firstLevelForkedRepo { + forked, err := findHeadRepoFromRootBase(ctx, repo, headUserID, traverseLevel-1) + if err != nil { + return nil, err + } + if forked != nil { + return forked, nil + } + } + return nil, nil +} + +// ParseComparePathParams Get compare information +// A full compare url is of the form: +// +// 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch} +// 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch} +// 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch} +// 4. /{:baseOwner}/{:baseRepoName}/compare/{:headBranch} +// 5. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}:{:headBranch} +// 6. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}/{:headRepoName}:{:headBranch} +// +// Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.PathParam("*") +// with the :baseRepo in ctx.Repo. +// +// Note: Generally :headRepoName is not provided here - we are only passed :headOwner. +// +// How do we determine the :headRepo? +// +// 1. If :headOwner is not set then the :headRepo = :baseRepo +// 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner +// 3. But... :baseRepo could be a fork of :headOwner's repo - so check that +// 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that +// +// format: ...[:] +// base<-head: master...head:feature +// same repo: master...feature func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { ci := &CompareInfo{} var err error @@ -159,12 +232,18 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep } if ci.IsSameRepo() { + ci.HeadOwnerName = baseRepo.Owner.Name + ci.HeadRepoName = baseRepo.Name ci.HeadUser = baseRepo.Owner ci.HeadRepo = baseRepo ci.HeadGitRepo = baseGitRepo } else { if ci.HeadOwnerName == baseRepo.Owner.Name { ci.HeadUser = baseRepo.Owner + if ci.HeadRepoName == "" { + ci.HeadRepoName = baseRepo.Name + ci.HeadRepo = baseRepo + } } else { ci.HeadUser, err = user_model.GetUserByName(ctx, ci.HeadOwnerName) if err != nil { @@ -172,9 +251,15 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep } } - ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, ci.HeadOwnerName, ci.HeadRepoName) - if err != nil { - return nil, err + if ci.HeadRepo == nil { + if ci.HeadRepoName != "" { + ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, ci.HeadOwnerName, ci.HeadRepoName) + } else { + ci.HeadRepo, err = findHeadRepo(ctx, baseRepo, ci.HeadUser.ID) + } + if err != nil { + return nil, err + } } ci.HeadRepo.Owner = ci.HeadUser ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 2341f8079280b..aa3038940105f 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -52,6 +52,7 @@ func TestCompareRouters(t *testing.T) { router: "develop", compareRouter: &CompareRouter{ HeadOriRef: "develop", + DotTimes: 3, }, }, { @@ -60,6 +61,7 @@ func TestCompareRouters(t *testing.T) { HeadOwnerName: "lunny", HeadRepoName: "forked_repo", HeadOriRef: "develop", + DotTimes: 3, }, }, { @@ -101,6 +103,22 @@ func TestCompareRouters(t *testing.T) { DotTimes: 3, }, }, + { + router: "teabot-patch-1...v0.0.1", + compareRouter: &CompareRouter{ + BaseOriRef: "teabot-patch-1", + HeadOriRef: "v0.0.1", + DotTimes: 3, + }, + }, + { + router: "teabot:feature1", + compareRouter: &CompareRouter{ + HeadOwnerName: "teabot", + HeadOriRef: "feature1", + DotTimes: 3, + }, + }, { router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", compareRouter: &CompareRouter{ diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 4279cf9b3a2f2..a6ce48506b185 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -189,32 +189,6 @@ func setCsvCompareContext(ctx *context.Context) { // ParseCompareInfo parse compare info between two commit for preparing comparing references // Permission check for base repository's code read should be checked before invoking this function func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { - // Get compared branches information - // A full compare url is of the form: - // - // 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch} - // 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch} - // 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch} - // 4. /{:baseOwner}/{:baseRepoName}/compare/{:headBranch} - // 5. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}:{:headBranch} - // 6. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}/{:headRepoName}:{:headBranch} - // - // Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.PathParam("*") - // with the :baseRepo in ctx.Repo. - // - // Note: Generally :headRepoName is not provided here - we are only passed :headOwner. - // - // How do we determine the :headRepo? - // - // 1. If :headOwner is not set then the :headRepo = :baseRepo - // 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner - // 3. But... :baseRepo could be a fork of :headOwner's repo - so check that - // 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that - // - // format: ...[:] - // base<-head: master...head:feature - // same repo: master...feature - fileOnly := ctx.FormBool("file-only") pathParam := ctx.PathParam("*") baseRepo := ctx.Repo.Repository @@ -250,90 +224,6 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { return nil } - ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) - ctx.Data["BaseName"] = baseRepo.OwnerName - ctx.Data["BaseBranch"] = ci.BaseOriRef - ctx.Data["HeadUser"] = ci.HeadUser - ctx.Data["HeadBranch"] = ci.HeadOriRef - ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() - - ctx.Data["BaseIsCommit"] = ci.IsBaseCommit - ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() - ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() - ctx.Data["IsPull"] = true - - // Now we have the repository that represents the base - - // The current base and head repositories and branches may not - // actually be the intended branches that the user wants to - // create a pull-request from - but also determining the head - // repo is difficult. - - // We will want therefore to offer a few repositories to set as - // our base and head - - // 1. First if the baseRepo is a fork get the "RootRepo" it was - // forked from - var rootRepo *repo_model.Repository - if baseRepo.IsFork { - err = baseRepo.GetBaseRepo(ctx) - if err != nil { - if !repo_model.IsErrRepoNotExist(err) { - ctx.ServerError("Unable to find root repo", err) - return nil - } - } else { - rootRepo = baseRepo.BaseRepo - } - } - - // 2. Now if the current user is not the owner of the baseRepo, - // check if they have a fork of the base repo and offer that as - // "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) - if repo != nil { - ownForkRepo = repo - ctx.Data["OwnForkRepo"] = ownForkRepo - } - } - - has := ci.HeadRepo != nil - // 3. If the base is a forked from "RootRepo" and the owner of - // the "RootRepo" is the :headUser - set headRepo to that - if !has && rootRepo != nil && rootRepo.OwnerID == ci.HeadUser.ID { - ci.HeadRepo = rootRepo - has = true - } - - // 4. If the ctx.Doer has their own fork of the baseRepo and the headUser is the ctx.Doer - // set the headRepo to the ownFork - if !has && ownForkRepo != nil && ownForkRepo.OwnerID == ci.HeadUser.ID { - ci.HeadRepo = ownForkRepo - has = true - } - - // 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) - 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) - has = ci.HeadRepo != nil - } - - // 7. Otherwise if we're not the same repo and haven't found a repo give up - if !ci.IsSameRepo() && !has { - ctx.Data["PageIsComparePull"] = false - } - - ctx.Data["HeadRepo"] = ci.HeadRepo - ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository - // If we're not merging from the same repo: if !ci.IsSameRepo() { // Assert ctx.Doer has permission to read headRepo's codes @@ -355,53 +245,23 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { ctx.Data["CanWriteToHeadRepo"] = permHead.CanWrite(unit.TypeCode) } - // If we have a rootRepo and it's different from: - // 1. the computed base - // 2. the computed head - // then get the branches of it - if rootRepo != nil && - rootRepo.ID != ci.HeadRepo.ID && - rootRepo.ID != baseRepo.ID { - canRead := access_model.CheckRepoUnitUser(ctx, rootRepo, ctx.Doer, unit.TypeCode) - if canRead { - ctx.Data["RootRepo"] = rootRepo - if !fileOnly { - branches, tags, err := getBranchesAndTagsForRepo(ctx, rootRepo) - if err != nil { - ctx.ServerError("GetBranchesForRepo", err) - return nil - } + // TODO: prepareRepos, branches and tags for dropdowns - ctx.Data["RootRepoBranches"] = branches - ctx.Data["RootRepoTags"] = tags - } - } - } + ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) + ctx.Data["BaseName"] = baseRepo.OwnerName + ctx.Data["BaseBranch"] = ci.BaseOriRef + ctx.Data["HeadUser"] = ci.HeadUser + ctx.Data["HeadBranch"] = ci.HeadOriRef + ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() - // If we have a ownForkRepo and it's different from: - // 1. The computed base - // 2. The computed head - // 3. The rootRepo (if we have one) - // then get the branches from it. - if ownForkRepo != nil && - ownForkRepo.ID != ci.HeadRepo.ID && - ownForkRepo.ID != baseRepo.ID && - (rootRepo == nil || ownForkRepo.ID != rootRepo.ID) { - canRead := access_model.CheckRepoUnitUser(ctx, ownForkRepo, ctx.Doer, unit.TypeCode) - if canRead { - ctx.Data["OwnForkRepo"] = ownForkRepo - if !fileOnly { - branches, tags, err := getBranchesAndTagsForRepo(ctx, ownForkRepo) - if err != nil { - ctx.ServerError("GetBranchesForRepo", err) - return nil - } - ctx.Data["OwnForkRepoBranches"] = branches - ctx.Data["OwnForkRepoTags"] = tags - } - } - } + ctx.Data["BaseIsCommit"] = ci.IsBaseCommit + ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() + ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() + ctx.Data["IsPull"] = true + // ctx.Data["OwnForkRepo"] = ownForkRepo FIXME: This is not used + ctx.Data["HeadRepo"] = ci.HeadRepo + ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository ctx.Data["HeadIsCommit"] = ci.IsHeadCommit ctx.Data["HeadIsBranch"] = ci.HeadFullRef.IsBranch() ctx.Data["HeadIsTag"] = ci.HeadFullRef.IsTag() 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 743fa68010cd69f400f4b27cde18002342bb64fb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 11 Dec 2024 00:28:29 -0800 Subject: [PATCH 06/28] make the code simpler --- services/pull/patch.go | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index ef7c366f0627b..296a84bb429bd 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -29,17 +29,6 @@ import ( "github.com/gobwas/glob" ) -// getDiffOrPatch generates either diff or formatted patch data between given revisions -func getDiffOrPatch(repo *git.Repository, compareString string, w io.Writer, patch, binary bool) error { - if patch { - return repo.GetPatch(compareString, w) - } - if binary { - return repo.GetDiffBinary(compareString, w) - } - return repo.GetDiff(compareString, w) -} - // DownloadDiffOrPatch will write the patch for the pr to the writer func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io.Writer, patch, binary bool) error { if err := pr.LoadBaseRepo(ctx); err != nil { @@ -53,7 +42,17 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io } defer closer.Close() - if err := getDiffOrPatch(gitRepo, pr.MergeBase+"..."+pr.GetGitRefName(), w, patch, binary); err != nil { + compareString := pr.MergeBase + "..." + pr.GetGitRefName() + switch { + case patch: + err = gitRepo.GetPatch(compareString, w) + case binary: + err = gitRepo.GetDiffBinary(compareString, w) + default: + err = gitRepo.GetDiff(compareString, w) + } + + if err != nil { log.Error("unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) } From 52639ffaa9d78df339984346e2f290099ae9c068 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 11 Dec 2024 15:41:02 -0800 Subject: [PATCH 07/28] 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 fe23f89fbc61a..b4a48a3c8bfc9 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -419,7 +419,7 @@ func CreatePullRequest(ctx *context.APIContext) { } defer ci.Close() - if ci.IsPull() { + if !ci.IsPull() { ctx.Error(http.StatusUnprocessableEntity, "Bad base or head refs", "Only support branch to branch comparison") return } From a3eb356f7a67c5e27658f8f6e3f326164b6b4400 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 00:37:51 -0800 Subject: [PATCH 08/28] Add rootRepo and ownForkRepo --- routers/api/v1/repo/compare.go | 2 +- routers/api/v1/repo/pull.go | 2 +- routers/common/compare.go | 42 +++++++++++- routers/web/repo/compare.go | 114 ++++++++++++++++++++------------- routers/web/repo/pull.go | 4 +- 5 files changed, 116 insertions(+), 48 deletions(-) diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 56e00db94304f..dae9a19bd8585 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -61,7 +61,7 @@ func CompareDiff(ctx *context.APIContext) { pathParam := ctx.PathParam("*") baseRepo := ctx.Repo.Repository - ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo, ctx.Doer) if err != nil { switch { case user_model.IsErrUserNotExist(err): diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index b4a48a3c8bfc9..0893d53713c40 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -403,7 +403,7 @@ func CreatePullRequest(ctx *context.APIContext) { ) // Get repo/branch information - ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo) + ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo, ctx.Doer) if err != nil { switch { case user_model.IsErrUserNotExist(err): diff --git a/routers/common/compare.go b/routers/common/compare.go index 0c9d7295fb2d5..f7616166bff79 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -101,6 +101,8 @@ type CompareInfo struct { HeadUser *user_model.User HeadRepo *repo_model.Repository HeadGitRepo *git.Repository + RootRepo *repo_model.Repository + OwnForkRepo *repo_model.Repository CompareInfo *git.CompareInfo close func() IsBaseCommit bool @@ -190,6 +192,20 @@ func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Reposito return nil, nil } +func getRootRepo(ctx context.Context, repo *repo_model.Repository) (*repo_model.Repository, error) { + curRepo := repo + for curRepo.IsFork { + if err := curRepo.GetBaseRepo(ctx); err != nil { + return nil, err + } + if curRepo.BaseRepo == nil { + break + } + curRepo = curRepo.BaseRepo + } + return curRepo, nil +} + // ParseComparePathParams Get compare information // A full compare url is of the form: // @@ -215,7 +231,7 @@ func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Reposito // format: ...[:] // base<-head: master...head:feature // same repo: master...feature -func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { +func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository, doer *user_model.User) (*CompareInfo, error) { ci := &CompareInfo{} var err error @@ -273,13 +289,37 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep } } + // find root repo + if !baseRepo.IsFork { + ci.RootRepo = baseRepo + } else { + if !ci.HeadRepo.IsFork { + ci.RootRepo = ci.HeadRepo + } else { + ci.RootRepo, err = getRootRepo(ctx, baseRepo) + if err != nil { + return nil, err + } + } + } + + // find ownfork repo + if doer != nil && ci.HeadRepo.OwnerID != doer.ID && baseRepo.OwnerID != doer.ID { + ci.OwnForkRepo, err = findHeadRepo(ctx, baseRepo, doer.ID) + if err != nil { + return nil, err + } + } + ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef) if err != nil { + ci.Close() return nil, err } ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) if err != nil { + ci.Close() return nil, err } return ci, nil diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index a6ce48506b185..f4ca1ceb384ec 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -14,6 +14,7 @@ import ( "net/http" "net/url" "path/filepath" + "slices" "strings" "code.gitea.io/gitea/models/db" @@ -193,7 +194,7 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { pathParam := ctx.PathParam("*") baseRepo := ctx.Repo.Repository - ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo, ctx.Doer) if err != nil { switch { case user_model.IsErrUserNotExist(err): @@ -207,7 +208,6 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { } return nil } - defer ci.Close() // remove the check when we support compare with carets if ci.CaretTimes > 0 { @@ -245,8 +245,6 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { ctx.Data["CanWriteToHeadRepo"] = permHead.CanWrite(unit.TypeCode) } - // TODO: prepareRepos, branches and tags for dropdowns - ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) ctx.Data["BaseName"] = baseRepo.OwnerName ctx.Data["BaseBranch"] = ci.BaseOriRef @@ -258,7 +256,6 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() ctx.Data["IsPull"] = true - // ctx.Data["OwnForkRepo"] = ownForkRepo FIXME: This is not used ctx.Data["HeadRepo"] = ci.HeadRepo ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository @@ -399,14 +396,8 @@ func PrepareCompareDiff( return false } -func getBranchesAndTagsForRepo(ctx gocontext.Context, repo *repo_model.Repository) (branches, tags []string, err error) { - gitRepo, err := gitrepo.OpenRepository(ctx, repo) - if err != nil { - return nil, nil, err - } - defer gitRepo.Close() - - branches, err = git_model.FindBranchNames(ctx, git_model.FindBranchOptions{ +func getBranchesAndTagsForRepo(ctx gocontext.Context, repo *repo_model.Repository) ([]string, []string, error) { + branches, err := git_model.FindBranchNames(ctx, git_model.FindBranchOptions{ RepoID: repo.ID, ListOptions: db.ListOptionsAll, IsDeletedBranch: optional.Some(false), @@ -414,19 +405,82 @@ func getBranchesAndTagsForRepo(ctx gocontext.Context, repo *repo_model.Repositor if err != nil { return nil, nil, err } - tags, err = gitRepo.GetTags(0, 0) + // always put default branch on the top if it exists + if slices.Contains(branches, repo.DefaultBranch) { + branches = util.SliceRemoveAll(branches, repo.DefaultBranch) + branches = append([]string{repo.DefaultBranch}, branches...) + } + + tags, err := repo_model.GetTagNamesByRepoID(ctx, repo.ID) if err != nil { return nil, nil, err } + return branches, tags, nil } +func prepareCompareRepoBranchesTagsDropdowns(ctx *context.Context, ci *common.CompareInfo) { + baseRepo := ctx.Repo.Repository + // For compare repo branches + baseBranches, baseTags, err := getBranchesAndTagsForRepo(ctx, baseRepo) + if err != nil { + ctx.ServerError("getBranchesAndTagsForRepo", err) + return + } + + ctx.Data["Branches"] = baseBranches + ctx.Data["Tags"] = baseTags + + if ci.IsSameRepo() { + ctx.Data["HeadBranches"] = baseBranches + ctx.Data["HeadTags"] = baseTags + } else { + headBranches, headTags, err := getBranchesAndTagsForRepo(ctx, ci.HeadRepo) + if err != nil { + ctx.ServerError("getBranchesAndTagsForRepo", err) + return + } + ctx.Data["HeadBranches"] = headBranches + ctx.Data["HeadTags"] = headTags + } + + if ci.RootRepo != nil && + ci.RootRepo.ID != ci.HeadRepo.ID && + ci.RootRepo.ID != baseRepo.ID { + canRead := access_model.CheckRepoUnitUser(ctx, ci.RootRepo, ctx.Doer, unit.TypeCode) + if canRead { + ctx.Data["RootRepo"] = ci.RootRepo + branches, tags, err := getBranchesAndTagsForRepo(ctx, ci.RootRepo) + if err != nil { + ctx.ServerError("GetBranchesForRepo", err) + return + } + ctx.Data["RootRepoBranches"] = branches + ctx.Data["RootRepoTags"] = tags + } + } + + if ci.OwnForkRepo != nil && + ci.OwnForkRepo.ID != ci.HeadRepo.ID && + ci.OwnForkRepo.ID != baseRepo.ID && + (ci.RootRepo == nil || ci.OwnForkRepo.ID != ci.RootRepo.ID) { + ctx.Data["OwnForkRepo"] = ci.OwnForkRepo + branches, tags, err := getBranchesAndTagsForRepo(ctx, ci.OwnForkRepo) + if err != nil { + ctx.ServerError("GetBranchesForRepo", err) + return + } + ctx.Data["OwnForkRepoBranches"] = branches + ctx.Data["OwnForkRepoTags"] = tags + } +} + // CompareDiff show different from one commit to another commit func CompareDiff(ctx *context.Context) { ci := ParseCompareInfo(ctx) defer func() { - if ci != nil && ci.HeadGitRepo != nil { - ci.HeadGitRepo.Close() + if ci != nil { + ci.Close() } }() if ctx.Written() { @@ -448,43 +502,17 @@ func CompareDiff(ctx *context.Context) { return } - baseTags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) - if err != nil { - ctx.ServerError("GetTagNamesByRepoID", err) - return - } - ctx.Data["Tags"] = baseTags - fileOnly := ctx.FormBool("file-only") if fileOnly { ctx.HTML(http.StatusOK, tplDiffBox) return } - headBranches, err := git_model.FindBranchNames(ctx, git_model.FindBranchOptions{ - RepoID: ci.HeadRepo.ID, - ListOptions: db.ListOptionsAll, - IsDeletedBranch: optional.Some(false), - }) - if err != nil { - ctx.ServerError("GetBranches", err) - return - } - ctx.Data["HeadBranches"] = headBranches - - // For compare repo branches - PrepareBranchList(ctx) + prepareCompareRepoBranchesTagsDropdowns(ctx, ci) if ctx.Written() { return } - headTags, err := repo_model.GetTagNamesByRepoID(ctx, ci.HeadRepo.ID) - if err != nil { - ctx.ServerError("GetTagNamesByRepoID", err) - return - } - ctx.Data["HeadTags"] = headTags - if ctx.Data["PageIsComparePull"] == true { pr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index fe0469e95ee55..a3907e738da89 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1266,8 +1266,8 @@ func CompareAndPullRequestPost(ctx *context.Context) { ci := ParseCompareInfo(ctx) defer func() { - if ci != nil && ci.HeadGitRepo != nil { - ci.HeadGitRepo.Close() + if ci != nil { + ci.Close() } }() if ctx.Written() { From 743cebe66fe07111f389bfa2067e9e71994cbebc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 11:53:04 -0800 Subject: [PATCH 09/28] Some improvements --- routers/api/v1/repo/pull.go | 3 +++ routers/common/compare.go | 45 ++++++++++++++++++++----------------- routers/web/repo/compare.go | 30 +++++++++++++++---------- 3 files changed, 46 insertions(+), 32 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 57eb8ff999db9..324e7d3725937 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -423,6 +423,9 @@ func CreatePullRequest(ctx *context.APIContext) { return } + // we just need to check the head repository's permission here because the base + // repository's permission is already checked in api.go with + // mustAllowPulls, reqRepoReader(unit.TypeCode) if !ci.IsSameRepo() { // user should have permission to read headrepo's codes permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) diff --git a/routers/common/compare.go b/routers/common/compare.go index f7616166bff79..a3cd0887e1fc9 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -101,8 +101,6 @@ type CompareInfo struct { HeadUser *user_model.User HeadRepo *repo_model.Repository HeadGitRepo *git.Repository - RootRepo *repo_model.Repository - OwnForkRepo *repo_model.Repository CompareInfo *git.CompareInfo close func() IsBaseCommit bool @@ -289,38 +287,45 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep } } + ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef) + if err != nil { + ci.Close() + return nil, err + } + + ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) + if err != nil { + ci.Close() + return nil, err + } + return ci, nil +} + +func (ci *CompareInfo) LoadRootRepoAndOwnForkRepo(ctx context.Context, baseRepo *repo_model.Repository, doer *user_model.User) (*repo_model.Repository, *repo_model.Repository, error) { // find root repo + var rootRepo *repo_model.Repository + var err error if !baseRepo.IsFork { - ci.RootRepo = baseRepo + rootRepo = baseRepo } else { if !ci.HeadRepo.IsFork { - ci.RootRepo = ci.HeadRepo + rootRepo = ci.HeadRepo } else { - ci.RootRepo, err = getRootRepo(ctx, baseRepo) + rootRepo, err = getRootRepo(ctx, baseRepo) if err != nil { - return nil, err + return nil, nil, err } } } // find ownfork repo + var ownForkRepo *repo_model.Repository if doer != nil && ci.HeadRepo.OwnerID != doer.ID && baseRepo.OwnerID != doer.ID { - ci.OwnForkRepo, err = findHeadRepo(ctx, baseRepo, doer.ID) + ownForkRepo, err = findHeadRepo(ctx, baseRepo, doer.ID) if err != nil { - return nil, err + return nil, nil, err } } - ci.BaseFullRef, ci.IsBaseCommit, err = detectFullRef(ctx, baseRepo.ID, baseGitRepo, ci.BaseOriRef) - if err != nil { - ci.Close() - return nil, err - } - - ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) - if err != nil { - ci.Close() - return nil, err - } - return ci, nil + return rootRepo, ownForkRepo, nil } diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index f4ca1ceb384ec..99e10b412416b 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -444,13 +444,19 @@ func prepareCompareRepoBranchesTagsDropdowns(ctx *context.Context, ci *common.Co ctx.Data["HeadTags"] = headTags } - if ci.RootRepo != nil && - ci.RootRepo.ID != ci.HeadRepo.ID && - ci.RootRepo.ID != baseRepo.ID { - canRead := access_model.CheckRepoUnitUser(ctx, ci.RootRepo, ctx.Doer, unit.TypeCode) + rootRepo, ownForkRepo, err := ci.LoadRootRepoAndOwnForkRepo(ctx, baseRepo, ctx.Doer) + if err != nil { + ctx.ServerError("LoadRootRepoAndOwnForkRepo", err) + return + } + + if rootRepo != nil && + rootRepo.ID != ci.HeadRepo.ID && + rootRepo.ID != baseRepo.ID { + canRead := access_model.CheckRepoUnitUser(ctx, rootRepo, ctx.Doer, unit.TypeCode) if canRead { - ctx.Data["RootRepo"] = ci.RootRepo - branches, tags, err := getBranchesAndTagsForRepo(ctx, ci.RootRepo) + ctx.Data["RootRepo"] = rootRepo + branches, tags, err := getBranchesAndTagsForRepo(ctx, rootRepo) if err != nil { ctx.ServerError("GetBranchesForRepo", err) return @@ -460,12 +466,12 @@ func prepareCompareRepoBranchesTagsDropdowns(ctx *context.Context, ci *common.Co } } - if ci.OwnForkRepo != nil && - ci.OwnForkRepo.ID != ci.HeadRepo.ID && - ci.OwnForkRepo.ID != baseRepo.ID && - (ci.RootRepo == nil || ci.OwnForkRepo.ID != ci.RootRepo.ID) { - ctx.Data["OwnForkRepo"] = ci.OwnForkRepo - branches, tags, err := getBranchesAndTagsForRepo(ctx, ci.OwnForkRepo) + if ownForkRepo != nil && + ownForkRepo.ID != ci.HeadRepo.ID && + ownForkRepo.ID != baseRepo.ID && + (rootRepo == nil || ownForkRepo.ID != rootRepo.ID) { + ctx.Data["OwnForkRepo"] = ownForkRepo + branches, tags, err := getBranchesAndTagsForRepo(ctx, ownForkRepo) if err != nil { ctx.ServerError("GetBranchesForRepo", err) return From 050fad97d6c5ddcfda478a732895175459fda5b3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 12:21:40 -0800 Subject: [PATCH 10/28] revert change to fork --- models/repo/fork.go | 29 ++++++++++++++++++++--------- models/repo/fork_test.go | 4 ++-- routers/common/compare.go | 12 +++++++----- routers/web/repo/fork.go | 6 +----- services/repository/fork.go | 4 ++-- 5 files changed, 32 insertions(+), 23 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index b69c539ccce03..1c75e86458b2f 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -21,17 +21,15 @@ 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, error) { +func GetForkedRepo(ctx context.Context, ownerID, repoID int64) *Repository { repo := new(Repository) - has, err := db.GetEngine(ctx). + has, _ := db.GetEngine(ctx). Where("owner_id=? AND fork_id=?", ownerID, repoID). Get(repo) - if err != nil { - return nil, err - } else if !has { - return nil, ErrRepoNotExist{ID: repoID} + if has { + return repo } - return repo, nil + return nil } // HasForkedRepo checks if given user has already forked a repository with given ID. @@ -43,6 +41,19 @@ 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) @@ -76,8 +87,8 @@ 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) - if err != nil && !IsErrRepoNotExist(err) { + forkedRepo, err := GetUserFork(ctx, repo.ID, user.ID) + if err != nil { return repoList, err } if forkedRepo != nil { diff --git a/models/repo/fork_test.go b/models/repo/fork_test.go index 7a15874329e8c..e8dca204cc457 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.GetForkedRepo(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetUserFork(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.GetForkedRepo(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetUserFork(db.DefaultContext, repo.ID, 13) assert.NoError(t, err) assert.Nil(t, repo) } diff --git a/routers/common/compare.go b/routers/common/compare.go index a3cd0887e1fc9..2f5edf635d1c3 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -119,6 +119,8 @@ func (ci *CompareInfo) Close() { } } +// detectFullRef detects a short name as a branch, tag or commit's full ref name and type. +// It's the same job as git.UnstableGuessRefByShortName but with a database read instead of git read. func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, oriRef string) (git.RefName, bool, error) { b, err := git_model.GetBranch(ctx, repoID, oriRef) if err != nil && !git_model.IsErrBranchNotExist(err) { @@ -166,13 +168,13 @@ func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Reposito return nil, nil } // test if we are lucky - repo, err := repo_model.GetForkedRepo(ctx, headUserID, baseRepo.ID) - if err == nil { - return repo, nil - } - if !repo_model.IsErrRepoNotExist(err) { + repo, err := repo_model.GetUserFork(ctx, headUserID, baseRepo.ID) + if err != nil { return nil, err } + if repo != nil { + return repo, nil + } firstLevelForkedRepo, err := repo_model.GetRepositoriesByForkID(ctx, baseRepo.ID) if err != nil { diff --git a/routers/web/repo/fork.go b/routers/web/repo/fork.go index c0d823c079c25..27e42a8f98e44 100644 --- a/routers/web/repo/fork.go +++ b/routers/web/repo/fork.go @@ -166,11 +166,7 @@ func ForkPost(ctx *context.Context) { ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_has_same_repo"), tplFork, &form) return } - repo, err := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) - if err != nil && !repo_model.IsErrRepoNotExist(err) { - ctx.ServerError("GetForkedRepo", err) - return - } + repo := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) 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 26057266c5f18..bc4fdf85627b0 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.GetForkedRepo(ctx, opts.BaseRepo.ID, owner.ID) - if err != nil && !repo_model.IsErrRepoNotExist(err) { + forkedRepo, err := repo_model.GetUserFork(ctx, opts.BaseRepo.ID, owner.ID) + if err != nil { return nil, err } if forkedRepo != nil { From 95bccdbc3b8946f50bafa35c6c2910919d6e91eb Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 15:08:58 -0800 Subject: [PATCH 11/28] remove unnecessary parameter --- routers/api/v1/repo/compare.go | 2 +- routers/api/v1/repo/pull.go | 2 +- routers/common/compare.go | 2 +- routers/web/repo/compare.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index dae9a19bd8585..56e00db94304f 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -61,7 +61,7 @@ func CompareDiff(ctx *context.APIContext) { pathParam := ctx.PathParam("*") baseRepo := ctx.Repo.Repository - ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo, ctx.Doer) + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) if err != nil { switch { case user_model.IsErrUserNotExist(err): diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 324e7d3725937..ffd651ec0b02b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -402,7 +402,7 @@ func CreatePullRequest(ctx *context.APIContext) { ) // Get repo/branch information - ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo, ctx.Doer) + ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo) if err != nil { switch { case user_model.IsErrUserNotExist(err): diff --git a/routers/common/compare.go b/routers/common/compare.go index 2f5edf635d1c3..1bbd9f35fdcd0 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -231,7 +231,7 @@ func getRootRepo(ctx context.Context, repo *repo_model.Repository) (*repo_model. // format: ...[:] // base<-head: master...head:feature // same repo: master...feature -func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository, doer *user_model.User) (*CompareInfo, error) { +func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { ci := &CompareInfo{} var err error diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 99e10b412416b..2bdc6dcc82770 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -194,7 +194,7 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { pathParam := ctx.PathParam("*") baseRepo := ctx.Repo.Repository - ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo, ctx.Doer) + ci, err := common.ParseComparePathParams(ctx, pathParam, baseRepo, ctx.Repo.GitRepo) if err != nil { switch { case user_model.IsErrUserNotExist(err): From 91147db3bba2a4b7edc96cead21deda352ff8f7f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 15:25:43 -0800 Subject: [PATCH 12/28] Fix test --- modules/git/repo_compare_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index a5d113b3645f7..d4597bd948682 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -53,7 +53,7 @@ func TestGetFormatPatch(t *testing.T) { defer repo.Close() rd := &bytes.Buffer{} - err = repo.GetPatch("8d92fc95^ 8d92fc95", rd) + err = repo.GetPatch("8d92fc95^...8d92fc95", rd) if err != nil { assert.NoError(t, err) return From 92e68ae4075005b52c7c47810d7a9dec211058b6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 12 Dec 2024 15:35:54 -0800 Subject: [PATCH 13/28] Revert changes related to getpatch/getdiff because they are extracted to another PR --- modules/git/repo_compare.go | 90 ++++++++++++++++++++------------ modules/git/repo_compare_test.go | 27 +--------- services/pull/patch.go | 18 ++----- 3 files changed, 62 insertions(+), 73 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 5d7fbe6df64c6..16fcdcf4c8f96 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -233,63 +233,72 @@ func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int, return numFiles, totalAdditions, totalDeletions, err } -func parseCompareArgs(compareArgs string) (args []string) { - parts := strings.Split(compareArgs, "...") - if len(parts) == 2 { - return []string{compareArgs} +// GetDiffOrPatch generates either diff or formatted patch data between given revisions +func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, patch, binary bool) error { + if patch { + return repo.GetPatch(base, head, w) } - parts = strings.Split(compareArgs, "..") - if len(parts) == 2 { - return parts + if binary { + return repo.GetDiffBinary(base, head, w) } - parts = strings.Fields(compareArgs) - if len(parts) == 2 { - return parts - } - - return nil + return repo.GetDiff(base, head, w) } // GetDiff generates and returns patch data between given revisions, optimized for human readability -func (repo *Repository) GetDiff(compareArgs string, w io.Writer) error { - args := parseCompareArgs(compareArgs) - if len(args) == 0 { - return fmt.Errorf("invalid compareArgs: %s", compareArgs) - } +func (repo *Repository) GetDiff(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) - return NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(args...). + err := NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(base + "..." + head). Run(&RunOpts{ Dir: repo.Path, Stdout: w, Stderr: stderr, }) + if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { + return NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(base, head). + Run(&RunOpts{ + Dir: repo.Path, + Stdout: w, + }) + } + return err } // GetDiffBinary generates and returns patch data between given revisions, including binary diffs. -func (repo *Repository) GetDiffBinary(compareArgs string, w io.Writer) error { - args := parseCompareArgs(compareArgs) - if len(args) == 0 { - return fmt.Errorf("invalid compareArgs: %s", compareArgs) +func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error { + stderr := new(bytes.Buffer) + err := NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(base + "..." + head). + Run(&RunOpts{ + Dir: repo.Path, + Stdout: w, + Stderr: stderr, + }) + if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { + return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(base, head). + Run(&RunOpts{ + Dir: repo.Path, + Stdout: w, + }) } - return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(args...).Run(&RunOpts{ - Dir: repo.Path, - Stdout: w, - }) + return err } // GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply` -func (repo *Repository) GetPatch(compareArgs string, w io.Writer) error { - args := parseCompareArgs(compareArgs) - if len(args) == 0 { - return fmt.Errorf("invalid compareArgs: %s", compareArgs) - } +func (repo *Repository) GetPatch(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) - return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(args...). + err := NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(base + "..." + head). Run(&RunOpts{ Dir: repo.Path, Stdout: w, Stderr: stderr, }) + if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { + return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(base, head). + Run(&RunOpts{ + Dir: repo.Path, + Stdout: w, + }) + } + return err } // GetFilesChangedBetween returns a list of all files that have been changed between the given commits @@ -320,6 +329,21 @@ func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, err return split, err } +// GetDiffFromMergeBase generates and return patch data from merge base to head +func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error { + stderr := new(bytes.Buffer) + err := NewCommand(repo.Ctx, "diff", "-p", "--binary").AddDynamicArguments(base + "..." + head). + Run(&RunOpts{ + Dir: repo.Path, + Stdout: w, + Stderr: stderr, + }) + if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { + return repo.GetDiffBinary(base, head, w) + } + return err +} + // ReadPatchCommit will check if a diff patch exists and return stats func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error) { // Migrated repositories download patches to "pulls" location diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index d4597bd948682..9983873186720 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -12,31 +12,6 @@ import ( "github.com/stretchr/testify/assert" ) -func Test_parseCompareArgs(t *testing.T) { - testCases := []struct { - compareString string - expected []string - }{ - { - "master..develop", - []string{"master", "develop"}, - }, - { - "master HEAD", - []string{"master", "HEAD"}, - }, - { - "HEAD...develop", - []string{"HEAD...develop"}, - }, - } - - for _, tc := range testCases { - args := parseCompareArgs(tc.compareString) - assert.Equal(t, tc.expected, args) - } -} - func TestGetFormatPatch(t *testing.T) { bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") clonedPath, err := cloneRepo(t, bareRepo1Path) @@ -53,7 +28,7 @@ func TestGetFormatPatch(t *testing.T) { defer repo.Close() rd := &bytes.Buffer{} - err = repo.GetPatch("8d92fc95^...8d92fc95", rd) + err = repo.GetPatch("8d92fc95^", "8d92fc95", rd) if err != nil { assert.NoError(t, err) return diff --git a/services/pull/patch.go b/services/pull/patch.go index 296a84bb429bd..0934a86c89a65 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -42,19 +42,9 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io } defer closer.Close() - compareString := pr.MergeBase + "..." + pr.GetGitRefName() - switch { - case patch: - err = gitRepo.GetPatch(compareString, w) - case binary: - err = gitRepo.GetDiffBinary(compareString, w) - default: - err = gitRepo.GetDiff(compareString, w) - } - - if err != nil { - log.Error("unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) - return fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) + if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch, binary); err != nil { + log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) + return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) } return nil } @@ -365,7 +355,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * _ = util.Remove(tmpPatchFile.Name()) }() - if err := gitRepo.GetDiffBinary(pr.MergeBase+"...tracking", tmpPatchFile); err != nil { + if err := gitRepo.GetDiffBinary(pr.MergeBase, "tracking", tmpPatchFile); err != nil { tmpPatchFile.Close() log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return false, fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) From b7f2be5c4d3a0300043abe2170c7f7efecaef1cc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 20 Dec 2024 11:13:28 -0800 Subject: [PATCH 14/28] Fix possible bug --- routers/api/v1/repo/pull.go | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index ffd651ec0b02b..cfd84fd5c3e16 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -396,13 +396,20 @@ func CreatePullRequest(ctx *context.APIContext) { } var ( - repo = ctx.Repo.Repository + baseRepo = ctx.Repo.Repository labelIDs []int64 milestoneID int64 ) + baseGitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, baseRepo) + if err != nil { + ctx.ServerError("OpenRepository", err) + return + } + defer closer.Close() + // Get repo/branch information - ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, repo, ctx.Repo.GitRepo) + ci, err := common.ParseComparePathParams(ctx, form.Base+"..."+form.Head, baseRepo, baseGitRepo) if err != nil { switch { case user_model.IsErrUserNotExist(err): @@ -446,7 +453,7 @@ func CreatePullRequest(ctx *context.APIContext) { } // Check if another PR exists with the same targets - existingPr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, ctx.Repo.Repository.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) + existingPr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, baseRepo.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { ctx.Error(http.StatusInternalServerError, "GetUnmergedPullRequest", err) @@ -466,7 +473,7 @@ func CreatePullRequest(ctx *context.APIContext) { } if len(form.Labels) > 0 { - labels, err := issues_model.GetLabelsInRepoByIDs(ctx, ctx.Repo.Repository.ID, form.Labels) + labels, err := issues_model.GetLabelsInRepoByIDs(ctx, baseRepo.ID, form.Labels) if err != nil { ctx.Error(http.StatusInternalServerError, "GetLabelsInRepoByIDs", err) return @@ -493,7 +500,7 @@ func CreatePullRequest(ctx *context.APIContext) { } if form.Milestone > 0 { - milestone, err := issues_model.GetMilestoneByRepoID(ctx, ctx.Repo.Repository.ID, form.Milestone) + milestone, err := issues_model.GetMilestoneByRepoID(ctx, baseRepo.ID, form.Milestone) if err != nil { if issues_model.IsErrMilestoneNotExist(err) { ctx.NotFound() @@ -512,7 +519,7 @@ func CreatePullRequest(ctx *context.APIContext) { } prIssue := &issues_model.Issue{ - RepoID: repo.ID, + RepoID: baseRepo.ID, Title: form.Title, PosterID: ctx.Doer.ID, Poster: ctx.Doer, @@ -523,11 +530,11 @@ func CreatePullRequest(ctx *context.APIContext) { } pr := &issues_model.PullRequest{ HeadRepoID: ci.HeadRepo.ID, - BaseRepoID: repo.ID, + BaseRepoID: baseRepo.ID, HeadBranch: ci.HeadOriRef, BaseBranch: ci.BaseOriRef, HeadRepo: ci.HeadRepo, - BaseRepo: repo, + BaseRepo: baseRepo, MergeBase: ci.CompareInfo.MergeBase, Type: issues_model.PullRequestGitea, } @@ -550,19 +557,19 @@ func CreatePullRequest(ctx *context.APIContext) { return } - valid, err := access_model.CanBeAssigned(ctx, assignee, repo, true) + valid, err := access_model.CanBeAssigned(ctx, assignee, baseRepo, true) if err != nil { ctx.Error(http.StatusInternalServerError, "canBeAssigned", err) return } if !valid { - ctx.Error(http.StatusUnprocessableEntity, "canBeAssigned", repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name}) + ctx.Error(http.StatusUnprocessableEntity, "canBeAssigned", repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: baseRepo.Name}) return } } prOpts := &pull_service.NewPullRequestOptions{ - Repo: repo, + Repo: baseRepo, Issue: prIssue, LabelIDs: labelIDs, PullRequest: pr, @@ -586,7 +593,7 @@ func CreatePullRequest(ctx *context.APIContext) { return } - log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID) + log.Trace("Pull request created: %d/%d", baseRepo.ID, prIssue.ID) ctx.JSON(http.StatusCreated, convert.ToAPIPullRequest(ctx, pr, ctx.Doer)) } From 831fb5641477424eb9423197f1dd2dc0537b6e3d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 23 Dec 2024 10:43:49 -0800 Subject: [PATCH 15/28] Add more test --- routers/api/v1/repo/pull.go | 6 ++ routers/common/compare.go | 52 +++++---- routers/common/compare_test.go | 190 +++++++++++++++++++++++++++++++++ 3 files changed, 225 insertions(+), 23 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9971e4512fc6f..d7394c18b7102 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -451,6 +451,12 @@ func CreatePullRequest(ctx *context.APIContext) { } } + ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), ci.BaseOriRef, ci.HeadOriRef, false, false) + if err != nil { + ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) + return + } + // Check if another PR exists with the same targets existingPr, err := issues_model.GetUnmergedPullRequest(ctx, ci.HeadRepo.ID, baseRepo.ID, ci.HeadOriRef, ci.BaseOriRef, issues_model.PullRequestFlowGithub) if err != nil { diff --git a/routers/common/compare.go b/routers/common/compare.go index 1bbd9f35fdcd0..71ace773b30c3 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -26,14 +26,6 @@ type CompareRouter struct { DotTimes int // 2(..) or 3(...) } -func (cr *CompareRouter) IsSameRepo() bool { - return cr.HeadOwnerName == "" && cr.HeadRepoName == "" -} - -func (cr *CompareRouter) IsSameRef() bool { - return cr.IsSameRepo() && cr.BaseOriRef == cr.HeadOriRef -} - func (cr *CompareRouter) DirectComparison() bool { return cr.DotTimes == 2 } @@ -98,6 +90,7 @@ func parseCompareRouter(router string) (*CompareRouter, error) { // CompareInfo represents the collected results from ParseCompareInfo type CompareInfo struct { *CompareRouter + BaseRepo *repo_model.Repository HeadUser *user_model.User HeadRepo *repo_model.Repository HeadGitRepo *git.Repository @@ -107,10 +100,18 @@ type CompareInfo struct { IsHeadCommit bool } +func (cr *CompareInfo) IsSameRepo() bool { + return cr.HeadRepo.ID == cr.BaseRepo.ID +} + +func (cr *CompareInfo) IsSameRef() bool { + return cr.IsSameRepo() && cr.BaseOriRef == cr.HeadOriRef +} + // display pull related information or not func (ci *CompareInfo) IsPull() bool { return ci.CaretTimes == 0 && !ci.DirectComparison() && - ci.BaseFullRef.IsBranch() && ci.HeadFullRef.IsBranch() + ci.BaseFullRef.IsBranch() && (ci.HeadRepo == nil || ci.HeadFullRef.IsBranch()) } func (ci *CompareInfo) Close() { @@ -232,7 +233,7 @@ func getRootRepo(ctx context.Context, repo *repo_model.Repository) (*repo_model. // base<-head: master...head:feature // same repo: master...feature func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { - ci := &CompareInfo{} + ci := &CompareInfo{BaseRepo: baseRepo} var err error if pathParam == "" { @@ -247,7 +248,8 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep ci.BaseOriRef = baseRepo.DefaultBranch } - if ci.IsSameRepo() { + if (ci.HeadOwnerName == "" && ci.HeadRepoName == "") || + (ci.HeadOwnerName == baseRepo.Owner.Name && ci.HeadRepoName == baseRepo.Name) { ci.HeadOwnerName = baseRepo.Owner.Name ci.HeadRepoName = baseRepo.Name ci.HeadUser = baseRepo.Owner @@ -277,14 +279,16 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep return nil, err } } - ci.HeadRepo.Owner = ci.HeadUser - ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) - if err != nil { - return nil, err - } - ci.close = func() { - if ci.HeadGitRepo != nil { - ci.HeadGitRepo.Close() + if ci.HeadRepo != nil { + ci.HeadRepo.Owner = ci.HeadUser + ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) + if err != nil { + return nil, err + } + ci.close = func() { + if ci.HeadGitRepo != nil { + ci.HeadGitRepo.Close() + } } } } @@ -295,10 +299,12 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep return nil, err } - ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) - if err != nil { - ci.Close() - return nil, err + if ci.HeadRepo != nil { + ci.HeadFullRef, ci.IsHeadCommit, err = detectFullRef(ctx, ci.HeadRepo.ID, ci.HeadGitRepo, ci.HeadOriRef) + if err != nil { + ci.Close() + return nil, err + } } return ci, nil } diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index aa3038940105f..aa1b7a594952c 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -4,8 +4,13 @@ package common import ( + "context" "testing" + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/gitrepo" "github.com/stretchr/testify/assert" ) @@ -136,3 +141,188 @@ func TestCompareRouters(t *testing.T) { }) } } + +func Test_ParseComparePathParams(t *testing.T) { + baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + assert.NotNil(t, baseRepo) + assert.NoError(t, baseRepo.LoadOwner(db.DefaultContext)) + baseGitRepo, err := gitrepo.OpenRepository(context.Background(), baseRepo) + assert.NoError(t, err) + defer baseGitRepo.Close() + + kases := []struct { + router string + compareInfo *CompareInfo + }{ + { + router: "main...develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 3, + }, + BaseRepo: baseRepo, + HeadUser: baseRepo.Owner, + HeadRepo: baseRepo, + HeadGitRepo: baseGitRepo, + }, + }, + { + router: "main..develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + DotTimes: 2, + }, + BaseRepo: baseRepo, + HeadUser: baseRepo.Owner, + HeadRepo: baseRepo, + HeadGitRepo: baseGitRepo, + }, + }, + { + router: "main^...develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 1, + DotTimes: 3, + }, + BaseRepo: baseRepo, + HeadUser: baseRepo.Owner, + HeadRepo: baseRepo, + HeadGitRepo: baseGitRepo, + }, + }, + { + router: "main^^^^^...develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOriRef: "develop", + CaretTimes: 5, + DotTimes: 3, + }, + BaseRepo: baseRepo, + HeadUser: baseRepo.Owner, + HeadRepo: baseRepo, + HeadGitRepo: baseGitRepo, + }, + }, + { + router: "develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: baseRepo.DefaultBranch, + HeadOriRef: "develop", + DotTimes: 3, + }, + BaseRepo: baseRepo, + HeadUser: baseRepo.Owner, + HeadRepo: baseRepo, + HeadGitRepo: baseGitRepo, + }, + }, + { + router: "lunny/forked_repo:develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: baseRepo.DefaultBranch, + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + BaseRepo: baseRepo, + }, + }, + { + router: "main...lunny/forked_repo:develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + }, + { + router: "main...lunny/forked_repo:develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + }, + }, + }, + { + router: "main^...lunny/forked_repo:develop", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "main", + HeadOwnerName: "lunny", + HeadRepoName: "forked_repo", + HeadOriRef: "develop", + DotTimes: 3, + CaretTimes: 1, + }, + }, + }, + { + router: "v1.0...v1.1", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "v1.0", + HeadOriRef: "v1.1", + DotTimes: 3, + }, + }, + }, + { + router: "teabot-patch-1...v0.0.1", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "teabot-patch-1", + HeadOriRef: "v0.0.1", + DotTimes: 3, + }, + }, + }, + { + router: "teabot:feature1", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + HeadOwnerName: "teabot", + HeadOriRef: "feature1", + DotTimes: 3, + }, + }, + }, + { + router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "8eb19a5ae19abae15c0666d4ab98906139a7f439", + HeadOriRef: "283c030497b455ecfa759d4649f9f8b45158742e", + DotTimes: 3, + }, + }, + }, + } + + for _, kase := range kases { + t.Run(kase.router, func(t *testing.T) { + r, err := ParseComparePathParams(context.Background(), kase.router, baseRepo, baseGitRepo) + assert.NoError(t, err) + assert.EqualValues(t, kase.compareInfo, r) + }) + } +} From 4579e495322edb212cbce783ea0436f97471fea6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 24 Dec 2024 21:30:19 -0800 Subject: [PATCH 16/28] Fix lint --- routers/common/compare.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/routers/common/compare.go b/routers/common/compare.go index 71ace773b30c3..84576bc3fd5d6 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -109,14 +109,14 @@ func (cr *CompareInfo) IsSameRef() bool { } // display pull related information or not -func (ci *CompareInfo) IsPull() bool { - return ci.CaretTimes == 0 && !ci.DirectComparison() && - ci.BaseFullRef.IsBranch() && (ci.HeadRepo == nil || ci.HeadFullRef.IsBranch()) +func (cr *CompareInfo) IsPull() bool { + return cr.CaretTimes == 0 && !cr.DirectComparison() && + cr.BaseFullRef.IsBranch() && (cr.HeadRepo == nil || cr.HeadFullRef.IsBranch()) } -func (ci *CompareInfo) Close() { - if ci.close != nil { - ci.close() +func (cr *CompareInfo) Close() { + if cr.close != nil { + cr.close() } } @@ -309,15 +309,15 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep return ci, nil } -func (ci *CompareInfo) LoadRootRepoAndOwnForkRepo(ctx context.Context, baseRepo *repo_model.Repository, doer *user_model.User) (*repo_model.Repository, *repo_model.Repository, error) { +func (cr *CompareInfo) LoadRootRepoAndOwnForkRepo(ctx context.Context, baseRepo *repo_model.Repository, doer *user_model.User) (*repo_model.Repository, *repo_model.Repository, error) { // find root repo var rootRepo *repo_model.Repository var err error if !baseRepo.IsFork { rootRepo = baseRepo } else { - if !ci.HeadRepo.IsFork { - rootRepo = ci.HeadRepo + if !cr.HeadRepo.IsFork { + rootRepo = cr.HeadRepo } else { rootRepo, err = getRootRepo(ctx, baseRepo) if err != nil { @@ -328,7 +328,7 @@ func (ci *CompareInfo) LoadRootRepoAndOwnForkRepo(ctx context.Context, baseRepo // find ownfork repo var ownForkRepo *repo_model.Repository - if doer != nil && ci.HeadRepo.OwnerID != doer.ID && baseRepo.OwnerID != doer.ID { + if doer != nil && cr.HeadRepo.OwnerID != doer.ID && baseRepo.OwnerID != doer.ID { ownForkRepo, err = findHeadRepo(ctx, baseRepo, doer.ID) if err != nil { return nil, nil, err From d08942e30063c6387d0dbfc3ede9df3d30fcd967 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 24 Dec 2024 21:52:13 -0800 Subject: [PATCH 17/28] Fix lint --- routers/common/compare_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index aa1b7a594952c..2717866b36a53 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -11,6 +11,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/gitrepo" + "github.com/stretchr/testify/assert" ) From 33286791a2f1ed033479bc35abc254f86422e8e2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 21:11:17 -0800 Subject: [PATCH 18/28] Fix test --- models/fixtures/branch.yml | 38 +++++ routers/common/compare.go | 2 +- routers/common/compare_test.go | 274 ++++++++++++++++++++++----------- 3 files changed, 221 insertions(+), 93 deletions(-) diff --git a/models/fixtures/branch.yml b/models/fixtures/branch.yml index 17b1869ab6364..78eac8f9a13ff 100644 --- a/models/fixtures/branch.yml +++ b/models/fixtures/branch.yml @@ -93,3 +93,41 @@ is_deleted: false deleted_by_id: 0 deleted_unix: 0 + +- + id: 16 + repo_id: 1 + name: 'DefaultBranch' + commit_id: '90c1019714259b24fb81711d4416ac0f18667dfa' + commit_message: 'add license' + commit_time: 1709259547 + pusher_id: 1 + is_deleted: false + +- + id: 17 + repo_id: 1 + name: 'develop' + commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d' + commit_message: 'first commit' + commit_time: 978307100 + pusher_id: 1 + +- + id: 18 + repo_id: 11 + name: 'develop' + commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d' + commit_message: 'Initial commit' + commit_time: 1489956479 + pusher_id: 1 + +- + id: 19 + repo_id: 10 + name: 'DefaultBranch' + commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d' + commit_message: 'Initial commit' + commit_time: 1489956479 + pusher_id: 1 + is_deleted: false \ No newline at end of file diff --git a/routers/common/compare.go b/routers/common/compare.go index 84576bc3fd5d6..e73bd077c9699 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -169,7 +169,7 @@ func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Reposito return nil, nil } // test if we are lucky - repo, err := repo_model.GetUserFork(ctx, headUserID, baseRepo.ID) + repo, err := repo_model.GetUserFork(ctx, baseRepo.ID, headUserID) if err != nil { return nil, err } diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 2717866b36a53..88d20a88f9bc6 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "github.com/stretchr/testify/assert" @@ -144,185 +145,274 @@ func TestCompareRouters(t *testing.T) { } func Test_ParseComparePathParams(t *testing.T) { - baseRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - assert.NotNil(t, baseRepo) - assert.NoError(t, baseRepo.LoadOwner(db.DefaultContext)) - baseGitRepo, err := gitrepo.OpenRepository(context.Background(), baseRepo) + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + assert.NotNil(t, repo1) + assert.NoError(t, repo1.LoadOwner(db.DefaultContext)) + gitRepo1, err := gitrepo.OpenRepository(context.Background(), repo1) + assert.NoError(t, err) + defer gitRepo1.Close() + + repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) + assert.NotNil(t, repo10) + assert.NoError(t, repo10.LoadOwner(db.DefaultContext)) + gitRepo10, err := gitrepo.OpenRepository(context.Background(), repo10) + assert.NoError(t, err) + defer gitRepo10.Close() + + repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) + assert.NotNil(t, repo11) + assert.NoError(t, repo11.LoadOwner(db.DefaultContext)) + gitRepo11, err := gitrepo.OpenRepository(context.Background(), repo11) assert.NoError(t, err) - defer baseGitRepo.Close() + defer gitRepo11.Close() + assert.True(t, repo11.IsFork) // repo11 is a fork of repo10 kases := []struct { + repoName string + hasClose bool router string compareInfo *CompareInfo }{ { - router: "main...develop", - compareInfo: &CompareInfo{ - CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOriRef: "develop", - DotTimes: 3, - }, - BaseRepo: baseRepo, - HeadUser: baseRepo.Owner, - HeadRepo: baseRepo, - HeadGitRepo: baseGitRepo, - }, - }, - { - router: "main..develop", - compareInfo: &CompareInfo{ - CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOriRef: "develop", - DotTimes: 2, - }, - BaseRepo: baseRepo, - HeadUser: baseRepo.Owner, - HeadRepo: baseRepo, - HeadGitRepo: baseGitRepo, - }, - }, - { - router: "main^...develop", + repoName: "repo1", + router: "master...branch2", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOriRef: "develop", - CaretTimes: 1, - DotTimes: 3, + BaseOriRef: "master", + BaseFullRef: git.RefNameFromBranch("master"), + HeadOriRef: "branch2", + HeadFullRef: git.RefNameFromBranch("branch2"), + HeadOwnerName: repo1.OwnerName, + HeadRepoName: repo1.Name, + DotTimes: 3, }, - BaseRepo: baseRepo, - HeadUser: baseRepo.Owner, - HeadRepo: baseRepo, - HeadGitRepo: baseGitRepo, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, }, }, { - router: "main^^^^^...develop", + repoName: "repo1", + router: "DefaultBranch..branch2", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOriRef: "develop", - CaretTimes: 5, - DotTimes: 3, + BaseOriRef: "DefaultBranch", + BaseFullRef: git.RefNameFromBranch("DefaultBranch"), + HeadOriRef: "branch2", + HeadFullRef: git.RefNameFromBranch("branch2"), + HeadOwnerName: repo1.Owner.Name, + HeadRepoName: repo1.Name, + DotTimes: 2, }, - BaseRepo: baseRepo, - HeadUser: baseRepo.Owner, - HeadRepo: baseRepo, - HeadGitRepo: baseGitRepo, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, }, }, { - router: "develop", + repoName: "repo1", + router: "DefaultBranch^...branch2", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: baseRepo.DefaultBranch, - HeadOriRef: "develop", - DotTimes: 3, + BaseOriRef: "DefaultBranch", + BaseFullRef: git.RefNameFromBranch("DefaultBranch"), + HeadOriRef: "branch2", + HeadFullRef: git.RefNameFromBranch("branch2"), + HeadOwnerName: repo1.Owner.Name, + HeadRepoName: repo1.Name, + CaretTimes: 1, + DotTimes: 3, }, - BaseRepo: baseRepo, - HeadUser: baseRepo.Owner, - HeadRepo: baseRepo, - HeadGitRepo: baseGitRepo, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, }, }, { - router: "lunny/forked_repo:develop", + repoName: "repo1", + router: "branch2", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: baseRepo.DefaultBranch, - HeadOwnerName: "lunny", - HeadRepoName: "forked_repo", - HeadOriRef: "develop", + BaseOriRef: repo1.DefaultBranch, + BaseFullRef: git.RefNameFromBranch(repo1.DefaultBranch), + HeadOriRef: "branch2", + HeadOwnerName: repo1.Owner.Name, + HeadRepoName: repo1.Name, + HeadFullRef: git.RefNameFromBranch("branch2"), DotTimes: 3, }, - BaseRepo: baseRepo, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, }, }, { - router: "main...lunny/forked_repo:develop", + repoName: "repo10", + hasClose: true, + router: "user13/repo11:develop", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOwnerName: "lunny", - HeadRepoName: "forked_repo", + BaseOriRef: repo10.DefaultBranch, + BaseFullRef: git.RefNameFromBranch(repo10.DefaultBranch), + HeadOwnerName: "user13", + HeadRepoName: "repo11", HeadOriRef: "develop", + HeadFullRef: git.RefNameFromBranch("develop"), DotTimes: 3, }, + BaseRepo: repo10, + HeadUser: repo11.Owner, + HeadRepo: repo11, + HeadGitRepo: gitRepo11, }, }, { - router: "main...lunny/forked_repo:develop", + repoName: "repo10", + hasClose: true, + router: "master...user13/repo11:develop", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOwnerName: "lunny", - HeadRepoName: "forked_repo", + BaseOriRef: "master", + BaseFullRef: git.RefNameFromBranch("master"), + HeadOwnerName: "user13", + HeadRepoName: "repo11", HeadOriRef: "develop", + HeadFullRef: git.RefNameFromBranch("develop"), DotTimes: 3, }, + BaseRepo: repo10, + HeadUser: repo11.Owner, + HeadRepo: repo11, + HeadGitRepo: gitRepo11, }, }, { - router: "main^...lunny/forked_repo:develop", + repoName: "repo10", + hasClose: true, + router: "DefaultBranch^...user13/repo11:develop", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "main", - HeadOwnerName: "lunny", - HeadRepoName: "forked_repo", + BaseOriRef: "DefaultBranch", + BaseFullRef: git.RefNameFromBranch("DefaultBranch"), + HeadOwnerName: "user13", + HeadRepoName: "repo11", HeadOriRef: "develop", + HeadFullRef: git.RefNameFromBranch("develop"), DotTimes: 3, CaretTimes: 1, }, + BaseRepo: repo10, + HeadUser: repo11.Owner, + HeadRepo: repo11, + HeadGitRepo: gitRepo11, }, }, { - router: "v1.0...v1.1", + repoName: "repo1", + router: "master...v1.1", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "v1.0", - HeadOriRef: "v1.1", - DotTimes: 3, + BaseOriRef: "master", + BaseFullRef: git.RefNameFromBranch("master"), + HeadOwnerName: repo1.Owner.Name, + HeadRepoName: repo1.Name, + HeadOriRef: "v1.1", + HeadFullRef: git.RefNameFromTag("v1.1"), + DotTimes: 3, }, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, }, }, { - router: "teabot-patch-1...v0.0.1", + repoName: "repo10", + hasClose: true, + router: "user13:develop", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "teabot-patch-1", - HeadOriRef: "v0.0.1", - DotTimes: 3, + BaseOriRef: repo10.DefaultBranch, + BaseFullRef: git.RefNameFromBranch(repo10.DefaultBranch), + HeadOwnerName: "user13", + HeadOriRef: "develop", + HeadFullRef: git.RefNameFromBranch("develop"), + DotTimes: 3, }, + BaseRepo: repo10, + HeadUser: repo11.Owner, + HeadRepo: repo11, + HeadGitRepo: gitRepo11, }, }, { - router: "teabot:feature1", + repoName: "repo1", + router: "65f1bf27bc3bf70f64657658635e66094edbcb4d...90c1019714259b24fb81711d4416ac0f18667dfa", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - HeadOwnerName: "teabot", - HeadOriRef: "feature1", + BaseOriRef: "65f1bf27bc3bf70f64657658635e66094edbcb4d", + BaseFullRef: git.RefName("65f1bf27bc3bf70f64657658635e66094edbcb4d"), + HeadOwnerName: repo1.Owner.Name, + HeadRepoName: repo1.Name, + HeadOriRef: "90c1019714259b24fb81711d4416ac0f18667dfa", + HeadFullRef: git.RefName("90c1019714259b24fb81711d4416ac0f18667dfa"), DotTimes: 3, }, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, + IsBaseCommit: true, + IsHeadCommit: true, }, }, { - router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", + repoName: "repo1", + router: "5c050d3b6d2db231ab1f64e324f1b6b9a0b181c2^...985f0301dba5e7b34be866819cd15ad3d8f508ee", compareInfo: &CompareInfo{ CompareRouter: &CompareRouter{ - BaseOriRef: "8eb19a5ae19abae15c0666d4ab98906139a7f439", - HeadOriRef: "283c030497b455ecfa759d4649f9f8b45158742e", - DotTimes: 3, + BaseOriRef: "5c050d3b6d2db231ab1f64e324f1b6b9a0b181c2", + BaseFullRef: git.RefName("5c050d3b6d2db231ab1f64e324f1b6b9a0b181c2"), + HeadOwnerName: repo1.Owner.Name, + HeadRepoName: repo1.Name, + HeadOriRef: "985f0301dba5e7b34be866819cd15ad3d8f508ee", + HeadFullRef: git.RefName("985f0301dba5e7b34be866819cd15ad3d8f508ee"), + DotTimes: 3, + CaretTimes: 1, }, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, + IsBaseCommit: true, + IsHeadCommit: true, }, }, } for _, kase := range kases { t.Run(kase.router, func(t *testing.T) { + var baseRepo *repo_model.Repository + var baseGitRepo *git.Repository + if kase.repoName == "repo1" { + baseRepo = repo1 + baseGitRepo = gitRepo1 + } else if kase.repoName == "repo10" { + baseRepo = repo10 + baseGitRepo = gitRepo10 + } r, err := ParseComparePathParams(context.Background(), kase.router, baseRepo, baseGitRepo) assert.NoError(t, err) + if kase.hasClose { + assert.NotNil(t, r.close) + r.close = nil // close is a function, so we can't compare it + } assert.EqualValues(t, kase.compareInfo, r) }) } From 361ca4002ec817bc51477aa85e367b5a131c4cd7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 21:13:00 -0800 Subject: [PATCH 19/28] Fix fixture --- models/fixtures/branch.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/fixtures/branch.yml b/models/fixtures/branch.yml index 78eac8f9a13ff..abd774618421d 100644 --- a/models/fixtures/branch.yml +++ b/models/fixtures/branch.yml @@ -130,4 +130,4 @@ commit_message: 'Initial commit' commit_time: 1489956479 pusher_id: 1 - is_deleted: false \ No newline at end of file + is_deleted: false From 2b830ee90486e64909039f7970c641c30e711719 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 22:26:40 -0800 Subject: [PATCH 20/28] Fix tests --- models/fixtures/branch.yml | 21 +++++++++++++++++++++ routers/web/repo/compare.go | 2 ++ tests/integration/api_branch_test.go | 4 ++-- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/models/fixtures/branch.yml b/models/fixtures/branch.yml index abd774618421d..a4af5400f64cd 100644 --- a/models/fixtures/branch.yml +++ b/models/fixtures/branch.yml @@ -131,3 +131,24 @@ commit_time: 1489956479 pusher_id: 1 is_deleted: false + +- + id: 20 + repo_id: 1 + name: 'pr-to-update' + commit_id: '62fb502a7172d4453f0322a2cc85bddffa57f07a' + commit_message: 'add WoW File' + commit_time: 1579200695 + pusher_id: 1 + +- + id: 21 + repo_id: 10 + name: 'develop' + commit_id: '65f1bf27bc3bf70f64657658635e66094edbcb4d' + commit_message: 'Initial commit' + commit_time: 1489927679 + pusher_id: 12 + is_deleted: false + deleted_by_id: 0 + deleted_unix: 0 \ No newline at end of file diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 0b8dc58ea5b98..9e93ada5eb475 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -204,6 +204,8 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { ctx.NotFound("GetRepositoryByOwnerAndName", nil) case errors.Is(err, util.ErrInvalidArgument): ctx.NotFound("ParseComparePathParams", nil) + case git.IsErrNotExist(err): + ctx.NotFound("ParseComparePathParams", nil) default: ctx.ServerError("GetRepositoryByOwnerAndName", err) } diff --git a/tests/integration/api_branch_test.go b/tests/integration/api_branch_test.go index 8a0bd2e4ffa4b..564c25f99270c 100644 --- a/tests/integration/api_branch_test.go +++ b/tests/integration/api_branch_test.go @@ -270,7 +270,7 @@ func TestAPICreateBranchWithSyncBranches(t *testing.T) { RepoID: 1, }) assert.NoError(t, err) - assert.Len(t, branches, 4) + assert.Len(t, branches, 7) // make a broke repository with no branch on database _, err = db.DeleteByBean(db.DefaultContext, git_model.Branch{RepoID: 1}) @@ -287,7 +287,7 @@ func TestAPICreateBranchWithSyncBranches(t *testing.T) { RepoID: 1, }) assert.NoError(t, err) - assert.Len(t, branches, 5) + assert.Len(t, branches, 8) branches, err = db.Find[git_model.Branch](db.DefaultContext, git_model.FindBranchOptions{ RepoID: 1, From 453adc24d4eb077cb5eca789fa01c62036022094 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Jan 2025 22:34:11 -0800 Subject: [PATCH 21/28] Fix lint --- models/fixtures/branch.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/fixtures/branch.yml b/models/fixtures/branch.yml index a4af5400f64cd..77b0647488b4c 100644 --- a/models/fixtures/branch.yml +++ b/models/fixtures/branch.yml @@ -151,4 +151,4 @@ pusher_id: 12 is_deleted: false deleted_by_id: 0 - deleted_unix: 0 \ No newline at end of file + deleted_unix: 0 From dfe8cb7bbab06ef26f72e2848122b719308343d1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 Jan 2025 10:55:17 -0800 Subject: [PATCH 22/28] Fix tests --- models/fixtures/branch.yml | 3 +++ tests/integration/pull_merge_test.go | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/models/fixtures/branch.yml b/models/fixtures/branch.yml index 77b0647488b4c..e8dc4c4a286c0 100644 --- a/models/fixtures/branch.yml +++ b/models/fixtures/branch.yml @@ -112,6 +112,7 @@ commit_message: 'first commit' commit_time: 978307100 pusher_id: 1 + is_deleted: false - id: 18 @@ -121,6 +122,7 @@ commit_message: 'Initial commit' commit_time: 1489956479 pusher_id: 1 + is_deleted: false - id: 19 @@ -140,6 +142,7 @@ commit_message: 'add WoW File' commit_time: 1579200695 pusher_id: 1 + is_deleted: false - id: 21 diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index 2edc95d4c8d5b..0e8729e425a5a 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -30,6 +30,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/queue" + repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" @@ -308,7 +309,8 @@ func TestCantMergeUnrelated(t *testing.T) { assert.NoError(t, err) sha := strings.TrimSpace(stdout.String()) - _, _, err = git.NewCommand(git.DefaultContext, "update-index", "--add", "--replace", "--cacheinfo").AddDynamicArguments("100644", sha, "somewher-over-the-rainbow").RunStdString(&git.RunOpts{Dir: path}) + _, _, err = git.NewCommand(git.DefaultContext, "update-index", "--add", "--replace", "--cacheinfo"). + AddDynamicArguments("100644", sha, "somewher-over-the-rainbow").RunStdString(&git.RunOpts{Dir: path}) assert.NoError(t, err) treeSha, _, err := git.NewCommand(git.DefaultContext, "write-tree").RunStdString(&git.RunOpts{Dir: path}) @@ -344,6 +346,10 @@ func TestCantMergeUnrelated(t *testing.T) { _, _, err = git.NewCommand(git.DefaultContext, "branch", "unrelated").AddDynamicArguments(commitSha).RunStdString(&git.RunOpts{Dir: path}) assert.NoError(t, err) + // we created a branch to git repository directly, now we need to do a sync to make it available in the database + _, err = repo_module.SyncRepoBranches(db.DefaultContext, repo1.ID, user1.ID) + assert.NoError(t, err) + testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n") // Use API to create a conflicting pr From def6e8cc6b169f0f92b461eb0d2a7e513a314ad0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 Jan 2025 12:06:56 -0800 Subject: [PATCH 23/28] Add a new test compare from base repository to forked reposotry --- routers/common/compare_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 88d20a88f9bc6..2c7178be12372 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -313,6 +313,26 @@ func Test_ParseComparePathParams(t *testing.T) { HeadGitRepo: gitRepo11, }, }, + { + repoName: "repo11", + hasClose: true, + router: "user12/repo10:master", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: repo11.DefaultBranch, + BaseFullRef: git.RefNameFromBranch(repo11.DefaultBranch), + HeadOwnerName: "user12", + HeadRepoName: "repo10", + HeadOriRef: "master", + HeadFullRef: git.RefNameFromBranch("master"), + DotTimes: 3, + }, + BaseRepo: repo11, + HeadUser: repo10.Owner, + HeadRepo: repo10, + HeadGitRepo: gitRepo10, + }, + }, { repoName: "repo1", router: "master...v1.1", @@ -406,6 +426,11 @@ func Test_ParseComparePathParams(t *testing.T) { } else if kase.repoName == "repo10" { baseRepo = repo10 baseGitRepo = gitRepo10 + } else if kase.repoName == "repo11" { + baseRepo = repo11 + baseGitRepo = gitRepo11 + } else { + t.Fatalf("unknown repo name: %s", kase.router) } r, err := ParseComparePathParams(context.Background(), kase.router, baseRepo, baseGitRepo) assert.NoError(t, err) From cf3145a4fea7ec34a6bdbf26a44e6f95b958c82c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 Jan 2025 12:41:20 -0800 Subject: [PATCH 24/28] Some improvements --- routers/api/v1/repo/compare.go | 3 +++ routers/api/v1/repo/pull.go | 2 ++ routers/common/compare.go | 4 ++++ routers/common/compare_test.go | 20 ++++++++++++++++++++ routers/web/repo/compare.go | 5 +---- 5 files changed, 30 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 33a04723a713f..4154727aae1c7 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -11,6 +11,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" 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" api "code.gitea.io/gitea/modules/structs" @@ -69,6 +70,8 @@ func CompareDiff(ctx *context.APIContext) { ctx.NotFound("GetRepositoryByOwnerAndName") case errors.Is(err, util.ErrInvalidArgument): ctx.NotFound("ParseComparePathParams") + case git.IsErrNotExist(err): + ctx.NotFound("ParseComparePathParams") default: ctx.ServerError("GetRepositoryByOwnerAndName", err) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index a10152609344c..5a80c93c8e251 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -417,6 +417,8 @@ func CreatePullRequest(ctx *context.APIContext) { ctx.NotFound("GetRepositoryByOwnerAndName") case errors.Is(err, util.ErrInvalidArgument): ctx.NotFound("ParseComparePathParams") + case git.IsErrNotExist(err): + ctx.NotFound("ParseComparePathParams") default: ctx.ServerError("GetRepositoryByOwnerAndName", err) } diff --git a/routers/common/compare.go b/routers/common/compare.go index e73bd077c9699..98e2082b0c618 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -30,6 +30,10 @@ func (cr *CompareRouter) DirectComparison() bool { return cr.DotTimes == 2 } +func (cr *CompareRouter) CompareDots() string { + return strings.Repeat(".", cr.DotTimes) +} + func parseBase(base string) (string, int) { parts := strings.SplitN(base, "^", 2) if len(parts) == 1 { diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 2c7178be12372..3469e424a3316 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -414,6 +414,26 @@ func Test_ParseComparePathParams(t *testing.T) { IsHeadCommit: true, }, }, + { + repoName: "repo1", + hasClose: true, + router: "user12/repo10:master", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: repo11.DefaultBranch, + BaseFullRef: git.RefNameFromBranch(repo11.DefaultBranch), + HeadOwnerName: "user12", + HeadRepoName: "repo10", + HeadOriRef: "master", + HeadFullRef: git.RefNameFromBranch("master"), + DotTimes: 3, + }, + BaseRepo: repo1, + HeadUser: repo10.Owner, + HeadRepo: repo10, + HeadGitRepo: gitRepo10, + }, + }, } for _, kase := range kases { diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 9e93ada5eb475..37d286f43f806 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -554,11 +554,8 @@ func CompareDiff(ctx *context.Context) { } beforeCommitID := ctx.Data["BeforeCommitID"].(string) afterCommitID := ctx.Data["AfterCommitID"].(string) + separator := ci.CompareDots() - separator := "..." - if ci.DirectComparison() { - separator = ".." - } ctx.Data["Title"] = "Comparing " + base.ShortSha(beforeCommitID) + separator + base.ShortSha(afterCommitID) ctx.Data["IsDiffCompare"] = true From 62e21e823f50e059d94bb814e2713f863be77f2d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 9 Jan 2025 19:45:57 -0800 Subject: [PATCH 25/28] Fix empty compare --- routers/common/compare.go | 5 ++++- routers/common/compare_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/routers/common/compare.go b/routers/common/compare.go index 98e2082b0c618..a4a6d91e4037c 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -241,7 +241,10 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep var err error if pathParam == "" { - ci.HeadOriRef = baseRepo.DefaultBranch + ci.CompareRouter = &CompareRouter{ + HeadOriRef: baseRepo.DefaultBranch, + DotTimes: 3, + } } else { ci.CompareRouter, err = parseCompareRouter(pathParam) if err != nil { diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 3469e424a3316..de6093a295ddc 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -21,6 +21,14 @@ func TestCompareRouters(t *testing.T) { router string compareRouter *CompareRouter }{ + { + router: "", + compareRouter: &CompareRouter{ + BaseOriRef: "", + HeadOriRef: "", + DotTimes: 3, + }, + }, { router: "main...develop", compareRouter: &CompareRouter{ @@ -175,6 +183,25 @@ func Test_ParseComparePathParams(t *testing.T) { router string compareInfo *CompareInfo }{ + { + repoName: "repo1", + router: "", + compareInfo: &CompareInfo{ + CompareRouter: &CompareRouter{ + BaseOriRef: "master", + BaseFullRef: git.RefNameFromBranch("master"), + HeadOriRef: "master", + HeadFullRef: git.RefNameFromBranch("master"), + HeadOwnerName: repo1.OwnerName, + HeadRepoName: repo1.Name, + DotTimes: 3, + }, + BaseRepo: repo1, + HeadUser: repo1.Owner, + HeadRepo: repo1, + HeadGitRepo: gitRepo1, + }, + }, { repoName: "repo1", router: "master...branch2", From 0d56e5835b5df6c47952581393a50aca0171134d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 27 Feb 2025 11:09:25 -0800 Subject: [PATCH 26/28] Fix lint error --- routers/common/compare_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index de6093a295ddc..56c8a9f19c811 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -4,7 +4,6 @@ package common import ( - "context" "testing" "code.gitea.io/gitea/models/db" @@ -158,21 +157,21 @@ func Test_ParseComparePathParams(t *testing.T) { repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) assert.NotNil(t, repo1) assert.NoError(t, repo1.LoadOwner(db.DefaultContext)) - gitRepo1, err := gitrepo.OpenRepository(context.Background(), repo1) + gitRepo1, err := gitrepo.OpenRepository(t.Context(), repo1) assert.NoError(t, err) defer gitRepo1.Close() repo10 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 10}) assert.NotNil(t, repo10) assert.NoError(t, repo10.LoadOwner(db.DefaultContext)) - gitRepo10, err := gitrepo.OpenRepository(context.Background(), repo10) + gitRepo10, err := gitrepo.OpenRepository(t.Context(), repo10) assert.NoError(t, err) defer gitRepo10.Close() repo11 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 11}) assert.NotNil(t, repo11) assert.NoError(t, repo11.LoadOwner(db.DefaultContext)) - gitRepo11, err := gitrepo.OpenRepository(context.Background(), repo11) + gitRepo11, err := gitrepo.OpenRepository(t.Context(), repo11) assert.NoError(t, err) defer gitRepo11.Close() assert.True(t, repo11.IsFork) // repo11 is a fork of repo10 @@ -479,7 +478,7 @@ func Test_ParseComparePathParams(t *testing.T) { } else { t.Fatalf("unknown repo name: %s", kase.router) } - r, err := ParseComparePathParams(context.Background(), kase.router, baseRepo, baseGitRepo) + r, err := ParseComparePathParams(t.Context(), kase.router, baseRepo, baseGitRepo) assert.NoError(t, err) if kase.hasClose { assert.NotNil(t, r.close) From 1f90568a5cc2b7f86054775d7adfc57beacbc1a2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 1 Mar 2025 12:58:09 -0800 Subject: [PATCH 27/28] Fix test --- routers/common/compare_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 56c8a9f19c811..5fd80b970175b 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -484,7 +484,13 @@ func Test_ParseComparePathParams(t *testing.T) { assert.NotNil(t, r.close) r.close = nil // close is a function, so we can't compare it } - assert.EqualValues(t, kase.compareInfo, r) + assert.EqualValues(t, *kase.compareInfo.CompareRouter, *r.CompareRouter) + assert.EqualValues(t, *kase.compareInfo.BaseRepo, *r.BaseRepo) + assert.EqualValues(t, *kase.compareInfo.HeadUser, *r.HeadUser) + assert.EqualValues(t, *kase.compareInfo.HeadRepo, *r.HeadRepo) + assert.EqualValues(t, kase.compareInfo.HeadGitRepo.Path, r.HeadGitRepo.Path) + assert.EqualValues(t, kase.compareInfo.IsBaseCommit, r.IsBaseCommit) + assert.EqualValues(t, kase.compareInfo.IsHeadCommit, r.IsHeadCommit) }) } } From 57405c34fc87b7def2b523ced94e1d12208e702d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 11 Mar 2025 16:35:11 -0700 Subject: [PATCH 28/28] Fix build --- routers/common/compare.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/common/compare.go b/routers/common/compare.go index a4a6d91e4037c..f7caf42c1032e 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -59,7 +59,7 @@ func parseCompareRouter(router string) (*CompareRouter, error) { dotTimes := 3 parts := strings.Split(router, "...") if len(parts) > 2 { - return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) + return nil, util.NewInvalidArgumentErrorf("invalid compare router: %s", router) } if len(parts) != 2 { parts = strings.Split(router, "..") @@ -72,7 +72,7 @@ func parseCompareRouter(router string) (*CompareRouter, error) { DotTimes: dotTimes, }, nil } else if len(parts) > 2 { - return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) + return nil, util.NewInvalidArgumentErrorf("invalid compare router: %s", router) } dotTimes = 2 }