From 3163abedd6c3814d04b380c036ca19a7bffe908f Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Sat, 2 Dec 2017 18:20:12 -0800 Subject: [PATCH] Fix ref parsing in commit messages (#3067) --- models/action.go | 127 +++++++++++++++++++----------------------- models/action_test.go | 30 ++++++++++ models/issue.go | 32 ----------- 3 files changed, 87 insertions(+), 102 deletions(-) diff --git a/models/action.go b/models/action.go index ead34dbac..699b32f31 100644 --- a/models/action.go +++ b/models/action.go @@ -14,15 +14,14 @@ import ( "time" "unicode" - "github.com/Unknwon/com" - "github.com/go-xorm/builder" - "code.gitea.io/git" - api "code.gitea.io/sdk/gitea" - "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + api "code.gitea.io/sdk/gitea" + + "github.com/Unknwon/com" + "github.com/go-xorm/builder" ) // ActionType represents the type of an action. @@ -59,14 +58,16 @@ var ( issueReferenceKeywordsPat *regexp.Regexp ) +const issueRefRegexpStr = `(?:\S+/\S=)?#\d+` + func assembleKeywordsPattern(words []string) string { - return fmt.Sprintf(`(?i)(?:%s) \S+`, strings.Join(words, "|")) + return fmt.Sprintf(`(?i)(?:%s) %s`, strings.Join(words, "|"), issueRefRegexpStr) } func init() { issueCloseKeywordsPat = regexp.MustCompile(assembleKeywordsPattern(issueCloseKeywords)) issueReopenKeywordsPat = regexp.MustCompile(assembleKeywordsPattern(issueReopenKeywords)) - issueReferenceKeywordsPat = regexp.MustCompile(`(?i)(?:)(^| )\S+`) + issueReferenceKeywordsPat = regexp.MustCompile(issueRefRegexpStr) } // Action represents user operation type and other information to @@ -390,6 +391,49 @@ func (pc *PushCommits) AvatarLink(email string) string { return pc.avatars[email] } +// getIssueFromRef returns the issue referenced by a ref. Returns a nil *Issue +// if the provided ref is misformatted or references a non-existent issue. +func getIssueFromRef(repo *Repository, ref string) (*Issue, error) { + ref = ref[strings.IndexByte(ref, ' ')+1:] + ref = strings.TrimRightFunc(ref, issueIndexTrimRight) + + var refRepo *Repository + poundIndex := strings.IndexByte(ref, '#') + if poundIndex < 0 { + return nil, nil + } else if poundIndex == 0 { + refRepo = repo + } else { + slashIndex := strings.IndexByte(ref, '/') + if slashIndex < 0 || slashIndex >= poundIndex { + return nil, nil + } + ownerName := ref[:slashIndex] + repoName := ref[slashIndex+1 : poundIndex] + var err error + refRepo, err = GetRepositoryByOwnerAndName(ownerName, repoName) + if err != nil { + if IsErrRepoNotExist(err) { + return nil, nil + } + return nil, err + } + } + issueIndex, err := strconv.ParseInt(ref[poundIndex+1:], 10, 64) + if err != nil { + return nil, nil + } + + issue, err := GetIssueByIndex(refRepo.ID, int64(issueIndex)) + if err != nil { + if IsErrIssueNotExist(err) { + return nil, nil + } + return nil, err + } + return issue, nil +} + // UpdateIssuesCommit checks if issues are manipulated by commit message. func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) error { // Commits are appended in the reverse order. @@ -398,31 +442,12 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err refMarked := make(map[int64]bool) for _, ref := range issueReferenceKeywordsPat.FindAllString(c.Message, -1) { - ref = ref[strings.IndexByte(ref, byte(' '))+1:] - ref = strings.TrimRightFunc(ref, issueIndexTrimRight) - - if len(ref) == 0 { - continue - } - - // Add repo name if missing - if ref[0] == '#' { - ref = fmt.Sprintf("%s%s", repo.FullName(), ref) - } else if !strings.Contains(ref, "/") { - // FIXME: We don't support User#ID syntax yet - // return ErrNotImplemented - continue - } - - issue, err := GetIssueByRef(ref) + issue, err := getIssueFromRef(repo, ref) if err != nil { - if IsErrIssueNotExist(err) || err == errMissingIssueNumber || err == errInvalidIssueNumber { - continue - } return err } - if refMarked[issue.ID] { + if issue == nil || refMarked[issue.ID] { continue } refMarked[issue.ID] = true @@ -436,31 +461,12 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err refMarked = make(map[int64]bool) // FIXME: can merge this one and next one to a common function. for _, ref := range issueCloseKeywordsPat.FindAllString(c.Message, -1) { - ref = ref[strings.IndexByte(ref, byte(' '))+1:] - ref = strings.TrimRightFunc(ref, issueIndexTrimRight) - - if len(ref) == 0 { - continue - } - - // Add repo name if missing - if ref[0] == '#' { - ref = fmt.Sprintf("%s%s", repo.FullName(), ref) - } else if !strings.Contains(ref, "/") { - // We don't support User#ID syntax yet - // return ErrNotImplemented - continue - } - - issue, err := GetIssueByRef(ref) + issue, err := getIssueFromRef(repo, ref) if err != nil { - if IsErrIssueNotExist(err) || err == errMissingIssueNumber || err == errInvalidIssueNumber { - continue - } return err } - if refMarked[issue.ID] { + if issue == nil || refMarked[issue.ID] { continue } refMarked[issue.ID] = true @@ -476,31 +482,12 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err // It is conflict to have close and reopen at same time, so refsMarked doesn't need to reinit here. for _, ref := range issueReopenKeywordsPat.FindAllString(c.Message, -1) { - ref = ref[strings.IndexByte(ref, byte(' '))+1:] - ref = strings.TrimRightFunc(ref, issueIndexTrimRight) - - if len(ref) == 0 { - continue - } - - // Add repo name if missing - if ref[0] == '#' { - ref = fmt.Sprintf("%s%s", repo.FullName(), ref) - } else if !strings.Contains(ref, "/") { - // We don't support User#ID syntax yet - // return ErrNotImplemented - continue - } - - issue, err := GetIssueByRef(ref) + issue, err := getIssueFromRef(repo, ref) if err != nil { - if IsErrIssueNotExist(err) || err == errMissingIssueNumber || err == errInvalidIssueNumber { - continue - } return err } - if refMarked[issue.ID] { + if issue == nil || refMarked[issue.ID] { continue } refMarked[issue.ID] = true diff --git a/models/action_test.go b/models/action_test.go index 3f29e1556..016917905 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -1,6 +1,7 @@ package models import ( + "fmt" "path" "strings" "testing" @@ -154,6 +155,35 @@ func TestPushCommits_AvatarLink(t *testing.T) { pushCommits.AvatarLink("nonexistent@example.com")) } +func Test_getIssueFromRef(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + for _, test := range []struct { + Ref string + ExpectedIssueID int64 + }{ + {"#2", 2}, + {"reopen #2", 2}, + {"user2/repo2#1", 4}, + {"fixes user2/repo2#1", 4}, + } { + issue, err := getIssueFromRef(repo, test.Ref) + assert.NoError(t, err) + if assert.NotNil(t, issue) { + assert.EqualValues(t, test.ExpectedIssueID, issue.ID) + } + } + + for _, badRef := range []string{ + "doesnotexist/doesnotexist#1", + fmt.Sprintf("#%d", NonexistentID), + } { + issue, err := getIssueFromRef(repo, badRef) + assert.NoError(t, err) + assert.Nil(t, issue) + } +} + func TestUpdateIssuesCommit(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) pushCommits := []*PushCommit{ diff --git a/models/issue.go b/models/issue.go index 00e0bf802..5f576be4a 100644 --- a/models/issue.go +++ b/models/issue.go @@ -5,7 +5,6 @@ package models import ( - "errors" "fmt" "path" "sort" @@ -22,11 +21,6 @@ import ( "code.gitea.io/gitea/modules/util" ) -var ( - errMissingIssueNumber = errors.New("No issue number specified") - errInvalidIssueNumber = errors.New("Invalid issue number") -) - // Issue represents an issue or pull request of repository. type Issue struct { ID int64 `xorm:"pk autoincr"` @@ -961,32 +955,6 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) return nil } -// GetIssueByRef returns an Issue specified by a GFM reference. -// See https://help.github.com/articles/writing-on-github#references for more information on the syntax. -func GetIssueByRef(ref string) (*Issue, error) { - n := strings.IndexByte(ref, '#') - if n == -1 { - return nil, errMissingIssueNumber - } - - index, err := com.StrTo(ref[n+1:]).Int64() - if err != nil { - return nil, errInvalidIssueNumber - } - - i := strings.IndexByte(ref[:n], '/') - if i < 2 { - return nil, ErrInvalidReference - } - - repo, err := GetRepositoryByOwnerAndName(ref[:i], ref[i+1:n]) - if err != nil { - return nil, err - } - - return GetIssueByIndex(repo.ID, index) -} - // GetRawIssueByIndex returns raw issue without loading attributes by index in a repository. func GetRawIssueByIndex(repoID, index int64) (*Issue, error) { issue := &Issue{