fix(gitread): answer a diff with a type change

internal/api/diff_typechange_test.go

@@ -0,0 +1,91 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"maps"
+	"net/http"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/coder/openstorage/internal/auth"
+)
+
+// A type change (a file that becomes a symlink or a submodule, and back)
+// is one entry with state T, and the diff answers 200: git's --patch
+// splits it into a deletion and an addition.
+func TestDiffTypeChange(t *testing.T) {
+	runBackends(t, func(t *testing.T, e *testEnv) {
+		org := e.newOrg(t)
+		repoURL := "https://example.com/" + org + "/typechange-" + randomHex(t, 4)
+		repo, err := e.store.CreateRepo(context.Background(), org, repoURL, "main")
+		if err != nil {
+			t.Fatalf("create repo: %v", err)
+		}
+		dir := t.TempDir()
+		coreGit(t, dir, "init", "-q", "-b", "main", ".")
+		coreWriteFile(t, dir, "typefile", "one\n")
+		coreWriteFile(t, dir, "filetosub", "two\n")
+		coreWriteFile(t, dir, "keep", "x\n")
+		coreGit(t, dir, "add", ".")
+		coreGit(t, dir, "commit", "-q", "-m", "files")
+		files := coreRevParse(t, dir, "HEAD")
+		if err := os.Remove(filepath.Join(dir, "typefile")); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Symlink("keep", filepath.Join(dir, "typefile")); err != nil {
+			t.Fatal(err)
+		}
+		coreGit(t, dir, "rm", "-q", "--cached", "filetosub")
+		if err := os.Remove(filepath.Join(dir, "filetosub")); err != nil {
+			t.Fatal(err)
+		}
+		coreGit(t, dir, "update-index", "--add", "--cacheinfo", "160000,"+files+",filetosub")
+		coreGit(t, dir, "add", "typefile")
+		coreGit(t, dir, "commit", "-q", "-m", "a symlink and a submodule")
+		other := coreRevParse(t, dir, "HEAD")
+		// The files again, on top: the diff reads base...ref.
+		back := strings.TrimSpace(coreGit(t, dir, "commit-tree", "-p", other, "-m", "files again", coreRevParse(t, dir, files+"^{tree}")))
+		coreGit(t, dir, "update-ref", "refs/heads/main", back)
+		coreGit(t, dir, "update-ref", "refs/heads/files", files)
+		coreGit(t, dir, "update-ref", "refs/heads/other", other)
+		coreSeedStores(t, e, repo.RepoID, dir, map[string]string{"refs/heads/main": back, "refs/heads/files": files, "refs/heads/other": other})
+		token := e.mint(t, tokenSpec{iss: org, repo: repoURL, scopes: []string{auth.ScopeGitRead}})
+		for _, tc := range []struct {
+			route string
+			want  map[string]string // path -> "state bytes is_eof"
+		}{
+			{"/api/v1/repos/diff?ref=" + other + "&base_ref=" + files, map[string]string{"typefile": "T 4 false", "filetosub": "T 0 false"}},
+			{"/api/v1/repos/diff?ref=" + back + "&base_ref=" + other, map[string]string{"typefile": "T 4 true", "filetosub": "T 4 true"}},
+			{"/api/v1/repos/branches/diff?branch=other&base=files", map[string]string{"typefile": "T 4 false", "filetosub": "T 0 false"}},
+			{"/api/v1/repos/branches/diff?branch=main&base=other", map[string]string{"typefile": "T 4 true", "filetosub": "T 4 true"}},
+		} {
+			resp, body := e.doReq(t, http.MethodGet, tc.route, token, nil, "")
+			if resp.StatusCode != http.StatusOK {
+				t.Errorf("%s: status = %d, want 200 (body %.300s)", tc.route, resp.StatusCode, body)
+				continue
+			}
+			var diff struct {
+				Files []struct {
+					Path  string `json:"path"`
+					State string `json:"state"`
+					Bytes int64  `json:"bytes"`
+					IsEOF bool   `json:"is_eof"`
+				} `json:"files"`
+			}
+			if err := json.Unmarshal(body, &diff); err != nil {
+				t.Fatal(err)
+			}
+			got := map[string]string{}
+			for _, f := range diff.Files {
+				got[f.Path] = fmt.Sprintf("%s %d %v", f.State, f.Bytes, f.IsEOF)
+			}
+			if !maps.Equal(got, tc.want) {
+				t.Errorf("%s: files %v, want %v", tc.route, got, tc.want)
+			}
+		}
+	})
+}

