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.

Resolve a change request's head on the remote, not the clone

6755b7515c2e3e404e8325eabc0b1e0b74ab901f · Keenan Nemetz · 10d ago · parent 26c9be33 

Resolve a change request's head on the remote, not the clone

`change new --head <branch>` resolved the branch in the local clone. A
clone that was behind recorded the old commit, and when that commit was
already in the base the request contained nothing — which is how #117
merged nothing, closed its issue, and left the work on a side branch.

- A branch's head is resolved with `git ls-remote` against the remote the
  metadata write pushes to, so the recorded commit is the forge's tip. If
  local and remote differ, the remote tip is recorded and stderr says so;
  a local-only branch is recorded with a note; `--no-push` may fall back
  to the local ref when the remote cannot be reached.
- A request whose head is already in the base (the same commit, or an
  ancestor) is refused before anything is written, for `change new` and
  `change update --head`. The base is resolved on the remote too: a
  feature worktree commits on its local base branch, so the local base can
  be the head itself and would call every request empty.
- The tag/commit path is unchanged: it still pins the request to that
  commit with no branch.

Tests cover a branch that moved after the clone, a local-only branch, a
head equal to the base, a head that is an ancestor of the base, and the
`change update --head` refusal. The head-resolution note is documented in
docs/change-requests.md.

server/change_heads_test.go +211

@@ -0,0 +1,211 @@
1 package server_test
2
3 // The head of a change request is the commit the forge will review, so it has to
4 // come from the forge's view of the branch, not from a clone that may be behind.
5 // These tests hold the three cases: a stale local branch uses the remote tip, a
6 // head already in the base is refused, and a local-only branch is created with a
7 // note rather than silently.
8
9 import (
10 "os"
11 "path/filepath"
12 "strings"
13 "testing"
14 )
15
16 func requestRecord(t *testing.T, f *fixture, id string) string {
17 t.Helper()
18 raw, _, err := runGit(t, filepath.Join(f.cfg.ReposDir(), f.repo+".git"),
19 "show", "refs/heads/vole:requests/"+id+"/request.md")
20 if err != nil {
21 t.Fatalf("read request %s: %v\n%s", id, err, raw)
22 }
23 return raw
24 }
25
26 func TestChangeNewUsesTheRemoteTipWhenTheLocalBranchIsBehind(t *testing.T) {
27 t.Parallel()
28 f := newFixture(t)
29 f.publish(f.repo)
30
31 shared := workRepo(t, map[string]string{"readme.md": "v1\n"})
32 if _, errs, err := f.git(shared, "push", f.sshURL(f.repo), "main:refs/heads/main"); err != nil {
33 t.Fatalf("push main: %s", errs)
34 }
35 runGit(t, shared, "checkout", "-q", "-b", "feature")
36 if err := os.WriteFile(filepath.Join(shared, "readme.md"), []byte("v1\nfeature\n"), 0o644); err != nil {
37 t.Fatal(err)
38 }
39 runGit(t, shared, "commit", "-qam", "feature")
40 if _, errs, err := f.git(shared, "push", f.sshURL(f.repo), "feature:refs/heads/feature"); err != nil {
41 t.Fatalf("push feature: %s", errs)
42 }
43 oldTip, _, err := runGit(t, shared, "rev-parse", "feature")
44 if err != nil {
45 t.Fatal(err)
46 }
47 oldTip = strings.TrimSpace(oldTip)
48
49 clone := filepath.Join(t.TempDir(), "clone")
50 if _, errs, err := f.git("", "clone", "-q", f.sshURL(f.repo), clone); err != nil {
51 t.Fatalf("clone: %s", errs)
52 }
53 if _, errs, err := runGit(t, clone, "checkout", "-q", "-b", "feature", "origin/feature"); err != nil {
54 t.Fatalf("checkout feature: %s", errs)
55 }
56
57 // The branch moves on the forge after the clone was taken. The clone still
58 // has the old tip, which is exactly the state that recorded a no-op request
59 // in #117.
60 if err := os.WriteFile(filepath.Join(shared, "readme.md"), []byte("v1\nfeature\nmore\n"), 0o644); err != nil {
61 t.Fatal(err)
62 }
63 runGit(t, shared, "commit", "-qam", "feature moves on")
64 if _, errs, err := f.git(shared, "push", f.sshURL(f.repo), "feature:refs/heads/feature"); err != nil {
65 t.Fatalf("push moved feature: %s", errs)
66 }
67 newTip, _, err := runGit(t, shared, "rev-parse", "feature")
68 if err != nil {
69 t.Fatal(err)
70 }
71 newTip = strings.TrimSpace(newTip)
72 if newTip == oldTip {
73 t.Fatal("the fixture did not move the branch")
74 }
75
76 out, errs, err := f.cli(clone, "change", "new", "--title", "stale local head",
77 "--base", "main", "--head", "feature", "-m", "why")
78 if err != nil {
79 t.Fatalf("change new: %v\n%s", err, errs)
80 }
81 if !strings.Contains(errs, "remote tip") {
82 t.Fatalf("the stale branch was not explained:\n%s", errs)
83 }
84 id := strings.Fields(out)[0]
85 record := requestRecord(t, f, id)
86 if !strings.Contains(record, "head: "+newTip) {
87 t.Fatalf("request did not record the remote tip %s:\n%s", newTip, record)
88 }
89 if !strings.Contains(record, "head_ref: feature") {
90 t.Fatalf("request does not follow the branch:\n%s", record)
91 }
92 if strings.Contains(record, "head: "+oldTip) {
93 t.Fatalf("request recorded the stale local tip %s:\n%s", oldTip, record)
94 }
95 }
96
97 func TestChangeNewRefusesAHeadAlreadyInTheBase(t *testing.T) {
98 t.Parallel()
99 f := newFixture(t)
100 f.publish(f.repo)
101
102 shared := workRepo(t, map[string]string{"readme.md": "v1\n"})
103 if err := os.WriteFile(filepath.Join(shared, "readme.md"), []byte("v1\nv2\n"), 0o644); err != nil {
104 t.Fatal(err)
105 }
106 runGit(t, shared, "commit", "-qam", "second")
107 if _, errs, err := f.git(shared, "push", f.sshURL(f.repo), "main:refs/heads/main"); err != nil {
108 t.Fatalf("push main: %s", errs)
109 }
110 first, _, err := runGit(t, shared, "rev-parse", "main~1")
111 if err != nil {
112 t.Fatal(err)
113 }
114 first = strings.TrimSpace(first)
115
116 clone := filepath.Join(t.TempDir(), "clone")
117 if _, errs, err := f.git("", "clone", "-q", f.sshURL(f.repo), clone); err != nil {
118 t.Fatalf("clone: %s", errs)
119 }
120 if _, errs, err := runGit(t, clone, "config", "user.name", "Keenan Nemetz"); err != nil {
121 t.Fatalf("config: %s", errs)
122 }
123 if _, errs, err := runGit(t, clone, "config", "user.email", "keenan@example.com"); err != nil {
124 t.Fatalf("config: %s", errs)
125 }
126
127 for _, head := range []string{"main", first} {
128 _, errs, err := f.cli(clone, "change", "new", "--title", "nothing to review",
129 "--base", "main", "--head", head, "-m", "why")
130 if err == nil {
131 t.Fatalf("change new --head %s accepted a head already in the base", head)
132 }
133 if !strings.Contains(errs, "nothing to review") {
134 t.Fatalf("change new --head %s: %s", head, errs)
135 }
136 }
137
138 // Nothing was written by the refusals.
139 if out, _, err := f.cli(clone, "change", "list", "--state", "all"); err != nil {
140 t.Fatalf("change list: %v", err)
141 } else if strings.TrimSpace(out) != "" {
142 t.Fatalf("a refused request was written:\n%s", out)
143 }
144
145 // A real request is still allowed, and moving it onto its base is refused too.
146 runGit(t, clone, "checkout", "-q", "-b", "topic")
147 if err := os.WriteFile(filepath.Join(clone, "readme.md"), []byte("v1\nv2\ntopic\n"), 0o644); err != nil {
148 t.Fatal(err)
149 }
150 runGit(t, clone, "commit", "-qam", "topic")
151 if _, errs, err := f.git(clone, "push", f.sshURL(f.repo), "topic:refs/heads/topic"); err != nil {
152 t.Fatalf("push topic: %s", errs)
153 }
154 out, errs, err := f.cli(clone, "change", "new", "--title", "real change",
155 "--base", "main", "--head", "topic", "-m", "why")
156 if err != nil {
157 t.Fatalf("change new: %v\n%s", err, errs)
158 }
159 id := strings.Fields(out)[0]
160 if _, errs, err := f.cli(clone, "change", "update", id, "--head", "main"); err == nil {
161 t.Fatal("change update moved a request's head onto its base")
162 } else if !strings.Contains(errs, "nothing to review") {
163 t.Fatalf("change update --head main: %s", errs)
164 }
165 }
166
167 func TestChangeNewNotesALocalOnlyBranch(t *testing.T) {
168 t.Parallel()
169 f := newFixture(t)
170 f.publish(f.repo)
171
172 shared := workRepo(t, map[string]string{"readme.md": "v1\n"})
173 if _, errs, err := f.git(shared, "push", f.sshURL(f.repo), "main:refs/heads/main"); err != nil {
174 t.Fatalf("push main: %s", errs)
175 }
176 clone := filepath.Join(t.TempDir(), "clone")
177 if _, errs, err := f.git("", "clone", "-q", f.sshURL(f.repo), clone); err != nil {
178 t.Fatalf("clone: %s", errs)
179 }
180 for _, kv := range [][2]string{{"user.name", "Keenan Nemetz"}, {"user.email", "keenan@example.com"}} {
181 if _, errs, err := runGit(t, clone, "config", kv[0], kv[1]); err != nil {
182 t.Fatalf("config: %s", errs)
183 }
184 }
185 runGit(t, clone, "checkout", "-q", "-b", "local-work")
186 if err := os.WriteFile(filepath.Join(clone, "readme.md"), []byte("v1\nlocal\n"), 0o644); err != nil {
187 t.Fatal(err)
188 }
189 runGit(t, clone, "commit", "-qam", "local only")
190 tip, _, err := runGit(t, clone, "rev-parse", "local-work")
191 if err != nil {
192 t.Fatal(err)
193 }
194 tip = strings.TrimSpace(tip)
195
196 out, errs, err := f.cli(clone, "change", "new", "--title", "local only",
197 "--base", "main", "--head", "local-work", "-m", "why")
198 if err != nil {
199 t.Fatalf("change new: %v\n%s", err, errs)
200 }
201 if !strings.Contains(errs, "not on") {
202 t.Fatalf("the local-only branch was not explained:\n%s", errs)
203 }
204 id := strings.Fields(out)[0]
205 record := requestRecord(t, f, id)
206 for _, want := range []string{"head: " + tip, "head_ref: local-work"} {
207 if !strings.Contains(record, want) {
208 t.Fatalf("request is missing %q:\n%s", want, record)
209 }
210 }
211 }