Commit `84e1f08f23`

84e1f08f231254004e85a063d38a38ede6b1b931

parent: 37841c7286

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

cmc <hello@cleberg.net> · 2026-09-04 00:59 UTC

control: no write sequence leaves a half state

Merge: UpdateRefCAS then MarkMerged, and if the second failed the
branch had moved while the merge request stayed open, refused forever
with "target already contains the MR head". A head the target already
contains is now recorded as merged, which also covers a branch merged
by hand and pushed.

Fork: CreateRepo then SetForkOf; a failure between them left a plain
repository. CreateFork sets the parent in the insert.

Transfer: the revert after a failed directory move ignored its own
error; it is reported now, naming the state left behind.

Quota: the count and the insert were check-then-act with no lock; a
process-wide mutex covers create, fork and import.

TestMergeRecordedWhenTargetContainsHead and TestCreateForkSetsParent.

Closes #108

Layout: unified · split

e2e/mergerecord_test.go added +49

		@@ -0,0 +1,49 @@
	1	package e2e
	2
	3	import (
	4	"os"
	5	"path/filepath"
	6	"strings"
	7	"testing"
	8	)
	9
	10	// A merge request whose head the target already contains is recorded as
	11	// merged rather than refused: the branch was merged by hand and pushed,
	12	// or a merge moved the ref and then failed to record itself. Either way
	13	// refusing left the merge request open for good (#108).
	14	func TestMergeRecordedWhenTargetContainsHead(t *testing.T) {
	15	inst := startInstance(t)
	16	aliceKey := inst.newKey(t, "alice")
	17	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
	18	if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/app"); code != 0 {
	19	t.Fatalf("repo create: %s", errOut)
	20	}
	21	work := t.TempDir()
	22	env := inst.gitEnv(aliceKey)
	23	mustGit(t, work, env, "clone", inst.sshURL("alice/app"), "w")
	24	dir := filepath.Join(work, "w")
	25	os.WriteFile(filepath.Join(dir, "f.txt"), []byte("x\n"), 0o644)
	26	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	27	mustGit(t, dir, env, "add", ".")
	28	mustGit(t, dir, env, "commit", "-q", "-m", "base")
	29	mustGit(t, dir, env, "push", "-q", "origin", "main")
	30	mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
	31	os.WriteFile(filepath.Join(dir, "f.txt"), []byte("y\n"), 0o644)
	32	mustGit(t, dir, env, "add", ".")
	33	mustGit(t, dir, env, "commit", "-q", "-m", "change")
	34	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	35	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/app", "--source", "feat", "--target", "main", "--title", "change"); code != 0 {
	36	t.Fatalf("mr create: %s", errOut)
	37	}
	38	// Merged by hand: main fast-forwards to the branch outside the forge.
	39	mustGit(t, dir, env, "push", "-q", "origin", "feat:main")
	40
	41	out, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/app", "1", "--json")
	42	if code != 0 \|\| !strings.Contains(out, `"strategy":"recorded"`) {
	43	t.Fatalf("merge of an already-landed head: exit %d\n%s%s", code, out, errOut)
	44	}
	45	show, _, _ := inst.ssh(t, aliceKey, "", "mr", "show", "alice/app", "1", "--json")
	46	if !strings.Contains(show, `"state":"merged"`) \|\| !strings.Contains(show, `"merged_by":"alice"`) {
	47	t.Fatalf("merge request not recorded as merged:\n%s", show)
	48	}
	49	}

