Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Use complete SHA to create and query commit status (#22244) #22258

Merged
merged 1 commit into from
Dec 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions integrations/repo_commits_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ func doTestRepoCommitWithStatus(t *testing.T, state string, classes ...string) {
reqOne := NewRequest(t, "GET", "/api/v1/repos/user2/repo1/commits/"+path.Base(commitURL)+"/status")
testRepoCommitsWithStatus(t, session.MakeRequest(t, req, http.StatusOK), session.MakeRequest(t, reqOne, http.StatusOK), state)

// By short SHA
req = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/commits/"+path.Base(commitURL)[:10]+"/statuses")
reqOne = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/commits/"+path.Base(commitURL)[:10]+"/status")
testRepoCommitsWithStatus(t, session.MakeRequest(t, req, http.StatusOK), session.MakeRequest(t, reqOne, http.StatusOK), state)

// By Ref
req = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/commits/master/statuses")
reqOne = NewRequest(t, "GET", "/api/v1/repos/user2/repo1/commits/master/status")
Expand Down
2 changes: 1 addition & 1 deletion models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func (a *Action) GetRefLink() string {
return a.GetRepoLink() + "/src/branch/" + util.PathEscapeSegments(strings.TrimPrefix(a.RefName, git.BranchPrefix))
case strings.HasPrefix(a.RefName, git.TagPrefix):
return a.GetRepoLink() + "/src/tag/" + util.PathEscapeSegments(strings.TrimPrefix(a.RefName, git.TagPrefix))
case len(a.RefName) == 40 && git.IsValidSHAPattern(a.RefName):
case len(a.RefName) == git.SHAFullLength && git.IsValidSHAPattern(a.RefName):
return a.GetRepoLink() + "/src/commit/" + a.RefName
default:
// FIXME: we will just assume it's a branch - this was the old way - at some point we may want to enforce that there is always a ref here.
Expand Down
4 changes: 4 additions & 0 deletions models/git/commit_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,10 @@ func NewCommitStatus(opts NewCommitStatusOptions) error {
return fmt.Errorf("NewCommitStatus[%s, %s]: no user specified", repoPath, opts.SHA)
}

if _, err := git.NewIDFromString(opts.SHA); err != nil {
return fmt.Errorf("NewCommitStatus[%s, %s]: invalid sha: %w", repoPath, opts.SHA, err)
}

// Get the next Status Index
idx, err := GetNextCommitStatusIndex(opts.Repo.ID, opts.SHA)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion modules/context/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ func RepoRefForAPI(next http.Handler) http.Handler {
return
}
ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
} else if len(refName) == 40 {
} else if len(refName) == git.SHAFullLength {
ctx.Repo.CommitID = refName
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetCommit(refName)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions modules/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ func getRefName(ctx *Context, pathType RepoRefType) string {
}
// For legacy and API support only full commit sha
parts := strings.Split(path, "/")
if len(parts) > 0 && len(parts[0]) == 40 {
if len(parts) > 0 && len(parts[0]) == git.SHAFullLength {
ctx.Repo.TreePath = strings.Join(parts[1:], "/")
return parts[0]
}
Expand Down Expand Up @@ -843,7 +843,7 @@ func getRefName(ctx *Context, pathType RepoRefType) string {
return getRefNameFromPath(ctx, path, ctx.Repo.GitRepo.IsTagExist)
case RepoRefCommit:
parts := strings.Split(path, "/")
if len(parts) > 0 && len(parts[0]) >= 7 && len(parts[0]) <= 40 {
if len(parts) > 0 && len(parts[0]) >= 7 && len(parts[0]) <= git.SHAFullLength {
ctx.Repo.TreePath = strings.Join(parts[1:], "/")
return parts[0]
}
Expand Down Expand Up @@ -952,7 +952,7 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context
return
}
ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
} else if len(refName) >= 7 && len(refName) <= 40 {
} else if len(refName) >= 7 && len(refName) <= git.SHAFullLength {
ctx.Repo.IsViewCommit = true
ctx.Repo.CommitID = refName

Expand All @@ -962,7 +962,7 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context
return
}
// If short commit ID add canonical link header
if len(refName) < 40 {
if len(refName) < git.SHAFullLength {
ctx.RespHeader().Set("Link", fmt.Sprintf("<%s>; rel=\"canonical\"",
util.URLJoin(setting.AppURL, strings.Replace(ctx.Req.URL.RequestURI(), util.PathEscapeSegments(refName), url.PathEscape(ctx.Repo.Commit.ID.String()), 1))))
}
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_commit_gogit.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func (repo *Repository) RemoveReference(name string) error {

// ConvertToSHA1 returns a Hash object from a potential ID string
func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) {
if len(commitID) == 40 {
if len(commitID) == SHAFullLength {
sha1, err := NewIDFromString(commitID)
if err == nil {
return sha1, nil
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_commit_nogogit.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func (repo *Repository) getCommitFromBatchReader(rd *bufio.Reader, id SHA1) (*Co

// ConvertToSHA1 returns a Hash object from a potential ID string
func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) {
if len(commitID) == 40 && IsValidSHAPattern(commitID) {
if len(commitID) == SHAFullLength && IsValidSHAPattern(commitID) {
sha1, err := NewIDFromString(commitID)
if err == nil {
return sha1, nil
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (

// ReadTreeToIndex reads a treeish to the index
func (repo *Repository) ReadTreeToIndex(treeish string, indexFilename ...string) error {
if len(treeish) != 40 {
if len(treeish) != SHAFullLength {
res, _, err := NewCommand(repo.Ctx, "rev-parse", "--verify", treeish).RunStdString(&RunOpts{Dir: repo.Path})
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_tree_gogit.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func (repo *Repository) getTree(id SHA1) (*Tree, error) {

// GetTree find the tree object in the repository.
func (repo *Repository) GetTree(idStr string) (*Tree, error) {
if len(idStr) != 40 {
if len(idStr) != SHAFullLength {
res, _, err := NewCommand(repo.Ctx, "rev-parse", "--verify", idStr).RunStdString(&RunOpts{Dir: repo.Path})
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion modules/git/repo_tree_nogogit.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func (repo *Repository) getTree(id SHA1) (*Tree, error) {

// GetTree find the tree object in the repository.
func (repo *Repository) GetTree(idStr string) (*Tree, error) {
if len(idStr) != 40 {
if len(idStr) != SHAFullLength {
res, err := repo.GetRefCommitID(idStr)
if err != nil {
return nil, err
Expand Down
5 changes: 4 additions & 1 deletion modules/git/sha1.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ const EmptySHA = "0000000000000000000000000000000000000000"
// EmptyTreeSHA is the SHA of an empty tree
const EmptyTreeSHA = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"

// SHAFullLength is the full length of a git SHA
const SHAFullLength = 40

// SHAPattern can be used to determine if a string is an valid sha
var shaPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)

Expand Down Expand Up @@ -51,7 +54,7 @@ func MustIDFromString(s string) SHA1 {
func NewIDFromString(s string) (SHA1, error) {
var id SHA1
s = strings.TrimSpace(s)
if len(s) != 40 {
if len(s) != SHAFullLength {
return id, fmt.Errorf("Length must be 40: %s", s)
}
b, err := hex.DecodeString(s)
Expand Down
1 change: 1 addition & 0 deletions routers/api/v1/repo/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ func getCommitStatuses(ctx *context.APIContext, sha string) {
ctx.Error(http.StatusBadRequest, "ref/sha not given", nil)
return
}
sha = utils.MustConvertToSHA1(ctx.Context, sha)
repo := ctx.Repo.Repository

listOptions := utils.GetListOptions(ctx)
Expand Down
29 changes: 28 additions & 1 deletion routers/api/v1/utils/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func ResolveRefOrSha(ctx *context.APIContext, ref string) string {
return refSHA
}
}
return ref
return MustConvertToSHA1(ctx.Context, ref)
}

// GetGitRefs return git references based on filter
Expand All @@ -55,3 +55,30 @@ func searchRefCommitByType(ctx *context.APIContext, refType, filter string) (str
}
return "", "", nil
}

// ConvertToSHA1 returns a full-length SHA1 from a potential ID string
func ConvertToSHA1(ctx *context.Context, commitID string) (git.SHA1, error) {
if len(commitID) == git.SHAFullLength && git.IsValidSHAPattern(commitID) {
sha1, err := git.NewIDFromString(commitID)
if err == nil {
return sha1, nil
}
}

gitRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, ctx.Repo.Repository.RepoPath())
if err != nil {
return git.SHA1{}, fmt.Errorf("RepositoryFromContextOrOpen: %w", err)
}
defer closer.Close()

return gitRepo.ConvertToSHA1(commitID)
}

// MustConvertToSHA1 returns a full-length SHA1 string from a potential ID string, or returns origin input if it can't convert to SHA1
func MustConvertToSHA1(ctx *context.Context, commitID string) string {
sha, err := ConvertToSHA1(ctx, commitID)
if err != nil {
return commitID
}
return sha.String()
}
2 changes: 1 addition & 1 deletion routers/web/repo/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ func Diff(ctx *context.Context) {
}
return
}
if len(commitID) != 40 {
if len(commitID) != git.SHAFullLength {
commitID = commit.ID.String()
}

Expand Down
12 changes: 6 additions & 6 deletions services/pull/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,19 +200,19 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
return nil, fmt.Errorf("ReadFile(%s): %v", headFile, err)
}
commitID := string(commitIDBytes)
if len(commitID) < 40 {
if len(commitID) < git.SHAFullLength {
return nil, fmt.Errorf(`ReadFile(%s): invalid commit-ID "%s"`, headFile, commitID)
}
cmd := commitID[:40] + ".." + pr.BaseBranch
cmd := commitID[:git.SHAFullLength] + ".." + pr.BaseBranch

// Get the commit from BaseBranch where the pull request got merged
mergeCommit, _, err := git.NewCommand(ctx, "rev-list", "--ancestry-path", "--merges", "--reverse", cmd).
RunStdString(&git.RunOpts{Dir: "", Env: []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}})
if err != nil {
return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %v", err)
} else if len(mergeCommit) < 40 {
} else if len(mergeCommit) < git.SHAFullLength {
// PR was maybe fast-forwarded, so just use last commit of PR
mergeCommit = commitID[:40]
mergeCommit = commitID[:git.SHAFullLength]
}

gitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath())
Expand All @@ -221,9 +221,9 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com
}
defer gitRepo.Close()

commit, err := gitRepo.GetCommit(mergeCommit[:40])
commit, err := gitRepo.GetCommit(mergeCommit[:git.SHAFullLength])
if err != nil {
return nil, fmt.Errorf("GetMergeCommit[%v]: %v", mergeCommit[:40], err)
return nil, fmt.Errorf("GetMergeCommit[%v]: %v", mergeCommit[:git.SHAFullLength], err)
}

return commit, nil
Expand Down
2 changes: 1 addition & 1 deletion services/pull/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@ func MergedManually(pr *issues_model.PullRequest, doer *user_model.User, baseGit
return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: repo_model.MergeStyleManuallyMerged}
}

if len(commitID) < 40 {
if len(commitID) < git.SHAFullLength {
return fmt.Errorf("Wrong commit ID")
}

Expand Down
2 changes: 1 addition & 1 deletion services/pull/temp_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func createTemporaryRepo(ctx context.Context, pr *issues_model.PullRequest) (str
var headBranch string
if pr.Flow == issues_model.PullRequestFlowGithub {
headBranch = git.BranchPrefix + pr.HeadBranch
} else if len(pr.HeadCommitID) == 40 { // for not created pull request
} else if len(pr.HeadCommitID) == git.SHAFullLength { // for not created pull request
headBranch = pr.HeadCommitID
} else {
headBranch = pr.GetGitRefName()
Expand Down
5 changes: 4 additions & 1 deletion services/repository/files/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,12 @@ func CreateCommitStatus(ctx context.Context, repo *repo_model.Repository, creato
}
defer closer.Close()

if _, err := gitRepo.GetCommit(sha); err != nil {
if commit, err := gitRepo.GetCommit(sha); err != nil {
gitRepo.Close()
return fmt.Errorf("GetCommit[%s]: %v", sha, err)
} else if len(sha) != git.SHAFullLength {
// use complete commit sha
sha = commit.ID.String()
}
gitRepo.Close()

Expand Down
2 changes: 1 addition & 1 deletion services/repository/files/tree.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func GetTreeBySHA(ctx context.Context, repo *repo_model.Repository, gitRepo *git
copy(treeURL[apiURLLen:], "/git/trees/")

// 40 is the size of the sha1 hash in hexadecimal format.
copyPos := len(treeURL) - 40
copyPos := len(treeURL) - git.SHAFullLength

if perPage <= 0 || perPage > setting.API.DefaultGitTreesPerPage {
perPage = setting.API.DefaultGitTreesPerPage
Expand Down