owen/gitocean
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: display code comments on removed lines-of-code to correct locations in PR view (#12092)

Browse Source 
With the completion of #12015, when a comment is left on a changed line in a pull request, we track the comment against the line of code with `git blame` and then identify where it currently is in any diff with `git blame --reverse`.  However, this strategy only works for the *modified* lines of code -- eg. the `+...` in diffs, and not the `-...` in diffs.  The reason is that `git blame --reverse` can't track a line of code's location past the commit that it was removed in.

To permit comments that are left on lines of code that are removed to appear correctly in the UI, a separate approach is required for those comments.  This PR performs two major changes, which have been complex to figure out, but are reasonably easy to understand:

- When a comment is placed on a removed line in a PR, perform a `git blame --reverse` from the PR's base to the currently viewed commit, and use this information to record in the comment:
    - the **last commit that the line of code existed in** (stored in the `commit_sha` field)
    - the **line of code as of that commit** (stored in the `line` field, negative, to indicate that the comment is on a removal).
    - the **patch** where the comment was placed (stored in the field `patch`); existing functionality unchanged in this PR
- When viewing any diff in the PR, for each comment on a removal, perform a diff from the `commit_sha` (last commit that the line of code existed in) to the current commit being viewed, and verify that within that diff the left-hand-side line removal still exists at the same line of code in the diff, by comparing the current diff with the stored patch.
    - If present, place the commit in the UI at the line number.
    - If the line of code no longer exists in the diff at that point (for example, it was removed, commented upon, and then re-added in a later commit), then the comment is considered outdated and isn't displayed.

The algorithm used for marking a comment as "outdated" is also updated to use this approach.

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests for Go changes

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [x] `make pr-go` before pushing

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12092
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak
2026-04-13 18:26:53 +02:00
committed by Mathieu Fenniak
parent 86898a7d05
commit a797a71dea
8 changed files with 656 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
services/mailer/incoming/incoming_handler.go
+20 -2
View File
@@ -12,6 +12,7 @@ import (
access_model "forgejo.org/models/perm/access"
repo_model "forgejo.org/models/repo"
user_model "forgejo.org/models/user"
"forgejo.org/modules/gitrepo"
"forgejo.org/modules/log"
"forgejo.org/modules/setting"
"forgejo.org/modules/util"
@@ -124,7 +125,24 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
return fmt.Errorf("CreateIssueComment failed: %w", err)
}
case issues_model.CommentTypeCode:
_, err := pull_service.CreateCodeComment(
err := issue.LoadRepo(ctx)
if err != nil {
return fmt.Errorf("LoadRepo failed: %w", err)
}
err = issue.LoadPullRequest(ctx)
if err != nil {
return fmt.Errorf("LoadPullRequest failed: %w", err)
}
repo, err := gitrepo.OpenRepository(ctx, issue.Repo)
defer repo.Close()
if err != nil {
return fmt.Errorf("OpenRepository failed: %w", err)
}
afterCommitID, err := repo.GetRefCommitID(issue.PullRequest.GetGitRefName())
if err != nil {
return fmt.Errorf("GetRefCommitID failed: %w", err)
}
_, err = pull_service.CreateCodeComment(
ctx,
doer,
nil,
@@ -134,7 +152,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
comment.TreePath,
false, // not pending review but a single review
comment.ReviewID,
"",
afterCommitID,
attachmentIDs,
)
if err != nil {
services/pull/pull.go
+3 -3
View File
@@ -263,14 +263,14 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer
return nil
}
func checkForInvalidation(ctx context.Context, requests issues_model.PullRequestList, repoID int64, doer *user_model.User, branch, newCommitID string) error {
func checkForInvalidation(ctx context.Context, requests issues_model.PullRequestList, repoID int64, doer *user_model.User, newCommitID string) error {
repo, err := repo_model.GetRepositoryByID(ctx, repoID)
if err != nil {
return fmt.Errorf("GetRepositoryByIDCtx: %w", err)
}
go func() {
// FIXME: graceful: We need to tell the manager we're doing something...
err := InvalidateCodeComments(ctx, requests, doer, repo, branch, newCommitID)
err := InvalidateCodeComments(ctx, requests, doer, repo, newCommitID)
if err != nil {
log.Error("PullRequestList.InvalidateCodeComments: %v", err)
}
@@ -335,7 +335,7 @@ func TestPullRequest(ctx context.Context, doer *user_model.User, repoID, olderTh
if err = requests.LoadAttributes(ctx); err != nil {
log.Error("PullRequestList.LoadAttributes: %v", err)
}
if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch, newCommitID); invalidationErr != nil {
if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, newCommitID); invalidationErr != nil {
log.Error("checkForInvalidation: %v", invalidationErr)
}
if err == nil {
services/pull/review.go
+28 -31
View File
@@ -42,18 +42,7 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error {
// checkInvalidation checks if the line of code comment got changed by another commit.
// If the line got changed the comment is going to be invalidated.
func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, branch, newCommitID string) error {
if c.Line < 0 {
// Comment is on a removed line of code -- the `git blame --reverse` codepath doesn't work for this. Retain the
// originalbehaviour until a revision is done specific to removed lines:
gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo)
if err != nil {
return fmt.Errorf("failed to open repo: %w", err)
}
defer closer.Close()
return obsoleteCheckInvalidation(ctx, c, gitRepo, branch)
}
func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_model.Repository, newCommitID string) error {
reverseBlame, err := c.ResolveCurrentLine(ctx, repo, newCommitID)
if err != nil {
log.Warn("ResolveCurrentLine failed: %s", err.Error())
@@ -64,25 +53,8 @@ func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *repo_
return nil
}
func obsoleteCheckInvalidation(ctx context.Context, c *issues_model.Comment, repo *git.Repository, branch string) error {
// FIXME differentiate between previous and proposed line
commit, _, err := repo.LineBlame(branch, c.TreePath, c.UnsignedLine())
if err != nil && (errors.Is(err, git.ErrBlameFileDoesNotExist) || errors.Is(err, git.ErrBlameFileNotEnoughLines)) {
c.Invalidated = true
return issues_model.UpdateCommentInvalidate(ctx, c)
}
if err != nil {
return err
}
if c.CommitSHA != "" && c.CommitSHA != commit.ID.String() {
c.Invalidated = true
return issues_model.UpdateCommentInvalidate(ctx, c)
}
return nil
}
// InvalidateCodeComments will lookup the prs for code comments which got invalidated by change
func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, branch, newCommitID string) error {
func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestList, doer *user_model.User, repo *repo_model.Repository, newCommitID string) error {
if len(prs) == 0 {
return nil
}
@@ -98,7 +70,7 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis
return fmt.Errorf("find code comments: %v", err)
}
for _, comment := range codeComments {
if err := checkInvalidation(ctx, comment, repo, branch, newCommitID); err != nil {
if err := checkInvalidation(ctx, comment, repo, newCommitID); err != nil {
return err
}
}
@@ -261,6 +233,31 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
} else {
blamedCommitID = commitID
}
} else {
// Commenting on a line that was removed. In this case, what we want to track in the comment is which line of
// code was this, in the last commit that the line of code actually existed in. We'll use a reverse git blame to
// identify this, from the PR base -> commit being viewed.
blame, err := gitRepo.ReverseLineBlame(pr.MergeBase, treePath, uint64(-1*line), afterCommitID)
if err != nil {
return nil, fmt.Errorf("ReverseLineBlame[%s, %s, %d, %s]: %w", pr.MergeBase, treePath, -1*line, afterCommitID, err)
} else if blame.CommitID == afterCommitID {
// Although this is a comment on the "previous" side of the diff, the reverse blame indicates that the line
// of code still exists in the commit being viewed (eg. it was a comment on a white line in the left-side of
// the diff, not a red removed line). In order to record the right information for where to place this
// commit, we'll convert this into a right-hand comment -- using the present line number that the reverse
// blame gave us:
commit, lineres, err := gitRepo.LineBlame(afterCommitID, treePath, blame.LineNumber)
if err == nil {
blamedCommitID = commit.ID.String()
blamedLine = int64(lineres)
} else if !errors.Is(err, git.ErrBlameFileDoesNotExist) && !errors.Is(err, git.ErrBlameFileNotEnoughLines) {
return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
}
} else {
blamedCommitID = blame.CommitID
// retain negative line numbering to identify we're commenting on the "previous" side of the diff
blamedLine = -1 * int64(blame.LineNumber)
}
}
// Only fetch diff if comment is review comment