Fix the wrong push commits in the pull request when force push

[lunny]

Fix #36905

The changes focus on force-push PR timeline handling and commit range calculation:

• Reworked pull-request push comment creation to use a new gitrepo.GetCommitIDsBetweenReverse helper, with special handling for force pushes (merge-base based range, tolerate missing/invalid old commits, and keep force-push timeline entries).
• Added Comment.GetPushActionContent to parse push comment payloads and used it to delete only non-force-push push comments during force pushes.
• Removed the old Repository.CommitsBetweenNotBase helper from modules/git/repo_commit.go in favor of the new commit ID range helper.
• Added tests for GetCommitIDsBetweenReverse (normal range, notRef filtering, fallback branch usage) and expanded pull comment tests to cover force-push edge cases.

[Copilot]

Pull request overview

This PR addresses #36905 by fixing how commit lists are computed for pull request timeline “push” entries after a force-push, ensuring the timeline reflects the full set of commits currently on the PR branch (not just the delta from the previous head).

Changes:

• Update force-push handling in CreatePushPullComment to compute PR commit IDs via merge-base comparison against the base branch.
• Add a new helper (GetCompareCommitIDsWithMergeBase) in services/git plus unit test coverage for it.
• Add a pull service test case intended to cover “missing old commit” scenarios during force-push.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
services/pull/comment.go	Switch force-push commit enumeration to merge-base-based compare logic.
services/pull/comment_test.go	Add a force-push test case targeting missing/unreachable old commit scenarios.
services/git/compare.go	Introduce GetCompareCommitIDsWithMergeBase helper to list commits from merge-base..head.
services/git/compare_test.go	Add unit test validating the helper’s output against CommitsBetweenNotBase.
services/git/main_test.go	Add TestMain bootstrap for services/git tests (unittest DB init).

---

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

You can also share your feedback on Copilot code review. Take the survey.

[wxiaoguang]

Don't make it depends on "db"

Don't use func TestMain(m *testing.M) { unittest.MainTest(m) }

Why the simple git operation should depend on db?

[silverwind]

In essence the fix should be pretty simple: The commit timeline (and tab count) should always show the actual commits present in the branch, ordered into the timeline with their authoring date. Force-push can rewrite the entire commit timeline, so must invalidate all commits in the timeline.

Could just make it completely git-based and skip storing in the DB entirely, but do whatever perform better, but a DB cache does bring the invalidation complexity.

[silverwind]

Review by Claude on behalf of @silverwind

[Copilot]

Pull request overview

This PR addresses incorrect commit lists shown in pull request timeline “push” comments after a force-push by changing how commit IDs are collected for force-push events.

Changes:

• Update force-push handling to compute commit IDs from the PR base branch to the new head, instead of oldCommitID..newCommitID.
• Add a unit test case intended to cover force-push when the old commit is missing.
• Introduce gitrepo.GetCommitIDsBetween() plus tests for it.

Reviewed changes

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

File	Description
services/pull/comment.go	Changes force-push commit range calculation to a base-ref → new-head comparison.
services/pull/comment_test.go	Adds a regression-oriented test for missing old commit in force-push.
modules/gitrepo/compare.go	Adds a helper to return commit IDs for a rev-list range.
modules/gitrepo/compare_test.go	Adds tests covering the new rev-list helper.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Fixes incorrect “pushed commits” timeline entries after force-pushes by changing how commit ranges are computed, adding a shared gitrepo helper, and extending tests around force-push behavior.

Changes:

• Adjust force-push push-comment behavior to recalculate commit IDs differently and to delete only non-force-push push comments.
• Add gitrepo.GetCommitIDsBetween (with tests) to centralize commit-range retrieval.
• Add Comment.GetPushActionContent and expand pull push-comment tests for force-push scenarios.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
services/pull/comment.go	Changes push-comment commit ID computation and force-push deletion logic.
services/pull/comment_test.go	Adds/adjusts tests for force-push deletion and missing-old-commit scenarios.
modules/gitrepo/compare.go	Introduces GetCommitIDsBetween helper based on git rev-list.
modules/gitrepo/compare_test.go	Adds unit tests for GetCommitIDsBetween.
modules/git/repo_commit.go	Removes CommitsBetweenNotBase implementation.
models/issues/comment.go	Adds GetPushActionContent JSON decoder for PR push comment payloads.

