From 6b0b4d27655b12cffe1e7d5a651d226f94fa95ec Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sun, 8 Nov 2026 15:41:30 +0000 Subject: [PATCH] fix(gitread): give a type change both of its --patch sections A diff with a type change (a file that becomes a symlink or a submodule, or back) answered 500: "diff --patch has 2 file headers, want 1". --raw and --numstat give a type change one record, and --patch splits it into a deletion and an addition. mergePatches now counts two sections for a T entry, and that file's patch holds both. Found by the #780 root verifier. Change-Id: Ic78a86450eba7f54279165c199c07090341f1df0 Signed-off-by: Thomas Kosiewski --- internal/api/diff_typechange_test.go | 91 ++++++++++++++++++++++++++++ internal/gitread/README.md | 1 + internal/gitread/diff.go | 27 +++++++-- internal/gitread/diff_test.go | 72 ++++++++++++++++++++++ 4 files changed, 185 insertions(+), 6 deletions(-) create mode 100644 internal/api/diff_typechange_test.go diff --git a/internal/api/diff_typechange_test.go b/internal/api/diff_typechange_test.go new file mode 100644 index 000000000..d941ad339 --- /dev/null +++ b/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) + } + } + }) +} diff --git a/internal/gitread/README.md b/internal/gitread/README.md index 1c77b2573..9786828fb 100644 --- a/internal/gitread/README.md +++ b/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 `/`) is `ErrNotFound`, and any other failure is returned [blame.go:Blame] diff --git a/internal/gitread/diff.go b/internal/gitread/diff.go index 149b63309..b523c5bf3 100644 --- a/internal/gitread/diff.go +++ b/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 } diff --git a/internal/gitread/diff_test.go b/internal/gitread/diff_test.go index 12a12bef9..9dd9d82ae 100644 --- a/internal/gitread/diff_test.go +++ b/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) + } + }) + } +}