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

@@ -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 {

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

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

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)