Comments suppressed due to low confidence (1)

services/pull/comment_test.go:173

• The head-vs-base-branch subtest validates comment counts, but it doesn't assert anything about the commit IDs captured in the non-force-push push comment created during a force-push. Given the bug being fixed is incorrect commit lists/counts after force-push, it would be good to fetch the non-force-push comment (where is_force_push is false) and assert that its commit_ids match the expected PR commits and do not include base-only commits.

		comment, err := CreatePushPullComment(t.Context(), pusher, pr, oldCommit.ID.String(), headCommit.ID.String(), true)
		assert.NoError(t, err)
		assert.NotNil(t, comment)
		var createdData issues_model.PushActionContent
		assert.NoError(t, json.Unmarshal([]byte(comment.Content), &createdData))
		assert.True(t, createdData.IsForcePush)

		comments, err = issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{
			IssueID: pr.IssueID,
			Type:    issues_model.CommentTypePullRequestPush,
		})
		assert.NoError(t, err)
		// Two comments should exist now: one regular push comment and one force-push comment.
		assert.Len(t, comments, 2)

---

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

[lunny]

1. GetPushActionContent always returns nil error despite the signature — callers get a silent zero-value struct on malformed content; either propagate the unmarshal error or drop it from the signature entirely

We can do nothing if unmarshal failed.

2. prepareOldCommitCommentsToDelete mutates newData.CommitIDs as a hidden side effect — the name only hints at returning IDs to delete, this will confuse future readers; rename or make the mutation explicit

Renamed at 628d769

3. GetLastCommitIDsBetweenReverse — "Last" and "Reverse" are both confusing here; "Last" refers to the -n limit cap silently dropping old commits, "Reverse" is just the output ordering; GetCommitIDsBetween would be clearer

Renamed at 94345ee

4. cleanUpOldCommitCommentsForNewForcePush logs errors internally and doesn't return them, so db.WithTx2 can't roll back on partial failures — the cleanup can leave comments in an inconsistent state if the content update succeeds but the delete fails

We don't need to roll back here.

5. Passing the zero SHA (EmptyObjectID) as oldRef goes through gitRepo.GetRefCommitID which may not handle it gracefully — the force-push-with-missing-old-commit test case asserts require.NotNil(t, comment) but if GetRefCommitID errors on zero SHA the whole function returns early with created=false; this path needs an explicit guard

d3286f7

6. The commit deduplication logic (stripping already-accounted-for commits from newData) has no test — a subtest where an old comment shares some but not all commits with the new push would cover the core prepareOldCommitCommentsToDelete logic
7. Typo in GetLastCommitIDsBetweenReverse: "--not should be kep as the last parameter" → kept; also the "future versions of git >= 2.28" comment is stale copy-paste, 2.28 shipped in 2020

Fixed.

Written by Claude Code

[silverwind]

Review update (considering previous review)

What's been addressed

1. --not baseBranch regression — Fixed. The non-force-push path now passes pr.BaseBranch as notRef, correctly excluding base branch commits from push timeline entries.

2. CommitsBetweenNotBase removal — Addressed. The notRef parameter in GetCommitIDsBetweenReverse replicates the --not baseBranch semantics.

3. "No merge base" fallback unbounded output — Partially addressed. The limit parameter caps output at 1000. The fallback semantics (listing commits reachable from EITHER ref) are still wrong in principle, but the limit prevents catastrophic behavior.

4. genCmd mutation bug — Fixed. genCmd now creates a new command each call instead of mutating a shared one.

