Add commit comment when push to pull request branch #2548

lunny · 2017-09-19T03:31:15Z

ethantkoenig · 2017-09-19T03:40:24Z

Should these comments be removed on a force push (like in Github)?

lunny · 2017-09-19T04:16:06Z

@ethantkoenig yes, I think you are right. I will update this.

jonasfranz · 2017-09-19T06:57:34Z

models/issue_comment.go

-	CommitSHA string `xorm:"VARCHAR(40)"`
+	CommitSHA    string        `xorm:"VARCHAR(40)"`
+	Link         string        `xorm:"VARCHAR(2048)"`
+	CommitStatus *CommitStatus `xorm:"-"`


Do we need a migration for this?

What's migration? We didn't change anything, the extra fields only for new comment type.

You added the Link column. Do we need a migration for syncing this?

Currently Link is only for the added comment type.

lafriks · 2017-09-19T07:49:00Z

models/action.go

+			return fmt.Errorf("IssueList.LoadRepositories: %v", err)
+		}
+
+		for _, issue := range issues {


Do we really need to store link to commit? I think we should generate it on the fly as it is internal url and will become invalid if user changes Gitea configuration

If we didn't save link before, then we have to read repo table to know the repo path for every comment. I don't think that's necessary.

The link is a relative link not an absolute link.

yes but it will include AppSubURL that can be changed in settings. Maybe save only issue.Repo.FullName() in link and create new method for comment type that depending on comment type would generate URL (in this case AppSubURL + "/" + comment.Link + "/commit/" + comment.Sha1).
Additional change needed is to update link column on user/org rename and repository rename

lafriks · 2017-09-19T08:48:42Z

models/action.go

+			return fmt.Errorf("IssueList.LoadRepositories: %v", err)
+		}
+
+		for _, issue := range issues {


yes but it will include AppSubURL that can be changed in settings. Maybe save only issue.Repo.FullName() in link and create new method for comment type that depending on comment type would generate URL (in this case AppSubURL + "/" + comment.Link + "/commit/" + comment.Sha1).
Additional change needed is to update link column on user/org rename and repository rename

codecov-io · 2017-09-22T06:39:11Z

Codecov Report

Merging #2548 into master will decrease coverage by 0.22%.
The diff coverage is 1.86%.

@@            Coverage Diff             @@
##           master    #2548      +/-   ##
==========================================
- Coverage   26.85%   26.62%   -0.23%     
==========================================
  Files          89       89              
  Lines       17607    17774     +167     
==========================================
+ Hits         4728     4732       +4     
- Misses      12193    12354     +161     
- Partials      686      688       +2

Impacted Files	Coverage Δ
models/pull.go	`20.38% <ø> (ø)`	⬆️
models/repo.go	`13.03% <0%> (ø)`	⬆️
models/status.go	`3.4% <0%> (ø)`	⬆️
models/issue_comment.go	`23.35% <0%> (-7.25%)`	⬇️
models/action.go	`50.92% <8.82%> (-3.16%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdc4600...ea4e6a0. Read the comment docs.

lunny · 2017-09-22T06:39:49Z

@ethantkoenig done.

lafriks · 2017-09-23T10:06:49Z

templates/repo/issue/view_content/comments.tmpl

+			<a class="ui avatar image" href="{{.Poster.HomeLink}}">
+				<img src="{{.Poster.RelAvatarLink}}">
+			</a>
+			<span class="text grey"><a href="{{AppSubUrl}}/{{.RepoFullName}}/commit/{{.CommitSHA}}">{{.Content | Str2html}}</a></span>


Please create method for Comment type to generate target URL and use it here and lower

daviian · 2017-09-23T10:40:24Z

models/action.go

+			var isForcePush = false
+			if !isNewBranch {
+				// detect force push
+				if git.EmptySHA != opts.Commits.Commits[0].Sha1 {


Can be simplified to if !isNewBranch && git.EmptySHA != opts.Commit.Commits[0].Sha1 {

daviian · 2017-09-23T10:51:16Z

routers/repo/issue.go

@@ -683,6 +683,11 @@ func ViewIssue(ctx *context.Context) {
 				ctx.Handle(500, "LoadAssignees", err)
 				return
 			}
+		} else if comment.Type == models.CommentTypePullPushCommit {


Can we replace this if construct by a switch statement?

daviian · 2017-09-23T10:56:51Z

templates/repo/issue/view_content/comments.tmpl

@@ -1,7 +1,7 @@
 {{range .Issue.Comments}}
 	{{ $createdStr:= TimeSince .Created $.Lang }}

-	<!-- 0 = COMMENT, 1 = REOPEN, 2 = CLOSE, 3 = ISSUE_REF, 4 = COMMIT_REF, 5 = COMMENT_REF, 6 = PULL_REF, 7 = COMMENT_LABEL, 12 = START_TRACKING, 13 = STOP_TRACKING, 14 = ADD_TIME_MANUAL -->
+	<!-- 0 = COMMENT, 1 = REOPEN, 2 = CLOSE, 3 = ISSUE_REF, 4 = COMMIT_REF, 5 = COMMENT_REF, 6 = PULL_REF, 7 = COMMENT_LABEL, 12 = START_TRACKING, 13 = STOP_TRACKING, 14 = ADD_TIME_MANUAL, 15 = CancelTracking, 16 = PullPushCommit -->


Can we keep consistency either using camel case or upper case?

daviian · 2017-09-23T11:38:17Z

models/pull.go

@@ -66,6 +66,7 @@ type PullRequest struct {
 	HeadBranch   string
 	BaseBranch   string
 	MergeBase    string `xorm:"VARCHAR(40)"`
+	LastCommitID string `xorm:"VARCHAR(40)"`


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

@daviian #2519 will need it.

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.

Ok thanks for clarification 👍

lunny · 2017-09-23T12:55:46Z

@daviian @lafriks done and rebased.

lafriks · 2017-09-23T14:27:39Z

BTW what about updating full name when user/org is renamed or repository is renamed/transfered/deleted etc?

ethantkoenig · 2017-09-24T05:28:17Z

models/action.go

+				if err != nil {
+					return fmt.Errorf("rev-list: %v", err)
+				} else if len(output) > 0 {
+					isForcePush = true


I don't think this is right. Suppose I push 3 commits on top of a branch's current head (not a force push)

sha1 newest (my parent is sha2) sha2 older (my parent is sha3) sha3 even older (my parent is the previous head of the branch)

opts.Commits.Commits will contain sha1, sha2 and sha3, in that order. We will run git rev-list sha1 ^sha3, which will return sha1 and sha2, and this push will incorrectly be marked as a force push.

I think it should be this:
output, err := git.NewCommand("rev-list", opts.OldCommitID, "^"+opts.NewCommitID).RunInDir(repo.RepoPath())

ethantkoenig · 2017-09-24T05:28:44Z

models/action.go

+					}
+				}
+
+				for i := 0; i < len(opts.Commits.Commits); i++ {


nit: for _, c := range opts.Commits.Commits

ethantkoenig · 2017-09-24T05:34:35Z

models/issue_comment.go

@@ -571,6 +603,45 @@ func CreateRefComment(doer *User, repo *Repository, issue *Issue, content, commi
 	return err
 }

+// ClearPullPushComent clear all the push commit on issue since fore push
+func ClearPullPushComent(issue *Issue) error {


I don't think this right. Suppose there are two commits that have previously been pushed to a PR:

sha1 newest (my parent is sha2) sha2 older (my parent is something else)

Suppose sha1 is removed via a force push, but sha2 remains. sha2's comment should not be deleted. Even if we recreate it later, the new comment will have a different timestamp than the old deleted one, which affects the in order in which comments are listed.

Since it's a force push the old commit maybe changed also. We cannot detect that. And it seems github also always delete all previous commit comments.

We can detect it. The commits whose comments need to be deleted are exactly the commits listed in the output from the git rev-list ... command.

Also, from what I can tell Github does not delete unaffected commit comments.

@ethantkoenig could you give me some example pull request on github the commit comments are not deleted when force push?

ethantkoenig · 2017-09-24T05:35:09Z

templates/repo/commit_status.tmpl

-{{end}}
-{{if eq .State "warning"}}
-    <a href="{{.TargetURL}}" target=_blank><i class="commit-status warning sign icon yellow"></i></a>
+{{if .}}


This file uses spaces

lafriks · 2017-10-25T15:40:55Z

@lunny can you resolve requested changes before 1.3?

lunny · 2017-10-25T15:51:29Z

@lafriks Yes, I will.

…transfer owner

ethantkoenig · 2017-11-07T06:24:46Z

models/issue_comment.go

+			}
+		}
+		if startIdx > -1 {
+			commits = commits[:startIdx]


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].

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.

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

@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.

ethantkoenig · 2017-11-07T06:32:27Z

models/issue_comment.go

+			}
+		}
+		if startIdx > -1 {
+			commits = commits[:startIdx]


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.

techknowlogick · 2018-10-29T02:48:29Z

@lunny closing as outdated. Please re-open if needed.

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 19, 2017

lunny added this to the 1.3.0 milestone Sep 19, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 19, 2017

lunny added the pr/wip This PR is not ready for review label Sep 19, 2017

lunny mentioned this pull request Sep 19, 2017

Add commit status on repo and user pull request lists #2519

Closed

jonasfranz reviewed Sep 19, 2017

View reviewed changes

lafriks requested changes Sep 19, 2017

View reviewed changes

lunny changed the title ~~Add commit comment when push to pull~~ Add commit comment when push to pull request branch Sep 21, 2017

lunny force-pushed the lunny/add_commit_comment_pull branch 2 times, most recently from bace2ef to b543ab2 Compare September 22, 2017 06:16

lunny removed the pr/wip This PR is not ready for review label Sep 22, 2017

daviian mentioned this pull request Sep 23, 2017

Pull request webhook didn't fired when push to an exist pull request branch #2518

Closed

lafriks requested changes Sep 23, 2017

View reviewed changes

daviian suggested changes Sep 23, 2017

View reviewed changes

lunny force-pushed the lunny/add_commit_comment_pull branch from c646458 to 85b6cef Compare September 23, 2017 12:55

lafriks approved these changes Sep 23, 2017

View reviewed changes

daviian approved these changes Sep 23, 2017

View reviewed changes

ethantkoenig suggested changes Sep 24, 2017

View reviewed changes

daviian mentioned this pull request Oct 11, 2017

Refactor CommitsBefore method to match github go-gitea/git#88

Merged

lunny force-pushed the lunny/add_commit_comment_pull branch from 85b6cef to cde7ed1 Compare October 26, 2017 07:30

lunny added 11 commits November 6, 2017 13:27

detected force push when needed

3219e95

add TargetURL for comment and some refactors

d23dc98

fix force push detect & some improvements

9a5f7a1

some improvements

19a5c66

typo and function refactor

d8dec2c

update repo RefFullName on comment when repo change owner, rename or …

a134401

…transfer owner

simplified code via func

82a7cb9

change the sequence of commit comment save

5cc2686

Remove unecessary commit comments when force push

a5444f8

don't delete duplicated commit comment

9e4d716

remove unused blank line

d6655d2

lunny force-pushed the lunny/add_commit_comment_pull branch from 60f6e9e to d6655d2 Compare November 6, 2017 05:27

remove unused repoFullName from comment

ea4e6a0

ethantkoenig suggested changes Nov 7, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.4.0 Nov 10, 2017

lafriks modified the milestones: 1.4.0, 1.5.0 Jan 17, 2018

techknowlogick modified the milestones: 1.5.0, 1.6.0 May 25, 2018

techknowlogick modified the milestones: 1.6.0, 1.7.0 Aug 28, 2018

techknowlogick closed this Oct 29, 2018

bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 29, 2018

lafriks removed this from the 1.7.0 milestone Oct 30, 2018

lunny deleted the lunny/add_commit_comment_pull branch November 18, 2020 04:34

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Add commit comment when push to pull request branch #2548

Add commit comment when push to pull request branch #2548

Conversation

lunny commented Sep 19, 2017 • edited by lafriks Loading

ethantkoenig commented Sep 19, 2017 • edited Loading

lunny commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 22, 2017 • edited Loading

Codecov Report

lunny commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Sep 23, 2017 • edited Loading

lafriks commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Oct 29, 2017 • edited Loading

Choose a reason for hiding this comment

ethantkoenig Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Oct 25, 2017

lunny commented Oct 25, 2017

ethantkoenig Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

ethantkoenig Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

ethantkoenig Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

techknowlogick commented Oct 29, 2018

lunny commented Sep 19, 2017 •

edited by lafriks

Loading

ethantkoenig commented Sep 19, 2017 •

edited

Loading

codecov-io commented Sep 22, 2017 •

edited

Loading

lunny Sep 23, 2017 •

edited

Loading

lunny commented Sep 23, 2017 •

edited

Loading

ethantkoenig Oct 27, 2017 •

edited

Loading

ethantkoenig Oct 29, 2017 •

edited

Loading

ethantkoenig Sep 24, 2017 •

edited

Loading

ethantkoenig Nov 7, 2017 •

edited

Loading

ethantkoenig Nov 7, 2017 •

edited

Loading

ethantkoenig Nov 7, 2017 •

edited

Loading

ethantkoenig Nov 7, 2017 •

edited

Loading