Replies to outdated code comments should also be outdated #13217

zeripath · 2020-10-20T11:23:06Z

This happened because the comment took the latest commit as its base instead of the
commit_sha of the first comment that it was replying to.

There was also no way of creating an already outdated comment - and a
reply to a review on an outdated line should be outdated.

Signed-off-by: Andrew Thornton art27@cantab.net

…s page This happened because the comment took the latest commitID as its base instead of the reviewID that it was replying to. There was also no way of creating an already outdated comment - and a reply to a review on an outdated line should be outdated. Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2020-10-20T11:23:40Z

I think we can backport this unless there have been other new migrations since the 1.13 branch.

Signed-off-by: Andrew Thornton <art27@cantab.net>

codecov-io · 2020-10-20T17:27:32Z

Codecov Report

Merging #13217 into master will increase coverage by 0.00%.
The diff coverage is 36.27%.

@@           Coverage Diff           @@
##           master   #13217   +/-   ##
=======================================
  Coverage   41.96%   41.96%           
=======================================
  Files         683      684    +1     
  Lines       75279    75360   +81     
=======================================
+ Hits        31589    31628   +39     
- Misses      38524    38563   +39     
- Partials     5166     5169    +3

Impacted Files	Coverage Δ
models/migrations/migrations.go	`2.44% <ø> (ø)`
models/migrations/v156.go	`0.00% <0.00%> (ø)`
services/pull/review.go	`50.72% <65.30%> (+2.54%)`	⬆️
models/issue_comment.go	`52.87% <100.00%> (+0.66%)`	⬆️
services/pull/pull.go	`40.78% <0.00%> (-0.50%)`	⬇️
routers/repo/view.go	`38.11% <0.00%> (+0.64%)`	⬆️
modules/queue/workerpool.go	`62.04% <0.00%> (+2.04%)`	⬆️
models/unit.go	`49.31% <0.00%> (+2.73%)`	⬆️
... and 3 more

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 b50448b...8cba82e. Read the comment docs.

6543 · 2020-10-22T10:02:27Z

@zeripath pleace resolve :)

zeripath · 2020-10-22T12:46:51Z

I think we should still backport just without the migration perhaps adding to doctor instead...

zeripath · 2020-10-22T12:47:14Z

BTW @6543 could you take a look at the WIP of mine that refactored doctor? #12264 (don't worry about the conflict at present - I want to see if the idea makes sense before I fix that.)

…should-have-the-same-sha

…3217) * When replying to an outdated comment it should not appear on the files page This happened because the comment took the latest commitID as its base instead of the reviewID that it was replying to. There was also no way of creating an already outdated comment - and a reply to a review on an outdated line should be outdated. Signed-off-by: Andrew Thornton <art27@cantab.net> * fix test Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>

6543 · 2020-11-05T18:04:22Z

Backport: #13433

…13433) * When replying to an outdated comment it should not appear on the files page This happened because the comment took the latest commitID as its base instead of the reviewID that it was replying to. There was also no way of creating an already outdated comment - and a reply to a review on an outdated line should be outdated. Signed-off-by: Andrew Thornton <art27@cantab.net> * fix test Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io> Co-authored-by: zeripath <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>

lunny · 2020-11-06T05:26:28Z

models/issue_comment.go