internal/gitread/README.md

@@ -32,6 +32,7 @@ Read-side git plumbing for the `git:read` API. It hydrates a cache repo, resolve
 - `PrepareArchive` fails before any byte is written when a kept blob exceeds the size limit [archive.go:PrepareArchive]
 - `ValidArchivePrefix` rejects NUL, backslash, a leading slash, and `.`/`..` segments [archive.go:ValidArchivePrefix]
 - `mergeNumstat` and `mergePatches` error on any mismatch between the three diff passes [diff.go:mergeNumstat]
+- A type change (`T`) is one `--raw` and one `--numstat` record, and two `--patch` sections (its deletion, then its addition): its `Patch` holds both [diff.go:mergePatches]
 - `LastCommits` fails with `ErrTooManyLastCommits` past `maxDistinct`; spool lines over 1 MiB or spool over 256 MiB fail with `ErrOutputLimit` [lastcommits.go:spoolWriter]
 - `Note` maps git exit 1 to `ErrNotFound`; `PeeledTags` skips non-commit tags [notes.go:Note]
 - A failed `Blame` reads only the exit status and fixed parts of git's stderr, never the path, which git rewrites in its messages. With ranges, exit 129, or exit 128 with stderr that starts `fatal: -L `, or that starts `fatal: file ` and ends `has only N line(s)`, read from the first line that starts `fatal: `, is `ErrBadRange`: git prints no other fatal line before that message, and a path in it comes after the fixed head. Otherwise `ls-tree` decides: a path that names no file (not one blob or symlink, or entries under `<path>/`) is `ErrNotFound`, and any other failure is returned [blame.go:Blame]

internal/gitread/diff.go

