Sign in

Search issue titles and bodies, change requests and comments. Private repositories you cannot read are not included.

vole
Clone

Read only, no credentials:

https://rodent.software/vole.git

Write over SSH with a registered key:

git clone ssh://git@rodent.software:2222/vole.git

A free and open-source forge that lives in your Git repo.

Phase 3: a request says whether it would actually land

3c5d11faadd2f1ca4788e0f7f41a77a11840517e · Keenan Nemetz · 17d ago · parent a63475ce

Phase 3: a request says whether it would actually land

A diff answers "what changed" and nothing else. A branch can read perfectly line by
line and still refuse to merge, and until now nobody found that out until they ran
`git merge` locally — after an approval had been given. The request page now asks
`git merge-tree --write-tree --name-only <base-tip> <head>`, which merges in memory
without a worktree or an index, and says either "this branch no longer merges into
main cleanly: code.txt — rebase or merge main before landing it" or "merges into
main cleanly". `vole request show` prints the same line from a clone.

Three rules, each because the obvious version is wrong:

- Surfaced, not gated. A gate would only ever be right at the instant it ran: the
  base can move after an approval, and the roadmap asked for detection before the
  push rather than a second way to refuse one.
- Silence when the question cannot be asked. merge-tree exits 1 both for a conflict
  and for a revision it cannot resolve, so the exit status alone is not an answer;
  the answer is believed only when the output starts with a tree id, which is what a
  real merge prints first. A base branch that is gone, or a head that was never
  pushed, therefore produces no claim at all — a page that says "clean" about a
  commit it could not load is worse than one that says nothing.
- The clone says what it does not know. `vole request show` reports "cannot check:
  <sha> is not in this clone (git fetch origin)" rather than reading a head it does
  not have as mergeable.

The same view now states the requirement, "0 of 1 required approval(s)", and counts
the approvals that currently apply — the number used to live in the reviewer's head.

Verified: TestMergeConflictsNamesThePaths and TestMergeConflictsIsEmptyWhenItWould
Land build two branches that touch the same line and one that touches another file;
TestMergeConflictsReportsAnUnaskableQuestion pins that an unknown revision is an
error rather than "no conflicts" (that is the bug the tree-id check exists for);
TestParseMergeTree covers clean, empty, one path, two paths and the message section;
TestRequestPageSaysWhenItWouldConflict drives a real conflict through the binary —
main moves the same line after the feature branch was opened — checks the page, the
"0 of 1" line, a clean branch, the CLI's conflict line, the CLI's "cannot check" for
a head that is not in a fresh clone, and a request whose head was never pushed,
which must say nothing. scripts/ci.sh green (gofmt, vet, build, markdown links,
go test -race ./...).

internal/gitx/merge_test.go +124

@@ -0,0 +1,124 @@
1 package gitx
2
3 // The merge check is the one thing a reviewer needs that a diff cannot answer:
4 // a branch can look right line by line and still refuse to merge. These tests pin
5 // the two shapes (clean, conflicted) and the parse, which is the part that would
6 // silently pass a broken branch if it were wrong.
7
8 import (
9 "os"
10 "os/exec"
11 "path/filepath"
12 "strings"
13 "testing"
14 )
15
16 // conflictRepo builds main and a side branch that both change the middle line of
17 // one file, so merging them conflicts, plus a second branch that touches a
18 // different file so a clean merge can be asked for too.
19 func conflictRepo(t *testing.T) string {
20 t.Helper()
21 dir := t.TempDir()
22 run := func(args ...string) {
23 t.Helper()
24 cmd := exec.Command("git", args...)
25 cmd.Dir = dir
26 cmd.Env = append(os.Environ(),
27 "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null",
28 "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.com",
29 "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.com")
30 if out, err := cmd.CombinedOutput(); err != nil {
31 t.Fatalf("git %s: %v: %s", strings.Join(args, " "), err, out)
32 }
33 }
34 write := func(name, body string) {
35 t.Helper()
36 if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil {
37 t.Fatal(err)
38 }
39 }
40 run("init", "-q", "-b", "main")
41 write("f.txt", "a\nb\nc\n")
42 write("g.txt", "x\n")
43 run("add", "-A")
44 run("commit", "-qm", "base")
45
46 run("switch", "-q", "-c", "side")
47 write("f.txt", "a\nSIDE\nc\n")
48 run("commit", "-qam", "side changes the middle")
49
50 run("switch", "-q", "main")
51 write("f.txt", "a\nMAIN\nc\n")
52 write("g.txt", "x\ny\n")
53 run("add", "-A")
54 run("commit", "-qm", "main changes the middle too")
55
56 run("switch", "-q", "-c", "elsewhere", "main")
57 write("g.txt", "x\nz\n")
58 run("commit", "-qam", "elsewhere touches another file")
59 run("switch", "-q", "main")
60 return dir
61 }
62
63 func TestMergeConflictsNamesThePaths(t *testing.T) {
64 r := open(t, conflictRepo(t))
65 paths, err := r.MergeConflicts("main", "side")
66 if err != nil {
67 t.Fatalf("MergeConflicts: %v", err)
68 }
69 if len(paths) != 1 || paths[0] != "f.txt" {
70 t.Fatalf("conflicts = %v, want [f.txt]", paths)
71 }
72 }
73
74 func TestMergeConflictsIsEmptyWhenItWouldLand(t *testing.T) {
75 r := open(t, conflictRepo(t))
76 paths, err := r.MergeConflicts("main", "elsewhere")
77 if err != nil {
78 t.Fatalf("MergeConflicts: %v", err)
79 }
80 if len(paths) != 0 {
81 t.Fatalf("a clean merge reported conflicts: %v", paths)
82 }
83 // Merging a branch into itself is the trivial case and must stay clean.
84 if paths, err := r.MergeConflicts("main", "main"); err != nil || len(paths) != 0 {
85 t.Fatalf("self-merge: %v %v", paths, err)
86 }
87 }
88
89 // TestMergeConflictsReportsAnUnaskableQuestion: an unknown revision is an error,
90 // not "no conflicts". A page that treated it as clean would vouch for a branch it
91 // never checked.
92 func TestMergeConflictsReportsAnUnaskableQuestion(t *testing.T) {
93 r := open(t, conflictRepo(t))
94 if _, err := r.MergeConflicts("main", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); err == nil {
95 t.Fatal("an unknown revision was reported as no conflicts")
96 }
97 }
98
99 // TestParseMergeTree covers the output shapes merge-tree can produce, including
100 // the informational section that follows the paths when messages are enabled.
101 func TestParseMergeTree(t *testing.T) {
102 cases := []struct {
103 name string
104 out string
105 want []string
106 }{
107 {"clean", "904a7da1538496ad28d0611296e5bcf7bb0cbe58\n", nil},
108 {"empty", "", nil},
109 {"one path", "baadc1af8cd4fcf7d36031de2001ae4346cfacc5\nf.txt\n", []string{"f.txt"}},
110 {"two paths", "tree-oid\na.txt\nb.txt\n", []string{"a.txt", "b.txt"}},
111 {"messages after a blank line", "tree-oid\nf.txt\n\nAuto-merging f.txt\nCONFLICT (content): Merge conflict in f.txt\n", []string{"f.txt"}},
112 }
113 for _, c := range cases {
114 got := parseMergeTree(c.out)
115 if len(got) != len(c.want) {
116 t.Fatalf("%s: %v, want %v", c.name, got, c.want)
117 }
118 for i := range got {
119 if got[i] != c.want[i] {
120 t.Fatalf("%s: %v, want %v", c.name, got, c.want)
121 }
122 }
123 }
124 }