Skip to content

Add rebase push display wrong comments bug #35560

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Oct 3, 2025
Merged

Add rebase push display wrong comments bug #35560

merged 11 commits into from
Oct 3, 2025

Conversation

lunny
Copy link
Member

@lunny lunny commented Oct 1, 2025

Fix #35518

@lunny lunny added type/bug backport/v1.24 This PR should be backported to Gitea 1.24 backport/v1.25 labels Oct 1, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 1, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Oct 1, 2025
@lunny lunny changed the title Add rebase push comment missing bug Add rebase push display wrong comments bug Oct 1, 2025
@silverwind
Copy link
Member

Will this erase commits that were force-pushed over from the history? This is a very old bug that I noticed once, and I would appreciate if these commits were erased from history, just like on GitHub.

@lunny
Copy link
Member Author

lunny commented Oct 1, 2025

Will this erase commits that were force-pushed over from the history? This is a very old bug that I noticed once, and I would appreciate if these commits were erased from history, just like on GitHub.

This will fix a bug caused by my previous PR which forgot set IsForceBranch as true. That means this will restore the features before my previous PR and the old push comments will not be deleted.

Are you sure Github will delete the old push comments?
P.S. Changing target branch will delete all the old push comments.

@silverwind
Copy link
Member

silverwind commented Oct 2, 2025 •
edited
Loading

Yes they do. Say for example I push up commits 1,2,3 to a PR and I git rebase -i origin/master and squash commits 2 and 3 and force-push that, they will be erased from the commit history on GitHub, but not in Gitea. Example PR.

Before:

Screenshot 2025-10-02 at 06 37 09

After:

Screenshot 2025-10-02 at 06 37 41

@GiteaBot 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 2, 2025
wxiaoguang
wxiaoguang reviewed Oct 2, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed Oct 2, 2025
View reviewed changes
tests/integration/pull_comment_test.go Outdated
Comment on lines 49 to 53
err = os.WriteFile(dstPath+"/README.md", []byte("Hello, World (Edited Conflict Resolved)\n"), 0o644)
assert.NoError(t, err)
_, _, err = gitcmd.NewCommand().AddArguments("add", "--all").RunStdString(t.Context(), &gitcmd.RunOpts{Dir: dstPath})
assert.NoError(t, err)
doGitCommit(dstPath, "Resolve conflict")(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already doGitAddSomeCommits?

Copy link
Member Author

@lunny lunny Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That function can not be used here because we need to make a conflict line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see why it should duplicate the code by copy&paste

2bea40d

wxiaoguang
wxiaoguang reviewed Oct 2, 2025
View reviewed changes
wxiaoguang
wxiaoguang reviewed Oct 2, 2025
View reviewed changes
@GiteaBot 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 Oct 3, 2025
@lunny lunny merged commit 17c8aa6 into go-gitea:main Oct 3, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Oct 3, 2025
@lunny lunny deleted the lunny/fix_rebase_bug branch October 3, 2025 22:16
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 3, 2025
@lunny @wxiaoguang @GiteaBot
Add rebase push display wrong comments bug (go-gitea#35560)
1800482 
Fix go-gitea#35518

---------

Co-authored-by: wxiaoguang <[email protected]>
@GiteaBot GiteaBot mentioned this pull request Oct 3, 2025
Merged
silverwind pushed a commit that referenced this pull request Oct 4, 2025
@GiteaBot @lunny @wxiaoguang
Add rebase push display wrong comments bug (#35560) (#35580)
006fe2a 
Backport #35560 by @lunny

Fix #35518

Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
rossigee pushed a commit to rossigee/gitea that referenced this pull request Oct 4, 2025
@lunny @wxiaoguang @rossigee
Add rebase push display wrong comments bug (go-gitea#35560)
032ea62 
Fix go-gitea#35518

---------

Co-authored-by: wxiaoguang <[email protected]>
rossigee pushed a commit to rossigee/gitea that referenced this pull request Oct 4, 2025
@lunny @wxiaoguang @rossigee
Add rebase push display wrong comments bug (go-gitea#35560)
0d7d00a 
Fix go-gitea#35518

---------

Co-authored-by: wxiaoguang <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 5, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
06198a4 
* giteaofficial/main:
  fix: auto-expand and auto-scroll for actions logs (go-gitea#35570) (go-gitea#35583)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Fix creating pull request failure when the target branch name is the same as some tag (go-gitea#35552)
  Use bundled version of spectral (go-gitea#35573)
  Add rebase push display wrong comments bug (go-gitea#35560)
  Address some CodeQL security concerns (go-gitea#35572)
  fix(webhook): prevent tag events from bypassing branch filters targets go-gitea#35449 (go-gitea#35567)
  Added button to copy file name in PR files (go-gitea#35509)
  Update JS and PY deps (go-gitea#35565)
  Enable a few more tsconfig options (go-gitea#35553)
  Bump github.com/wneessen/go-mail from 0.6.2 to 0.7.1 (go-gitea#35557)
  add more routes to the "expensive" list (go-gitea#35547)
  Drop json-iterator dependency (go-gitea#35544)
  Add proper error message if session provider can not be created (go-gitea#35520)
  use experimental go json v2 library (go-gitea#35392)
  Use global lock instead of status pool for cron lock (go-gitea#35507)
  Move some functions to gitrepo package (go-gitea#35503)
  Move GetDiverging functions to gitrepo (go-gitea#35524)
  [skip ci] Updated translations via Crowdin
@badhezi
Copy link
Contributor

badhezi commented Oct 8, 2025

@lunny
I think the TestPullComment test might be flaky, I had it fail and pass on the same code (without changes related to it)
Failed: https://github.com/go-gitea/gitea/actions/runs/18338982161/job/52229590592
Passed: https://github.com/go-gitea/gitea/actions/runs/18343618920/job/52244747162

Appears to be from nil pointer on:
testPullCommentRebase > LoadCommentPushCommits > RepositoryFromContextOrOpen

gitea/services/issue/comments.go

Line 174 in 03fce8f

gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, c.Issue.Repo)

@wxiaoguang wxiaoguang mentioned this pull request Oct 9, 2025
Open
@lunny
Copy link
Member Author

lunny commented Oct 14, 2025

@lunny I think the TestPullComment test might be flaky, I had it fail and pass on the same code (without changes related to it) Failed: https://github.com/go-gitea/gitea/actions/runs/18338982161/job/52229590592 Passed: https://github.com/go-gitea/gitea/actions/runs/18343618920/job/52244747162

Appears to be from nil pointer on: testPullCommentRebase > LoadCommentPushCommits > RepositoryFromContextOrOpen

gitea/services/issue/comments.go

Line 174 in 03fce8f

gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, c.Issue.Repo)

I have sent #35647 to try to fix it.

@lunny lunny mentioned this pull request Oct 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@silverwind silverwind silverwind approved these changes

+1 more reviewer

@hiifong hiifong hiifong approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/v1.24 This PR should be backported to Gitea 1.24 backport/v1.25 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code type/bug

Projects

None yet

Milestone

1.26.0

Development

Successfully merging this pull request may close these issues.

Pull Request Timeline is Wrong When Force Pushing

6 participants

@lunny @silverwind @badhezi @wxiaoguang @hiifong @GiteaBot