chore: refactor LineBlame (#8419)

- Refactor arguments of the function to make more sense.
  - `path` can be inferred from `repo` receiver.
  - `line` can be `uint64`.
- The two calls to this function check for specific errors, do this error checking in the function.
- The ID of a object format is not 40 in the case of SHA256, get the object format and use the correct length.
- Add test coverage for `LineBlame`, notably it checks for the errors that can legitimately happen.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8419
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

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) {
+func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, error) {
-	res, _, err := NewCommand(repo.Ctx, "blame").
+	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])
 }

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)
+	})
+}

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()))
+	commit, err := repo.LineBlame(branch, c.TreePath, c.UnsignedLine())
-	if err != nil && (strings.Contains(err.Error(), "fatal: no such path") || notEnoughLines.MatchString(err.Error())) {
+	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)
 			}
 		}