@@ -976,6 +980,12 @@ func (opts *FindCommentsOptions) toConds() builder.Cond {
 	if opts.Type != CommentTypeUnknown {
 		cond = cond.And(builder.Eq{"comment.type": opts.Type})
 	}
+	if opts.Line > 0 {


Are there database index on this columns? If not, this will be slow when the database is big.

You only need one index per a query. ReviewID has an index.

lunny · 2020-11-06T05:29:10Z

models/migrations/v158.go

+				AND comment.tree_path = first.tree_path AND comment.line = first.line
+		WHERE comment.type = 21
+			AND comment.id != first.id
+			AND comment.commit_sha != first.commit_sha`).Limit(batchSize, start).Find(&comments); err != nil {


No. SQL and Limit cannot be used both. This may result in horrible memory usage if the sites have many comments.

lunny · 2020-11-06T05:30:22Z

models/migrations/v158.go

+		}
+
+		for _, comment := range comments {
+			if _, err := sess.Table("comment").Cols("commit_sha", "patch", "invalidated").Update(comment); err != nil {


ID(comment.ID)., oh. God, this will update all the comments

…3217)" This reverts commit 3cab3be.

…3217)" (#13439) This reverts commit 3cab3be.

6543 · 2020-11-06T13:23:51Z

@lunny should we revert #13433 too ?!?

CirnoT · 2020-11-06T13:26:17Z

No, only migration is broken.

lunny · 2020-11-06T14:00:39Z

@6543 As @CirnoT said.

6543 · 2020-11-06T15:09:58Z

this could be why I had issues with #13436 ... too

silverwind · 2020-11-06T15:46:57Z

So a migration was reverted which made existing installs unable to start because it won't downgrade the database. Makes me wonder why we don't have a test that tries to start the server which would have failed in this case :)

zeripath added type/bug backport/v1.13 labels Oct 20, 2020

zeripath added this to the 1.14.0 milestone Oct 20, 2020

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 20, 2020

fix test

abc626d

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath force-pushed the comments-on-review-should-have-the-same-sha branch from ca9469e to abc626d Compare October 20, 2020 16:54

Merge branch 'master' into comments-on-review-should-have-the-same-sha

8cba82e

6543 removed the backport/v1.13 label Oct 22, 2020

zeripath added 2 commits October 22, 2020 13:48

Merge remote-tracking branch 'origin/master' into comments-on-review-…

128d19b

…should-have-the-same-sha

Merge remote-tracking branch 'origin/master' into comments-on-review-…

4af6acc

…should-have-the-same-sha

zeripath added the backport/v1.13 label Oct 30, 2020

silverwind approved these changes Oct 30, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 30, 2020

CirnoT approved these changes Nov 4, 2020

View reviewed changes

GiteaBot 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 Nov 4, 2020

Merge branch 'master' into comments-on-review-should-have-the-same-sha

ecf46e6

lafriks approved these changes Nov 4, 2020

View reviewed changes

techknowlogick merged commit 3cab3be into go-gitea:master Nov 4, 2020

6543 mentioned this pull request Nov 5, 2020

Replies to outdated code comments should also be outdated (#13217) #13433

Merged

6543 added the backport/done All backports for this PR have been created label Nov 5, 2020

zeripath deleted the comments-on-review-should-have-the-same-sha branch November 5, 2020 18:19

6543 mentioned this pull request Nov 5, 2020

Doctor: Add FixUpdatableCodeCommentReplies #13436

Closed

lunny reviewed Nov 6, 2020

View reviewed changes

lunny added a commit that referenced this pull request Nov 6, 2020

Revert "Replies to outdated code comments should also be outdated (#1…

c2dee5c

…3217)" This reverts commit 3cab3be.

lunny mentioned this pull request Nov 6, 2020

Revert "Replies to outdated code comments should also be outdated" #13439

Merged

lafriks pushed a commit that referenced this pull request Nov 6, 2020

Revert "Replies to outdated code comments should also be outdated (#1…

35cc82a

…3217)" (#13439) This reverts commit 3cab3be.

lunny mentioned this pull request Nov 6, 2020

Gitea fails to start after #13439 reverted db migration #13440

Closed

4 tasks

zeripath mentioned this pull request Nov 6, 2020

Comments on review should have the same sha #13448

Merged

go-gitea locked and limited conversation to collaborators Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replies to outdated code comments should also be outdated #13217

Replies to outdated code comments should also be outdated #13217

zeripath commented Oct 20, 2020

zeripath commented Oct 20, 2020

codecov-io commented Oct 20, 2020

6543 commented Oct 22, 2020

zeripath commented Oct 22, 2020

zeripath commented Oct 22, 2020 •

edited

Loading

6543 commented Nov 5, 2020

lunny Nov 6, 2020

zeripath Nov 6, 2020

lunny Nov 6, 2020

lunny Nov 6, 2020

6543 commented Nov 6, 2020

CirnoT commented Nov 6, 2020

lunny commented Nov 6, 2020

6543 commented Nov 6, 2020 •

edited

Loading

silverwind commented Nov 6, 2020 •

edited

Loading

Replies to outdated code comments should also be outdated #13217

Replies to outdated code comments should also be outdated #13217

Conversation

zeripath commented Oct 20, 2020

zeripath commented Oct 20, 2020

codecov-io commented Oct 20, 2020

Codecov Report

6543 commented Oct 22, 2020

zeripath commented Oct 22, 2020

zeripath commented Oct 22, 2020 • edited Loading

6543 commented Nov 5, 2020

lunny Nov 6, 2020

Choose a reason for hiding this comment

zeripath Nov 6, 2020

Choose a reason for hiding this comment

lunny Nov 6, 2020

Choose a reason for hiding this comment

lunny Nov 6, 2020

Choose a reason for hiding this comment

6543 commented Nov 6, 2020

CirnoT commented Nov 6, 2020

lunny commented Nov 6, 2020

6543 commented Nov 6, 2020 • edited Loading

silverwind commented Nov 6, 2020 • edited Loading

zeripath commented Oct 22, 2020 •

edited

Loading

6543 commented Nov 6, 2020 •

edited

Loading

silverwind commented Nov 6, 2020 •

edited

Loading