Skip to content

Add commit comment when push to pull request branch #2548

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

Closed
wants to merge 14 commits into from
54 changes: 49 additions & 5 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -549,6 +548,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {

isNewBranch := false
opType := ActionCommitRepo

// Check it's tag push or branch.
if strings.HasPrefix(opts.RefFullName, git.TagPrefix) {
opType = ActionPushTag
Expand All @@ -572,6 +572,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
}
}

var commentCommits = opts.Commits.Commits
if len(opts.Commits.Commits) > setting.UI.FeedMaxCommitNum {
opts.Commits.Commits = opts.Commits.Commits[:setting.UI.FeedMaxCommitNum]
}
Expand Down Expand Up @@ -629,6 +630,49 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
}
}

// if there is a pull request from this branch, then update pull request
prs, err := GetUnmergedPullRequestsByHeadInfo(repo.ID, refName)
if err != nil {
return fmt.Errorf("GetUnmergedPullRequestsByHeadInfo: %v", err)
}

if len(prs) > 0 {
var isForcePush bool

// detect force push
if !isNewBranch && git.EmptySHA != opts.OldCommitID {
output, err := git.NewCommand("rev-list", opts.OldCommitID, "^"+opts.NewCommitID).RunInDir(repo.RepoPath())
if err != nil {
return fmt.Errorf("rev-list: %v", err)
}
isForcePush = len(output) > 0
}

if err := PullRequestList(prs).LoadAttributes(); err != nil {
return fmt.Errorf("PullRequestList.LoadAttributes: %v", err)
}

issues := make([]*Issue, 0, len(prs))
lastCommitID := commentCommits[len(commentCommits)-1].Sha1
for _, pr := range prs {
pr.LastCommitID = lastCommitID
if err := pr.UpdateCols("last_commit_id"); err != nil {
return fmt.Errorf("UpdateCols: %v", err)
}

pr.Issue.PullRequest = pr
issues = append(issues, pr.Issue)
}

if _, err := IssueList(issues).LoadRepositories(); err != nil {
return fmt.Errorf("IssueList.LoadRepositories: %v", err)
}

if err := CreatePullPushComments(pusher, issues, commentCommits, isForcePush); err != nil {
return fmt.Errorf("CreatePullPushComments: %v", err)
}
}

case ActionDeleteBranch: // Delete Branch
isHookEventPush = true

Expand Down
217 changes: 192 additions & 25 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ import (
"strings"
"time"

"code.gitea.io/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/markup"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/sdk/gitea"

"github.com/Unknwon/com"
"github.com/go-xorm/builder"
"github.com/go-xorm/xorm"

api "code.gitea.io/sdk/gitea"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/markup"
)

