chore: fix various problems (#39298)

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

modules/charset/escape.go

@@ -28,23 +28,21 @@ func EscapeOptionsForView() EscapeOptions {
 	}
 }
 
-func EscapeControlHTMLTo(html template.HTML, locale translation.Locale, w htmlutil.HTMLWriter, opts ...EscapeOptions) *EscapeStatus {
-	if !setting.UI.AmbiguousUnicodeDetection {
-		w.WriteHTML(html)
-		return &EscapeStatus{}
-	}
-	escaped, _ := EscapeControlReader(strings.NewReader(string(html)), w.OriginWriter(), locale, opts...)
-	return escaped
-}
-
 // EscapeControlHTML escapes the Unicode control sequences in a provided html document
 func EscapeControlHTML(html template.HTML, locale translation.Locale, opts ...EscapeOptions) (escaped *EscapeStatus, output template.HTML) {
-	sb, w := htmlutil.NewHTMLStringWriter()
-	escaped = EscapeControlHTMLTo(html, locale, w, opts...)
-	return escaped, template.HTML(sb.String())
+	if !setting.UI.AmbiguousUnicodeDetection {
+		return &EscapeStatus{}, html
+	}
+	w := &htmlutil.HTMLBuilder{}
+	escaped, _ = EscapeControlReader(strings.NewReader(string(html)), w, locale, opts...)
+	return escaped, w.HTMLString()
 }
 
 // EscapeControlReader escapes the Unicode control sequences in a provided reader of HTML content and writer in a locale and returns the findings as an EscapeStatus