@@ -181,7 +181,9 @@ func mergeNumstat(files []FileDiff, out string) error {
 // lines and assigns the slices to files in order (git emits --patch,
 // --numstat, and --name-status in the same file order). Content lines can
 // never start with "diff --git " at column 0: they carry a ' ', '+', or '-'
-// prefix.
+// prefix. The other two give a type change (T: a file that becomes a
+// symlink or a submodule, or back) one record, and --patch gives it two
+// sections, its deletion and its addition: that file's patch is both.
 func mergePatches(files []FileDiff, patch string) error {
 	if len(files) == 0 {
 		if strings.TrimSpace(patch) != "" {
@@ -201,15 +203,28 @@ func mergePatches(files []FileDiff, patch string) error {
 		}
 		pos += next + 1
 	}
-	if len(starts) != len(files) {
-		return fmt.Errorf("diff --patch has %d file headers, want %d", len(starts), len(files))
+	sections := func(f FileDiff) int {
+		if f.Status == "T" {
+			return 2
+		}
+		return 1
+	}
+	want := 0
+	for _, f := range files {
+		want += sections(f)
+	}
+	if len(starts) != want {
+		return fmt.Errorf("diff --patch has %d file headers, want %d", len(starts), want)
 	}
+	k := 0
 	for i := range files {
+		first := k
+		k += sections(files[i])
 		end := len(patch)
-		if i+1 < len(starts) {
-			end = starts[i+1]
+		if k < len(starts) {
+			end = starts[k]
 		}
-		files[i].Patch = patch[starts[i]:end]
+		files[i].Patch = patch[starts[first]:end]
 	}
 	return nil
 }

internal/gitread/diff_test.go

@@ -120,3 +120,75 @@ func TestDiffPatchesApplyCleanly(t *testing.T) {
 		t.Fatalf("git apply --check failed: %v\n%s\npatch:\n%s", err, out, concat.String())
 	}
 }
+
+// A type change (T) is one --raw and one --numstat record, and two
+// --patch sections, a deletion and an addition: the file's patch holds
+// both, the file after it keeps its own, and the patches apply.
+func TestDiffTypeChange(t *testing.T) {
+	dir := t.TempDir()
+	mustGit(t, dir, "init", "-q", "--initial-branch=main")
+	writeFile(t, dir, "typefile", "one\n", 0o644)
+	writeFile(t, dir, "filetosub", "two\n", 0o644)
+	writeFile(t, dir, "keep", "x\n", 0o644)
+	mustGit(t, dir, "add", "-A")
+	mustGit(t, dir, "commit", "-q", "-m", "files")
+	files := strings.TrimSpace(mustGit(t, dir, "rev-parse", "HEAD"))
+	if err := os.Remove(filepath.Join(dir, "typefile")); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink("keep", filepath.Join(dir, "typefile")); err != nil {
+		t.Fatal(err)
+	}
+	mustGit(t, dir, "rm", "-q", "--cached", "filetosub")
+	if err := os.Remove(filepath.Join(dir, "filetosub")); err != nil {
+		t.Fatal(err)
+	}
+	mustGit(t, dir, "update-index", "--add", "--cacheinfo", "160000,"+files+",filetosub")
+	writeFile(t, dir, "keep", "x\ny\n", 0o644)
+	mustGit(t, dir, "add", "typefile", "keep")
+	mustGit(t, dir, "commit", "-q", "-m", "a symlink and a submodule")
+	other := strings.TrimSpace(mustGit(t, dir, "rev-parse", "HEAD"))
+	for _, tc := range []struct {
+		name, base, target string
+		oldMode, newMode   [2]string // typefile's, filetosub's
+	}{
+		{"to a symlink and a submodule", files, other, [2]string{"100644", "100644"}, [2]string{"120000", "160000"}},
+		{"back to files", other, files, [2]string{"120000", "160000"}, [2]string{"100644", "100644"}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			got, stats, err := Diff(context.Background(), dir, tc.base, tc.target, DiffOptions{})
+			if err != nil {
+				t.Fatal(err)
+			}
+			m := diffByPath(got)
+			if len(got) != 3 || stats.Files != 3 || m["keep"].Status != "M" || !strings.HasPrefix(m["keep"].Patch, "diff --git a/keep b/keep\n") || strings.Count(m["keep"].Patch, "diff --git ") != 1 {
+				t.Fatalf("files = %+v, want filetosub, keep (M, its own patch) and typefile", got)
+			}
+			for i, path := range []string{"typefile", "filetosub"} {
+				d := m[path]
+				if d.Status != "T" || d.Additions != 1 || d.Deletions != 1 || d.Binary {
+					t.Errorf("%s = %+v, want T with 1 addition and 1 deletion", path, d)
+				}
+				header := "diff --git a/" + path + " b/" + path + "\n"
+				del := header + "deleted file mode " + tc.oldMode[i] + "\n"
+				add := header + "new file mode " + tc.newMode[i] + "\n"
+				if !strings.HasPrefix(d.Patch, del) || strings.Count(d.Patch, header) != 2 || !strings.Contains(d.Patch, "\n"+add) {
+					t.Errorf("%s patch = %q, want its deletion, then its addition", path, d.Patch)
+				}
+			}
+			var all strings.Builder
+			for _, d := range got {
+				all.WriteString(d.Patch)
+			}
+			work := t.TempDir()
+			mustGit(t, work, "init", "-q")
+			mustGit(t, work, "fetch", "-q", dir, tc.base)
+			mustGit(t, work, "read-tree", "FETCH_HEAD")
+			cmd := exec.Command("git", "apply", "--cached", "--check")
+			cmd.Dir, cmd.Stdin = work, strings.NewReader(all.String())
+			if out, err := cmd.CombinedOutput(); err != nil {
+				t.Fatalf("git apply --cached --check: %v\n%s", err, out)
+			}
+		})
+	}
+}