// CommentType defines whether a comment is just a simple comment, an action (like close) or a reference.
Expand All @@ -30,36 +31,38 @@ const (
// Enumerate all the comment types
const (
// Plain comment, can be associated with a commit (CommitID > 0) and a line (LineNum > 0)
CommentTypeComment CommentType = iota
CommentTypeReopen
CommentTypeClose
CommentTypeComment CommentType = iota // 0
CommentTypeReopen // 1
CommentTypeClose // 2

// References.
CommentTypeIssueRef
CommentTypeIssueRef // 3
// Reference from a commit (not part of a pull request)
CommentTypeCommitRef
CommentTypeCommitRef // 4
// Reference from a comment
CommentTypeCommentRef
CommentTypeCommentRef // 5
// Reference from a pull request
CommentTypePullRef
CommentTypePullRef // 6
// Labels changed
CommentTypeLabel
CommentTypeLabel // 7
// Milestone changed
CommentTypeMilestone
CommentTypeMilestone // 8
// Assignees changed
CommentTypeAssignees
CommentTypeAssignees // 9
// Change Title
CommentTypeChangeTitle
CommentTypeChangeTitle // 10
// Delete Branch
CommentTypeDeleteBranch
CommentTypeDeleteBranch // 11
// Start a stopwatch for time tracking
CommentTypeStartTracking
CommentTypeStartTracking // 12
// Stop a stopwatch for time tracking
CommentTypeStopTracking
CommentTypeStopTracking // 13
// Add time manual for time tracking
CommentTypeAddTimeManual
CommentTypeAddTimeManual // 14
// Cancel a stopwatch for time tracking
CommentTypeCancelTracking
CommentTypeCancelTracking // 15
// Push commit to a pull request
CommentTypePullPushCommit // 16
)

// CommentTag defines comment tag type
Expand All @@ -77,9 +80,10 @@ const (
type Comment struct {
ID int64 `xorm:"pk autoincr"`
Type CommentType
PosterID int64 `xorm:"INDEX"`
Poster *User `xorm:"-"`
IssueID int64 `xorm:"INDEX"`
PosterID int64 `xorm:"INDEX"`
Poster *User `xorm:"-"`
IssueID int64 `xorm:"INDEX"`
Issue *Issue `xorm:"-"`
LabelID int64
Label *Label `xorm:"-"`
OldMilestoneID int64
Expand All @@ -104,7 +108,8 @@ type Comment struct {
UpdatedUnix int64 `xorm:"INDEX updated"`

// Reference issue in commit message
CommitSHA string `xorm:"VARCHAR(40)"`
CommitSHA string `xorm:"VARCHAR(40)"`
CommitStatus *CommitStatus `xorm:"-"`

Attachments []*Attachment `xorm:"-"`

Expand Down Expand Up @@ -181,6 +186,14 @@ func (c *Comment) PRURL() string {
return issue.HTMLURL()
}

// TargetURL returns the main link on this comment
func (c *Comment) TargetURL() string {
if c.Type == CommentTypePullPushCommit {
return fmt.Sprintf("%s/%s/commit/%s", setting.AppSubURL, c.Issue.Repo.FullName(), c.CommitSHA)
}
return ""
}

// APIFormat converts a Comment to the api.Comment format
func (c *Comment) APIFormat() *api.Comment {
return &api.Comment{
Expand Down Expand Up @@ -264,6 +277,25 @@ func (c *Comment) LoadAssignees() error {
return nil
}

// LoadCommitStatus if comment.Type is CommentTypePullPushCommit, then load commit status
func (c *Comment) LoadCommitStatus() error {
if c.CommitStatus != nil {
return nil
}

issue, err := getIssueByID(x, c.IssueID)
if err != nil {
return err
}
commitStatuses, err := GetLatestCommitStatus(issue.RepoID, c.CommitSHA, 0)
if err != nil {
return err
}

c.CommitStatus = CalcCommitStatus(commitStatuses)
return nil
}

// MailParticipants sends new comment emails to repository watchers
// and mentioned people.
func (c *Comment) MailParticipants(e Engine, opType ActionType, issue *Issue) (err error) {
Expand Down Expand Up @@ -566,6 +598,141 @@ func CreateRefComment(doer *User, repo *Repository, issue *Issue, content, commi
return err
}

// ClearPullPushComment clear all the push commit on issue since fore push
func ClearPullPushComment(issue *Issue) error {
_, err := x.Delete(&Comment{
Type: CommentTypePullPushCommit,
IssueID: issue.ID,
})
return err
}

func createPullPushCommentsNonForcePush(doer *User, issues []*Issue, commits []*PushCommit) error {
sess := x.NewSession()
defer sess.Close()

if err := sess.Begin(); err != nil {
return err
}

for _, issue := range issues {
for i := len(commits) - 1; i >= 0; i-- {
_, err := createComment(sess, &CreateCommentOptions{
Type: CommentTypePullPushCommit,
Doer: doer,
Repo: issue.Repo,
Issue: issue,
CommitSHA: commits[i].Sha1,
Content: commits[i].Message,
})
if err != nil {
return err
}
}
}

return sess.Commit()
}

// CreatePullPushComments creates commit comments when push commits to a pull request.
func CreatePullPushComments(doer *User, issues []*Issue, commits []*PushCommit, isForcePush bool) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in a previous comment, we can determine which comments need to be deleted from the git rev-list command that is run in CommitRepoAction(..). I believe that would be simpler and more efficient (since we have to run git rev-list regardless) than using the database.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested some times. It seems git rev-list returned different commits from opts.Commits.Commits.

Copy link
Member

@ethantkoenig ethantkoenig Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also done several examples (e.g. just removing commits, removing commits and adding new ones), and in every case the commit shas returned by git rev-list were exactly the commits whose comments would need to be removed (i.e. the commits that previously existed but do not exist after the force push)

Do you have an example where git rev-list does not work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. git rev-list is easier than current method. But it may add the master commits to this PR. When I rebase and push to a feature test. Some of commits are already in master and another not. We only add commit comments that not in target branch. See below screenshots is what the method you recommend. 13fabd0 has been pushed to master branch. Then I rebase master to test and then push to 777:test it has been pull requested. Then commit comment 13fabd0 occupied but it should not.

image

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny I think he meant to use rev-list to detect commits that need to be deleted not added

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see. but it's not enough. For my example, 3fabd0261 should not be as a commit comment, because it has been submited to maser. rev-list will not remove 3fabd0261. So if force push, we have to find the HEAD of target branch and remove all commits before that.

if !isForcePush {
return createPullPushCommentsNonForcePush(doer, issues, commits)
}

sess := x.NewSession()
defer sess.Close()

if err := sess.Begin(); err != nil {
return err
}

var commitIDCache = make(map[string]string)
var ok bool
for _, issue := range issues {
var commitID string
var key = fmt.Sprintf("%s/%s", issue.Repo.RepoPath(), issue.PullRequest.BaseBranch)
if commitID, ok = commitIDCache[key]; !ok {
gitRepo, err := git.OpenRepository(issue.Repo.RepoPath())
if err != nil {
return fmt.Errorf("OpenRepository: %v", err)
}
commit, err := gitRepo.GetBranchCommit(issue.PullRequest.BaseBranch)
if err != nil {
return fmt.Errorf("GetBranchCommit: %v", err)
}
commitID = commit.ID.String()
commitIDCache[key] = commitID
}

var startIdx = -1
for i := len(commits) - 1; i >= 0; i-- {
if commitID == commits[i].Sha1 {
startIdx = i
break
}
}
if startIdx > -1 {
commits = commits[:startIdx]
Copy link
Member

@ethantkoenig ethantkoenig Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell, this is not right.

Suppose I have the following (where later-in-the-alphabet commits have a later timestamp; it appears this affects the order of opts.Commits, which is itself kind of concerning)

A---B(master, origin/master)
 \
  C(develop, origin/develop)

and I rebase develop onto master, resulting in

      C'(develop)
     /
A---B(master, origin/master)
 \
  C(origin/develop)

If I force push develop, the opts.Commits will contain [C', C, B], in that order. In this case (since BaseBranch is master), the start index will be 2, and both C' and C will be included in commits[:startIdx].

Copy link
Member

@ethantkoenig ethantkoenig Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it is possible that the head commit of the base branch is not in commits at all.

Suppose I have

A(master, origin/master)
 \
  B(develop, origin/develop)

and I replace commit B with a new commit C locally

  C(develop)
 /
A(master, origin/master)
 \
  B(origin/develop)

If I force push develop, opts.Commits will contain [C, B], in that order. In this case, startIdx will still be -1 after the for loop, and commits will still contain both C and B, which is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C will not in the commits list since it has been commit before. See https://github.com/go-gitea/gitea/blob/master/models/update.go#L257

Copy link
Member

@ethantkoenig ethantkoenig Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny I have added print statements to the beginning of CommitRepoActions (which is run after line 257 of models/update.go), to print the contents of opts.Commits. In both of the cases described in my comments, I have gotten the results that match what my comments describe. If you are unable to reproduce these results, or if I am missing something, please let me know.

}
if len(commits) <= 0 {
continue
}

var alreadyComments []*Comment
err := sess.Find(&alreadyComments, &Comment{
Type: CommentTypePullPushCommit,
IssueID: issue.ID,
})
if err != nil {
return fmt.Errorf("check pull push comment: %v", err)
}

var deleteCommitIDs = make([]int64, 0, len(alreadyComments))
var keepCommentSha1s = make(map[string]struct{})
for _, comment := range alreadyComments {
var find bool
for _, commit := range commits {
if commit.Sha1 == comment.CommitSHA {
find = true
break
}
}
if !find {
deleteCommitIDs = append(deleteCommitIDs, comment.ID)
} else {
keepCommentSha1s[comment.CommitSHA] = struct{}{}
}
}

if len(deleteCommitIDs) > 0 {
if _, err := sess.In("id", deleteCommitIDs).Delete(new(Comment)); err != nil {
return fmt.Errorf("Delete unused commit comment: %v", err)
}
}

for i := len(commits) - 1; i >= 0; i-- {
if _, ok := keepCommentSha1s[commits[i].Sha1]; ok {
continue
}

_, err := createComment(sess, &CreateCommentOptions{
Type: CommentTypePullPushCommit,
Doer: doer,
Repo: issue.Repo,
Issue: issue,
CommitSHA: commits[i].Sha1,
Content: commits[i].Message,
})
if err != nil {
return err
}
}
}

return sess.Commit()
}

// GetCommentByID returns the comment by given ID.
func GetCommentByID(id int64) (*Comment, error) {
c := new(Comment)
Expand Down
1 change: 1 addition & 0 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type PullRequest struct {
HeadBranch string
BaseBranch string
MergeBase string `xorm:"VARCHAR(40)"`
LastCommitID string `xorm:"VARCHAR(40)"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need LastCommitID? It is only used once in assignment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daviian #2519 will need it.

Copy link
Member Author

@lunny lunny Sep 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will help to show the pull's commit status. If we didn't cache it, we will spent many time to invoke git command on that PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for clarification 👍


HasMerged bool `xorm:"INDEX"`
MergedCommitID string `xorm:"VARCHAR(40)"`
Expand Down
8 changes: 2 additions & 6 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -1567,7 +1567,8 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
}

// ChangeRepositoryName changes all corresponding setting from old repository name to new one.
func ChangeRepositoryName(u *User, oldRepoName, newRepoName string) (err error) {
func ChangeRepositoryName(repo *Repository, oldRepoName, newRepoName string) (err error) {
u := repo.Owner
oldRepoName = strings.ToLower(oldRepoName)
newRepoName = strings.ToLower(newRepoName)
if err = IsUsableRepoName(newRepoName); err != nil {
Expand All @@ -1581,11 +1582,6 @@ func ChangeRepositoryName(u *User, oldRepoName, newRepoName string) (err error)
return ErrRepoAlreadyExist{u.Name, newRepoName}
}

repo, err := GetRepositoryByName(u.ID, oldRepoName)
if err != nil {
return fmt.Errorf("GetRepositoryByName: %v", err)
}

// Change repository directory name.
if err = os.Rename(repo.RepoPath(), RepoPath(u.Name, newRepoName)); err != nil {
return fmt.Errorf("rename repository directory: %v", err)
Expand Down
Loading