From 4413462a1c12b4cadfb8a21e5ad1d378b7659d0f Mon Sep 17 00:00:00 2001 From: omartelo Date: Wed, 21 Oct 2026 13:30:32 -0300 Subject: [PATCH] feat(native): preview a pull request's images and edited Markdown on the Pulls screen /blob reads an object the clone lacks from GitHub's contents API, the way FileLines does, and the new PullRequestMergeBase RPC names the commit a pull request's old side stands on, computed locally when the clone holds both tips. The window asks it once per head for every image card. ReadFileAt reads a file whole at a revision, so an edited Markdown file renders at the head. --- CHANGELOG.md | 1 + docs/ceilings.md | 17 ++- internal/project/blob.go | 66 ++++++++- internal/project/blob_test.go | 128 ++++++++++++++++++ internal/project/filelines.go | 10 ++ internal/project/filelines_test.go | 26 ++++ native/cmd/lich-native/blob_preview.go | 22 +++ native/cmd/lich-native/dock_review.go | 7 +- native/cmd/lich-native/dock_review_binary.go | 32 +++-- .../cmd/lich-native/dock_review_markdown.go | 25 ++-- .../lich-native/dock_review_markdown_test.go | 2 +- native/cmd/lich-native/dock_review_test.go | 7 +- native/cmd/lich-native/merge_base_memo.go | 53 ++++++++ .../cmd/lich-native/merge_base_memo_test.go | 71 ++++++++++ native/cmd/lich-native/pulls_files.go | 2 +- native/lichclient/blob.go | 3 +- native/lichclient/pulls.go | 7 + 17 files changed, 437 insertions(+), 42 deletions(-) create mode 100644 native/cmd/lich-native/merge_base_memo.go create mode 100644 native/cmd/lich-native/merge_base_memo_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 32eb658d..bf14c0c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- The Pulls screen shows a changed image in a pull request's Files tab, before and after side by side, as the Review tab does, where it used to say only "Binary file". An edited Markdown file there has the preview button too, where only an added one had it. lich reads the file from your clone when it holds the commit and from GitHub otherwise. - `lich mcp` answers MCP clients on protocol revision 2026-07-28, which name the version on each request instead of opening with `initialize`, and serves their `server/discover`. Clients that open with `initialize` are answered exactly as before. ### Fixed diff --git a/docs/ceilings.md b/docs/ceilings.md index 3eb1a77e..9b041499 100644 --- a/docs/ceilings.md +++ b/docs/ceilings.md @@ -439,11 +439,13 @@ work when nobody knows it and that the call site never shows. The mechanism and And there is no expanding *past the last hunk*: a unified diff carries no file length, so nothing here knows whether anything follows it, and an affordance drawn there would be a no-op on every file whose change reaches the end. -- **An image preview reads only what this clone holds** (`internal/project/blob.go`): `/blob` serves the - working tree, `HEAD` or a tree object, and has no GitHub fallback the way `FileLines` does, so the Pulls - screen still prints "Binary file". Adding it there takes two reads this endpoint does not make: the head blob - from the contents API, and the old side at the merge base, which gh does not report (`baseRefOid` is the base - branch's tip, and an image the base also changed would show the wrong "before"). SVG is refused on purpose: +- **A pull request's image costs GitHub calls the clone could not answer** (`internal/project/blob.go`): `/blob` + reads an object this clone lacks from the contents API, the way `FileLines` does, and the old side stands on + the merge base, which gh does not report (`baseRefOid` is the base branch's tip, wrong once the base moved). + `PullRequestMergeBase` asks GitHub for both tips, and computes the merge base locally only when the clone + holds both, else asks the compare API too. The window asks it once per pull request head, shared by every + image card, and again only after a failure; each image side the clone lacks is still one contents call, and + a GitHub read is checked against the size cap only after the whole body arrived. SVG is refused on purpose: git diffs it as text, and the window has no SVG renderer. One side is capped at `maxPreviewBytes` (20 MiB) and held whole in memory, and a Git LFS file shows the pointer's text diff, never the image. The window decodes in Go (`native/cmd/lich-native/blob_preview.go`) and `/blob` serves only what it @@ -463,8 +465,9 @@ work when nobody knows it and that the call site never shows. The mechanism and - **A Markdown preview shows the file's new version, never the change** (`native/cmd/lich-native/dock_review_markdown.go`): the Review card's preview renders the whole file as the Pulls screen renders a PR body, with nothing marking what was added or removed, no line to hang a review comment on and no selection to inject. It reads the - working tree (`project.ReadFile`, as the Code tab does), so on the Pulls screen only an added file, whose patch is the whole file, has a - preview; an edited one has no toggle for the same reason an image there has no preview. A relative image + working tree (`project.ReadFile`, as the Code tab does); on the Pulls screen an added file renders its patch, + and an edited one is read at the pull request's head (`project.ReadFileAt`), from GitHub when the clone does + not hold it, under ReadFile's size cap. A relative image shows its alt text (the renderer fetches nothing) and raw HTML is left out. A new version of the file puts the card back on its diff, as it reopens or folds a card by its size. diff --git a/internal/project/blob.go b/internal/project/blob.go index 31b1d161..05ed2ea5 100644 --- a/internal/project/blob.go +++ b/internal/project/blob.go @@ -46,10 +46,15 @@ type Blob struct { ContentType string } +// githubBlobTimeout bounds one image read from the contents API: up to +// maxPreviewBytes over the network, well past what prReadTimeout allows a +// JSON answer. +const githubBlobTimeout = 30 * time.Second + // Blob reads rel at ref for an image preview. ref is "" for the working tree, // "HEAD" for the commit a checkout's uncommitted diff stands on, or an object -// this clone holds. A pull request's head is -// not reached: unlike FileLines there is no GitHub fallback. +// id. An object this clone does not hold, a pull request's head or merge base, +// is read from GitHub, the way FileLines reads it. // // A path absent at ref is fs.ErrNotExist: the side of an added or deleted file // that has nothing to show. @@ -66,10 +71,13 @@ func (s *Service) Blob(dir, rel, ref string) (Blob, error) { } var data []byte var err error - if ref == "" { + switch { + case ref == "": data, err = workTreeBlob(dir, rel) - } else { + case ref == "HEAD" || holdsObject(dir, ref): data, err = objectBlob(dir, rel, ref) + default: + data, err = s.githubBlob(dir, rel, ref) } if err != nil { return Blob{}, err @@ -132,6 +140,56 @@ func objectBlob(dir, rel, ref string) ([]byte, error) { return []byte(out), nil } +// holdsObject reports whether this clone has the object ref names. +func holdsObject(dir, ref string) bool { + _, ok := gitQuiet(dir, "cat-file", "-e", ref) + return ok +} + +// githubBlob reads rel at a commit this clone does not hold. The size is only +// known once the bytes are here: the raw contents answer carries no length +// worth trusting ahead of the body. +func (s *Service) githubBlob(dir, rel, ref string) ([]byte, error) { + out, err := s.gh(githubBlobTimeout, dir, "api", "-H", ghRawAccept, contentsPath(rel, ref)) + if err != nil { + return nil, err + } + if err := checkPreviewSize(int64(len(out))); err != nil { + return nil, err + } + return out, nil +} + +// PullRequestMergeBase is the commit pull request number's diff is taken +// from, the old side of every file GitHub shows. The base branch's tip is not +// it once the base moved past where the branch left it. The tips come from +// GitHub; their merge base is computed here when the clone holds both, and +// asked of GitHub otherwise. +func (s *Service) PullRequestMergeBase(path string, number int) (string, error) { + out, err := s.gh(prReadTimeout, path, "api", fmt.Sprintf("repos/{owner}/{repo}/pulls/%d", number), + "--jq", `.base.sha + " " + .head.sha`) + if err != nil { + return "", err + } + base, head, _ := strings.Cut(strings.TrimSpace(string(out)), " ") + if !isOID(base) || !isOID(head) { + return "", errUnreadableAnswer(fmt.Errorf("pull request tips %q", out)) + } + if local, ok := gitQuiet(path, "merge-base", base, head); ok { + return strings.TrimSpace(local), nil + } + out, err = s.gh(prReadTimeout, path, "api", "repos/{owner}/{repo}/compare/"+base+"..."+head, + "--jq", ".merge_base_commit.sha") + if err != nil { + return "", err + } + mergeBase := strings.TrimSpace(string(out)) + if !isOID(mergeBase) { + return "", errUnreadableAnswer(fmt.Errorf("merge base %q", out)) + } + return mergeBase, nil +} + func checkPreviewSize(size int64) error { if size > maxPreviewBytes { return fmt.Errorf("%.1f MB, %w", float64(size)/(1<<20), errPreviewTooLarge) diff --git a/internal/project/blob_test.go b/internal/project/blob_test.go index ea758225..6e3f4ed1 100644 --- a/internal/project/blob_test.go +++ b/internal/project/blob_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "slices" "strings" "testing" ) @@ -68,6 +69,133 @@ func TestBlobMissingSideIsNotExist(t *testing.T) { } } +// A pull request's commits need not be in the clone; GitHub answers for them. +func TestBlobMissingObjectAsksGitHub(t *testing.T) { + t.Parallel() + repo, _ := initRepo(t) + missing := strings.Repeat("b", 40) + + gh := &fakeGH{out: []byte(pngBytes)} + blob, err := withGH(gh).Blob(repo, "img/logo.png", missing) + if err != nil { + t.Fatalf("Blob: %v", err) + } + if string(blob.Data) != pngBytes || blob.ContentType != "image/png" { + t.Errorf("got %q as %q, want GitHub's image", blob.Data, blob.ContentType) + } + want := []string{"api", "-H", ghRawAccept, contentsPath("img/logo.png", missing)} + if !slices.Equal(gh.args, want) { + t.Errorf("gh args = %v, want %v", gh.args, want) + } +} + +// A commit the clone holds is read locally, even when the path is absent at +// it: that answer is the clone's to give, not GitHub's. +func TestBlobLocalObjectNeverAsksGitHub(t *testing.T) { + t.Parallel() + repo, git := initRepo(t) + write(t, repo, "logo.png", pngBytes) + git("add", ".") + git("commit", "-m", "logo") + head := strings.TrimSpace(git("rev-parse", "HEAD")) + + gh := &fakeGH{err: errors.New("gh must not run")} + svc := withGH(gh) + if blob, err := svc.Blob(repo, "logo.png", head); err != nil || string(blob.Data) != pngBytes { + t.Errorf("present: %q, %v; want the committed image", blob.Data, err) + } + if _, err := svc.Blob(repo, "gone.png", head); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("absent: err = %v, want fs.ErrNotExist", err) + } + if gh.calls != 0 { + t.Errorf("gh ran %d times, want 0", gh.calls) + } +} + +func TestBlobGitHubSizeLimit(t *testing.T) { + t.Parallel() + repo, _ := initRepo(t) + gh := &fakeGH{out: make([]byte, maxPreviewBytes+1)} + if _, err := withGH(gh).Blob(repo, "big.png", strings.Repeat("c", 40)); !errors.Is(err, errPreviewTooLarge) { + t.Errorf("err = %v, want errPreviewTooLarge", err) + } +} + +func TestBlobGitHubFailureSurfaces(t *testing.T) { + t.Parallel() + repo, _ := initRepo(t) + gh := &fakeGH{err: errors.New("gh: Not Found (HTTP 404)")} + if _, err := withGH(gh).Blob(repo, "x.png", strings.Repeat("c", 40)); err == nil || errors.Is(err, fs.ErrNotExist) { + t.Errorf("err = %v, want gh's failure, not a missing side", err) + } +} + +// Both tips in the clone: git computes the merge base, GitHub is asked only +// for the tips. +func TestPullRequestMergeBaseLocal(t *testing.T) { + t.Parallel() + repo, git := initRepo(t) + write(t, repo, "fork.txt", "fork\n") + git("add", ".") + git("commit", "-m", "fork point") + fork := strings.TrimSpace(git("rev-parse", "HEAD")) + git("checkout", "-q", "-b", "topic") + write(t, repo, "b.txt", "topic\n") + git("add", ".") + git("commit", "-m", "topic") + head := strings.TrimSpace(git("rev-parse", "HEAD")) + git("checkout", "-q", "-") + write(t, repo, "c.txt", "base moved\n") + git("add", ".") + git("commit", "-m", "base") + base := strings.TrimSpace(git("rev-parse", "HEAD")) + + gh := &fakeGH{out: []byte(base + " " + head + "\n")} + got, err := withGH(gh).PullRequestMergeBase(repo, 7) + if err != nil { + t.Fatalf("PullRequestMergeBase: %v", err) + } + if got != fork { + t.Errorf("merge base = %s, want the fork point %s", got, fork) + } + if want := []string{"api", "repos/{owner}/{repo}/pulls/7", "--jq", `.base.sha + " " + .head.sha`}; gh.calls != 1 || !slices.Equal(gh.args, want) { + t.Errorf("gh calls = %v, want one: %v", gh.history, want) + } +} + +func TestPullRequestMergeBaseAsksGitHub(t *testing.T) { + t.Parallel() + repo, _ := initRepo(t) + base, head, mergeBase := strings.Repeat("a", 40), strings.Repeat("b", 40), strings.Repeat("c", 40) + + gh := &fakeGH{outs: [][]byte{[]byte(base + " " + head + "\n"), []byte(mergeBase + "\n")}} + got, err := withGH(gh).PullRequestMergeBase(repo, 7) + if err != nil { + t.Fatalf("PullRequestMergeBase: %v", err) + } + if got != mergeBase { + t.Errorf("merge base = %s, want %s", got, mergeBase) + } + want := []string{"api", "repos/{owner}/{repo}/compare/" + base + "..." + head, "--jq", ".merge_base_commit.sha"} + if !slices.Equal(gh.args, want) { + t.Errorf("gh args = %v, want %v", gh.args, want) + } +} + +func TestPullRequestMergeBaseRejectsAnUnreadableAnswer(t *testing.T) { + t.Parallel() + repo, _ := initRepo(t) + tips := []byte(strings.Repeat("a", 40) + " " + strings.Repeat("b", 40)) + for name, outs := range map[string][][]byte{ + "tips": {[]byte("null null")}, + "merge base": {tips, []byte("null")}, + } { + if _, err := withGH(&fakeGH{outs: outs}).PullRequestMergeBase(repo, 7); err == nil { + t.Errorf("%s: want an error, got nil", name) + } + } +} + func TestBlobRefusals(t *testing.T) { t.Parallel() repo, git := initRepo(t) diff --git a/internal/project/filelines.go b/internal/project/filelines.go index 1dfba671..11a05317 100644 --- a/internal/project/filelines.go +++ b/internal/project/filelines.go @@ -47,6 +47,16 @@ func (s *Service) FileLines(path, rel, ref string, from, to int) ([]string, erro return sliceLines(text, from, to), nil } +// ReadFileAt is ReadFile at ref, for a preview of a file's new version that +// the working tree does not hold: a pull request's head. See FileLines for +// what a ref can be and where it is read from. +func (s *Service) ReadFileAt(path, rel, ref string) (string, error) { + if err := relpath.Validate(rel); err != nil { + return "", err + } + return s.fileAt(path, rel, ref) +} + // fileAt reads one file whole at ref. See FileLines for what a ref can be. func (s *Service) fileAt(path, rel, ref string) (string, error) { if ref == "" { diff --git a/internal/project/filelines_test.go b/internal/project/filelines_test.go index 498830a4..41dec535 100644 --- a/internal/project/filelines_test.go +++ b/internal/project/filelines_test.go @@ -205,6 +205,32 @@ func TestFileLinesMissingObjectAsksGitHub(t *testing.T) { } } +func TestReadFileAt(t *testing.T) { + t.Parallel() + repo, git := initRepo(t) + write(t, repo, "notes.md", "# old\n") + git("add", ".") + git("commit", "-m", "notes") + head := strings.TrimSpace(git("rev-parse", "HEAD")) + write(t, repo, "notes.md", "# new\n") + + gh := &fakeGH{out: []byte("# from GitHub\n")} + svc := withGH(gh) + if text, err := svc.ReadFileAt(repo, "notes.md", head); err != nil || text != "# old\n" { + t.Errorf("local commit: %q, %v; want the committed text", text, err) + } + if gh.calls != 0 { + t.Errorf("gh ran %d times for a local commit, want 0", gh.calls) + } + missing := strings.Repeat("e", 40) + if text, err := svc.ReadFileAt(repo, "notes.md", missing); err != nil || text != "# from GitHub\n" { + t.Errorf("absent commit: %q, %v; want GitHub's text", text, err) + } + if _, err := svc.ReadFileAt(repo, "../x.md", head); err == nil { + t.Error("traversal: want an error, got nil") + } +} + func TestFileLinesGitHubRejectsOversize(t *testing.T) { t.Parallel() repo, _ := initRepo(t) diff --git a/native/cmd/lich-native/blob_preview.go b/native/cmd/lich-native/blob_preview.go index 23e9714b..7117b7bf 100644 --- a/native/cmd/lich-native/blob_preview.go +++ b/native/cmd/lich-native/blob_preview.go @@ -14,6 +14,7 @@ import ( _ "image/jpeg" _ "image/png" "path" + "strconv" "strings" "gioui.org/layout" @@ -155,6 +156,27 @@ func (v *blobView) want(client *lichclient.Client, checkout, rel, ref, version s }) } +// wantMergeBase reads rel at pull request pr's merge base, the commit its +// old side stands on. +func (v *blobView) wantMergeBase(client *lichclient.Client, checkout, rel string, pr *prFiles, version string) { + number := pr.number + key := strings.Join([]string{checkout, rel, strconv.Itoa(number), version}, "\x00") + baseKey := strings.Join([]string{checkout, strconv.Itoa(number), pr.head}, "\x00") + v.read.want(key, checkout, func(ctx context.Context) (*previewImage, error) { + base, err := pr.mergeBase.get(ctx, baseKey, func() (string, error) { + return client.PullRequestMergeBase(context.Background(), checkout, number) + }) + if err != nil { + return nil, err + } + data, err := client.Blob(ctx, checkout, rel, base) + if err != nil { + return nil, err + } + return decodePreview(data) + }) +} + // layout is the image over its caption, or the line saying why there is none. func (v *blobView) layout(gtx C, th *ui.Theme) D { img, loading, err := v.read.get() diff --git a/native/cmd/lich-native/dock_review.go b/native/cmd/lich-native/dock_review.go index 1ae53a66..efc7499e 100644 --- a/native/cmd/lich-native/dock_review.go +++ b/native/cmd/lich-native/dock_review.go @@ -96,8 +96,11 @@ type reviewPanel struct { // checkout's working tree: nothing to revert or discard, its gaps read at the // PR's head, and each file tickable as viewed. type prFiles struct { - head string - url string // the pull request's page + number int + head string + // mergeBase is the old side every image card reads, asked once per head. + mergeBase mergeBaseMemo + url string // the pull request's page // viewed is each ticked file's diff text when it was ticked: a commit // that changes the file unticks it, one that does not leaves it. viewed map[string]string diff --git a/native/cmd/lich-native/dock_review_binary.go b/native/cmd/lich-native/dock_review_binary.go index 64b66dfe..b42832a7 100644 --- a/native/cmd/lich-native/dock_review_binary.go +++ b/native/cmd/lich-native/dock_review_binary.go @@ -1,9 +1,8 @@ package main // A binary file's card body (BinaryDiffBody): the image on each side of the -// working tree's change, or the line saying it is binary with the way out to -// the desktop's application, or to the file on GitHub in a pull request, -// whose blobs this clone does not hold (docs/ceilings.md). +// change, or the line saying it is binary with the way out to the desktop's +// application, or to the file on GitHub in a pull request. import ( "context" @@ -28,10 +27,10 @@ type binaryPreview struct { github widget.Clickable } -// previewsImage reports whether the card draws its file's image: a working -// tree change in a format this window decodes. +// previewsImage reports whether the card draws its file's image: a format +// this window decodes. func (c *fileCard) previewsImage() bool { - return c.git.binary && c.panel.pr == nil && previewKindOf(c.git.path) == blobImage + return c.git.binary && previewKindOf(c.git.path) == blobImage } func (c *fileCard) binaryBody(gtx C, p *reviewPanel) D { @@ -39,10 +38,10 @@ func (c *fileCard) binaryBody(gtx C, p *reviewPanel) D { c.bin = &binaryPreview{before: newBlobView(p.invalidate), after: newBlobView(p.invalidate)} } switch { - case p.pr != nil: - return c.prBinary(gtx, p) case c.previewsImage(): return c.imageSides(gtx, p) + case p.pr != nil: + return c.prBinary(gtx, p) } c.bin.exits.update(gtx, p.client, p.notify, p.target, c.git.path) return layout.Flex{Axis: layout.Vertical}.Layout(gtx, @@ -82,26 +81,33 @@ func prFileURL(prURL, path string) string { } // imageSides is the grid of BinaryDiffBody, px-3 pt-2.5 pb-3.5 gap-2.5: both -// sides of a change, or the one an added or deleted file has. +// sides of a change, or the one an added or deleted file has. A working tree +// change stands on HEAD; a pull request's on its merge base and its head. func (c *fileCard) imageSides(gtx C, p *reviewPanel) D { old := c.git.oldPath if old == "" { old = c.git.path } + wantBefore := func() { c.bin.before.want(p.client, p.target, old, "HEAD", c.git.blobs) } + afterRef := "" + if f := p.pr; f != nil { + wantBefore = func() { c.bin.before.wantMergeBase(p.client, p.target, old, f, c.git.blobs) } + afterRef = f.head + } var sides []layout.FlexChild side := func(v *blobView, label string, tone color.NRGBA) layout.FlexChild { return layout.Flexed(1, func(gtx C) D { return sideColumn(gtx, p.th, v, label, tone) }) } switch c.git.status { case statusAdded: - c.bin.after.want(p.client, p.target, c.git.path, "", c.git.blobs) + c.bin.after.want(p.client, p.target, c.git.path, afterRef, c.git.blobs) sides = append(sides, side(c.bin.after, i18n.T("diff.binaryPreview.added"), p.th.TonePass)) case statusDeleted: - c.bin.before.want(p.client, p.target, old, "HEAD", c.git.blobs) + wantBefore() sides = append(sides, side(c.bin.before, i18n.T("diff.binaryPreview.deleted"), p.th.Destructive)) default: - c.bin.before.want(p.client, p.target, old, "HEAD", c.git.blobs) - c.bin.after.want(p.client, p.target, c.git.path, "", c.git.blobs) + wantBefore() + c.bin.after.want(p.client, p.target, c.git.path, afterRef, c.git.blobs) sides = append(sides, side(c.bin.before, i18n.T("diff.binaryPreview.before"), p.th.Destructive), ui.Gap(2.5), side(c.bin.after, i18n.T("diff.binaryPreview.after"), p.th.TonePass)) } return layout.Inset{Left: ui.Space(3), Right: ui.Space(3), Top: ui.Space(2.5), Bottom: ui.Space(3.5)}.Layout(gtx, func(gtx C) D { diff --git a/native/cmd/lich-native/dock_review_markdown.go b/native/cmd/lich-native/dock_review_markdown.go index 81ba9d46..610dcb6a 100644 --- a/native/cmd/lich-native/dock_review_markdown.go +++ b/native/cmd/lich-native/dock_review_markdown.go @@ -25,14 +25,9 @@ type markdownPreview struct { } // previewsMarkdown reports whether the card offers its file rendered: a -// Markdown file whose new version this window can read. A pull request's -// clone holds none of its blobs (docs/ceilings.md), so there only an added -// file, whose patch is the whole file, has one. +// Markdown file with a new version. func (c *fileCard) previewsMarkdown() bool { - if c.git.binary || c.git.status == statusDeleted || !isMarkdown(c.git.path) { - return false - } - return c.panel.pr == nil || c.git.status == statusAdded + return !c.git.binary && c.git.status != statusDeleted && isMarkdown(c.git.path) } func isMarkdown(path string) bool { @@ -86,17 +81,25 @@ func (c *fileCard) markdownBody(gtx C, p *reviewPanel) D { }) } -// markdownSource is the file's new version: an added file's patch, or the -// working tree's file, read again when the index line names a new blob. +// markdownSource is the file's new version: an added file's patch, the +// working tree's file, or a pull request's at its head, read again when the +// index line names a new blob. func (c *fileCard) markdownSource(p *reviewPanel) (src string, loading bool, err error) { if c.git.status == statusAdded { return addedText(c.git), false, nil } client, checkout, rel := p.client, p.target, c.git.path - key := strings.Join([]string{checkout, rel, c.git.blobs}, "\x00") + ref := "" + if p.pr != nil { + ref = p.pr.head + } + key := strings.Join([]string{checkout, rel, ref, c.git.blobs}, "\x00") c.md.read.want(key, checkout, func(ctx context.Context) (string, error) { var text string - return text, client.Call(ctx, "project.ReadFile", &text, checkout, rel) + if ref == "" { + return text, client.Call(ctx, "project.ReadFile", &text, checkout, rel) + } + return text, client.Call(ctx, "project.ReadFileAt", &text, checkout, rel, ref) }) return c.md.read.get() } diff --git a/native/cmd/lich-native/dock_review_markdown_test.go b/native/cmd/lich-native/dock_review_markdown_test.go index 81d10b44..88d84089 100644 --- a/native/cmd/lich-native/dock_review_markdown_test.go +++ b/native/cmd/lich-native/dock_review_markdown_test.go @@ -22,7 +22,7 @@ func TestMarkdownPreviewIsOfferedWhereTheNewVersionIsReadable(t *testing.T) { {"an edited file in the working tree", tree, editedReadme, true}, {"an added file in the working tree", tree, addedReadme, true}, {"an added file in a pull request", pr, addedReadme, true}, - {"an edited file in a pull request, whose blobs the clone lacks", pr, editedReadme, false}, + {"an edited file in a pull request, read at its head", pr, editedReadme, true}, {"a deleted file", tree, gonePage, false}, {"a file that is not Markdown", tree, twoHunks, false}, } { diff --git a/native/cmd/lich-native/dock_review_test.go b/native/cmd/lich-native/dock_review_test.go index a76e57aa..2fe45d38 100644 --- a/native/cmd/lich-native/dock_review_test.go +++ b/native/cmd/lich-native/dock_review_test.go @@ -111,8 +111,11 @@ func TestBinaryFilesOpenWhenTheyHaveAPreview(t *testing.T) { t.Error("a binary with no preview starts open, want collapsed") } p.pr = &prFiles{} - if c := newFileCard(p, img, false, 0); c.open { - t.Error("an image in a pull request starts open, want collapsed: this clone holds neither side") + if c := newFileCard(p, img, false, 0); !c.open { + t.Error("an image in a pull request starts collapsed, want open: its sides are read from the clone or GitHub") + } + if c := newFileCard(p, zip, false, 0); c.open { + t.Error("a binary with no preview in a pull request starts open, want collapsed") } } diff --git a/native/cmd/lich-native/merge_base_memo.go b/native/cmd/lich-native/merge_base_memo.go new file mode 100644 index 00000000..e3e67f89 --- /dev/null +++ b/native/cmd/lich-native/merge_base_memo.go @@ -0,0 +1,53 @@ +package main + +import ( + "context" + "sync" +) + +// mergeBaseMemo holds one pull request's merge base for every image card that +// stands on it: each ask spends GitHub calls of the hourly rate limit, so the +// cards share one, asked again only for a new key (another head) or after a +// failure. +type mergeBaseMemo struct { + mu sync.Mutex + key string + call *mergeBaseCall +} + +type mergeBaseCall struct { + done chan struct{} + oid string + err error +} + +// get is key's merge base, fetched once whoever asks first. fetch runs on +// its own, not under ctx: a card that stops waiting must not fail the others. +func (m *mergeBaseMemo) get(ctx context.Context, key string, fetch func() (string, error)) (string, error) { + m.mu.Lock() + call := m.call + if call == nil || m.key != key { + call = &mergeBaseCall{done: make(chan struct{})} + m.key, m.call = key, call + go m.run(call, fetch) + } + m.mu.Unlock() + select { + case <-call.done: + return call.oid, call.err + case <-ctx.Done(): + return "", ctx.Err() + } +} + +func (m *mergeBaseMemo) run(call *mergeBaseCall, fetch func() (string, error)) { + call.oid, call.err = fetch() + if call.err != nil { + m.mu.Lock() + if m.call == call { + m.call = nil + } + m.mu.Unlock() + } + close(call.done) +} diff --git a/native/cmd/lich-native/merge_base_memo_test.go b/native/cmd/lich-native/merge_base_memo_test.go new file mode 100644 index 00000000..36811f2d --- /dev/null +++ b/native/cmd/lich-native/merge_base_memo_test.go @@ -0,0 +1,71 @@ +package main + +import ( + "context" + "errors" + "sync" + "sync/atomic" + "testing" +) + +func TestMergeBaseMemoAsksOncePerKey(t *testing.T) { + var m mergeBaseMemo + var calls atomic.Int32 + release := make(chan struct{}) + fetch := func() (string, error) { + calls.Add(1) + <-release + return "abc1234", nil + } + var wg sync.WaitGroup + for range 5 { + wg.Add(1) + go func() { + defer wg.Done() + if oid, err := m.get(context.Background(), "pr@head1", fetch); err != nil || oid != "abc1234" { + t.Errorf("get = %q, %v; want abc1234", oid, err) + } + }() + } + close(release) + wg.Wait() + if _, err := m.get(context.Background(), "pr@head1", fetch); err != nil { + t.Fatal(err) + } + if n := calls.Load(); n != 1 { + t.Errorf("fetched %d times for one key, want 1", n) + } + if _, err := m.get(context.Background(), "pr@head2", fetch); err != nil { + t.Fatal(err) + } + if n := calls.Load(); n != 2 { + t.Errorf("fetched %d times after a new head, want 2", n) + } +} + +func TestMergeBaseMemoRetriesAfterAFailure(t *testing.T) { + var m mergeBaseMemo + fail := errors.New("rate limited") + if _, err := m.get(context.Background(), "k", func() (string, error) { return "", fail }); !errors.Is(err, fail) { + t.Fatalf("err = %v, want the fetch's failure", err) + } + oid, err := m.get(context.Background(), "k", func() (string, error) { return "def5678", nil }) + if err != nil || oid != "def5678" { + t.Errorf("after a failure: %q, %v; want a new fetch", oid, err) + } +} + +func TestMergeBaseMemoWaiterGivesUpAlone(t *testing.T) { + var m mergeBaseMemo + release := make(chan struct{}) + fetch := func() (string, error) { <-release; return "abc1234", nil } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, err := m.get(ctx, "k", fetch); !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled waiter: err = %v, want context.Canceled", err) + } + close(release) + if oid, err := m.get(context.Background(), "k", fetch); err != nil || oid != "abc1234" { + t.Errorf("another waiter: %q, %v; want the fetch's answer", oid, err) + } +} diff --git a/native/cmd/lich-native/pulls_files.go b/native/cmd/lich-native/pulls_files.go index 8d8cc902..e3d6b788 100644 --- a/native/cmd/lich-native/pulls_files.go +++ b/native/cmd/lich-native/pulls_files.go @@ -108,7 +108,7 @@ func (v *pullView) filesTab(gtx C, th *ui.Theme, p *pullsScreen, d lichclient.PR viewed = map[string]string{} v.viewed[d.URL] = viewed } - files.pr.viewed, files.pr.url = viewed, d.URL + files.pr.viewed, files.pr.url, files.pr.number = viewed, d.URL, d.Number review := v.pendingOf(d.URL) files.pr.toReview = review.add v.inline = inlineFrame{th: th, p: p, d: d, act: act, now: gtx.Now} diff --git a/native/lichclient/blob.go b/native/lichclient/blob.go index 947d0496..e952daaf 100644 --- a/native/lichclient/blob.go +++ b/native/lichclient/blob.go @@ -16,7 +16,8 @@ var ErrNoBlob = errors.New("not in this revision") // Blob reads rel, a file of the checkout at path, through /blob for an image // or PDF preview. ref is "" for the working tree, "HEAD" for the commit under -// it, or an object id. A side with no file is ErrNoBlob; a file the backend +// it, or an object id, which the backend reads from GitHub when the clone +// does not hold it. A side with no file is ErrNoBlob; a file the backend // refuses (a type with no preview, one above its size limit) is a // RemoteError whose Message is the backend's own sentence. func (c *Client) Blob(ctx context.Context, path, rel, ref string) ([]byte, error) { diff --git a/native/lichclient/pulls.go b/native/lichclient/pulls.go index 9136e8c9..1a82142a 100644 --- a/native/lichclient/pulls.go +++ b/native/lichclient/pulls.go @@ -207,6 +207,13 @@ func (c *Client) PullRequestDiff(ctx context.Context, path string, number int) ( return diff, c.Call(ctx, "project.PullRequestDiff", &diff, path, number) } +// PullRequestMergeBase is the commit pull request number's diff is taken +// from: the old side of every file in it. +func (c *Client) PullRequestMergeBase(ctx context.Context, path string, number int) (string, error) { + var oid string + return oid, c.Call(ctx, "project.PullRequestMergeBase", &oid, path, number) +} + // PRConversation mirrors project.PRConversation. type PRConversation struct { HeadRefOID string `json:"headRefOid"`