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

@@ -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)
+	}
+}

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
+}

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)]