fix(repo): commit page fails to render unsigned commits with a different committer (#39381)

Since #39229 the commit page header dereferences
`.Verification.CommittingUser` when the committer is not the author.
`Verification` is `nil` for unsigned commits (see `repo.Diff`), so
opening such a commit — a rebased or cherry-picked one, for example —
logs a template error and the page comes out truncated:

```
Render failed: failed to render template: repo/commit_page, error: template error: builtin(bindata):repo/commit_page:138:22 : executing "repo/commit_page" at <.Verification.CommittingUser>: nil pointer evaluating interface {}.CommittingUser
```

This guards the access and adds an integration test that creates a
commit with distinct author and committer identities and checks the page
renders completely (the status stays 200 on a mid-render failure, so the
test looks at the body).

_The fix was worked out with help from an AI assistant; I reviewed and
tested it myself._

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/git/fastimport.go

@@ -29,6 +29,8 @@ type FastImportCommit struct {
 	Ref     string
 	Message string
 	Files   []FastImportFile
+
+	Author, Committer *Signature
 }
 
 // ForceFastImportWithInit is for mainly for testing purpose
@@ -48,15 +50,32 @@ func ForceFastImportWithInit(ctx context.Context, repoLocalPath string, commits
 
 // ForceFastImport is for mainly for testing purpose
 func ForceFastImport(ctx context.Context, repo RepositoryFacade, commits []FastImportCommit) error {
-	var buf bytes.Buffer
+	buf := &bytes.Buffer{}
 	for i, c := range commits {
-		msg := util.IfZero(c.Message, fmt.Sprintf("commit %d", i+1))
-		_, _ = fmt.Fprintf(&buf, "reset %s\n", c.Ref)
-		_, _ = fmt.Fprintf(&buf, "commit %s\nmark :%d\ncommitter Gitea <gitea@example.com> 1500000000 +0000\n", c.Ref, i+1)
-		_, _ = fmt.Fprintf(&buf, "data %d\n%s\n", len(msg), msg)
+		_, _ = fmt.Fprintf(buf, "reset %s\n", c.Ref)
+		_, _ = fmt.Fprintf(buf, "commit %s\n", c.Ref)
+		_, _ = fmt.Fprintf(buf, "mark :%d\n", i+1)
+
+		if c.Author != nil {
+			buf.WriteString("author ")
+			_ = c.Author.Encode(buf)
+			buf.WriteByte('\n')
+		}
+		if c.Committer != nil {
+			buf.WriteString("committer ")
+			_ = c.Committer.Encode(buf)
+			buf.WriteByte('\n')
+		} else {
+			// "committer" is required, so we use a default one if not provided
+			buf.WriteString("committer Gitea <gitea@example.com> 1500000000 +0000\n")
+		}
+
+		msg := util.IfZero(c.Message, fmt.Sprintf("test commit %d", i+1))
+		_, _ = fmt.Fprintf(buf, "data %d\n%s\n", len(msg), msg)
+
 		for _, f := range c.Files {
 			mode := util.IfZero(f.Mode, EntryModeBlob)
-			_, _ = fmt.Fprintf(&buf, "M %s inline %s\ndata %d\n%s\n", mode.String(), f.Path, len(f.Content), f.Content)
+			_, _ = fmt.Fprintf(buf, "M %s inline %s\ndata %d\n%s\n", mode.String(), f.Path, len(f.Content), f.Content)
 		}
 	}
 	buf.WriteString("done\n")

modules/git/signature_nogogit.go

@@ -8,6 +8,7 @@ package git
 
 import (
 	"fmt"
+	"io"
 	"time"
 
 	"gitea.dev/modules/util"
@@ -24,7 +25,13 @@ func (s *Signature) String() string {
 	return fmt.Sprintf("%s <%s>", s.Name, s.Email)
 }
 
-// Decode decodes a byte array representing a signature to signature
+// Encode writes the signature for git commit object (same as gogit's object.Signature Encode method)
+func (s *Signature) Encode(w io.Writer) error {
+	_, err := fmt.Fprintf(w, "%s <%s> %d %s", s.Name, s.Email, max(0, s.When.Unix()), s.When.Format("-0700"))
+	return err
+}
+
+// Decode parses the signature for git commit object (same as gogit's object.Signature Decode method)
 func (s *Signature) Decode(b []byte) {
 	*s = *parseSignatureFromCommitLine(util.UnsafeBytesToString(b))
 }

modules/test/utils.go

@@ -64,7 +64,7 @@ func ParseJSONRedirect(buf []byte) (ret struct {
 }
 
 func IsNormalPageCompleted(s string) bool {
-	return strings.Contains(s, `<footer class="page-footer"`) && strings.Contains(s, `</html>`)
+	return strings.Contains(s, `<footer class="page-footer"`) && strings.HasSuffix(strings.TrimSpace(s), `</html>`)
 }
 
 func MockVariableValue[T any](p *T, v ...T) (reset func()) {

routers/common/errpage.go

@@ -21,7 +21,11 @@ import (
 	"gitea.dev/services/context"
 )
 
-const tplStatus500 templates.TplName = "status/500"
+const (
+	tplStatus500 templates.TplName = "status/500"
+
+	PageInternalServerErrorMark = "status-page-500"
+)
 
 func renderServerErrorPage(w http.ResponseWriter, req *http.Request, respCode int, tmpl templates.TplName, ctxData map[string]any, plainMsg string) {
 	acceptsHTML := false

routers/common/errpage_test.go

@@ -17,8 +17,8 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestRenderPanicErrorPage(t *testing.T) {
-	t.Run("HTML", func(t *testing.T) {
+func TestRenderErrorPage(t *testing.T) {
+	t.Run("PanicHTML", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		req := &http.Request{URL: &url.URL{}, Header: http.Header{"Accept": []string{"text/html"}}}
 		req = req.WithContext(reqctx.NewRequestContextForTest(t))
@@ -33,7 +33,7 @@ func TestRenderPanicErrorPage(t *testing.T) {
 		// the different "footer" is the only way to know whether a page is fully rendered without error.
 		assert.False(t, test.IsNormalPageCompleted(respContent))
 	})
-	t.Run("Plain", func(t *testing.T) {
+	t.Run("ServiceUnavailablePlain", func(t *testing.T) {
 		w := httptest.NewRecorder()
 		req := &http.Request{URL: &url.URL{}}
 		req = req.WithContext(reqctx.NewRequestContextForTest(t))

routers/web/repo/pull_review_test.go

@@ -12,6 +12,7 @@ import (
 	issues_model "gitea.dev/models/issues"
 	"gitea.dev/models/unittest"
 	"gitea.dev/modules/templates"
+	"gitea.dev/routers/common"
 	"gitea.dev/services/context"
 	"gitea.dev/services/contexttest"
 	"gitea.dev/services/pull"
@@ -78,7 +79,7 @@ func TestRenderConversation(t *testing.T) {
 		ctx.Data["ShowOutdatedComments"] = true
 		renderConversation(ctx, preparedComment, "diff")
 		assert.Equal(t, http.StatusOK, resp.Code)
-		assert.NotContains(t, resp.Body.String(), `status-page-500`)
+		assert.NotContains(t, resp.Body.String(), common.PageInternalServerErrorMark)
 	})
 	run("timeline non-existing review", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
 		err := db.TruncateBeans(t.Context(), &issues_model.Review{})
@@ -86,6 +87,6 @@ func TestRenderConversation(t *testing.T) {
 		ctx.Data["ShowOutdatedComments"] = true
 		renderConversation(ctx, preparedComment, "timeline")
 		assert.Equal(t, http.StatusOK, resp.Code)
-		assert.NotContains(t, resp.Body.String(), `status-page-500`)
+		assert.NotContains(t, resp.Body.String(), common.PageInternalServerErrorMark)
 	})
 }

templates/repo/commit_page.tmpl

@@ -135,7 +135,7 @@
 				<span class="flex-text-inline tw-text-text-light">{{ctx.Locale.Tr "repo.diff.committed_at" $authors $committedAt}}</span>
 			{{else}}
 				{{$committerAvatar := ""}}{{$committerDisplayName := ""}}
-				{{if .Verification.CommittingUser}}
+				{{if and .Verification .Verification.CommittingUser}}
 					{{$committerAvatar = ctx.AvatarUtils.Avatar .Verification.CommittingUser 20}}
 					{{$committerDisplayName = HTMLFormat `%s%s` .Verification.CommittingUser.GetShortDisplayNameLinkHTML (ctx.RenderUtils.UserTypeLabel .Verification.CommittingUser)}}
 				{{else}}

templates/repo/settings/options.tmpl

@@ -907,7 +907,6 @@
 		</div>
 		{{end}}
 	</div>
-{{template "repo/settings/layout_footer" .}}
 
 {{if $.CanManagerDangerZone}}
 	{{if .Repository.IsMirror}}
@@ -1083,3 +1082,5 @@
 {{end}}
 
 {{template "repo/settings/push_mirror_sync_modal" .}}
+
+{{template "repo/settings/layout_footer" .}}

tests/integration/integration_test.go

@@ -31,6 +31,7 @@ import (
 	"gitea.dev/modules/web"
 	"gitea.dev/modules/web/middleware"
 	"gitea.dev/routers"
+	"gitea.dev/routers/common"
 	gitea_context "gitea.dev/services/context"
 	"gitea.dev/tests"
 
@@ -289,6 +290,9 @@ func MakeRequest(t testing.TB, rw *RequestWrapper, expectedStatus int) *httptest
 			// don't use "require" which exits the test case and makes "wait group" wait forever
 			assert.Equal(t, expectedStatus, recorder.Code, "Request: %s %s", req.Method, req.URL.String())
 		}
+		if expectedStatus != http.StatusInternalServerError {
+			assert.NotContains(t, recorder.Body.String(), common.PageInternalServerErrorMark, "Request: %s %s, response should not contain internal server error", req.Method, req.URL.String())
+		}
 	}
 	return recorder
 }

tests/integration/repo_commits_test.go

@@ -14,7 +14,10 @@ import (
 	auth_model "gitea.dev/models/auth"
 	"gitea.dev/models/db"
 	git_model "gitea.dev/models/git"
+	repo_model "gitea.dev/models/repo"
+	"gitea.dev/models/unittest"
 	"gitea.dev/modules/commitstatus"
+	"gitea.dev/modules/git"
 	"gitea.dev/modules/json"
 	"gitea.dev/modules/setting"
 	api "gitea.dev/modules/structs"
@@ -77,7 +80,6 @@ func TestRepoCommits(t *testing.T) {
 		const (
 			commitID              = "5099b81332712fe655e34e8dd63574f503f61811"
 			expectedCommitterTime = "2017-08-06T19:56:13+02:00"
-			authorTime            = "2017-08-06T19:55:01+02:00"
 		)
 
 		req := NewRequest(t, "GET", "/user2/repo16/commits/branch/master")
@@ -104,6 +106,24 @@ func TestRepoCommits(t *testing.T) {
 		authorElem := doc.doc.Find(".latest-commit .avatar-stack-names")
 		assert.Equal(t, "6543", strings.TrimSpace(authorElem.Text()))
 	})
+
+	t.Run("CommitterIsNotAuthor", func(t *testing.T) {
+		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		err := git.ForceFastImport(t.Context(), repo1, []git.FastImportCommit{
+			{
+				Ref:       "refs/heads/test-branch-committer",
+				Files:     []git.FastImportFile{{Path: "dummy-file.txt", Content: "dummy-content"}},
+				Author:    &git.Signature{Name: "real-commit-author", Email: "dummy-email1@example.com"},
+				Committer: &git.Signature{Name: "non-author-committer", Email: "dummy-email2@example.com"},
+			},
+		})
+		require.NoError(t, err)
+		commitID, err := git.GetBranchCommitID(t.Context(), repo1, "test-branch-committer")
+		require.NoError(t, err)
+		req := NewRequest(t, "GET", "/user2/repo1/commit/"+commitID)
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		assert.Contains(t, resp.Body.String(), "non-author-committer")
+	})
 }
 
 func TestRepoCommitsWithStatus(t *testing.T) {