Fix force push time-line commit comments of pull request

[lunny]

Fix #36647
Fix #25827
Fix #25870

[silverwind]

"comments"? I think you mean "timeline entries", right?

[lunny]

"comments"? I think you mean "timeline entries", right?

Yes. But it stored as comment in the database.

[silverwind]

Comment written by Claude Code (claude-opus-4-6)

Some observations from reviewing this PR:

1. Raw DELETE bypasses DeleteComment cleanup logic

The bulk delete at services/pull/comment.go uses a raw XORM delete:

db.GetEngine(ctx).Where("issue_id = ?", pr.IssueID).
    And("type = ?", issues_model.CommentTypePullRequestPush).
    NoAutoCondition().
    Delete(new(issues_model.Comment))

The existing DeleteComment function (models/issues/comment.go:1160) does additional cleanup: deleting ContentHistory records, marking action records as deleted, and neutering cross-references. While push comments likely don't have any of these associated records in practice, bypassing the established delete path could become a problem if future changes add cleanup steps to DeleteComment.

2. getCommitIDsFromRepo is now called unconditionally, including for force pushes

The old code only called getCommitIDsFromRepo for non-force pushes. Now it's called even when isForcePush is true. For a force push where the old commit history was completely rewritten (e.g. a full rebase), CommitsBetweenNotBase(newCommit, oldCommit, baseBranch) is operating on commits where oldCommit is not necessarily an ancestor of newCommit. This probably works fine for typical rebases where the old objects still exist in the repo, but it's a behavioral change worth being aware of.

3. Inconsistent use of isForcePush vs opts.IsForcePush

Inside the transaction, the deletion guard uses the function parameter isForcePush, while the force push comment creation uses opts.IsForcePush. They are set to the same value so there's no actual bug, but it would be cleaner to use one consistently.

[silverwind]

"comments"? I think you mean "timeline entries", right?

Yes. But it stored as comment in the database.

Oh ok, definitely a odd decision, but one that is probably hard to correct. We need to ensure comment-related logic stays intact.

[silverwind]

On more observation: When force-pushing over a single commit, it works as expected and the commit is replaced in the timeline, so I think that issue only showed when force-pushing over multiple commits.

#36666 is a semi-related issue with incorrect status icon on force-pushed commits.

[lunny]

On more observation: When force-pushing over a single commit, it works as expected and the commit is replaced in the timeline, so I think that issue only showed when force-pushing over multiple commits.

#36666 is a semi-related issue with incorrect status icon on force-pushed commits.

I will send another PR to fix that issue after this merged.

[Copilot]

Pull request overview

This PR addresses long-standing issues where force-pushing a PR branch leaves stale “pushed commits” entries in the PR timeline, by clearing prior push timeline comments on force-push and recreating the appropriate entries.

Changes:

• Update CreatePushPullComment to delete existing PR push-timeline comments on force-push, then recreate comments to reflect the new state.
• Wrap force-push comment recreation in a DB transaction to keep deletion/recreation atomic.
• Add a unit test validating that force-push removes prior PR push-timeline comments.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
services/pull/comment.go	Deletes old PR push comments on force-push and recreates timeline comments within a transaction.
services/pull/comment_test.go	Adds a test ensuring old PR push comments are removed when simulating a force-push.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[silverwind]

@lunny let me know when this is ready, then I will test a few force-push scenarios.

[lunny]

@lunny let me know when this is ready, then I will test a few force-push scenarios.

It's ready to review again now.