-func EscapeControlReader(reader io.Reader, writer io.Writer, locale translation.Locale, opts ...EscapeOptions) (*EscapeStatus, error) {
-	return escapeStream(locale, reader, writer, opts...)
+func EscapeControlReader(reader io.Reader, writer htmlutil.HTMLWriter, locale translation.Locale, opts ...EscapeOptions) (*EscapeStatus, error) {
+	if !setting.UI.AmbiguousUnicodeDetection {
+		_, err := io.Copy(writer.OriginWriter(), reader)
+		return &EscapeStatus{}, err
+	}
+	return escapeStream(locale, reader, writer.OriginWriter(), opts...)
 }

modules/charset/escape_test.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"testing"
 
+	"gitea.dev/modules/htmlutil"
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/test"
 	"gitea.dev/modules/translation"
@@ -145,7 +146,7 @@ then resh (ר), and finally heh (ה) (which should appear leftmost).`,
 func TestEscapeControlReader(t *testing.T) {
 	for _, tt := range escapeControlTests {
 		t.Run(tt.name, func(t *testing.T) {
-			output := &strings.Builder{}
+			output := &htmlutil.HTMLBuilder{}
 			status, err := EscapeControlReader(strings.NewReader(tt.text), output, &translation.MockLocale{})
 			assert.NoError(t, err)
 			assert.Equal(t, tt.status, *status)

modules/htmlutil/html.go

@@ -90,18 +90,27 @@ func EscapeString(s string) template.HTML {
 }
 
 type HTMLWriter interface {
+	Err() error
 	OriginWriter() io.Writer
 	WriteString(s string) HTMLWriter
 	WriteHTML(s template.HTML) HTMLWriter
 	WriteFormatf(fmt template.HTML, args ...any) HTMLWriter
-	Err() error
 }
 
+var (
+	_ HTMLWriter = (*htmlWriter)(nil)
+	_ HTMLWriter = (*HTMLBuilder)(nil)
+)
+
 type htmlWriter struct {
 	w    io.Writer
 	errs []error
 }
 
+func (h *htmlWriter) Err() error {
+	return errors.Join(h.errs...)
+}
+
 func (h *htmlWriter) OriginWriter() io.Writer {
 	return h.w
 }
@@ -127,10 +136,6 @@ func (h *htmlWriter) WriteFormatf(fmt template.HTML, args ...any) HTMLWriter {
 	return h
 }
 
-func (h *htmlWriter) Err() error {
-	return errors.Join(h.errs...)
-}
-
 func NewHTMLWriter(w io.Writer) HTMLWriter {
 	return &htmlWriter{w: w}
 }
@@ -144,17 +149,29 @@ type HTMLBuilder struct {
 	sb strings.Builder
 }
 
-func (b *HTMLBuilder) WriteString(s string) *HTMLBuilder {
+func (b *HTMLBuilder) Err() error {
+	return nil
+}
+
+func (b *HTMLBuilder) OriginWriter() io.Writer {
+	return &b.sb
+}
+
+func (b *HTMLBuilder) Reset() {
+	b.sb.Reset()
+}
+
+func (b *HTMLBuilder) WriteString(s string) HTMLWriter {
 	b.sb.WriteString(template.HTMLEscapeString(s))
 	return b
 }
 
-func (b *HTMLBuilder) WriteHTML(s template.HTML) *HTMLBuilder {
+func (b *HTMLBuilder) WriteHTML(s template.HTML) HTMLWriter {
 	b.sb.WriteString(string(s))
 	return b
 }
 
-func (b *HTMLBuilder) WriteFormatf(fmt template.HTML, args ...any) *HTMLBuilder {
+func (b *HTMLBuilder) WriteFormatf(fmt template.HTML, args ...any) HTMLWriter {
 	_, _ = HTMLPrintf(&b.sb, fmt, args...)
 	return b
 }

modules/util/string.go

@@ -4,6 +4,7 @@
 package util
 
 import (
+	"iter"
 	"strings"
 	"unsafe"
 )
@@ -131,3 +132,10 @@ func AsciiEqualFold(s, t string) bool {
 	}
 	return true
 }
+
+func StringSplitSeq[T, S ~string](s T, sep S) iter.Seq[T] {
+	f := strings.SplitSeq(string(s), string(sep))
+	return func(yield func(T) bool) {
+		f(func(v string) bool { return yield(T(v)) })
+	}
+}

routers/web/repo/setting/lfs.go

@@ -6,7 +6,7 @@ package setting
 import (
 	"bytes"
 	"fmt"
-	gotemplate "html/template"
+	"html/template"
 	"io"
 	"net/http"
 	"net/url"
@@ -20,6 +20,7 @@ import (
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/git/attribute"
 	"gitea.dev/modules/git/pipeline"
+	"gitea.dev/modules/htmlutil"
 	"gitea.dev/modules/lfs"
 	"gitea.dev/modules/log"
 	repo_module "gitea.dev/modules/repository"
@@ -267,57 +268,48 @@ func LFSFileGet(ctx *context.Context) {
 	buf = buf[:n]
 
 	st := typesniffer.DetectContentType(buf)
-	// FIXME: there is no IsPlainText set, but template uses it
 	ctx.Data["IsTextFile"] = st.IsText()
 	ctx.Data["FileSize"] = meta.Size
 	ctx.Data["RawFileLink"] = fmt.Sprintf("%s/%s/%s.git/info/lfs/objects/%s", setting.AppSubURL, url.PathEscape(ctx.Repo.Repository.OwnerName), url.PathEscape(ctx.Repo.Repository.Name), url.PathEscape(meta.Oid))
 	switch {
-	case st.IsRepresentableAsText():
-		if meta.Size >= setting.UI.MaxDisplayFileSize {
-			ctx.Data["IsFileTooLarge"] = true
-			break
-		}
-
-		if st.IsSvgImage() {
-			ctx.Data["IsImageFile"] = true
-		}
-
-		rd := charset.ToUTF8WithFallbackReader(io.MultiReader(bytes.NewReader(buf), dataRc), charset.ConvertOpts{})
-
-		// Building code view blocks with line number on server side.
-		// FIXME: the logic is not right here: it first calls EscapeControlReader then calls HTMLEscapeString: double-escaping
-		escapedContent := &bytes.Buffer{}
-		ctx.Data["EscapeStatus"], _ = charset.EscapeControlReader(rd, escapedContent, ctx.Locale)
-
-		var output bytes.Buffer
-		lines := strings.Split(escapedContent.String(), "\n")
-		// Remove blank line at the end of file
-		if len(lines) > 0 && lines[len(lines)-1] == "" {
-			lines = lines[:len(lines)-1]
-		}
-		for index, line := range lines {
-			line = gotemplate.HTMLEscapeString(line)
-			if index != len(lines)-1 {
-				line += "\n"
-			}
-			fmt.Fprintf(&output, `<li class="L%d" rel="L%d">%s</li>`, index+1, index+1, line)
-		}
-		ctx.Data["FileContent"] = gotemplate.HTML(output.String())
-
-		output.Reset()
-		for i := 0; i < len(lines); i++ {
-			fmt.Fprintf(&output, `<span id="L%d">%d</span>`, i+1, i+1)
-		}
-		ctx.Data["LineNums"] = gotemplate.HTML(output.String())
-
 	case st.IsVideo():
 		ctx.Data["IsVideoFile"] = true
 	case st.IsAudio():
 		ctx.Data["IsAudioFile"] = true
 	case st.IsImage() && (setting.UI.SVG.Enabled || !st.IsSvgImage()):
 		ctx.Data["IsImageFile"] = true
+	case st.IsRepresentableAsText():
+		if meta.Size >= setting.UI.MaxDisplayFileSize {
+			ctx.Data["IsFileTooLarge"] = true
+			break
+		}
+
+		rd := charset.ToUTF8WithFallbackReader(io.MultiReader(bytes.NewReader(buf), dataRc), charset.ConvertOpts{})
+		fileContentBytes, _ := io.ReadAll(io.LimitReader(rd, setting.UI.MaxDisplayFileSize))
+		fileContentHTML := htmlutil.EscapeString(util.UnsafeBytesToString(fileContentBytes))
+		escapeStatus, fileContentHTML := charset.EscapeControlHTML(fileContentHTML, ctx.Locale)
+
+		output := &htmlutil.HTMLBuilder{}
+		output.WriteHTML(`<table>`)
+		writeLine := func(lineNum int, line template.HTML) {
+			output.WriteFormatf(`<tr><td class="lines-num">%d</td><td class="lines-code"><code class="code-inner">%s</code></td></tr>`, lineNum, line)
+		}
+		prevLineIndex, prevLine := -1, template.HTML("")
+		for line := range util.StringSplitSeq(fileContentHTML, "\n") {
+			if prevLineIndex >= 0 {
+				writeLine(prevLineIndex+1, prevLine)
+			}
+			prevLineIndex, prevLine = prevLineIndex+1, line
+		}
+		if prevLine != "" { // trim last empty line
+			writeLine(prevLineIndex+1, prevLine)
+		}
+		output.WriteHTML(`</table>`)
+
+		ctx.Data["EscapeStatus"] = escapeStatus
+		ctx.Data["FileContentHTML"] = output.HTMLString()
 	default:
-		// TODO: the logic is not the same as "renderFile" in "view.go"
+		// the logic is not the same as "renderFile" in "view.go" because here it just needs to render a simple view
 	}
 	ctx.HTML(http.StatusOK, tplSettingsLFSFile)
 }

routers/web/repo/view.go

@@ -13,7 +13,6 @@ import (
 	"net/http"
 	"net/url"
 	"path"
-	"strings"
 	"time"
 
 	activities_model "gitea.dev/models/activities"
@@ -29,6 +28,7 @@ import (
 	"gitea.dev/modules/charset"
 	"gitea.dev/modules/fileicon"
 	"gitea.dev/modules/git"
+	"gitea.dev/modules/htmlutil"
 	"gitea.dev/modules/lfs"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/markup"
@@ -158,14 +158,14 @@ func markupRenderToHTML(ctx *context.Context, renderCtx *markup.RenderContext, r
 
 	done := make(chan struct{})
 	go func() {
-		sb := &strings.Builder{}
+		sb := &htmlutil.HTMLBuilder{}
 		if markup.RendererNeedPostProcess(renderer) {
 			escaped, _ = charset.EscapeControlReader(markupRd, sb, ctx.Locale, charset.EscapeOptionsForView())
 		} else {
 			escaped = &charset.EscapeStatus{}
-			_, _ = io.Copy(sb, markupRd)
+			_, _ = io.Copy(sb.OriginWriter(), markupRd)
 		}
-		output = template.HTML(sb.String())
+		output = sb.HTMLString()
 		close(done)
 	}()
 

routers/web/repo/wiki.go

@@ -20,6 +20,7 @@ import (
 	"gitea.dev/modules/base"
 	"gitea.dev/modules/charset"
 	"gitea.dev/modules/git"
+	"gitea.dev/modules/htmlutil"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/markup"
 	"gitea.dev/modules/markup/markdown"
@@ -252,13 +253,13 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) {
 	rctx := renderhelper.NewRenderContextRepoWiki(ctx, ctx.Repo.Repository)
 
 	renderFn := func(data []byte) (escaped *charset.EscapeStatus, output template.HTML, err error) {
-		buf := &strings.Builder{}
+		buf := &htmlutil.HTMLBuilder{}
 		markupRd, markupWr := io.Pipe()
 		defer markupWr.Close()
 		done := make(chan struct{})
 		go func() {
 			escaped, _ = charset.EscapeControlReader(markupRd, buf, ctx.Locale, charset.EscapeOptionsForView())
-			output = template.HTML(buf.String())
+			output = buf.HTMLString()
 			buf.Reset()
 			close(done)
 		}()

services/gitdiff/gitdiff.go

@@ -339,7 +339,7 @@ type DiffInlineComputed struct {
 // computeDiffInline makes a DiffInline with computed content, e.g.: Unicode escaping, truncation hint, etc
 func computeDiffInline(s template.HTML, isTruncated bool, locale translation.Locale) DiffInlineComputed {
 	sb, w := htmlutil.NewHTMLStringWriter()
-	status := charset.EscapeControlHTMLTo(s, locale, w)
+	status, _ := charset.EscapeControlReader(strings.NewReader(string(s)), w, locale)
 	if isTruncated {
 		w.WriteFormatf(`<span class="ui label diff-line-truncated">%s</span>`, locale.Tr("repo.diff.line_truncated"))
 	}

templates/repo/settings/lfs.tmpl

@@ -3,8 +3,8 @@
 		<h4 class="ui top attached header">
 			{{ctx.Locale.Tr "repo.settings.lfs_filelist"}} ({{ctx.Locale.Tr "admin.total" .Total}})
 			<div class="ui right">
-				<a class="ui tiny button" href="{{.Link}}/locks">{{ctx.Locale.Tr "repo.settings.lfs_locks"}}</a>
-				<a class="ui primary tiny button" href="{{.Link}}/pointers">&nbsp;{{ctx.Locale.Tr "repo.settings.lfs_findpointerfiles"}}</a>
+				<a class="ui tiny compact button" href="{{.Link}}/locks">{{ctx.Locale.Tr "repo.settings.lfs_locks"}}</a>
+				<a class="ui primary tiny compact button" href="{{.Link}}/pointers">&nbsp;{{ctx.Locale.Tr "repo.settings.lfs_findpointerfiles"}}</a>
 			</div>
 		</h4>
 		<table id="lfs-files-table" class="ui attached segment single line table">
@@ -12,17 +12,21 @@
 				{{range .LFSFiles}}
 					<tr>
 						<td>
-							<a href="{{$.Link}}/show/{{.Oid}}" title="{{.Oid}}" class="ui button tw-font-mono">
+							<a href="{{$.Link}}/show/{{.Oid}}" title="{{.Oid}}" class="ui label commit-id-short">
 								{{ShortSha .Oid}}
 							</a>
 						</td>
 						<td>{{FormatByteSize .Size}}</td>
 						<td>{{DateUtils.TimeSince .CreatedUnix}}</td>
-						<td class="tw-text-right">
-							<a class="ui primary button" href="{{$.Link}}/find?oid={{.Oid}}&size={{.Size}}">{{ctx.Locale.Tr "repo.settings.lfs_findcommits"}}</a>
-							<button type="button" class="ui basic show-modal icon button red" data-modal="#delete-{{.Oid}}">
-								<span class="btn-octicon btn-octicon-danger" data-tooltip-content="{{ctx.Locale.Tr "repo.editor.delete_this_file"}}">{{svg "octicon-trash"}}</span>
-							</button>
+						<td>
+							<div class="flex-text-block tw-justify-end">
+								<a class="ui basic primary tiny compact button" href="{{$.Link}}/find?oid={{.Oid}}&size={{.Size}}">{{ctx.Locale.Tr "repo.settings.lfs_findcommits"}}</a>
+								<button type="button" class="ui basic tiny compact red icon button show-modal" data-tooltip-content="{{ctx.Locale.Tr "repo.editor.delete_this_file"}}"
+									data-modal="#delete-lfs-object" data-modal-header="{{ctx.Locale.Tr "repo.settings.lfs_delete" .Oid}}" data-modal-form.url="{{$.Link}}/delete/{{.Oid}}"
+								>
+									{{svg "octicon-trash" 14}}
+								</button>
+							</div>
 						</td>
 					</tr>
 				{{else}}
@@ -33,20 +37,12 @@
 			</tbody>
 		</table>
 		{{template "base/paginate" .}}
-		{{range .LFSFiles}}
-			<div class="ui g-modal-confirm modal" id="delete-{{.Oid}}">
-				<div class="header">
-					{{ctx.Locale.Tr "repo.settings.lfs_delete" .Oid}}
-				</div>
-				<div class="content">
-					<p>
-						{{ctx.Locale.Tr "repo.settings.lfs_delete_warning"}}
-					</p>
-					<form class="ui form" action="{{$.Link}}/delete/{{.Oid}}" method="post">
-						{{template "base/modal_actions_confirm"}}
-					</form>
-				</div>
-			</div>
-		{{end}}
+		<div class="ui g-modal-confirm modal" id="delete-lfs-object">
+			<div class="header"></div>
+			<form class="content" method="post">
+				<p>{{ctx.Locale.Tr "repo.settings.lfs_delete_warning"}}</p>
+				{{template "base/modal_actions_confirm"}}
+			</form>
+		</div>
 	</div>
 {{template "repo/settings/layout_footer" .}}

templates/repo/settings/lfs_file.tmpl

@@ -12,13 +12,15 @@
 				</div>
 			</h4>
 			<div class="ui bottom attached segment file-view-container">
-				{{template "repo/unicode_escape_prompt" dict "EscapeStatus" .EscapeStatus "root" $}}
-				<div class="file-view {{if .IsPlainText}}plain-text{{else if .IsTextFile}}code-view{{end}}">
+				{{template "repo/unicode_escape_prompt" dict "EscapeStatus" .EscapeStatus}}
+				<div class="file-view">
 					{{if .IsFileTooLarge}}
 						{{template "shared/filetoolarge" dict "RawFileLink" .RawFileLink}}
 					{{else if not .FileSize}}
 						{{template "shared/fileisempty"}}
-					{{else if not .IsTextFile}}
+					{{else if .FileContentHTML}}
+						{{.FileContentHTML}}
+					{{else}}
 						<div class="view-raw">
 							{{if .IsImageFile}}
 								<img loading="lazy" alt="{{$.RawFileLink}}" src="{{$.RawFileLink}}">
@@ -34,15 +36,6 @@
 								<a href="{{$.RawFileLink}}" rel="nofollow" class="tw-p-4">{{ctx.Locale.Tr "repo.file_view_raw"}}</a>
 							{{end}}
 						</div>
-					{{else if .FileSize}}
-						<table>
-							<tbody>
-								<tr>
-									<td class="lines-num">{{.LineNums}}</td>
-									<td class="lines-code"><pre>{{.FileContent}}</pre></td>
-								</tr>
-							</tbody>
-						</table>
 					{{end}}
 				</div>
 			</div>

templates/repo/settings/lfs_pointers.tmpl

@@ -31,12 +31,13 @@
 					{{range .Pointers}}
 						<tr>
 							<td>
-								<a href="{{$.RepoLink}}/raw/blob/{{.SHA}}" rel="nofollow" target="_blank" title="{{.SHA}}" class="ui button tw-font-mono">
+								<a href="{{$.RepoLink}}/raw/blob/{{.SHA}}" rel="nofollow" target="_blank" title="{{.SHA}}" class="ui label commit-id-short">
 									{{ShortSha .SHA}}
 								</a>
 							</td>
 							<td>
-								<a {{if and .Exists .InRepo}}href="{{$.LFSFilesLink}}/show/{{.Oid}}" rel="nofollow" target="_blank"{{end}} title="{{.Oid}}" class="ui button tw-font-mono">
+								{{$lfsObjectViewable := and .Exists .InRepo}}
+								<a href="{{$.LFSFilesLink}}/show/{{.Oid}}" rel="nofollow" target="_blank" title="{{.Oid}}" class="ui label commit-id-short {{if not $lfsObjectViewable}}disabled{{end}}">
 									{{ShortSha .Oid}}
 								</a>
 							</td>

web_src/css/base.css

@@ -324,6 +324,12 @@ a.silenced:hover {
   text-decoration-line: none;
 }
 
+a.disabled {
+  cursor: default;
+  pointer-events: none;
+  opacity: var(--opacity-disabled);
+}
+
 a.label,
 .ui .menu a,
 .ui.cards a.card {
@@ -657,18 +663,6 @@ overflow-menu .ui.label:empty {
   vertical-align: top;
 }
 
-.lines-code pre {
-  background-color: inherit;
-  margin: 0;
-  padding: 0 !important;
-}
-
-.lines-code pre li {
-  display: block;
-  width: calc(100% - 1ch);
-  padding-left: 1ch;
-}
-
 .lines-escape {
   width: 0;
   white-space: nowrap;

web_src/css/modules/button.css

@@ -104,12 +104,12 @@
 .ui.compact.buttons .button,
 .ui.compact.button {
   gap: var(--gap-inline);
-  padding: 0.42em /* around 8px */ 1.07em /* around 15px */;
+  padding: 6px 15px;
   min-height: 32px;
 }
 .ui.compact.icon.buttons .button,
 .ui.compact.icon.button {
-  padding: 0.57em /* around 8px */;
+  padding: 6px 8px;
 }
 
 /* reference size: mini: padding-x=16, height=30 ; compact: padding-x=12, height=26 */

web_src/css/repo.css

@@ -1780,6 +1780,7 @@ tbody.commit-list {
   padding: 10px;
   border: 1px solid transparent;
   border-bottom: none;
+  border-radius: 0;
 }
 
 .branch-selector-dropdown .branch-tag-item.active {