From f9a6460cec719fe98594874ff570c7fa17490dc6 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 6 Nov 2025 13:23:35 +0100 Subject: [PATCH] chore: simplify `GetNote` (#9985) Return the Note object (avoid C-style functions). Motivation to refactor this function is to avoid the function that uses last commit cache for git-notes, because it is not needed at the scale of git-notes. In the worst case it can be considered to make a patch to git to get the message and commitID, because git seems to have efficient code to do this (for getting messages, but does not expose the commit id). Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9985 Reviewed-by: Mathieu Fenniak Reviewed-by: Michael Kriese Co-authored-by: Gusted Co-committed-by: Gusted --- modules/git/notes.go | 34 ++++++++++++---------------------- modules/git/notes_test.go | 26 +++++++++++++------------- routers/api/v1/repo/notes.go | 4 ++-- routers/web/repo/commit.go | 3 +-- routers/web/repo/pull.go | 3 +-- 5 files changed, 29 insertions(+), 41 deletions(-) diff --git a/modules/git/notes.go b/modules/git/notes.go index c36ab87fbd..a52314bdd7 100644 --- a/modules/git/notes.go +++ b/modules/git/notes.go @@ -1,4 +1,5 @@ // Copyright 2019 The Gitea Authors. All rights reserved. +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package git @@ -7,7 +8,6 @@ import ( "context" "io" "os" - "strings" "forgejo.org/modules/log" ) @@ -23,16 +23,14 @@ type Note struct { } // GetNote retrieves the git-notes data for a given commit. -// FIXME: Add LastCommitCache support -func GetNote(ctx context.Context, repo *Repository, commitID string, note *Note) error { +func GetNote(ctx context.Context, repo *Repository, commitID string) (*Note, error) { log.Trace("Searching for git note corresponding to the commit %q in the repository %q", commitID, repo.Path) notes, err := repo.GetCommit(NotesRef) if err != nil { - if IsErrNotExist(err) { - return err + if !IsErrNotExist(err) { + log.Error("Unable to get commit from ref %q. Error: %v", NotesRef, err) } - log.Error("Unable to get commit from ref %q. Error: %v", NotesRef, err) - return err + return nil, err } path := "" @@ -58,7 +56,7 @@ func GetNote(ctx context.Context, repo *Repository, commitID string, note *Note) if !IsErrNotExist(err) { log.Error("Unable to find git note corresponding to the commit %q. Error: %v", originalCommitID, err) } - return err + return nil, err } } @@ -66,7 +64,7 @@ func GetNote(ctx context.Context, repo *Repository, commitID string, note *Note) dataRc, err := blob.DataAsync() if err != nil { log.Error("Unable to read blob with ID %q. Error: %v", blob.ID, err) - return err + return nil, err } closed := false defer func() { @@ -77,26 +75,18 @@ func GetNote(ctx context.Context, repo *Repository, commitID string, note *Note) d, err := io.ReadAll(dataRc) if err != nil { log.Error("Unable to read blob with ID %q. Error: %v", blob.ID, err) - return err + return nil, err } _ = dataRc.Close() closed = true - note.Message = d - treePath := "" - if idx := strings.LastIndex(path, "/"); idx > -1 { - treePath = path[:idx] - path = path[idx+1:] - } - - lastCommits, err := GetLastCommitForPaths(ctx, notes, treePath, []string{path}) + lastCommit, err := repo.getCommitByPathWithID(notes.ID, path) if err != nil { - log.Error("Unable to get the commit for the path %q. Error: %v", treePath, err) - return err + log.Error("Unable to get the commit for the path %q. Error: %v", path, err) + return nil, err } - note.Commit = lastCommits[path] - return nil + return &Note{Message: d, Commit: lastCommit}, nil } func SetNote(ctx context.Context, repo *Repository, commitID, notes, doerName, doerEmail string) error { diff --git a/modules/git/notes_test.go b/modules/git/notes_test.go index c7fb433ecf..4dfe0bebf7 100644 --- a/modules/git/notes_test.go +++ b/modules/git/notes_test.go @@ -29,11 +29,11 @@ func TestGetNotes(t *testing.T) { require.NoError(t, err) defer bareRepo1.Close() - note := git.Note{} - err = git.GetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653", ¬e) + note, err := git.GetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653") require.NoError(t, err) assert.Equal(t, []byte("Note contents\n"), note.Message) assert.Equal(t, "Vladimir Panteleev", note.Commit.Author.Name) + assert.Equal(t, "ca6b5ddf303169a72d2a2971acde4f6eea194e5c", note.Commit.ID.String()) } func TestGetNestedNotes(t *testing.T) { @@ -42,13 +42,14 @@ func TestGetNestedNotes(t *testing.T) { require.NoError(t, err) defer repo.Close() - note := git.Note{} - err = git.GetNote(t.Context(), repo, "3e668dbfac39cbc80a9ff9c61eb565d944453ba4", ¬e) + note, err := git.GetNote(t.Context(), repo, "3e668dbfac39cbc80a9ff9c61eb565d944453ba4") require.NoError(t, err) assert.Equal(t, []byte("Note 2"), note.Message) - err = git.GetNote(t.Context(), repo, "ba0a96fa63532d6c5087ecef070b0250ed72fa47", ¬e) + assert.Equal(t, "654c8b6b63c08bf37f638d3f521626b7fbbd4d37", note.Commit.ID.String()) + note, err = git.GetNote(t.Context(), repo, "ba0a96fa63532d6c5087ecef070b0250ed72fa47") require.NoError(t, err) assert.Equal(t, []byte("Note 1"), note.Message) + assert.Equal(t, "654c8b6b63c08bf37f638d3f521626b7fbbd4d37", note.Commit.ID.String()) } func TestGetNonExistentNotes(t *testing.T) { @@ -57,10 +58,10 @@ func TestGetNonExistentNotes(t *testing.T) { require.NoError(t, err) defer bareRepo1.Close() - note := git.Note{} - err = git.GetNote(t.Context(), bareRepo1, "non_existent_sha", ¬e) + note, err := git.GetNote(t.Context(), bareRepo1, "non_existent_sha") require.Error(t, err) - assert.IsType(t, git.ErrNotExist{}, err) + assert.True(t, git.IsErrNotExist(err)) + assert.Nil(t, note) } func TestSetNote(t *testing.T) { @@ -75,8 +76,7 @@ func TestSetNote(t *testing.T) { require.NoError(t, git.SetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653", "This is a new note", "Test", "test@test.com")) - note := git.Note{} - err = git.GetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653", ¬e) + note, err := git.GetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653") require.NoError(t, err) assert.Equal(t, []byte("This is a new note\n"), note.Message) assert.Equal(t, "Test", note.Commit.Author.Name) @@ -95,8 +95,8 @@ func TestRemoveNote(t *testing.T) { require.NoError(t, git.RemoveNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653")) - note := git.Note{} - err = git.GetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653", ¬e) + note, err := git.GetNote(t.Context(), bareRepo1, "95bb4d39648ee7e325106df01a621c530863a653") require.Error(t, err) - assert.IsType(t, git.ErrNotExist{}, err) + assert.True(t, git.IsErrNotExist(err)) + assert.Nil(t, note) } diff --git a/routers/api/v1/repo/notes.go b/routers/api/v1/repo/notes.go index f3ceeaeacf..e458cbd43b 100644 --- a/routers/api/v1/repo/notes.go +++ b/routers/api/v1/repo/notes.go @@ -78,8 +78,8 @@ func getNote(ctx *context.APIContext, identifier string) { return } - var note git.Note - if err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID.String(), ¬e); err != nil { + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID.String()) + if err != nil { if git.IsErrNotExist(err) { ctx.NotFound(identifier) return diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 408a2844de..cc6f1a486c 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -402,8 +402,7 @@ func Diff(ctx *context.Context) { return } - note := &git.Note{} - err = git.GetNote(ctx, ctx.Repo.GitRepo, commitID, note) + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, commitID) if err == nil { ctx.Data["NoteCommit"] = note.Commit ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index dee630d96b..9d617e0c5c 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -997,8 +997,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi return } - note := &git.Note{} - err = git.GetNote(ctx, ctx.Repo.GitRepo, specifiedEndCommit, note) + note, err := git.GetNote(ctx, ctx.Repo.GitRepo, specifiedEndCommit) if err == nil { ctx.Data["NoteCommit"] = note.Commit ctx.Data["NoteAuthor"] = user_model.ValidateCommitWithEmail(ctx, note.Commit)