From e6053262e44cad06c9776a2060a094980bcfa4d7 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Wed, 21 Oct 2026 09:20:25 +0000 Subject: [PATCH 1/3] Apply a unified diff in the forge tool itself, for the patches the forge's diffpatch refuses (issue 1604) --- modules/gitea/cmd/forge/applypatch.go | 397 ++++++++++++++++++++++++++ 1 file changed, 397 insertions(+) create mode 100644 modules/gitea/cmd/forge/applypatch.go diff --git a/modules/gitea/cmd/forge/applypatch.go b/modules/gitea/cmd/forge/applypatch.go new file mode 100644 index 00000000..fe01015d --- /dev/null +++ b/modules/gitea/cmd/forge/applypatch.go @@ -0,0 +1,397 @@ +package main + +import ( + "encoding/base64" + "fmt" + "regexp" + "sort" + "strconv" + "strings" + "unicode/utf8" +) + +// A unified diff applied by the tool itself (novox/hq issue 1604). The forge's diffpatch answers 500 with an empty +// message when an unchanged context line holds a non-ASCII character, so a patch the forge did not take is applied +// here: each touched file is read at the commit the commit goes on, its hunks are applied, and the resulting +// files are committed through the same call a files commit uses. The forge's files call carries no file mode, so +// 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) + +// patchFile is one file section of a patch. +type patchFile struct { + oldPath, newPath string // "" when /dev/null + create, remove bool + renamed bool + oldSeen bool + hunks []hunk + modeBlock string // why the files call cannot carry this section, "" when it can +} + +type hunk struct { + oldStart int + old, new []string + oldNoNL, newNoNL bool +} + +var hunkHeader = regexp.MustCompile(`^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@`) + +// gitPath reads a path of a diff header: git quotes one holding unusual bytes in C style. +func gitPath(s string) string { + s = strings.TrimRight(s, "\t ") + if strings.HasPrefix(s, `"`) { + if u, err := strconv.Unquote(s); err == nil { + return u + } + } else if i := strings.IndexByte(s, '\t'); i >= 0 { + s = s[:i] + } + return s +} + +func stripPrefix(p string) string { + if p == "/dev/null" { + return "" + } + if i := strings.IndexByte(p, '/'); i >= 0 && (p[:i] == "a" || p[:i] == "b") { + return p[i+1:] + } + return p +} + +func nonRegular(mode string) bool { return mode != "" && mode != "100644" } + +// parsePatch reads a unified diff into its file sections. +func parsePatch(text string) ([]*patchFile, error) { + lines := strings.Split(text, "\n") + if n := len(lines); n > 0 && lines[n-1] == "" { + lines = lines[:n-1] + } + var files []*patchFile + var cur *patchFile + block := func(f *patchFile, what string) { + if f.modeBlock == "" { + f.modeBlock = what + } + } + for i := 0; i < len(lines); i++ { + l := strings.TrimSuffix(lines[i], "\r") + switch { + case strings.HasPrefix(l, "diff --git "): + cur = &patchFile{} + files = append(files, cur) + rest := strings.TrimPrefix(l, "diff --git ") + if k := strings.Index(rest, " b/"); k > 0 && !strings.HasPrefix(rest, `"`) { + cur.oldPath, cur.newPath = stripPrefix(rest[:k]), stripPrefix(rest[k+1:]) + } + case cur != nil && strings.HasPrefix(l, "new file mode "): + cur.create = true + if m := strings.TrimPrefix(l, "new file mode "); nonRegular(m) { + block(cur, "creates a file of mode "+m) + } + case cur != nil && strings.HasPrefix(l, "deleted file mode "): + cur.remove = true + case cur != nil && strings.HasPrefix(l, "old mode "): + block(cur, "changes the file mode ("+l+")") + case cur != nil && strings.HasPrefix(l, "new mode "): + block(cur, "changes the file mode ("+l+")") + case cur != nil && strings.HasPrefix(l, "index "): + if f := strings.Fields(l); len(f) == 3 && nonRegular(f[2]) { + block(cur, "touches a file of mode "+f[2]) + } + case cur != nil && strings.HasPrefix(l, "rename from "): + cur.oldPath, cur.renamed = gitPath(strings.TrimPrefix(l, "rename from ")), true + case cur != nil && strings.HasPrefix(l, "rename to "): + cur.newPath, cur.renamed = gitPath(strings.TrimPrefix(l, "rename to ")), true + case strings.HasPrefix(l, "copy from "), strings.HasPrefix(l, "copy to "): + return nil, fmt.Errorf("a copy in a patch (%s) is not applied by the tool", l) + case strings.HasPrefix(l, "GIT binary patch"), strings.HasPrefix(l, "Binary files "): + return nil, fmt.Errorf("a binary patch is not applied by the tool") + case strings.HasPrefix(l, "--- "): + if cur == nil || cur.oldSeen { + cur = &patchFile{} + files = append(files, cur) + } + p := stripPrefix(gitPath(strings.TrimPrefix(l, "--- "))) + cur.oldPath = p + if p == "" { + cur.create = true + } + cur.oldSeen = true + if i+1 >= len(lines) || !strings.HasPrefix(lines[i+1], "+++ ") { + return nil, fmt.Errorf("the line %d, %q, is not followed by a +++ line", i+1, l) + } + i++ + q := stripPrefix(gitPath(strings.TrimPrefix(strings.TrimSuffix(lines[i], "\r"), "+++ "))) + cur.newPath = q + if q == "" { + cur.remove = true + } + case cur != nil && hunkHeader.MatchString(l): + m := hunkHeader.FindStringSubmatch(l) + atoi := func(s string, def int) int { + if s == "" { + return def + } + n, _ := strconv.Atoi(s) + return n + } + h := hunk{oldStart: atoi(m[1], 0)} + oldN, newN := atoi(m[2], 1), atoi(m[4], 1) + startLine := i + 1 + noNL := func() { // "\ No newline at end of file" is about the line before it + prev := lines[i-1] + switch { + case prev != "" && prev[0] == '-': + h.oldNoNL = true + case prev != "" && prev[0] == '+': + h.newNoNL = true + default: + h.oldNoNL, h.newNoNL = true, true + } + } + 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 + } + 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])):]) + oldN-- + newN-- + case '-': + h.old = append(h.old, lines[i][1:]) + oldN-- + case '+': + h.new = append(h.new, lines[i][1:]) + newN-- + 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]) + } + } + for i+1 < len(lines) && strings.HasPrefix(lines[i+1], `\`) { + i++ + noNL() + } + cur.hunks = append(cur.hunks, h) + } + } + for _, f := range files { + if f.newPath == "" && f.oldPath == "" { + return nil, fmt.Errorf("a file section of the patch names no file") + } + } + if len(files) == 0 { + return nil, fmt.Errorf("the patch holds no file section") + } + return files, nil +} + +func (f *patchFile) name() string { + if f.newPath != "" { + return f.newPath + } + return f.oldPath +} + +// textFile is a file's content as lines, and whether its last line ends with a newline. +type textFile struct { + lines []string + endNL bool +} + +func splitText(b []byte) textFile { + if len(b) == 0 { + return textFile{endNL: true} + } + s := string(b) + endNL := strings.HasSuffix(s, "\n") + s = strings.TrimSuffix(s, "\n") + return textFile{lines: strings.Split(s, "\n"), endNL: endNL} +} + +func (t textFile) bytes() []byte { + if len(t.lines) == 0 { + return nil + } + s := strings.Join(t.lines, "\n") + if t.endNL { + s += "\n" + } + return []byte(s) +} + +// apply puts one hunk on the file, at where the hunk says or, as git apply does, at the nearest place its old +// lines are found exactly; delta is how far earlier hunks of this file moved lines. +func (t textFile) apply(h hunk, delta int) (textFile, int, error) { + want := h.oldStart - 1 + delta + if len(h.old) == 0 && h.oldStart > 0 { + want = h.oldStart + delta // a pure insertion is "after" the line it names + } + if want < 0 { + want = 0 + } + matches := func(at int) bool { + if at < 0 || at+len(h.old) > len(t.lines) { + return false + } + for k, l := range h.old { + if t.lines[at+k] != l { + return false + } + } + // the old side's last line lacks a newline only when the file's does + if at+len(h.old) == len(t.lines) && len(h.old) > 0 && h.oldNoNL == t.endNL { + return false + } + return true + } + at := -1 + for d := 0; d <= len(t.lines)+1 && at < 0; d++ { + switch { + case matches(want - d): + at = want - d + case matches(want + d): + at = want + d + } + } + if at < 0 { + return t, delta, fmt.Errorf("the hunk at line %d does not apply: its context and removed lines are not in the file (first of them: %q)", h.oldStart, firstOf(h.old)) + } + out := append(append(append([]string{}, t.lines[:at]...), h.new...), t.lines[at+len(h.old):]...) + endNL := t.endNL + if at+len(h.old) == len(t.lines) && (len(h.new) > 0 || len(h.old) > 0) { + endNL = !h.newNoNL + } + return textFile{lines: out, endNL: endNL}, delta + len(h.new) - len(h.old), nil +} + +func firstOf(l []string) string { + if len(l) == 0 { + return "" + } + return l[0] +} + +// patchAsFiles applies the patch to the files read through read, and says the files to commit. +func patchAsFiles(text string, read fileSource) ([]FileChange, error) { + sections, err := parsePatch(text) + if err != nil { + return nil, err + } + for _, s := range sections { + if s.modeBlock != "" { + return nil, fmt.Errorf("%s %s, and the forge's files call carries no file mode", s.name(), s.modeBlock) + } + } + work := map[string][]byte{} // the files as they are being changed; nil means absent + orig := map[string]bool{} // which paths existed + load := func(p string) error { + if _, seen := work[p]; seen { + return nil + } + b, ok, err := read(p) + if err != nil { + return err + } + orig[p] = ok + if ok { + if b == nil { + b = []byte{} + } + work[p] = b + } else { + work[p] = nil + } + return nil + } + for _, s := range sections { + from, to := s.oldPath, s.newPath + if s.create { + from = "" + } + var t textFile + if from != "" { + if err := load(from); err != nil { + return nil, err + } + if work[from] == nil { + return nil, fmt.Errorf("%s: the file is not there at the commit the patch goes on, so the patch does not apply", from) + } + if !utf8.Valid(work[from]) && len(s.hunks) > 0 { + return nil, fmt.Errorf("%s is not UTF-8 text: the tool applies text patches only", from) + } + t = splitText(work[from]) + } else { + if err := load(to); err != nil { + return nil, err + } + if work[to] != nil { + return nil, fmt.Errorf("%s: the patch creates it, but it is already there at the commit the patch goes on", to) + } + t = textFile{endNL: true} + } + delta := 0 + for _, h := range s.hunks { + if t, delta, err = t.apply(h, delta); err != nil { + return nil, fmt.Errorf("%s: %w", s.name(), err) + } + } + switch { + case s.remove: + if len(t.lines) != 0 { + return nil, fmt.Errorf("%s: the patch deletes the file, but lines are left after its hunks", from) + } + work[from] = nil + default: + if s.renamed && from != to { + work[from] = nil + if err := load(to); err != nil { + return nil, err + } + } + body := t.bytes() + if body == nil { + body = []byte{} + } + work[to] = body + } + } + var paths []string + for p := range work { + paths = append(paths, p) + } + sort.Strings(paths) + var out []FileChange + for _, p := range paths { + switch { + case work[p] == nil && !orig[p]: + case work[p] == nil: + out = append(out, FileChange{Path: p, Delete: true}) + default: + fc := FileChange{Path: p, size: len(work[p])} + if utf8.Valid(work[p]) { + s := string(work[p]) + fc.Content = &s + } else { + s := base64Std(work[p]) + fc.ContentBase64 = &s + } + out = append(out, fc) + } + } + if len(out) == 0 { + return nil, fmt.Errorf("the patch changes no file") + } + return out, nil +} + +func base64Std(b []byte) string { return base64.StdEncoding.EncodeToString(b) } From e5cfcb316ae919da25971ba478ce3c3d8c08c7a1 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Wed, 21 Oct 2026 09:21:59 +0000 Subject: [PATCH 2/3] Fall back to the tool's own patch applier when the forge answers 500, with tests (issue 1604) --- modules/gitea/cmd/forge/applypatch_test.go | 132 +++++++++++++++++++++ modules/gitea/cmd/forge/commit.go | 77 +++++++++++- modules/gitea/cmd/forge/forge_test.go | 8 +- 3 files changed, 211 insertions(+), 6 deletions(-) create mode 100644 modules/gitea/cmd/forge/applypatch_test.go diff --git a/modules/gitea/cmd/forge/applypatch_test.go b/modules/gitea/cmd/forge/applypatch_test.go new file mode 100644 index 00000000..413c3e37 --- /dev/null +++ b/modules/gitea/cmd/forge/applypatch_test.go @@ -0,0 +1,132 @@ +package main + +import ( + "context" + "strings" + "testing" +) + +// novox/hq issue 1604: the forge's diffpatch answers 500 when an unchanged context line holds a non-ASCII +// character; the tool then applies the patch itself. + +func sectionFake(t *testing.T) *fakeForge { + f := newFake(t) + f.trees["c0"]["doc.md"] = "# Title\n\nSee § 4 for the rule.\nold line\nend § here\n" + f.trees["c0"]["run.sh"] = "#!/bin/sh\necho a\n" + f.patchStatus = 500 + return f +} + +const sectionPatch = "diff --git a/doc.md b/doc.md\n--- a/doc.md\n+++ b/doc.md\n@@ -2,4 +2,4 @@\n \n See § 4 for the rule.\n-old line\n+new line\n end § here\n" + +func TestAPatchWhoseContextHoldsANonASCIICharacterIsAppliedByTheToolWhenTheForgeAnswers500(t *testing.T) { + f := sectionFake(t) + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": sectionPatch})) + if err != nil { + t.Fatal(err) + } + want := "# Title\n\nSee § 4 for the rule.\nnew line\nend § here\n" + if tree := f.trees[got.Commit]; tree["doc.md"] != want || tree["README.md"] != "hello\n" { + t.Fatalf("tree %v", tree) + } + if len(f.patches) != 1 || len(f.writes) != 1 || f.writes[0]["new_branch"] != "fix/1604" || f.writes[0]["branch"] != "main" { + t.Fatalf("patches %v writes %v", f.patches, f.writes) + } + if len(got.Files) != 1 || got.Files[0].Operation != "update" || got.Parent != "c0" { + t.Fatalf("%+v", got) + } + if !strings.Contains(strings.Join(got.Said, " "), "applied it") { + t.Errorf("not said: %v", got.Said) + } +} + +func TestAPatchTheForgeTakesIsStillTheForgesToApply(t *testing.T) { + f := sectionFake(t) + f.patchStatus = 0 + if _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": sectionPatch})); err != nil { + t.Fatal(err) + } + if len(f.writes) != 0 { + t.Fatal("the tool applied a patch the forge took") + } +} + +func TestAPatchThatCreatesDeletesAndRenamesIsApplied(t *testing.T) { + f := sectionFake(t) + patch := "diff --git a/new.txt b/new.txt\nnew file mode 100644\n--- /dev/null\n+++ b/new.txt\n@@ -0,0 +1,2 @@\n+one §\n+two\n" + + "diff --git a/docs/a.md b/docs/a.md\ndeleted file mode 100644\n--- a/docs/a.md\n+++ /dev/null\n@@ -1 +0,0 @@\n-a\n" + + "diff --git a/README.md b/README.txt\nsimilarity index 80%\nrename from README.md\nrename to README.txt\n--- a/README.md\n+++ b/README.txt\n@@ -1 +1 @@\n-hello\n+hello §\n" + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/r", "patch": patch})) + if err != nil { + t.Fatal(err) + } + tree := f.trees[got.Commit] + if tree["new.txt"] != "one §\ntwo\n" || tree["README.txt"] != "hello §\n" { + t.Fatalf("tree %v", tree) + } + if _, has := tree["README.md"]; has { + t.Error("the renamed file is still there") + } + if _, has := tree["docs/a.md"]; has { + t.Error("the deleted file is still there") + } +} + +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" + got, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/n", "patch": patch})) + if err != nil { + t.Fatal(err) + } + if g := f.trees[got.Commit]["x.txt"]; g != "pre\npre\nabc §\nd\nf" { + t.Fatalf("%q", g) + } +} + +func TestAPatchThatDoesNotApplyIsRefusedNamingTheFileAndLine(t *testing.T) { + f := sectionFake(t) + patch := strings.Replace(sectionPatch, "-old line", "-other line", 1) + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": patch})) + if err == nil || !strings.Contains(err.Error(), "doc.md") || !strings.Contains(err.Error(), "at line 2 does not apply") { + t.Fatalf("%v", err) + } + if len(f.writes) != 0 { + t.Fatal("something was written") + } +} + +func TestAPatchWithAnExecutableFileIsNotAppliedByTheToolAndSaysWhy(t *testing.T) { + f := sectionFake(t) + for _, patch := range []string{ + "diff --git a/s.sh b/s.sh\nnew file mode 100755\n--- /dev/null\n+++ b/s.sh\n@@ -0,0 +1 @@\n+echo §\n", + "diff --git a/run.sh b/run.sh\nold mode 100644\nnew mode 100755\n", + "diff --git a/run.sh b/run.sh\nindex 1111111..2222222 100755\n--- a/run.sh\n+++ b/run.sh\n@@ -1,2 +1,2 @@\n #!/bin/sh\n-echo a\n+echo b\n", + } { + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/x", "patch": patch})) + if err == nil || !strings.Contains(err.Error(), "the forge did not take the patch") || !strings.Contains(err.Error(), "carries no file mode") { + t.Errorf("%v", err) + } + } + if len(f.writes) != 0 { + t.Fatal("something was written") + } +} + +func TestAClientErrorOfTheForgeIsNotRetriedByTheTool(t *testing.T) { + f := sectionFake(t) + f.patchStatus = 422 + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "fix/1604", "patch": sectionPatch})) + if err == nil || len(f.writes) != 0 { + t.Fatalf("%v", err) + } +} + +func TestADefaultBranchIsStillRefusedForAPatch(t *testing.T) { + f := sectionFake(t) + _, err := commit(context.Background(), f.serve(), ask(t, map[string]any{"branch": "main", "patch": sectionPatch})) + if err == nil || !strings.Contains(err.Error(), "ADR 0266") || len(f.patches)+len(f.writes) != 0 { + t.Fatalf("%v", err) + } +} diff --git a/modules/gitea/cmd/forge/commit.go b/modules/gitea/cmd/forge/commit.go index 66e4ced0..95cc3318 100644 --- a/modules/gitea/cmd/forge/commit.go +++ b/modules/gitea/cmd/forge/commit.go @@ -326,14 +326,42 @@ func commit(ctx context.Context, f *Forge, a CommitAsk) (*Committed, error) { var made struct { Commit madeCommit `json:"commit"` } + // A patch goes to the forge's diffpatch first. When the forge answers a server error (it does so, with an + // empty message, when an unchanged context line holds a non-ASCII character: novox/hq issue 1604) the tool + // applies the patch itself and commits the resulting files the way a files commit does. + files := a.Files + forgeApplied := false if a.Patch != "" { on["content"] = a.Patch - if err := f.SendJSON(ctx, http.MethodPost, repo+"/diffpatch", on, &made); err != nil { + err := f.SendJSON(ctx, http.MethodPost, repo+"/diffpatch", on, &made) + var status *forgehttp.StatusError + switch { + case err == nil: + 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) } + 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) + } + total := 0 + for _, fc := range files { + if fc.size > maxFileBytes { + return nil, refuse("%s would be %s, more than the %s one file of a commit may be. Nothing was sent", fc.Path, mib(fc.size), mib(maxFileBytes)) + } + total += fc.size + } + if total > maxCommitBytes { + return nil, refuse("the patch's files are %s together, more than the %s one commit carries. Nothing was sent", mib(total), mib(maxCommitBytes)) + } + out.Said = append(out.Said, fmt.Sprintf("the forge answered %d to the patch, so the tool applied it to the files at %s and committed them", status.Status, short(ref))) + default: return nil, fmt.Errorf("the forge did not take the patch: %w", err) } - } else { - files := a.Files - for _, fc := range a.Files { + } + if !forgeApplied { + for _, fc := range files { if fc.CopyFrom != nil { // Read, and judged against the limits, before anything is sent. var err error @@ -391,7 +419,7 @@ func commit(ctx context.Context, f *Forge, a CommitAsk) (*Committed, error) { moved = "base " + out.Base } how := "each file it changes was checked against " + short(read) + " by its blob, so none of them changed in between" - if a.Patch != "" { + if forgeApplied { how = "the forge applied the patch there, and it refuses a patch whose context changed" } out.Said = append(out.Said, fmt.Sprintf("the %s moved from %s to %s between the read and the commit, so the "+ @@ -478,3 +506,42 @@ func short(sha string) string { } return sha } + +// contentAt is the content of a file at a ref; ok is false when there is no file there. +func (f *Forge) contentAt(ctx context.Context, repo, path, ref string) ([]byte, bool, error) { + seg, err := segments("file path", path) + if err != nil { + return nil, false, err + } + body, err := f.Do(ctx, http.MethodGet, repo+"/contents/"+seg+"?ref="+queryEscape(ref), nil, maxBody) + if err != nil { + if errors.Is(err, errNotFound) { + return nil, false, nil + } + return nil, false, err + } + var entry struct { + Type string `json:"type"` + Size int `json:"size"` + Content string `json:"content"` + Encoding string `json:"encoding"` + } + if err := jsonInto(body, &entry); err != nil { + return nil, false, fmt.Errorf("the forge's answer about %s is not readable: %w", path, err) + } + if entry.Type != "file" { + return nil, false, refuse("%s is a %s, not a file. Nothing was sent", path, entry.Type) + } + if entry.Encoding == "base64" && (entry.Content != "" || entry.Size == 0) { + raw, err := base64.StdEncoding.DecodeString(strings.ReplaceAll(entry.Content, "\n", "")) + if err != nil { + return nil, false, fmt.Errorf("the content of %s is not base64: %w", path, err) + } + return raw, true, nil + } + raw, err := f.Do(ctx, http.MethodGet, repo+"/raw/"+seg+"?ref="+queryEscape(ref), nil, maxFileBytes+1) + if err != nil { + return nil, false, err + } + return raw, true, nil +} diff --git a/modules/gitea/cmd/forge/forge_test.go b/modules/gitea/cmd/forge/forge_test.go index 1d4eb928..92249ca4 100644 --- a/modules/gitea/cmd/forge/forge_test.go +++ b/modules/gitea/cmd/forge/forge_test.go @@ -46,6 +46,7 @@ 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) } type move struct { @@ -128,7 +129,8 @@ func (f *fakeForge) handle(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - reply(map[string]any{"type": "file", "path": p, "sha": blob(content)}) + 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 == "POST" && path == repo+"/contents": var body map[string]any _ = json.NewDecoder(r.Body).Decode(&body) @@ -176,6 +178,10 @@ func (f *fakeForge) handle(w http.ResponseWriter, r *http.Request) { var body map[string]any _ = json.NewDecoder(r.Body).Decode(&body) f.patches = append(f.patches, body) + if f.patchStatus != 0 { + http.Error(w, `{"message":""}`, f.patchStatus) + return + } f.next++ made := fmt.Sprintf("p%d", f.next) from := f.branches[body["branch"].(string)] From 324e331184da02b8430a2779264a3fc92038d8b1 Mon Sep 17 00:00:00 2001 From: mesh-tools Date: Wed, 21 Oct 2026 09:52:10 +0000 Subject: [PATCH 3/3] 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)