diff --git a/modules/git/commit.go b/modules/git/commit.go index 010b56948e..0ed268e346 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -474,3 +474,17 @@ func (c *Commit) GetRepositoryDefaultPublicGPGKey(forceUpdate bool) (*GPGSetting } return c.repo.GetDefaultPublicGPGKey(forceUpdate) } + +func IsStringLikelyCommitID(objFmt ObjectFormat, s string, minLength ...int) bool { + minLen := util.OptionalArg(minLength, objFmt.FullLength()) + if len(s) < minLen || len(s) > objFmt.FullLength() { + return false + } + for _, c := range s { + isHex := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') + if !isHex { + return false + } + } + return true +} diff --git a/modules/git/ref.go b/modules/git/ref.go index 2db630e2ea..aab4c5d77d 100644 --- a/modules/git/ref.go +++ b/modules/git/ref.go @@ -142,7 +142,6 @@ func (ref RefName) RemoteName() string { // ShortName returns the short name of the reference name func (ref RefName) ShortName() string { - refName := string(ref) if ref.IsBranch() { return ref.BranchName() } @@ -158,8 +157,7 @@ func (ref RefName) ShortName() string { if ref.IsFor() { return ref.ForBranchName() } - - return refName + return string(ref) // usually it is a commit ID } // RefGroup returns the group type of the reference diff --git a/modules/git/repo_ref.go b/modules/git/repo_ref.go index 8eaa17cb04..850ec65502 100644 --- a/modules/git/repo_ref.go +++ b/modules/git/repo_ref.go @@ -61,3 +61,31 @@ func parseTags(refs []string) []string { } return results } + +// UnstableGuessRefByShortName does the best guess to see whether a "short name" provided by user is a branch, tag or commit. +// It could guess wrongly if the input is already ambiguous. For example: +// * "refs/heads/the-name" vs "refs/heads/refs/heads/the-name" +// * "refs/tags/1234567890" vs commit "1234567890" +// In most cases, it SHOULD AVOID using this function, unless there is an irresistible reason (eg: make API friendly to end users) +// If the function is used, the caller SHOULD CHECK the ref type carefully. +func (repo *Repository) UnstableGuessRefByShortName(shortName string) RefName { + if repo.IsBranchExist(shortName) { + return RefNameFromBranch(shortName) + } + if repo.IsTagExist(shortName) { + return RefNameFromTag(shortName) + } + if strings.HasPrefix(shortName, "refs/") { + if repo.IsReferenceExist(shortName) { + return RefName(shortName) + } + } + commit, err := repo.GetCommit(shortName) + if err == nil { + commitIDString := commit.ID.String() + if strings.HasPrefix(commitIDString, shortName) { + return RefName(commitIDString) + } + } + return "" +} diff --git a/modules/globallock/globallock_test.go b/modules/globallock/globallock_test.go index 88a555c86f..f14c7d656b 100644 --- a/modules/globallock/globallock_test.go +++ b/modules/globallock/globallock_test.go @@ -64,7 +64,7 @@ func TestLockAndDo(t *testing.T) { } func testLockAndDo(t *testing.T) { - const concurrency = 1000 + const concurrency = 50 ctx := context.Background() count := 0 diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 38e5330b3a..1678bc033c 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -64,22 +64,19 @@ func CompareDiff(ctx *context.APIContext) { } } - _, headGitRepo, ci, _, _ := parseCompareInfo(ctx, api.CreatePullRequestOption{ - Base: infos[0], - Head: infos[1], - }) + compareResult, closer := parseCompareInfo(ctx, api.CreatePullRequestOption{Base: infos[0], Head: infos[1]}) if ctx.Written() { return } - defer headGitRepo.Close() + defer closer() 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(compareResult.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(compareResult.compareInfo.Commits); i++ { + apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, compareResult.compareInfo.Commits[i], userCache, convert.ToCommitOptions{ Stat: true, Verification: verification, @@ -93,7 +90,7 @@ func CompareDiff(ctx *context.APIContext) { } ctx.JSON(http.StatusOK, &api.Compare{ - TotalCommits: len(ci.Commits), + TotalCommits: len(compareResult.compareInfo.Commits), Commits: apiCommits, }) } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 86b204f51e..6f4f3efaa1 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -389,8 +389,7 @@ func CreatePullRequest(ctx *context.APIContext) { form := *web.GetForm(ctx).(*api.CreatePullRequestOption) if form.Head == form.Base { - ctx.Error(http.StatusUnprocessableEntity, "BaseHeadSame", - "Invalid PullRequest: There are no changes between the head and the base") + ctx.Error(http.StatusUnprocessableEntity, "BaseHeadSame", "Invalid PullRequest: There are no changes between the head and the base") return } @@ -401,14 +400,22 @@ func CreatePullRequest(ctx *context.APIContext) { ) // Get repo/branch information - headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form) + compareResult, closer := parseCompareInfo(ctx, form) if ctx.Written() { return } - defer headGitRepo.Close() + defer closer() + + if !compareResult.baseRef.IsBranch() || !compareResult.headRef.IsBranch() { + ctx.Error(http.StatusUnprocessableEntity, "BaseHeadInvalidRefType", "Invalid PullRequest: base and head must be branches") + 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, compareResult.headRepo.ID, ctx.Repo.Repository.ID, + compareResult.headRef.ShortName(), compareResult.baseRef.ShortName(), + issues_model.PullRequestFlowGithub, + ) if err != nil { if !issues_model.IsErrPullRequestNotExist(err) { ctx.Error(http.StatusInternalServerError, "GetUnmergedPullRequest", err) @@ -484,13 +491,13 @@ func CreatePullRequest(ctx *context.APIContext) { DeadlineUnix: deadlineUnix, } pr := &issues_model.PullRequest{ - HeadRepoID: headRepo.ID, + HeadRepoID: compareResult.headRepo.ID, BaseRepoID: repo.ID, - HeadBranch: headBranch, - BaseBranch: baseBranch, - HeadRepo: headRepo, + HeadBranch: compareResult.headRef.ShortName(), + BaseBranch: compareResult.baseRef.ShortName(), + HeadRepo: compareResult.headRepo, BaseRepo: repo, - MergeBase: compareInfo.MergeBase, + MergeBase: compareResult.compareInfo.MergeBase, Type: issues_model.PullRequestGitea, } @@ -1080,32 +1087,32 @@ 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 +type parseCompareInfoResult struct { + headRepo *repo_model.Repository + headGitRepo *git.Repository + compareInfo *git.CompareInfo + baseRef git.RefName + headRef git.RefName +} +// parseCompareInfo returns non-nil if it succeeds, it always writes to the context and returns nil if it fails +func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) (result *parseCompareInfoResult, closer func()) { + var err error // Get compared branches information // format: ...[:] // base<-head: master...head:feature // same repo: master...feature + baseRepo := ctx.Repo.Repository + baseRefToGuess := form.Base - // 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] + headUser := ctx.Repo.Owner + headRefToGuess := form.Head + if headInfos := strings.Split(form.Head, ":"); len(headInfos) == 1 { + // If there is no head repository, it means pull request between same repository. + // Do nothing here because the head variables have been assigned above. } else if len(headInfos) == 2 { + // There is a head repository (the head repository could also be the same base repo) + headRefToGuess = headInfos[1] headUser, err = user_model.GetUserByName(ctx, headInfos[0]) if err != nil { if user_model.IsErrUserNotExist(err) { @@ -1113,38 +1120,29 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) } else { ctx.Error(http.StatusInternalServerError, "GetUserByName", err) } - return nil, nil, nil, "", "" + return 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, "", "" + return 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, "", "" - } + isSameRepo := ctx.Repo.Owner.ID == headUser.ID // Check if current user has fork of repository or in the same repository. headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID) if headRepo == nil && !isSameRepo { - err := baseRepo.GetBaseRepo(ctx) + err = baseRepo.GetBaseRepo(ctx) if err != nil { ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) - return nil, nil, nil, "", "" + return 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, "", "" + return nil, nil } // Assign headRepo so it can be used below. headRepo = baseRepo.BaseRepo @@ -1154,67 +1152,68 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) if isSameRepo { headRepo = ctx.Repo.Repository headGitRepo = ctx.Repo.GitRepo + closer = func() {} // no need to close the head repo because it shares the base repo } else { headGitRepo, err = gitrepo.OpenRepository(ctx, headRepo) if err != nil { ctx.Error(http.StatusInternalServerError, "OpenRepository", err) - return nil, nil, nil, "", "" + return nil, nil } + closer = func() { _ = headGitRepo.Close() } } + defer func() { + if result == nil && !isSameRepo { + _ = headGitRepo.Close() + } + }() // 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, "", "" + return nil, nil } - // user should have permission to read headrepo's codes + if !permBase.CanReadIssuesOrPulls(true) || !permBase.CanRead(unit.TypeCode) { + 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) + ctx.NotFound("Can't read pulls or can't read UnitTypeCode") + return nil, nil + } + + // user should have permission to read headRepo's codes + // TODO: could the logic be simplified if the headRepo is the same as the baseRepo? Need to think more about it. permHead, err := access_model.GetUserRepoPermission(ctx, headRepo, ctx.Doer) if err != nil { - headGitRepo.Close() ctx.Error(http.StatusInternalServerError, "GetUserRepoPermission", err) - return nil, nil, nil, "", "" + return 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() + log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", ctx.Doer, headRepo, permHead) ctx.NotFound("Can't read headRepo UnitTypeCode") - return nil, nil, nil, "", "" + return nil, nil } - // Check if head branch is valid. - if !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) { - headGitRepo.Close() + baseRef := ctx.Repo.GitRepo.UnstableGuessRefByShortName(baseRefToGuess) + headRef := headGitRepo.UnstableGuessRefByShortName(headRefToGuess) + + log.Trace("Repo path: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.GitRepo.Path, baseRefToGuess, baseRef, headRefToGuess, headRef) + + baseRefValid := baseRef.IsBranch() || baseRef.IsTag() || git.IsStringLikelyCommitID(git.ObjectFormatFromName(ctx.Repo.Repository.ObjectFormatName), baseRef.ShortName()) + headRefValid := headRef.IsBranch() || headRef.IsTag() || git.IsStringLikelyCommitID(git.ObjectFormatFromName(headRepo.ObjectFormatName), headRef.ShortName()) + // Check if base&head ref are valid. + if !baseRefValid || !headRefValid { ctx.NotFound() - return nil, nil, nil, "", "" + return nil, nil } - compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch, false, false) + compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseRef.ShortName(), headRef.ShortName(), false, false) if err != nil { - headGitRepo.Close() ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) - return nil, nil, nil, "", "" + return nil, nil } - return headRepo, headGitRepo, compareInfo, baseBranch, headBranch + result = &parseCompareInfoResult{headRepo: headRepo, headGitRepo: headGitRepo, compareInfo: compareInfo, baseRef: baseRef, headRef: headRef} + return result, closer } // UpdatePullRequest merge PR's baseBranch into headBranch diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 833f59981b..5397411b59 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -9,7 +9,6 @@ import ( "fmt" "html/template" "net/http" - "net/url" "strconv" "strings" @@ -114,7 +113,6 @@ func MustAllowPulls(ctx *context.Context) { // User can send pull request if owns a forked repository. if ctx.IsSigned && repo_model.HasForkedRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) { ctx.Repo.PullRequest.Allowed = true - ctx.Repo.PullRequest.HeadInfoSubURL = url.PathEscape(ctx.Doer.Name) + ":" + util.PathEscapeSegments(ctx.Repo.BranchName) } } diff --git a/services/context/repo.go b/services/context/repo.go index cf328ca97b..9b54439110 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -39,10 +39,9 @@ import ( // PullRequest contains information to make a pull request type PullRequest struct { - BaseRepo *repo_model.Repository - Allowed bool - SameRepo bool - HeadInfoSubURL string // [:] url segment + BaseRepo *repo_model.Repository + Allowed bool // it only used by the web tmpl: "PullRequestCtx.Allowed" + SameRepo bool // it only used by the web tmpl: "PullRequestCtx.SameRepo" } // Repository contains information to operate a repository @@ -401,6 +400,7 @@ func repoAssignment(ctx *Context, repo *repo_model.Repository) { // RepoAssignment returns a middleware to handle repository assignment func RepoAssignment(ctx *Context) context.CancelFunc { if _, repoAssignmentOnce := ctx.Data["repoAssignmentExecuted"]; repoAssignmentOnce { + // FIXME: it should panic in dev/test modes to have a clear behavior log.Trace("RepoAssignment was exec already, skipping second call ...") return nil } @@ -697,7 +697,6 @@ func RepoAssignment(ctx *Context) context.CancelFunc { ctx.Data["BaseRepo"] = repo.BaseRepo ctx.Repo.PullRequest.BaseRepo = repo.BaseRepo ctx.Repo.PullRequest.Allowed = canPush - ctx.Repo.PullRequest.HeadInfoSubURL = url.PathEscape(ctx.Repo.Owner.Name) + ":" + util.PathEscapeSegments(ctx.Repo.BranchName) } else if repo.AllowsPulls(ctx) { // Or, this is repository accepts pull requests between branches. canCompare = true @@ -705,7 +704,6 @@ func RepoAssignment(ctx *Context) context.CancelFunc { ctx.Repo.PullRequest.BaseRepo = repo ctx.Repo.PullRequest.Allowed = canPush ctx.Repo.PullRequest.SameRepo = true - ctx.Repo.PullRequest.HeadInfoSubURL = util.PathEscapeSegments(ctx.Repo.BranchName) } ctx.Data["CanCompareOrPull"] = canCompare ctx.Data["PullRequestCtx"] = ctx.Repo.PullRequest @@ -771,20 +769,6 @@ func getRefNameFromPath(repo *Repository, path string, isExist func(string) bool return "" } -func isStringLikelyCommitID(objFmt git.ObjectFormat, s string, minLength ...int) bool { - minLen := util.OptionalArg(minLength, objFmt.FullLength()) - if len(s) < minLen || len(s) > objFmt.FullLength() { - return false - } - for _, c := range s { - isHex := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') - if !isHex { - return false - } - } - return true -} - func getRefNameLegacy(ctx *Base, repo *Repository, optionalExtraRef ...string) (string, RepoRefType) { extraRef := util.OptionalArg(optionalExtraRef) reqPath := ctx.PathParam("*") @@ -799,7 +783,7 @@ func getRefNameLegacy(ctx *Base, repo *Repository, optionalExtraRef ...string) ( // For legacy support only full commit sha parts := strings.Split(reqPath, "/") - if isStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), parts[0]) { + if git.IsStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), parts[0]) { // FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists repo.TreePath = strings.Join(parts[1:], "/") return parts[0], RepoRefCommit @@ -849,7 +833,7 @@ func getRefName(ctx *Base, repo *Repository, pathType RepoRefType) string { return getRefNameFromPath(repo, path, repo.GitRepo.IsTagExist) case RepoRefCommit: parts := strings.Split(path, "/") - if isStringLikelyCommitID(repo.GetObjectFormat(), parts[0], 7) { + if git.IsStringLikelyCommitID(repo.GetObjectFormat(), parts[0], 7) { // FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists repo.TreePath = strings.Join(parts[1:], "/") return parts[0] @@ -985,7 +969,7 @@ func RepoRefByType(detectRefType RepoRefType, opts ...RepoRefByTypeOptions) func return cancel } ctx.Repo.CommitID = ctx.Repo.Commit.ID.String() - } else if isStringLikelyCommitID(ctx.Repo.GetObjectFormat(), refName, 7) { + } else if git.IsStringLikelyCommitID(ctx.Repo.GetObjectFormat(), refName, 7) { ctx.Repo.IsViewCommit = true ctx.Repo.CommitID = refName diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index f3188eb49f..9565e4d209 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -24,15 +24,27 @@ func TestAPICompareBranches(t *testing.T) { session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - repoName := "repo20" + t.Run("CompareBranches", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/add-csv...remove-files-b").AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) - req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/add-csv...remove-files-b", repoName). - AddTokenAuth(token) - resp := MakeRequest(t, req, http.StatusOK) + var apiResp *api.Compare + DecodeJSON(t, resp, &apiResp) - var apiResp *api.Compare - DecodeJSON(t, resp, &apiResp) + assert.Equal(t, 2, apiResp.TotalCommits) + assert.Len(t, apiResp.Commits, 2) + }) - assert.Equal(t, 2, apiResp.TotalCommits) - assert.Len(t, apiResp.Commits, 2) + t.Run("CompareCommits", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/808038d2f71b0ab02099...c8e31bc7688741a5287f").AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + var apiResp *api.Compare + DecodeJSON(t, resp, &apiResp) + + assert.Equal(t, 1, apiResp.TotalCommits) + assert.Len(t, apiResp.Commits, 1) + }) } diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index 4338e19617..8b6605eac8 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -440,7 +440,7 @@ func DecodeJSON(t testing.TB, resp *httptest.ResponseRecorder, v any) { t.Helper() decoder := json.NewDecoder(resp.Body) - assert.NoError(t, decoder.Decode(v)) + require.NoError(t, decoder.Decode(v)) } func VerifyJSONSchema(t testing.TB, resp *httptest.ResponseRecorder, schemaFile string) {