From e434ecdacaa0ea8004317fae1b355ad9bd8f274e Mon Sep 17 00:00:00 2001 From: angelnu Date: Sat, 16 Nov 2024 18:12:40 +0100 Subject: [PATCH 1/5] check IsCommitExist --- routers/api/v1/repo/pull.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index b4749cac37..0238e18ace 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1111,7 +1111,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) 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) { + if !ctx.Repo.GitRepo.IsCommitExist(baseBranch) && !ctx.Repo.GitRepo.IsBranchExist(baseBranch) && !ctx.Repo.GitRepo.IsTagExist(baseBranch) { ctx.NotFound("BaseNotExist") return nil, nil, nil, "", "" } @@ -1186,7 +1186,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) } // Check if head branch is valid. - if !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) { + if !ctx.Repo.GitRepo.IsCommitExist(baseBranch) && !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) { headGitRepo.Close() ctx.NotFound() return nil, nil, nil, "", "" From d2dc4fae3ae8d4810f9f9b537189c897361b58ab Mon Sep 17 00:00:00 2001 From: angelnu Date: Sat, 16 Nov 2024 18:12:40 +0100 Subject: [PATCH 2/5] review changes --- routers/api/v1/repo/pull.go | 48 ++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 0238e18ace..6ca23f1e6d 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1110,10 +1110,19 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) 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.IsCommitExist(baseBranch) && !ctx.Repo.GitRepo.IsBranchExist(baseBranch) && !ctx.Repo.GitRepo.IsTagExist(baseBranch) { - ctx.NotFound("BaseNotExist") - return nil, nil, nil, "", "" + baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(baseBranch) + baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(baseBranch) + baseIsTag := ctx.Repo.GitRepo.IsTagExist(baseBranch) + if !baseIsCommit && !baseIsBranch && !baseIsTag { + // Check for short SHA usage + if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(baseBranch); baseCommit != nil { + baseBranch = baseCommit.ID.String() + } else { + ctx.NotFound("BaseNotExist") + return nil, nil, nil, "", "" + } } // Check if current user has fork of repository or in the same repository. @@ -1186,13 +1195,34 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) } // Check if head branch is valid. - if !ctx.Repo.GitRepo.IsCommitExist(baseBranch) && !headGitRepo.IsBranchExist(headBranch) && !headGitRepo.IsTagExist(headBranch) { - headGitRepo.Close() - ctx.NotFound() - return nil, nil, nil, "", "" - } + headIsCommit := headGitRepo.IsBranchExist(headBranch) + headIsBranch := headGitRepo.IsTagExist(headBranch) + headIsTag := headGitRepo.IsCommitExist(baseBranch) + if !headIsCommit && !headIsBranch && !headIsTag { + // Check if headBranch is short sha commit hash + if headCommit, _ := headGitRepo.GetCommit(headBranch); headCommit != nil { + headBranch = headCommit.ID.String() + } else { + headGitRepo.Close() + ctx.NotFound("IsRefExist", nil) + return nil, nil, nil, "", "" + } + } - compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranch, headBranch, false, false) + baseBranchRef := baseBranch + if baseIsBranch { + baseBranchRef = git.BranchPrefix + baseBranch + } else if baseIsTag { + baseBranchRef = git.TagPrefix + baseBranch + } + headBranchRef := headBranch + if headIsBranch { + headBranchRef = headBranch + } else if headIsTag { + headBranchRef = headBranch + } + + compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranchRef, headBranchRef, false, false) if err != nil { headGitRepo.Close() ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) From 1b9d1240eb713c0e5ee8754a1d9bfd1f0926d44c Mon Sep 17 00:00:00 2001 From: angelnu Date: Sat, 16 Nov 2024 18:12:40 +0100 Subject: [PATCH 3/5] add test --- tests/integration/api_repo_compare_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index f3188eb49f..17847ec360 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -36,3 +36,24 @@ func TestAPICompareBranches(t *testing.T) { assert.Equal(t, 2, apiResp.TotalCommits) assert.Len(t, apiResp.Commits, 2) } + +func TestAPICompareCommits(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + repoName := "repo20" + + req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/c8e31bc...8babce9", repoName). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + var apiResp *api.Compare + DecodeJSON(t, resp, &apiResp) + + assert.Equal(t, 2, apiResp.TotalCommits) + assert.Len(t, apiResp.Commits, 2) +} \ No newline at end of file From 01c9c1953690f227dea5d60f814643280497ea46 Mon Sep 17 00:00:00 2001 From: Angel Nunez Mencias Date: Sat, 16 Nov 2024 18:12:40 +0100 Subject: [PATCH 4/5] fmt --- routers/api/v1/repo/pull.go | 66 +++++++++++----------- tests/integration/api_repo_compare_test.go | 2 +- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 6ca23f1e6d..1a791e8dd5 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1113,16 +1113,16 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) // Check if base branch is valid. baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(baseBranch) - baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(baseBranch) - baseIsTag := ctx.Repo.GitRepo.IsTagExist(baseBranch) - if !baseIsCommit && !baseIsBranch && !baseIsTag { - // Check for short SHA usage - if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(baseBranch); baseCommit != nil { - baseBranch = baseCommit.ID.String() - } else { - ctx.NotFound("BaseNotExist") - return nil, nil, nil, "", "" - } + baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(baseBranch) + baseIsTag := ctx.Repo.GitRepo.IsTagExist(baseBranch) + if !baseIsCommit && !baseIsBranch && !baseIsTag { + // Check for short SHA usage + if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(baseBranch); baseCommit != nil { + baseBranch = baseCommit.ID.String() + } else { + ctx.NotFound("BaseNotExist") + return nil, nil, nil, "", "" + } } // Check if current user has fork of repository or in the same repository. @@ -1197,30 +1197,30 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) // Check if head branch is valid. headIsCommit := headGitRepo.IsBranchExist(headBranch) headIsBranch := headGitRepo.IsTagExist(headBranch) - headIsTag := headGitRepo.IsCommitExist(baseBranch) - if !headIsCommit && !headIsBranch && !headIsTag { - // Check if headBranch is short sha commit hash - if headCommit, _ := headGitRepo.GetCommit(headBranch); headCommit != nil { - headBranch = headCommit.ID.String() - } else { - headGitRepo.Close() - ctx.NotFound("IsRefExist", nil) - return nil, nil, nil, "", "" - } - } + headIsTag := headGitRepo.IsCommitExist(baseBranch) + if !headIsCommit && !headIsBranch && !headIsTag { + // Check if headBranch is short sha commit hash + if headCommit, _ := headGitRepo.GetCommit(headBranch); headCommit != nil { + headBranch = headCommit.ID.String() + } else { + headGitRepo.Close() + ctx.NotFound("IsRefExist", nil) + return nil, nil, nil, "", "" + } + } - baseBranchRef := baseBranch - if baseIsBranch { - baseBranchRef = git.BranchPrefix + baseBranch - } else if baseIsTag { - baseBranchRef = git.TagPrefix + baseBranch - } - headBranchRef := headBranch - if headIsBranch { - headBranchRef = headBranch - } else if headIsTag { - headBranchRef = headBranch - } + baseBranchRef := baseBranch + if baseIsBranch { + baseBranchRef = git.BranchPrefix + baseBranch + } else if baseIsTag { + baseBranchRef = git.TagPrefix + baseBranch + } + headBranchRef := headBranch + if headIsBranch { + headBranchRef = headBranch + } else if headIsTag { + headBranchRef = headBranch + } compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranchRef, headBranchRef, false, false) if err != nil { diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index 17847ec360..bd800b9e79 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -56,4 +56,4 @@ func TestAPICompareCommits(t *testing.T) { assert.Equal(t, 2, apiResp.TotalCommits) assert.Len(t, apiResp.Commits, 2) -} \ No newline at end of file +} From ca0cd42d7aea9df3bde3a29e249bf178a5ef8557 Mon Sep 17 00:00:00 2001 From: Angel Nunez Mencias Date: Sat, 16 Nov 2024 22:31:14 +0100 Subject: [PATCH 5/5] simplify test based on feedback --- tests/integration/api_repo_compare_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index bd800b9e79..765d0cef08 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -45,9 +45,7 @@ func TestAPICompareCommits(t *testing.T) { session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - repoName := "repo20" - - req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/c8e31bc...8babce9", repoName). + req := NewRequestf(t, "GET", "/api/v1/repos/user2/repo20/compare/c8e31bc...8babce9"). AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK)