krz/gitbay

import: a migrated merge request has something to diff !243

merged merged by cmc on 2026-09-04 17:13 UTC · krz/gitbay:import-heads into main

4 files changed, +239 −17

Layout: unified · split

e2e/migratemr_test.go added +99
@@ -0,0 +1,99 @@
1package e2e
2
3import (
4 "os"
5 "path/filepath"
6 "strings"
7 "testing"
8)
9
10// TestMigratedMRHasADiff covers #128's concrete symptom: a merge request
11// replayed from a bundle used to arrive with an empty head, so `mr diff`
12// on it could only fail. The bundle carries the head and the merge base
13// now, and the head ref is set once the git objects are pushed.
14//
15// The merge request here is merged and its source branch deleted, which
16// is the case that actually breaks. While the branch still exists the
17// diff resolves through it and an empty head_sha is invisible — the first
18// version of this test made that mistake and passed without the fix.
19func TestMigratedMRHasADiff(t *testing.T) {
20 src := startInstance(t)
21 dst := startInstance(t)
22 key := src.newKey(t, "alice")
23 src.admin(t, "admin", "user", "create", "alice", "--key", key+".pub")
24 dst.admin(t, "admin", "user", "create", "alice", "--key", key+".pub")
25
26 if _, errOut, code := src.ssh(t, key, "", "repo", "create", "alice/tool"); code != 0 {
27 t.Fatalf("repo create: %s", errOut)
28 }
29 env := src.gitEnv(key)
30 work := t.TempDir()
31 mustGit(t, work, env, "clone", src.sshURL("alice/tool"), "w")
32 dir := filepath.Join(work, "w")
33 os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n"), 0o644)
34 mustGit(t, dir, env, "checkout", "-q", "-b", "main")
35 mustGit(t, dir, env, "add", ".")
36 mustGit(t, dir, env, "commit", "-q", "-m", "base")
37 mustGit(t, dir, env, "push", "-q", "origin", "main")
38
39 mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
40 os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n\nfunc F() {}\n"), 0o644)
41 mustGit(t, dir, env, "add", ".")
42 mustGit(t, dir, env, "commit", "-q", "-m", "add F")
43 mustGit(t, dir, env, "push", "-q", "origin", "feat")
44 if _, errOut, code := src.ssh(t, key, "", "mr", "create", "alice/tool",
45 "--source", "feat", "--target", "main", "--title", "'add F'"); code != 0 {
46 t.Fatalf("mr create: %s", errOut)
47 }
48 // Merge it and delete the branch, the way a finished merge request
49 // ends up. Now nothing but the recorded head says what it contained.
50 if _, errOut, code := src.ssh(t, key, "", "mr", "merge", "alice/tool", "1"); code != 0 {
51 t.Fatalf("merge: %s", errOut)
52 }
53 mustGit(t, dir, env, "push", "-q", "origin", "--delete", "feat")
54
55 // The source's own diff is the standard to match.
56 want, errOut, code := src.ssh(t, key, "", "mr", "diff", "alice/tool", "1")
57 if code != 0 || !strings.Contains(want, "func F()") {
58 t.Fatalf("source diff: %s\n%s", errOut, want)
59 }
60
61 bundle, errOut, code := src.ssh(t, key, "", "account", "export")
62 if code != 0 {
63 t.Fatalf("export: %s", errOut)
64 }
65 if !strings.Contains(bundle, `"head_sha"`) {
66 t.Fatalf("bundle carries no head_sha:\n%s", bundle)
67 }
68 if _, errOut, code := dst.ssh(t, key, bundle, "account", "import-bundle"); code != 0 {
69 t.Fatalf("import: %s", errOut)
70 }
71
72 // Git data moves separately, which is the order a real migration runs
73 // in: the bundle first, the push after.
74 // Only main: the feature branch is gone, exactly as at the source.
75 // The merge commit carries the head's objects, so they arrive anyway.
76 denv := dst.gitEnv(key)
77 mustGit(t, dir, denv, "remote", "add", "dst", dst.sshURL("alice/tool"))
78 mustGit(t, dir, denv, "fetch", "-q", "origin", "main")
79 mustGit(t, dir, denv, "push", "-q", "dst", "refs/remotes/origin/main:refs/heads/main")
80
81 // Re-importing the same bundle is how the head ref gets set once the
82 // objects are present; the merge request itself is already there.
83 out, errOut, code := dst.ssh(t, key, bundle, "account", "import-bundle")
84 if code != 0 {
85 t.Fatalf("re-import: %s", errOut)
86 }
87 if !strings.Contains(out, "already present") {
88 t.Fatalf("re-import did not skip what it had: %s", out)
89 }
90
91 show, _, _ := dst.ssh(t, key, "", "mr", "show", "alice/tool", "1", "--json")
92 got, errOut, code := dst.ssh(t, key, "", "mr", "diff", "alice/tool", "1")
93 if code != 0 {
94 t.Fatalf("migrated MR has no diff: %s\n%s\nmr show: %s", errOut, got, show)
95 }
96 if !strings.Contains(got, "func F()") {
97 t.Fatalf("migrated diff does not match the source:\nwant to contain func F()\ngot:\n%s", got)
98 }
99}
internal/control/ghimport.go +56 −4
@@ -2,11 +2,14 @@ package control
2 2
3import ( 3import (
4 "bufio" 4 "bufio"
5 "context"
5 "encoding/json" 6 "encoding/json"
6 "fmt" 7 "fmt"
7 "io" 8 "io"
8 "net/http" 9 "net/http"
9 "net/url" 10 "net/url"
11 "os"
12 "path/filepath"
10 "strconv" 13 "strconv"
11 "strings" 14 "strings"
12 "time" 15 "time"
@@ -140,9 +143,18 @@ func runImportIssues(c *Ctx, args []string) int {
140 } 143 }
141 g := &ghClient{base: apiBase, token: token, http: &http.Client{Timeout: 30 * time.Second}} 144 g := &ghClient{base: apiBase, token: token, http: &http.Client{Timeout: 30 * time.Second}}
142 dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name) 145 dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name)
146
147 // Pull heads first, so every merge request below has objects to point
148 // at. A mirror made with the default refspecs does not carry
149 // refs/pull/*, which is why imported pull requests used to have no
150 // head and `mr diff` could only fail on them (#128). Best-effort: an
151 // import of issues from a repository whose git data is not here yet
152 // is a legitimate thing to do, and the merge requests still arrive
153 // with their head SHA recorded.
154 fetchedPullHeads := fetchPullHeads(c, dir, from, token)
143 src := "github.com/" + from 155 src := "github.com/" + from
144 156
145 var issues, mrs, comments, skipped int 157 var issues, mrs, comments, skipped, headed int
146 for page := 1; ; page++ { 158 for page := 1; ; page++ {
147 var items []ghIssue 159 var items []ghIssue
148 q := fmt.Sprintf("/repos/%s/issues?state=all&sort=created&direction=asc&per_page=100&page=%d", from, page) 160 q := fmt.Sprintf("/repos/%s/issues?state=all&sort=created&direction=asc&per_page=100&page=%d", from, page)
@@ -182,10 +194,11 @@ func runImportIssues(c *Ctx, args []string) int {
182 } else { 194 } else {
183 c.Store.MarkClosed(mr.ID, 0, it.ClosedAt) 195 c.Store.MarkClosed(mr.ID, 0, it.ClosedAt)
184 } 196 }
185 // Point the MR head ref at the PR head when the mirror 197 // Point the MR head ref at the PR head, from the fetch
186 // already holds the objects (refs/pull backups). 198 // above or from objects a mirror already had.
187 if pr.Head.SHA != "" && gitutil.HasCommit(dir, pr.Head.SHA) { 199 if pr.Head.SHA != "" && gitutil.HasCommit(dir, pr.Head.SHA) {
188 gitutil.UpdateRefCAS(dir, fmt.Sprintf("refs/merge-requests/%d/head", localN), pr.Head.SHA, "") 200 gitutil.UpdateRefCAS(dir, fmt.Sprintf("refs/merge-requests/%d/head", localN), pr.Head.SHA, "")
201 headed++
189 } 202 }
190 c.Store.SetImportMarker(repo.ID, key, fmt.Sprintf("mr:%d", localN)) 203 c.Store.SetImportMarker(repo.ID, key, fmt.Sprintf("mr:%d", localN))
191 mrs++ 204 mrs++
@@ -218,10 +231,17 @@ func runImportIssues(c *Ctx, args []string) int {
218 fmt.Fprintf(c.Stderr, "%s#%d -> %s%d\n", src, it.Number, map[bool]string{true: "!", false: "#"}[isPR], localN) 231 fmt.Fprintf(c.Stderr, "%s#%d -> %s%d\n", src, it.Number, map[bool]string{true: "!", false: "#"}[isPR], localN)
219 } 232 }
220 } 233 }
221 d := map[string]any{"issues": issues, "mrs": mrs, "comments": comments, "already_imported": skipped} 234 d := map[string]any{"issues": issues, "mrs": mrs, "comments": comments,
235 "already_imported": skipped, "mrs_with_head": headed, "pull_heads_fetched": fetchedPullHeads}
222 return c.emit(d, func(w io.Writer) { 236 return c.emit(d, func(w io.Writer) {
223 fmt.Fprintf(w, "imported %d issues, %d merge requests, %d comments (%d items already imported)\n", 237 fmt.Fprintf(w, "imported %d issues, %d merge requests, %d comments (%d items already imported)\n",
224 issues, mrs, comments, skipped) 238 issues, mrs, comments, skipped)
239 if mrs > headed {
240 fmt.Fprintf(w, "%d merge request(s) have no head objects; `mr diff` cannot render them.\n", mrs-headed)
241 if !fetchedPullHeads {
242 fmt.Fprintln(w, "refs/pull/* could not be fetched — re-run with --token-stdin if the repository is private.")
243 }
244 }
225 }) 245 })
226} 246}
227 247
@@ -273,3 +293,35 @@ func importComments(c *Ctx, g *ghClient, repo store.Repo, from, src string, ghN,
273 } 293 }
274 } 294 }
275} 295}
296
297// ghAskpass answers git's credential prompts from the environment, so a
298// token never appears in argv where /proc would expose it. Same shape the
299// mirror worker uses.
300const ghAskpass = `#!/bin/sh
301case "$1" in
302 Username*) echo "x-access-token" ;;
303 *) echo "${GITBAY_GH_TOKEN}" ;;
304esac
305`
306
307// fetchPullHeads brings refs/pull/*/head into refs/gh-pull/*. Reports
308// whether it worked; a failure is not fatal, since importing issues from
309// a repository whose git data is not here yet is a reasonable thing to
310// do.
311func fetchPullHeads(c *Ctx, dir, from, token string) bool {
312 env := []string{"GIT_TERMINAL_PROMPT=0", "HOME=" + c.Cfg.Server.Root}
313 if token != "" {
314 askpass := filepath.Join(c.Cfg.Server.Root, "gh-import-askpass.sh")
315 if err := os.WriteFile(askpass, []byte(ghAskpass), 0o700); err != nil {
316 return false
317 }
318 env = append(env, "GIT_ASKPASS="+askpass, "GITBAY_GH_TOKEN="+token)
319 }
320 ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute)
321 defer cancel()
322 url := "https://github.com/" + from + ".git"
323 if err := gitutil.FetchPullHeads(ctx, dir, url, io.Discard, env); err != nil {
324 return false
325 }
326 return true
327}
internal/control/migrate.go +63 −13
@@ -4,6 +4,7 @@ import (
4 "encoding/json" 4 "encoding/json"
5 "fmt" 5 "fmt"
6 "io" 6 "io"
7 "strconv"
7 "strings" 8 "strings"
8 9
9 "gitbay.org/gitbay/internal/gitutil" 10 "gitbay.org/gitbay/internal/gitutil"
@@ -42,15 +43,23 @@ type bundleIssue struct {
42 Comments []bundleComment `json:"comments,omitempty"` 43 Comments []bundleComment `json:"comments,omitempty"`
43} 44}
44type bundleMR struct { 45type bundleMR struct {
45 Number int64 `json:"number"` 46 Number int64 `json:"number"`
46 Title string `json:"title"` 47 Title string `json:"title"`
47 Body string `json:"body"` 48 Body string `json:"body"`
48 State string `json:"state"` 49 State string `json:"state"`
49 Author string `json:"author"` 50 Author string `json:"author"`
50 SourceRef string `json:"source_ref"` 51 SourceRef string `json:"source_ref"`
51 TargetRef string `json:"target_ref"` 52 TargetRef string `json:"target_ref"`
52 CreatedAt string `json:"created_at"` 53 // HeadSHA and MergedBase are what a diff is measured between. The
53 Comments []bundleComment `json:"comments,omitempty"` 54 // bundle carried neither, so every migrated merge request arrived
55 // with an empty head and `mr diff` could only fail on it (#128).
56 // They are recorded whether or not the objects have been pushed yet;
57 // the ref is pointed at the head once they have.
58 HeadSHA string `json:"head_sha,omitempty"`
59 MergedBase string `json:"merged_base,omitempty"`
60 MergedAt string `json:"merged_at,omitempty"`
61 CreatedAt string `json:"created_at"`
62 Comments []bundleComment `json:"comments,omitempty"`
54} 63}
55type bundleRepo struct { 64type bundleRepo struct {
56 Name string `json:"name"` 65 Name string `json:"name"`
@@ -110,7 +119,9 @@ func runAccountExport(c *Ctx, args []string) int {
110 for i := len(mrs) - 1; i >= 0; i-- { 119 for i := len(mrs) - 1; i >= 0; i-- {
111 m := mrs[i] 120 m := mrs[i]
112 bm := bundleMR{Number: m.Number, Title: m.Title, Body: m.Body, State: m.State, 121 bm := bundleMR{Number: m.Number, Title: m.Title, Body: m.Body, State: m.State,
113 Author: m.Author, SourceRef: m.SourceRef, TargetRef: m.TargetRef, CreatedAt: m.CreatedAt} 122 Author: m.Author, SourceRef: m.SourceRef, TargetRef: m.TargetRef,
123 HeadSHA: m.HeadSHA, MergedBase: m.MergedBase, MergedAt: m.MergedAt,
124 CreatedAt: m.CreatedAt}
114 if cs, err := c.Store.ListMRComments(m.ID); err == nil { 125 if cs, err := c.Store.ListMRComments(m.ID); err == nil {
115 for _, cm := range cs { 126 for _, cm := range cs {
116 bm.Comments = append(bm.Comments, bundleComment{cm.Author, cm.Body, cm.CreatedAt}) 127 bm.Comments = append(bm.Comments, bundleComment{cm.Author, cm.Body, cm.CreatedAt})
@@ -251,12 +262,20 @@ func runAccountImportBundle(c *Ctx, args []string) int {
251 } 262 }
252 for _, bm := range br.MRs { 263 for _, bm := range br.MRs {
253 key := fmt.Sprintf("mig-mr:%d", bm.Number) 264 key := fmt.Sprintf("mig-mr:%d", bm.Number)
254 if _, seen, _ := c.Store.ImportMarker(repo.ID, key); seen { 265 if val, seen, _ := c.Store.ImportMarker(repo.ID, key); seen {
255 skipped++ 266 skipped++
267 // A bundle is imported before the git push as often as
268 // after, so the objects a merge request needs may only
269 // have arrived since. Re-running the import is how the
270 // head ref gets set, and doing that costs nothing when it
271 // is already right.
272 if local, err := strconv.ParseInt(val, 10, 64); err == nil {
273 setMigratedHead(c, repo, local, bm.HeadSHA)
274 }
256 continue 275 continue
257 } 276 }
258 body := migAttribution(src, "merge request", bm.Author, bm.CreatedAt, bm.Number) + bm.Body 277 body := migAttribution(src, "merge request", bm.Author, bm.CreatedAt, bm.Number) + bm.Body
259 n, err := c.Store.CreateMR(repo.ID, c.User.ID, repo.ID, bm.SourceRef, bm.TargetRef, bm.Title, body, "", "md", false) 278 n, err := c.Store.CreateMR(repo.ID, c.User.ID, repo.ID, bm.SourceRef, bm.TargetRef, bm.Title, body, bm.HeadSHA, "md", false)
260 if err != nil { 279 if err != nil {
261 return c.fail(protocol.ExitFailure, "%v", err) 280 return c.fail(protocol.ExitFailure, "%v", err)
262 } 281 }
@@ -264,13 +283,20 @@ func runAccountImportBundle(c *Ctx, args []string) int {
264 if err != nil { 283 if err != nil {
265 return c.fail(protocol.ExitFailure, "%v", err) 284 return c.fail(protocol.ExitFailure, "%v", err)
266 } 285 }
267 if bm.State != "open" { 286 switch {
287 case bm.State == "merged":
288 // MarkMerged, not SetMRState: a merged MR's diff is
289 // measured from the base recorded at merge time, and
290 // SetMRState leaves that empty.
291 c.Store.MarkMerged(mr.ID, bm.MergedBase, 0, bm.MergedAt)
292 case bm.State != "open":
268 state := bm.State 293 state := bm.State
269 if state == "source_gone" { 294 if state == "source_gone" {
270 state = "closed" 295 state = "closed"
271 } 296 }
272 c.Store.SetMRState(mr.ID, state) 297 c.Store.SetMRState(mr.ID, state)
273 } 298 }
299 setMigratedHead(c, repo, n, bm.HeadSHA)
274 for _, cm := range bm.Comments { 300 for _, cm := range bm.Comments {
275 c.Store.AddMRComment(mr.ID, c.User.ID, 301 c.Store.AddMRComment(mr.ID, c.User.ID,
276 fmt.Sprintf("> %s, %.10s\n\n%s", cm.Author, cm.CreatedAt, cm.Body), "md") 302 fmt.Sprintf("> %s, %.10s\n\n%s", cm.Author, cm.CreatedAt, cm.Body), "md")
@@ -299,3 +325,27 @@ func runAccountImportBundle(c *Ctx, args []string) int {
299} 325}
300 326
301var _ = strings.TrimSpace // placeholder against accidental import drops 327var _ = strings.TrimSpace // placeholder against accidental import drops
328
329// setMigratedHead points refs/merge-requests/<n>/head at the head a
330// bundle recorded, once the objects for it are present. `mr diff`
331// resolves through that ref, not through the stored head_sha, so a merge
332// request whose source branch is gone — every merged one — has nothing to
333// diff without it (#128).
334//
335// Best-effort by design: a bundle is imported before the git push as
336// often as after. head_sha is stored either way, and running the import
337// again after the push sets the ref.
338func setMigratedHead(c *Ctx, repo store.Repo, number int64, headSHA string) {
339 if headSHA == "" {
340 return
341 }
342 dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name)
343 if !gitutil.HasCommit(dir, headSHA) {
344 return
345 }
346 ref := fmt.Sprintf("refs/merge-requests/%d/head", number)
347 if cur, err := gitutil.ResolveRef(dir, ref); err == nil && cur == headSHA {
348 return
349 }
350 gitutil.UpdateRefCAS(dir, ref, headSHA, "")
351}
internal/gitutil/gitutil.go +21
@@ -157,6 +157,27 @@ func FetchMirror(ctx context.Context, dir, url string, errW io.Writer, extraEnv
157 return nil 157 return nil
158} 158}
159 159
160// FetchPullHeads pulls a GitHub repository's pull-request heads into
161// refs/gh-pull/*, so an imported pull request has something to diff.
162// GitHub publishes every PR head at refs/pull/<n>/head on the git remote,
163// but a mirror made with the default refspecs does not carry them, which
164// is why an import used to produce merge requests with no head at all.
165//
166// One fetch for every pull request rather than one each: the ref count is
167// the repository's history, and asking a hundred times is a hundred
168// handshakes. extraEnv carries credentials via GIT_ASKPASS; the URL must
169// never contain them.
170func FetchPullHeads(ctx context.Context, dir, url string, errW io.Writer, extraEnv []string) error {
171 cmd := exec.CommandContext(ctx, "git", "-C", dir, "fetch", "--no-write-fetch-head", "--no-tags",
172 url, "+refs/pull/*/head:refs/gh-pull/*")
173 cmd.Env = append(os.Environ(), extraEnv...)
174 cmd.Stderr = errW
175 if err := cmd.Run(); err != nil {
176 return fmt.Errorf("fetch pull heads from %s: %w", url, err)
177 }
178 return nil
179}
180
160// RemoteDefaultBranch asks the remote which branch HEAD points at. 181// RemoteDefaultBranch asks the remote which branch HEAD points at.
161func RemoteDefaultBranch(ctx context.Context, url string, extraEnv []string) (string, error) { 182func RemoteDefaultBranch(ctx context.Context, url string, extraEnv []string) (string, error) {
162 cmd := exec.CommandContext(ctx, "git", "ls-remote", "--symref", url, "HEAD") 183 cmd := exec.CommandContext(ctx, "git", "ls-remote", "--symref", url, "HEAD")