[API] Get a single commit via Ref (#10915)

* GET /repos/:owner/:repo/commits/:ref

* add Validation Checks

* Fix & Extend TEST

* add two new tast cases

integrations/api_repo_git_commits_test.go

@@ -21,17 +21,41 @@ func TestAPIReposGitCommits(t *testing.T) {
 	session := loginUser(t, user.Name)
 	token := getTokenForLoggedInUser(t, session)
 
+	//check invalid requests for GetCommitsBySHA
+	req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/master?token="+token, user.Name)
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+
+	req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/12345?token="+token, user.Name)
+	session.MakeRequest(t, req, http.StatusNotFound)
+
+	//check invalid requests for GetCommitsByRef
+	req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/commits/..?token="+token, user.Name)
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+
+	req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/commits/branch-not-exist?token="+token, user.Name)
+	session.MakeRequest(t, req, http.StatusNotFound)
+
 	for _, ref := range [...]string{
-		"commits/master", // Branch
+		"master", // Branch
-		"commits/v1.1",   // Tag
+		"v1.1",   // Tag
+		"65f1",   // short sha
+		"65f1bf27bc3bf70f64657658635e66094edbcb4d", // full sha
 	} {
-		req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/%s?token="+token, user.Name, ref)
+		req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/commits/%s?token="+token, user.Name, ref)
-		session.MakeRequest(t, req, http.StatusOK)
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		commitByRef := new(api.Commit)
+		DecodeJSON(t, resp, commitByRef)
+		assert.Len(t, commitByRef.SHA, 40)
+		assert.EqualValues(t, commitByRef.SHA, commitByRef.RepoCommit.Tree.SHA)
+		req = NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/%s?token="+token, user.Name, commitByRef.SHA)
+		resp = session.MakeRequest(t, req, http.StatusOK)
+		commitBySHA := new(api.Commit)
+		DecodeJSON(t, resp, commitBySHA)
+
+		assert.EqualValues(t, commitByRef.SHA, commitBySHA.SHA)
+		assert.EqualValues(t, commitByRef.HTMLURL, commitBySHA.HTMLURL)
+		assert.EqualValues(t, commitByRef.RepoCommit.Message, commitBySHA.RepoCommit.Message)
 	}
-
-	// Test getting non-existent refs
-	req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo1/git/commits/unknown?token="+token, user.Name)
-	session.MakeRequest(t, req, http.StatusNotFound)
 }
 
 func TestAPIReposGitCommitList(t *testing.T) {

modules/convert/convert.go

@@ -386,7 +386,6 @@ func ToCommitUser(sig *git.Signature) *api.CommitUser {
 func ToCommitMeta(repo *models.Repository, tag *git.Tag) *api.CommitMeta {
 	return &api.CommitMeta{
 		SHA: tag.Object.String(),
-		// TODO: Add the /commits API endpoint and use it here (https://developer.github.com/v3/repos/commits/#get-a-single-commit)
 		URL: util.URLJoin(repo.APIURL(), "git/commits", tag.ID.String()),
 	}
 }

modules/git/sha1.go

@@ -8,6 +8,7 @@ package git
 import (
 	"encoding/hex"
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/go-git/go-git/v5/plumbing"
@@ -16,6 +17,9 @@ import (
 // EmptySHA defines empty git SHA
 const EmptySHA = "0000000000000000000000000000000000000000"
 
+// SHAPattern can be used to determine if a string is an valid sha
+var SHAPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)
+
 // SHA1 a git commit name
 type SHA1 = plumbing.Hash
 

modules/validation/binding.go

@@ -22,12 +22,32 @@ const (
 )
 
 var (
-	// GitRefNamePattern is regular expression with unallowed characters in git reference name
+	// GitRefNamePatternInvalid is regular expression with unallowed characters in git reference name
 	// They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere.
 	// They cannot have question-mark ?, asterisk *, or open bracket [ anywhere
-	GitRefNamePattern = regexp.MustCompile(`[\000-\037\177 \\~^:?*[]+`)
+	GitRefNamePatternInvalid = regexp.MustCompile(`[\000-\037\177 \\~^:?*[]+`)
 )
 
+// CheckGitRefAdditionalRulesValid check name is valid on additional rules
+func CheckGitRefAdditionalRulesValid(name string) bool {
+
+	// Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
+	if strings.HasPrefix(name, "/") || strings.HasSuffix(name, "/") ||
+		strings.HasSuffix(name, ".") || strings.Contains(name, "..") ||
+		strings.Contains(name, "//") || strings.Contains(name, "@{") ||
+		name == "@" {
+		return false
+	}
+	parts := strings.Split(name, "/")
+	for _, part := range parts {
+		if strings.HasSuffix(part, ".lock") || strings.HasPrefix(part, ".") {
+			return false
+		}
+	}
+
+	return true
+}
+
 // AddBindingRules adds additional binding rules
 func AddBindingRules() {
 	addGitRefNameBindingRule()
@@ -44,25 +64,15 @@ func addGitRefNameBindingRule() {
 		IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) {
 			str := fmt.Sprintf("%v", val)
 
-			if GitRefNamePattern.MatchString(str) {
+			if GitRefNamePatternInvalid.MatchString(str) {
 				errs.Add([]string{name}, ErrGitRefName, "GitRefName")
 				return false, errs
 			}
-			// Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
+
-			if strings.HasPrefix(str, "/") || strings.HasSuffix(str, "/") ||
+			if !CheckGitRefAdditionalRulesValid(str) {
-				strings.HasSuffix(str, ".") || strings.Contains(str, "..") ||
-				strings.Contains(str, "//") || strings.Contains(str, "@{") ||
-				str == "@" {
 				errs.Add([]string{name}, ErrGitRefName, "GitRefName")
 				return false, errs
 			}
-			parts := strings.Split(str, "/")
-			for _, part := range parts {
-				if strings.HasSuffix(part, ".lock") || strings.HasPrefix(part, ".") {
-					errs.Add([]string{name}, ErrGitRefName, "GitRefName")
-					return false, errs
-				}
-			}
 
 			return true, errs
 		},

routers/api/v1/api.go

@@ -798,14 +798,14 @@ func RegisterRoutes(m *macaron.Macaron) {
 				m.Group("/commits", func() {
 					m.Get("", repo.GetAllCommits)
 					m.Group("/:ref", func() {
-						// TODO: Add m.Get("") for single commit (https://developer.github.com/v3/repos/commits/#get-a-single-commit)
+						m.Get("", repo.GetSingleCommitByRef)
 						m.Get("/status", repo.GetCombinedCommitStatusByRef)
 						m.Get("/statuses", repo.GetCommitStatusesByRef)
 					})
 				}, reqRepoReader(models.UnitTypeCode))
 				m.Group("/git", func() {
 					m.Group("/commits", func() {
-						m.Get("/:sha", repo.GetSingleCommit)
+						m.Get("/:sha", repo.GetSingleCommitBySHA)
 					})
 					m.Get("/refs", repo.GetGitAllRefs)
 					m.Get("/refs/*", repo.GetGitRefs)

routers/api/v1/repo/commits.go

@@ -6,6 +6,7 @@
 package repo
 
 import (
+	"fmt"
 	"math"
 	"net/http"
 	"strconv"
@@ -16,12 +17,13 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/validation"
 	"code.gitea.io/gitea/routers/api/v1/utils"
 )
 
-// GetSingleCommit get a commit via
+// GetSingleCommitBySHA get a commit via sha
-func GetSingleCommit(ctx *context.APIContext) {
+func GetSingleCommitBySHA(ctx *context.APIContext) {
-	// swagger:operation GET /repos/{owner}/{repo}/git/commits/{sha} repository repoGetSingleCommit
+	// swagger:operation GET /repos/{owner}/{repo}/git/commits/{sha} repository repoGetSingleCommitBySHA
 	// ---
 	// summary: Get a single commit from a repository
 	// produces:
@@ -45,16 +47,68 @@ func GetSingleCommit(ctx *context.APIContext) {
 	// responses:
 	//   "200":
 	//     "$ref": "#/responses/Commit"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
 	//   "404":
 	//     "$ref": "#/responses/notFound"
 
+	sha := ctx.Params(":sha")
+	if !git.SHAPattern.MatchString(sha) {
+		ctx.Error(http.StatusUnprocessableEntity, "no valid sha", fmt.Sprintf("no valid sha: %s", sha))
+		return
+	}
+	getCommit(ctx, sha)
+}
+
+// GetSingleCommitByRef get a commit via ref
+func GetSingleCommitByRef(ctx *context.APIContext) {
+	// swagger:operation GET /repos/{owner}/{repo}/commits/{ref} repository repoGetSingleCommitByRef
+	// ---
+	// summary: Get a single commit from a repository
+	// produces:
+	// - application/json
+	// parameters:
+	// - name: owner
+	//   in: path
+	//   description: owner of the repo
+	//   type: string
+	//   required: true
+	// - name: repo
+	//   in: path
+	//   description: name of the repo
+	//   type: string
+	//   required: true
+	// - name: ref
+	//   in: path
+	//   description: a git ref
+	//   type: string
+	//   required: true
+	// responses:
+	//   "200":
+	//     "$ref": "#/responses/Commit"
+	//   "422":
+	//     "$ref": "#/responses/validationError"
+	//   "404":
+	//     "$ref": "#/responses/notFound"
+
+	ref := ctx.Params("ref")
+
+	if validation.GitRefNamePatternInvalid.MatchString(ref) || !validation.CheckGitRefAdditionalRulesValid(ref) {
+		ctx.Error(http.StatusUnprocessableEntity, "no valid sha", fmt.Sprintf("no valid ref: %s", ref))
+		return
+	}
+
+	getCommit(ctx, ref)
+}
+
+func getCommit(ctx *context.APIContext, identifier string) {
 	gitRepo, err := git.OpenRepository(ctx.Repo.Repository.RepoPath())
 	if err != nil {
 		ctx.ServerError("OpenRepository", err)
 		return
 	}
 	defer gitRepo.Close()
-	commit, err := gitRepo.GetCommit(ctx.Params(":sha"))
+	commit, err := gitRepo.GetCommit(identifier)
 	if err != nil {
 		ctx.NotFoundOrServerError("GetCommit", git.IsErrNotExist, err)
 		return
@@ -65,7 +119,6 @@ func GetSingleCommit(ctx *context.APIContext) {
 		ctx.ServerError("toCommit", err)
 		return
 	}
-
 	ctx.JSON(http.StatusOK, json)
 }
 

templates/swagger/v1_json.tmpl

@@ -2511,6 +2511,52 @@
         }
       }
     },
+    "/repos/{owner}/{repo}/commits/{ref}": {
+      "get": {
+        "produces": [
+          "application/json"
+        ],
+        "tags": [
+          "repository"
+        ],
+        "summary": "Get a single commit from a repository",
+        "operationId": "repoGetSingleCommitByRef",
+        "parameters": [
+          {
+            "type": "string",
+            "description": "owner of the repo",
+            "name": "owner",
+            "in": "path",
+            "required": true
+          },
+          {
+            "type": "string",
+            "description": "name of the repo",
+            "name": "repo",
+            "in": "path",
+            "required": true
+          },
+          {
+            "type": "string",
+            "description": "a git ref",
+            "name": "ref",
+            "in": "path",
+            "required": true
+          }
+        ],
+        "responses": {
+          "200": {
+            "$ref": "#/responses/Commit"
+          },
+          "404": {
+            "$ref": "#/responses/notFound"
+          },
+          "422": {
+            "$ref": "#/responses/validationError"
+          }
+        }
+      }
+    },
     "/repos/{owner}/{repo}/commits/{ref}/statuses": {
       "get": {
         "produces": [
@@ -2976,7 +3022,7 @@
           "repository"
         ],
         "summary": "Get a single commit from a repository",
-        "operationId": "repoGetSingleCommit",
+        "operationId": "repoGetSingleCommitBySHA",
         "parameters": [
           {
             "type": "string",
@@ -3006,6 +3052,9 @@
           },
           "404": {
             "$ref": "#/responses/notFound"
+          },
+          "422": {
+            "$ref": "#/responses/validationError"
           }
         }
       }