What's still outstanding

The reconciliation approach is still overly complex. reconcileOldCommitCommentsForForcePush modifies old comments in-place (removing commit IDs, deleting empty ones, deduplicating against the new commit set). GitHub simply preserves old push comments unchanged on force-push — old commit links may 404, which is acceptable. The current approach:

• Mutates newData.CommitIDs as a side effect, making data flow hard to trace
• Updates individual old comments inside the transaction, with errors only logged (not returned)
• Has more surface area for edge-case bugs (malformed JSON, partial failures)

Tests still don't cover the reconciliation logic. All test cases create old comments with empty PushActionContent{} (no commit IDs). The reconciliation code's interesting paths — where old comments have real commit IDs that partially overlap with the new force-push commits — are never exercised.

New concerns

GetPushActionContent silently ignores unmarshal errors. _ = json.Unmarshal(...) returns an empty PushActionContent on malformed JSON (no error). During reconciliation, that empty content causes the comment to be deleted (no surviving commit IDs). This could silently destroy valid comments with corrupted content. Either propagate the error or don't return nil error when unmarshal fails.

cleanUpOldCommitCommentsForNewForcePush errors are swallowed inside the transaction. If cleanup partially fails, the transaction continues creating new comments on top of inconsistent state. This should either fail the transaction or the individual updates inside reconcileOldCommitCommentsForForcePush should also be resilient.

Multiple json.Marshal errors silently ignored. dataJSON, _ := json.Marshal(data) appears in 3 places.

[wxiaoguang]

GetPushActionContent silently ignores unmarshal errors. _ = json.Unmarshal(...) returns an empty PushActionContent on malformed JSON (no error).

Tolerate corrupted database values, otherwise json.Unmarshal should never fail.

Multiple json.Marshal errors silently ignored. dataJSON, _ := json.Marshal(data) appears in 3 places.

json.Marshal never fails in these cases.

[wxiaoguang]

cleanUpOldCommitCommentsForNewForcePush errors are swallowed inside the transaction.

It is also a right decision.

---

@silverwind Can you please show your thoughts, instead of just copying AI slop?

You should have your judgement, don't let AI slop waste everyone's time or mislead others.

#37040 (comment)

[wxiaoguang]

https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#ai-contribution-policy

Do not use AI to reply to questions about your issue or pull request. The questions are for you, not an AI model.

I believe it also applies to the reviews & questions: if you are reviewing or questioning, the reviews and questions are from you, not an AI model.

[silverwind]

@lunny will you address #36914 (comment)?

@silverwind Can you please show your thoughts, instead of just copying AI slop?

My requirement is:

• Exact representenation of commits on the branch (likely addressed)
• One force-push message per force-push, no "reconcilation" and no mutation (not addressed)

[lunny]

@lunny will you address #36914 (comment)?

@silverwind Can you please show your thoughts, instead of just copying AI slop?

My requirement is:

• Exact representenation of commits on the branch (likely addressed)
• One force-push message per force-push, no "reconcilation" and no mutation (not addressed)

I don't understand your second requirement? I think the current behavior matched what Github did.

[silverwind]

I guess it is an improvement, but should be iterated upon later.

[silverwind]

Don't have time or motivation to dig deeper now, so I trust it is correct.

[bircni]

I don't feel good with this
@wxiaoguang what it's your opinion?
@Zettat123 as there were more changes could you also have a look at it again?

[wxiaoguang]

I have done enough. My patience is limited.

• Fix the wrong push commits in the pull request when force push #36914 (comment)
• Fix the wrong push commits in the pull request when force push #36914 (comment)
• Fix the wrong push commits in the pull request when force push #36914 (comment)
• Fix the wrong push commits in the pull request when force push #36914 (comment)
• Fix the wrong push commits in the pull request when force push #36914 (comment)

[lunny]

I think we can merge this now. I could fix it if there is any bug.

[bircni]

👍