internal/control/import.go +10

		@@ -113,7 +113,17 @@ func runRepoImport(c *Ctx, args []string) int {
113	113	if private {
114	114	visibility = "private"
115	115	}
	116	// The early check above fails fast; this one holds the lock across
	117	// the insert so a concurrent create cannot slip past the count.
	118	repoCreateMu.Lock()
	119	if ownerKind == "user" {
	120	if code := checkRepoQuota(c); code >= 0 {
	121	repoCreateMu.Unlock()
	122	return code
	123	}
	124	}
116	125	id, err := c.Store.CreateRepo(ownerKind, ownerID, name, visibility)
	126	repoCreateMu.Unlock()
117	127	if err != nil {
118	128	return c.fail(protocol.ExitFailure, "%v", err)
119	129	}

internal/control/mr.go +15 −5

		@@ -86,16 +86,16 @@ func runRepoFork(c *Ctx, args []string) int {
86	86	if err := policy.ValidateName(name); err != nil {
87	87	return c.failErr(err)
88	88	}
	89	repoCreateMu.Lock()
89	90	if code := checkRepoQuota(c); code >= 0 {
	91	repoCreateMu.Unlock()
90	92	return code
91	93	}
92		id, err := c.Store.CreateRepo("user", c.User.ID, name, src.Visibility)
	94	id, err := c.Store.CreateFork("user", c.User.ID, name, src.Visibility, src.ID)
	95	repoCreateMu.Unlock()
93	96	if err != nil {
94	97	return c.fail(protocol.ExitFailure, "%v", err)
95	98	}
96		if err := c.Store.SetForkOf(id, src.ID); err != nil {
97		return c.fail(protocol.ExitFailure, "%v", err)
98		}
99	99	dstDir := RepoDir(c.Cfg.Server.Root, c.User.Username, name)
100	100	srcDir := RepoDir(c.Cfg.Server.Root, src.OwnerName, src.Name)
101	101	if err := gitutil.InitBare(dstDir, "main", HooksDir(c.Cfg.Server.Root)); err != nil {
		@@ -773,7 +773,17 @@ func runMRMerge(c *Ctx, args []string) int {
773	773	return c.fail(protocol.ExitFailure, "%v", err)
774	774	}
775	775	if upToDate {
776		return c.fail(protocol.ExitUsage, "target already contains the MR head")
	776	// The head is already on the target: merged by hand and pushed, or
	777	// a merge whose ref update landed and whose record did not. Record
	778	// it rather than refuse, so a merge request cannot be stuck open
	779	// with no way to close it as merged (#108).
	780	if err := c.Store.MarkMerged(mr.ID, targetSHA, c.User.ID, ""); err != nil {
	781	return c.fail(protocol.ExitFailure, "%v", err)
	782	}
	783	c.Store.RecordEvent(repo.ID, c.User.ID, "mr.merged", fmt.Sprintf(`{"number":%d}`, mr.Number))
	784	return c.emit(map[string]any{"number": mr.Number, "strategy": "recorded", "sha": headSHA}, func(w io.Writer) {
	785	fmt.Fprintf(w, "%s already contains !%d; recorded as merged at %.10s\n", mr.TargetRef, mr.Number, headSHA)
	786	})
777	787	}
778	788	ffPossible, err := gitutil.IsAncestor(dir, targetSHA, headSHA)
779	789	if err != nil {

internal/control/quota.go +6

		@@ -4,6 +4,7 @@ import (
4	4	"fmt"
5	5	"io"
6	6	"strconv"
	7	"sync"
7	8
8	9	"gitbay.org/gitbay/internal/config"
9	10	"gitbay.org/gitbay/internal/gitutil"
		@@ -60,6 +61,11 @@ func limitsOf(c *Ctx) configLimits {
60	61	}
61	62
62	63	// checkRepoQuota refuses a new user-owned repository past the cap.
	64	// repoCreateMu serialises the quota check with the insert that follows
	65	// it, so two concurrent creates cannot both pass the count (#108). One
	66	// process serves the instance, so a process-wide lock is the whole story.
	67	var repoCreateMu sync.Mutex
	68
63	69	func checkRepoQuota(c *Ctx) int {
64	70	limit := RepoLimit(c.Store, limitsOf(c), c.User.ID)
65	71	if limit == 0 {

internal/control/repo.go +9 −2

		@@ -176,12 +176,15 @@ func runRepoCreate(c *Ctx, args []string) int {
176	176	}
177	177	ownerKind, ownerID = "org", org.ID
178	178	}
	179	repoCreateMu.Lock()
179	180	if ownerKind == "user" {
180	181	if code := checkRepoQuota(c); code >= 0 {
	182	repoCreateMu.Unlock()
181	183	return code
182	184	}
183	185	}
184	186	id, err := c.Store.CreateRepo(ownerKind, ownerID, name, visibility)
	187	repoCreateMu.Unlock()
185	188	if err != nil {
186	189	return c.fail(protocol.ExitFailure, "%v", err)
187	190	}
		@@ -378,8 +381,12 @@ func runRepoTransfer(c *Ctx, args []string) int {
378	381	return c.fail(protocol.ExitFailure, "%v", err)
379	382	}
380	383	if err := os.Rename(oldDir, newDir); err != nil {
381		// Keep name and disk consistent: revert the database change.
382		c.Store.TransferRepo(repo.ID, repo.OwnerKind, repo.OwnerID)
	384	// Keep name and disk consistent: revert the database change, and
	385	// say so if even that fails, since the operator then has a row
	386	// pointing at a directory that is not there.
	387	if rerr := c.Store.TransferRepo(repo.ID, repo.OwnerKind, repo.OwnerID); rerr != nil {
	388	return c.fail(protocol.ExitFailure, "moving repository: %v; and reverting the record failed: %v (the record now names %s but the directory is still %s)", err, rerr, newOwner+"/"+repo.Name, repo.Path())
	389	}
383	390	return c.fail(protocol.ExitFailure, "moving repository: %v", err)
384	391	}
385	392	// The wiki companion follows its repo.

internal/store/fork_test.go added +29

		@@ -0,0 +1,29 @@
	1	package store
	2
	3	import "testing"
	4
	5	// A fork is created with its parent in the same insert; it is never a
	6	// plain repository for a moment between two statements (#108).
	7	func TestCreateForkSetsParent(t *testing.T) {
	8	s := open(t)
	9	if err := s.MigrateUp(); err != nil {
	10	t.Fatal(err)
	11	}
	12	uid, _ := s.CreateUser("alice", false)
	13	bob, _ := s.CreateUser("bob", false)
	14	parent, err := s.CreateRepo("user", uid, "app", "public")
	15	if err != nil {
	16	t.Fatal(err)
	17	}
	18	id, err := s.CreateFork("user", bob, "app", "public", parent)
	19	if err != nil {
	20	t.Fatal(err)
	21	}
	22	r, err := s.RepoByID(id)
	23	if err != nil \|\| r.ForkOf != parent {
	24	t.Fatalf("fork_of = %d, want %d (err %v)", r.ForkOf, parent, err)
	25	}
	26	if _, err := s.CreateFork("user", bob, "app", "public", parent); err == nil {
	27	t.Fatal("duplicate fork name accepted")
	28	}
	29	}

internal/store/repos.go +15

		@@ -100,6 +100,21 @@ func (s *Store) SetRepoSettings(repoID int64, settings RepoSettings) error {
100	100	return err
101	101	}
102	102
	103	// CreateFork is CreateRepo with fork_of set in the same insert, so a fork
	104	// never exists for a moment as a plain repository (#108).
	105	func (s *Store) CreateFork(ownerKind string, ownerID int64, name, visibility string, forkOf int64) (int64, error) {
	106	res, err := s.DB.Exec(
	107	"INSERT INTO repos (owner_kind, owner_id, name, visibility, fork_of) VALUES (?, ?, ?, ?, ?)",
	108	ownerKind, ownerID, name, visibility, forkOf)
	109	if err != nil {
	110	if isUniqueErr(err) {
	111	return 0, fmt.Errorf("repository %q already exists", name)
	112	}
	113	return 0, err
	114	}
	115	return res.LastInsertId()
	116	}
	117
103	118	func (s *Store) SetForkOf(repoID, parentID int64) error {
104	119	_, err := s.DB.Exec("UPDATE repos SET fork_of = ? WHERE id = ?", parentID, repoID)
105	120	return err

Commit 84e1f08f23

Commit `84e1f08f23`