diff --git a/modules/git/repo_blame.go b/modules/git/repo_blame.go index 139cdd7be9..d812354af5 100644 --- a/modules/git/repo_blame.go +++ b/modules/git/repo_blame.go @@ -4,20 +4,46 @@ package git import ( + "errors" "fmt" + "regexp" +) + +var ( + ErrBlameFileDoesNotExist = errors.New("the blamed file does not exist") + ErrBlameFileNotEnoughLines = errors.New("the blamed file has not enough lines") + + notEnoughLinesRe = regexp.MustCompile(`^fatal: file .+ has only \d+ lines?\n$`) ) // LineBlame returns the latest commit at the given line -func (repo *Repository) LineBlame(revision, path, file string, line uint) (*Commit, error) { - res, _, err := NewCommand(repo.Ctx, "blame"). +func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, error) { + res, _, gitErr := NewCommand(repo.Ctx, "blame"). AddOptionFormat("-L %d,%d", line, line). AddOptionValues("-p", revision). - AddDashesAndList(file).RunStdString(&RunOpts{Dir: path}) + AddDashesAndList(file).RunStdString(&RunOpts{Dir: repo.Path}) + if gitErr != nil { + stdErr := gitErr.Stderr() + + if stdErr == fmt.Sprintf("fatal: no such path %s in %s\n", file, revision) { + return nil, ErrBlameFileDoesNotExist + } + if notEnoughLinesRe.MatchString(stdErr) { + return nil, ErrBlameFileNotEnoughLines + } + + return nil, gitErr + } + + objectFormat, err := repo.GetObjectFormat() if err != nil { return nil, err } - if len(res) < 40 { - return nil, fmt.Errorf("invalid result of blame: %s", res) + + objectIDLen := objectFormat.FullLength() + if len(res) < objectIDLen { + return nil, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", res) } - return repo.GetCommit(res[:40]) + + return repo.GetCommit(res[:objectIDLen]) } diff --git a/modules/git/repo_blame_test.go b/modules/git/repo_blame_test.go new file mode 100644 index 0000000000..126b95386d --- /dev/null +++ b/modules/git/repo_blame_test.go @@ -0,0 +1,52 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package git + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLineBlame(t *testing.T) { + t.Run("SHA1", func(t *testing.T) { + repo, err := OpenRepository(t.Context(), filepath.Join(testReposDir, "repo1_bare")) + require.NoError(t, err) + defer repo.Close() + + commit, err := repo.LineBlame("HEAD", "foo/link_short", 1) + require.NoError(t, err) + assert.Equal(t, "37991dec2c8e592043f47155ce4808d4580f9123", commit.ID.String()) + + commit, err = repo.LineBlame("HEAD", "foo/link_short", 512) + require.ErrorIs(t, err, ErrBlameFileNotEnoughLines) + assert.Nil(t, commit) + + commit, err = repo.LineBlame("HEAD", "non-existent/path", 512) + require.ErrorIs(t, err, ErrBlameFileDoesNotExist) + assert.Nil(t, commit) + }) + + t.Run("SHA256", func(t *testing.T) { + skipIfSHA256NotSupported(t) + + repo, err := OpenRepository(t.Context(), filepath.Join(testReposDir, "repo1_bare_sha256")) + require.NoError(t, err) + defer repo.Close() + + commit, err := repo.LineBlame("HEAD", "foo/link_short", 1) + require.NoError(t, err) + assert.Equal(t, "6aae864a3d1d0d6a5be0cc64028c1e7021e2632b031fd8eb82afc5a283d1c3d1", commit.ID.String()) + + commit, err = repo.LineBlame("HEAD", "foo/link_short", 512) + require.ErrorIs(t, err, ErrBlameFileNotEnoughLines) + assert.Nil(t, commit) + + commit, err = repo.LineBlame("HEAD", "non-existent/path", 512) + require.ErrorIs(t, err, ErrBlameFileDoesNotExist) + assert.Nil(t, commit) + }) +} diff --git a/services/pull/review.go b/services/pull/review.go index c740328e4c..7d232d6d79 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -9,8 +9,6 @@ import ( "errors" "fmt" "io" - "regexp" - "strings" "forgejo.org/models/db" issues_model "forgejo.org/models/issues" @@ -25,8 +23,6 @@ import ( notify_service "forgejo.org/services/notify" ) -var notEnoughLines = regexp.MustCompile(`fatal: file .* has only \d+ lines?`) - // ErrDismissRequestOnClosedPR represents an error when an user tries to dismiss a review associated to a closed or merged PR. type ErrDismissRequestOnClosedPR struct{} @@ -48,8 +44,8 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error { // If the line got changed the comment is going to be invalidated. func checkInvalidation(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, repo.Path, c.TreePath, uint(c.UnsignedLine())) - if err != nil && (strings.Contains(err.Error(), "fatal: no such path") || notEnoughLines.MatchString(err.Error())) { + 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) } @@ -230,10 +226,10 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // FIXME validate treePath // Get latest commit referencing the commented line // No need for get commit for base branch changes - commit, err := gitRepo.LineBlame(head, gitRepo.Path, treePath, uint(line)) + commit, err := gitRepo.LineBlame(head, treePath, uint64(line)) if err == nil { commitID = commit.ID.String() - } else if !strings.Contains(err.Error(), "exit status 128 - fatal: no such path") && !notEnoughLines.MatchString(err.Error()) { + } 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) } }