From 324e331184da02b8430a2779264a3fc92038d8b1 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Wed, 21 Oct 2026 09:52:10 +0000 Subject: [PATCH] Anchor patch hunks as git apply does, refuse patches touching non-100644 files by their real mode, survive bare-CR context lines (issue 1604 review) --- modules/gitea/cmd/forge/applypatch.go | 47 ++++++++++++++--- modules/gitea/cmd/forge/applypatch_test.go | 59 +++++++++++++++++++++- modules/gitea/cmd/forge/commit.go | 49 +++++++++++++++++- modules/gitea/cmd/forge/forge_test.go | 43 +++++++++++++++- 4 files changed, 189 insertions(+), 9 deletions(-) diff --git a/modules/gitea/cmd/forge/applypatch.go b/modules/gitea/cmd/forge/applypatch.go index fe01015d..4ae0e49e 100644 --- a/modules/gitea/cmd/forge/applypatch.go +++ b/modules/gitea/cmd/forge/applypatch.go @@ -17,7 +17,7 @@ import ( // a patch that creates an executable file, changes a mode or touches a symlink is not applied here: the tool says so. // fileSource reads a file at the commit the patch is applied to; ok is false when there is no such file. -type fileSource func(path string) (content []byte, ok bool, err error) +type fileSource func(path string) (content []byte, mode string, ok bool, err error) // patchFile is one file section of a patch. type patchFile struct { @@ -33,6 +33,8 @@ type hunk struct { oldStart int old, new []string oldNoNL, newNoNL bool + leading int // unchanged lines before the first change + trailing int // unchanged lines after the last change } var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) @@ -151,28 +153,40 @@ func parsePatch(text string) ([]*patchFile, error) { h.oldNoNL, h.newNoNL = true, true } } + changed := false for oldN > 0 || newN > 0 { i++ if i >= len(lines) { return nil, fmt.Errorf("the hunk of %s at patch line %d ends before its %d old and %d new lines are all there", cur.name(), startLine, oldN, newN) } b := strings.TrimSuffix(lines[i], "\r") - if lines[i] == "" { - b = " " // a blank context line some tools strip to nothing + if b == "" { + b = " " // a blank context line some tools strip to nothing, or a bare CR } switch b[0] { case '\\': noNL() case ' ': - h.old, h.new = append(h.old, lines[i][min(1, len(lines[i])):]), append(h.new, lines[i][min(1, len(lines[i])):]) + text := lines[i] + if strings.HasPrefix(text, " ") { + text = text[1:] // a bare "" or "\r" is the whole blank line, its leading space stripped + } + h.old, h.new = append(h.old, text), append(h.new, text) oldN-- newN-- + if changed { + h.trailing++ + } else { + h.leading++ + } case '-': h.old = append(h.old, lines[i][1:]) oldN-- + changed, h.trailing = true, 0 case '+': h.new = append(h.new, lines[i][1:]) newN-- + changed, h.trailing = true, 0 default: return nil, fmt.Errorf("the hunk of %s at patch line %d has a line %d that is neither context, - nor +: %q", cur.name(), startLine, i+1, lines[i]) } @@ -254,8 +268,26 @@ func (t textFile) apply(h hunk, delta int) (textFile, int, error) { } return true } + // As git apply does: a hunk that starts at line 1 (or 0) begins the file, and a hunk with context but none + // after its last change ends it; a hunk whose old side lacks the final newline ends it too. + atStart := h.oldStart <= 1 + atEnd := (h.trailing == 0 && h.leading > 0) || h.oldNoNL at := -1 - for d := 0; d <= len(t.lines)+1 && at < 0; d++ { + switch { + case atStart && atEnd: + if len(t.lines) == len(h.old) && matches(0) { + at = 0 + } + case atStart: + if matches(0) { + at = 0 + } + case atEnd: + if e := len(t.lines) - len(h.old); matches(e) { + at = e + } + } + for d := 0; !atStart && !atEnd && d <= len(t.lines)+1 && at < 0; d++ { switch { case matches(want - d): at = want - d @@ -298,10 +330,13 @@ func patchAsFiles(text string, read fileSource) ([]FileChange, error) { if _, seen := work[p]; seen { return nil } - b, ok, err := read(p) + b, mode, ok, err := read(p) if err != nil { return err } + if ok && mode != "" && mode != "100644" { + return fmt.Errorf("%s is a file of mode %s, and the forge's files call carries no file mode, so committing it that way would make it 100644", p, mode) + } orig[p] = ok if ok { if b == nil { diff --git a/modules/gitea/cmd/forge/applypatch_test.go b/modules/gitea/cmd/forge/applypatch_test.go index 413c3e37..e5f4a07c 100644 --- a/modules/gitea/cmd/forge/applypatch_test.go +++ b/modules/gitea/cmd/forge/applypatch_test.go @@ -75,7 +75,8 @@ func TestAPatchThatCreatesDeletesAndRenamesIsApplied(t *testing.T) { func TestAHunkMovedByEarlierLinesIsFoundAndNoNewlineIsKept(t *testing.T) { f := sectionFake(t) f.trees["c0"]["x.txt"] = "pre\npre\nabc §\nd\ne" - patch := "--- a/x.txt\n+++ b/x.txt\n@@ -1,3 +1,3 @@\n abc §\n d\n-e\n\\ No newline at end of file\n+f\n\\ No newline at end of file\n" + // the hunk below starts at line 2, though its lines are at 3: the tool finds them one line on + patch := "--- a/x.txt\n+++ b/x.txt\n@@ -2,3 +2,3 @@\n abc §\n d\n-e\n\\ No newline at end of file\n+f\n\\ No newline at end of file\n" got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/n", "patch": patch})) if err != nil { t.Fatal(err) @@ -130,3 +131,59 @@ func TestADefaultBranchIsStillRefusedForAPatch(t *testing.T) { t.Fatalf("%v", err) } } + +// Review round 1. + +func appliedTo(t *testing.T, content, patch string) (string, error) { + t.Helper() + f := sectionFake(t) + f.trees["c0"]["y.txt"] = content + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/y", "patch": patch})) + if err != nil { + return "", err + } + return f.trees[got.Commit]["y.txt"], nil +} + +func TestAHunkAtLineOneMustMatchAtTheStartOfTheFile(t *testing.T) { + patch := "--- a/y.txt\n+++ b/y.txt\n@@ -1,3 +1,3 @@\n-a\n+A\n b\n c\n" + if got, err := appliedTo(t, "X\nb\nc\nzzz\na\nb\nc\n", patch); err == nil { + t.Fatalf("applied in the middle: %q", got) + } + if got, err := appliedTo(t, "a\nb\nc\nzzz\n", patch); err != nil || got != "A\nb\nc\nzzz\n" { + t.Fatalf("%q %v", got, err) + } +} + +func TestAHunkWithNoTrailingContextMustMatchAtTheEndOfTheFile(t *testing.T) { + patch := "--- a/y.txt\n+++ b/y.txt\n@@ -1,3 +1,4 @@\n a\n b\n c\n+NEW\n" + if got, err := appliedTo(t, "a\nb\nc\nd\ne\n", patch); err == nil { + t.Fatalf("added in the middle: %q", got) + } + patch = "--- a/y.txt\n+++ b/y.txt\n@@ -3,3 +3,4 @@\n a\n b\n c\n+NEW\n" + if got, err := appliedTo(t, "x\ny\na\nb\nc\n", patch); err != nil || got != "x\ny\na\nb\nc\nNEW\n" { + t.Fatalf("%q %v", got, err) + } +} + +func TestAnExistingExecutableFileIsNotSilentlyMadeNonExecutable(t *testing.T) { + for _, patch := range []string{ + "diff --git a/run.sh b/run.sh2\nsimilarity index 100%\nrename from run.sh\nrename to run.sh2\n", + "--- a/run.sh\n+++ b/run.sh\n@@ -1,2 +1,2 @@\n #!/bin/sh\n-echo a\n+echo b\n", + } { + f := sectionFake(t) + f.modes = map[string]string{"run.sh": "100755"} + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/m", "patch": patch})) + if err == nil || !strings.Contains(err.Error(), "100755") || len(f.writes) != 0 { + t.Errorf("%v (%d writes)", err, len(f.writes)) + } + } +} + +func TestAPatchWithCRLFLinesAndABareCRContextLineDoesNotPanic(t *testing.T) { + patch := "--- a/y.txt\r\n+++ b/y.txt\r\n@@ -1,3 +1,3 @@\r\n a\r\n\r\n-c\r\n+C\r\n" + got, err := appliedTo(t, "a\r\n\r\nc\r\n", patch) + if err != nil || got != "a\r\n\r\nC\r\n" { + t.Fatalf("%q %v", got, err) + } +} diff --git a/modules/gitea/cmd/forge/commit.go b/modules/gitea/cmd/forge/commit.go index 95cc3318..a2c0245e 100644 --- a/modules/gitea/cmd/forge/commit.go +++ b/modules/gitea/cmd/forge/commit.go @@ -340,7 +340,14 @@ func commit(ctx context.Context, f *Forge, a CommitAsk) (*Committed, error) { forgeApplied = true case errors.As(err, &status) && status.Status >= 500: delete(on, "content") - read := func(p string) ([]byte, bool, error) { return f.contentAt(ctx, repo, p, ref) } + read := func(p string) ([]byte, string, bool, error) { + b, ok, err := f.contentAt(ctx, repo, p, ref) + if err != nil || !ok { + return nil, "", false, err + } + mode, err := f.modeAt(ctx, repo, ref, p) + return b, mode, true, err + } var perr error if files, perr = patchAsFiles(a.Patch, read); perr != nil { return nil, fmt.Errorf("the forge did not take the patch: %w; the tool could not apply it itself: %v", err, perr) @@ -545,3 +552,43 @@ func (f *Forge) contentAt(ctx context.Context, repo, path, ref string) ([]byte, } return raw, true, nil } + +// modeAt is the git mode of a file at a commit, read through the git trees API: the contents API carries none. +func (f *Forge) modeAt(ctx context.Context, repo, ref, path string) (string, error) { + parts := strings.Split(path, "/") + tree := ref + for depth, name := range parts { + found := false + for page := 1; ; page++ { + var got struct { + Tree []struct { + Path string `json:"path"` + Mode string `json:"mode"` + Type string `json:"type"` + SHA string `json:"sha"` + } `json:"tree"` + Total int `json:"total_count"` + } + if err := f.GetJSON(ctx, fmt.Sprintf("%s/git/trees/%s?per_page=1000&page=%d", repo, url.PathEscape(tree), page), &got); err != nil { + return "", fmt.Errorf("the mode of %s could not be read: %w", path, err) + } + for _, e := range got.Tree { + if e.Path != name { + continue + } + if depth == len(parts)-1 { + return e.Mode, nil + } + tree, found = e.SHA, true + break + } + if found || len(got.Tree) == 0 || page*1000 >= got.Total { + break + } + } + if !found && depth < len(parts)-1 { + return "", fmt.Errorf("the mode of %s could not be read: %s is not a folder at %s", path, name, short(ref)) + } + } + return "", fmt.Errorf("the mode of %s could not be read: not in the tree of %s", path, short(ref)) +} diff --git a/modules/gitea/cmd/forge/forge_test.go b/modules/gitea/cmd/forge/forge_test.go index 92249ca4..01883b7c 100644 --- a/modules/gitea/cmd/forge/forge_test.go +++ b/modules/gitea/cmd/forge/forge_test.go @@ -46,7 +46,8 @@ type fakeForge struct { parentsBroken bool // and GET git/commits fails parents map[string]string // commit -> parent, as GET git/commits says it sources map[string]*sourceRepo - patchStatus int // the status diffpatch answers instead of taking the patch (issue 1604) + modes map[string]string // path -> git mode when not 100644 + patchStatus int // the status diffpatch answers instead of taking the patch (issue 1604) } type move struct { @@ -131,6 +132,46 @@ func (f *fakeForge) handle(w http.ResponseWriter, r *http.Request) { } reply(map[string]any{"type": "file", "path": p, "sha": blob(content), "size": len(content), "encoding": "base64", "content": base64.StdEncoding.EncodeToString([]byte(content))}) + case r.Method == "GET" && strings.HasPrefix(path, repo+"/git/trees/"): + id := strings.TrimPrefix(path, repo+"/git/trees/") + commitID, dir := id, "" + if strings.HasPrefix(id, "tree:") { + parts := strings.SplitN(id, ":", 3) + commitID, dir = parts[1], parts[2] + } + if c, isBranch := f.branches[commitID]; isBranch { + commitID = c + } + var entries []map[string]any + seen := map[string]bool{} + for p := range f.trees[commitID] { + if dir != "" { + if !strings.HasPrefix(p, dir+"/") { + continue + } + p = strings.TrimPrefix(p, dir+"/") + } + name, rest, nested := strings.Cut(p, "/") + if seen[name] { + continue + } + seen[name] = true + full := name + if dir != "" { + full = dir + "/" + name + } + if nested { + _ = rest + entries = append(entries, map[string]any{"path": name, "mode": "040000", "type": "tree", "sha": "tree:" + commitID + ":" + full}) + continue + } + mode := "100644" + if m, ok := f.modes[full]; ok { + mode = m + } + entries = append(entries, map[string]any{"path": name, "mode": mode, "type": "blob", "sha": blob(f.trees[commitID][full])}) + } + reply(map[string]any{"tree": entries, "total_count": len(entries)}) case r.Method == "POST" && path == repo+"/contents": var body map[string]any _ = json.NewDecoder(r.Body).Decode(&body)