Commit `9c7f3c8bef`

9c7f3c8bef697ae69ff985d5470df8cfc8f8ac78

parent: f8f4f38f31

Verified · cmc ci/build: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 17:01 UTC

import: a migrated merge request has something to diff

The bundle carried no head at all, so every replayed merge request had an
empty one. `mr diff` resolves through refs/merge-requests/<n>/head, which
was never set, and a merged merge request has no source branch left to
fall back on — so the diff could only fail.

The bundle carries head_sha, merged_base and merged_at now, a merged one
is replayed with MarkMerged rather than SetMRState so the base a diff is
measured from survives, and the head ref is pointed at the head once the
objects are here.

A bundle is imported before the git push as often as after, so setting
the ref is best-effort and running the import again after the push is
what repairs it. That path used to `continue` on an already-imported
merge request before reaching any of this; it now sets the head it could
not set the first time, which is the only reason to re-run an import at
all.

On the GitHub side the head was already recorded and the ref set when the
objects happened to be present — which for a mirror made with the default
refspecs they are not, since refs/pull/* is not fetched. The import pulls
refs/pull/*/head into refs/gh-pull/* first, in one fetch rather than one
per pull request, with the token going through GIT_ASKPASS the way the
mirror worker does it rather than into a URL in argv. It reports how many
merge requests ended up with no head objects instead of leaving that to
be discovered.

The first version of the test for this passed without the fix: with the
source branch still present the diff resolves through it and an empty
head is invisible. It uses a merged merge request whose branch is deleted
now, which is the case that actually breaks, and fails without the fix.

Closes #128

Layout: unified · split

e2e/migratemr_test.go added +99

@@ -0,0 +1,99 @@
	1	package e2e
	2
	3	import (
	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.
	19	func 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
3	import (	3	import (
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.
		300	const ghAskpass = `#!/bin/sh
		301	case "$1" in
		302	Username*) echo "x-access-token" ;;
		303	*) echo "${GITBAY_GH_TOKEN}" ;;
		304	esac
		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.
		311	func 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

 	"encoding/json"
 	"fmt"
 	"io"
+	"strconv"
 	"strings"
 	"gitbay.org/gitbay/internal/gitutil"
 	Comments  []bundleComment `json:"comments,omitempty"`
 }
 type bundleMR struct {
-	Number    int64           `json:"number"`
+	Number    int64  `json:"number"`
-	Title     string          `json:"title"`
+	Title     string `json:"title"`
-	Body      string          `json:"body"`
+	Body      string `json:"body"`
-	State     string          `json:"state"`
+	State     string `json:"state"`
-	Author    string          `json:"author"`
+	Author    string `json:"author"`
-	SourceRef string          `json:"source_ref"`
+	SourceRef string `json:"source_ref"`
-	TargetRef string          `json:"target_ref"`
+	TargetRef string `json:"target_ref"`
-	CreatedAt string          `json:"created_at"`
+	// HeadSHA and MergedBase are what a diff is measured between. The
-	Comments  []bundleComment `json:"comments,omitempty"`
+	// bundle carried neither, so every migrated merge request arrived
+	// with an empty head and `mr diff` could only fail on it (#128).
+	// They are recorded whether or not the objects have been pushed yet;
+	// the ref is pointed at the head once they have.
+	HeadSHA    string          `json:"head_sha,omitempty"`
+	MergedBase string          `json:"merged_base,omitempty"`
+	MergedAt   string          `json:"merged_at,omitempty"`
+	CreatedAt  string          `json:"created_at"`
+	Comments   []bundleComment `json:"comments,omitempty"`
 }
 type bundleRepo struct {
 	Name          string             `json:"name"`
 		for i := len(mrs) - 1; i >= 0; i-- {
 			m := mrs[i]
 			bm := bundleMR{Number: m.Number, Title: m.Title, Body: m.Body, State: m.State,
-				Author: m.Author, SourceRef: m.SourceRef, TargetRef: m.TargetRef, CreatedAt: m.CreatedAt}
+				Author: m.Author, SourceRef: m.SourceRef, TargetRef: m.TargetRef,
+				HeadSHA: m.HeadSHA, MergedBase: m.MergedBase, MergedAt: m.MergedAt,
+				CreatedAt: m.CreatedAt}
 			if cs, err := c.Store.ListMRComments(m.ID); err == nil {
 				for _, cm := range cs {
 					bm.Comments = append(bm.Comments, bundleComment{cm.Author, cm.Body, cm.CreatedAt})
 		}
 		for _, bm := range br.MRs {
 			key := fmt.Sprintf("mig-mr:%d", bm.Number)
-			if _, seen, _ := c.Store.ImportMarker(repo.ID, key); seen {
+			if val, seen, _ := c.Store.ImportMarker(repo.ID, key); seen {
 				skipped++
+				// A bundle is imported before the git push as often as
+				// after, so the objects a merge request needs may only
+				// have arrived since. Re-running the import is how the
+				// head ref gets set, and doing that costs nothing when it
+				// is already right.
+				if local, err := strconv.ParseInt(val, 10, 64); err == nil {
+					setMigratedHead(c, repo, local, bm.HeadSHA)
+				}
 				continue
 			}
 			body := migAttribution(src, "merge request", bm.Author, bm.CreatedAt, bm.Number) + bm.Body
-			n, err := c.Store.CreateMR(repo.ID, c.User.ID, repo.ID, bm.SourceRef, bm.TargetRef, bm.Title, body, "", "md", false)
+			n, err := c.Store.CreateMR(repo.ID, c.User.ID, repo.ID, bm.SourceRef, bm.TargetRef, bm.Title, body, bm.HeadSHA, "md", false)
 			if err != nil {
 				return c.fail(protocol.ExitFailure, "%v", err)
 			}
 			if err != nil {
 				return c.fail(protocol.ExitFailure, "%v", err)
 			}
-			if bm.State != "open" {
+			switch {
+			case bm.State == "merged":
+				// MarkMerged, not SetMRState: a merged MR's diff is
+				// measured from the base recorded at merge time, and
+				// SetMRState leaves that empty.
+				c.Store.MarkMerged(mr.ID, bm.MergedBase, 0, bm.MergedAt)
+			case bm.State != "open":
 				state := bm.State
 				if state == "source_gone" {
 					state = "closed"
 				}
 				c.Store.SetMRState(mr.ID, state)
 			}
+			setMigratedHead(c, repo, n, bm.HeadSHA)
 			for _, cm := range bm.Comments {
 				c.Store.AddMRComment(mr.ID, c.User.ID,
 					fmt.Sprintf("> %s, %.10s\n\n%s", cm.Author, cm.CreatedAt, cm.Body), "md")
 }
 var _ = strings.TrimSpace // placeholder against accidental import drops
+// setMigratedHead points refs/merge-requests/<n>/head at the head a
+// bundle recorded, once the objects for it are present. `mr diff`
+// resolves through that ref, not through the stored head_sha, so a merge
+// request whose source branch is gone — every merged one — has nothing to
+// diff without it (#128).
+//
+// Best-effort by design: a bundle is imported before the git push as
+// often as after. head_sha is stored either way, and running the import
+// again after the push sets the ref.
+func setMigratedHead(c *Ctx, repo store.Repo, number int64, headSHA string) {
+	if headSHA == "" {
+		return
+	}
+	dir := RepoDir(c.Cfg.Server.Root, repo.OwnerName, repo.Name)
+	if !gitutil.HasCommit(dir, headSHA) {
+		return
+	}
+	ref := fmt.Sprintf("refs/merge-requests/%d/head", number)
+	if cur, err := gitutil.ResolveRef(dir, ref); err == nil && cur == headSHA {
+		return
+	}
+	gitutil.UpdateRefCAS(dir, ref, headSHA, "")
+}

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.
		170	func 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.
161	func RemoteDefaultBranch(ctx context.Context, url string, extraEnv []string) (string, error) {	182	func 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")

Commit 9c7f3c8bef

Commit `9c7f3c8bef`