store: one admin's settings change no longer overwrites another's !231

merged merged by cmc on 2026-09-04 15:58 UTC · krz/gitbay:settings-json-set into main

5 files changed, +209 −43

Layout: unified · split

internal/control/migrate.go +3 −1

		@@ -215,8 +215,10 @@ func runAccountImportBundle(c *Ctx, args []string) int {
215	215	if len(cmds) > 0 {
216	216	deferrals = append(deferrals, deferred{path, cmds})
217	217	}
	218	// An import writes the whole blob: the repository was created a
	219	// few lines up and nobody else holds settings on it yet.
218	220	s.GitDaemon = false // instance-dependent; opt back in explicitly
219		c.Store.SetRepoSettings(repo.ID, s)
	221	c.Store.UpdateRepoSettings(repo.ID, func(cur store.RepoSettings) { cur = s })
220	222
221	223	for _, bi := range br.Issues {
222	224	key := fmt.Sprintf("mig-issue:%d", bi.Number)

internal/control/mr.go +10 −15

		@@ -133,9 +133,8 @@ func runRequireApprovals(c *Ctx, args []string) int {
133	133	if code >= 0 {
134	134	return code
135	135	}
136		s := repo.Settings
137		s.RequireApprovals = n
138		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	136	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireApprovals = n })
	137	if err != nil {
139	138	return c.fail(protocol.ExitFailure, "%v", err)
140	139	}
141	140	return c.emit(s, func(w io.Writer) {
		@@ -151,9 +150,8 @@ func runRequireResolved(c *Ctx, args []string) int {
151	150	if code >= 0 {
152	151	return code
153	152	}
154		s := repo.Settings
155		s.RequireResolved = args[1] == "on"
156		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	153	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireResolved = args[1] == "on" })
	154	if err != nil {
157	155	return c.fail(protocol.ExitFailure, "%v", err)
158	156	}
159	157	return c.emit(s, func(w io.Writer) {
		@@ -169,9 +167,8 @@ func runRequireCodeowners(c *Ctx, args []string) int {
169	167	if code >= 0 {
170	168	return code
171	169	}
172		s := repo.Settings
173		s.RequireCodeowners = args[1] == "on"
174		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	170	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireCodeowners = args[1] == "on" })
	171	if err != nil {
175	172	return c.fail(protocol.ExitFailure, "%v", err)
176	173	}
177	174	return c.emit(s, func(w io.Writer) {
		@@ -187,9 +184,8 @@ func runRequireChecks(c *Ctx, args []string) int {
187	184	if code >= 0 {
188	185	return code
189	186	}
190		s := repo.Settings
191		s.RequireChecks = args[1] == "on"
192		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	187	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireChecks = args[1] == "on" })
	188	if err != nil {
193	189	return c.fail(protocol.ExitFailure, "%v", err)
194	190	}
195	191	return c.emit(s, func(w io.Writer) {
		@@ -205,9 +201,8 @@ func runRequireSigned(c *Ctx, args []string) int {
205	201	if code >= 0 {
206	202	return code
207	203	}
208		s := repo.Settings
209		s.RequireSignedCommits = args[1] == "on"
210		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	204	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireSignedCommits = args[1] == "on" })
	205	if err != nil {
211	206	return c.fail(protocol.ExitFailure, "%v", err)
212	207	}
213	208	return c.emit(s, func(w io.Writer) {

internal/control/repo.go +19 −22

		@@ -562,9 +562,7 @@ func runSetWebsite(c *Ctx, args []string) int {
562	562	if code >= 0 {
563	563	return code
564	564	}
565		s := repo.Settings
566		s.Website = site
567		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	565	if _, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.Website = site }); err != nil {
568	566	return c.fail(protocol.ExitFailure, "%v", err)
569	567	}
570	568	return c.emit(map[string]string{"website": site}, func(w io.Writer) {
		@@ -601,9 +599,7 @@ func setRepoVisibility(c *Ctx, repo store.Repo, visibility string) int {
601	599	// Going private takes the repository off every anonymous surface, so
602	600	// git:// exposure cannot outlive the change.
603	601	if visibility == "private" && repo.Settings.GitDaemon {
604		s := repo.Settings
605		s.GitDaemon = false
606		c.Store.SetRepoSettings(repo.ID, s)
	602	c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.GitDaemon = false })
607	603	}
608	604	c.Store.Audit(c.User.ID, "repo.visibility", map[string]any{"repo": repo.ID, "visibility": visibility})
609	605	return c.emit(map[string]string{"visibility": visibility}, func(w io.Writer) {
		@@ -626,9 +622,8 @@ func runGitDaemon(c *Ctx, args []string) int {
626	622	if on && !c.Cfg.GitDaemon.Enabled {
627	623	return c.fail(protocol.ExitUsage, "this instance does not run the git:// daemon ([git_daemon] enabled = false)")
628	624	}
629		s := repo.Settings
630		s.GitDaemon = on
631		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	625	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.GitDaemon = on })
	626	if err != nil {
632	627	return c.fail(protocol.ExitFailure, "%v", err)
633	628	}
634	629	return c.emit(s, func(w io.Writer) { fmt.Fprintf(w, "git-daemon %s on %s\n", args[1], repo.Path()) })
		@@ -662,9 +657,8 @@ func archiveRepo(c *Ctx, repo store.Repo, archived bool) int {
662	657	if repo.Settings.Archived == archived {
663	658	return c.fail(protocol.ExitUsage, "%s is already %sd", repo.Path(), verb)
664	659	}
665		s := repo.Settings
666		s.Archived = archived
667		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	660	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.Archived = archived })
	661	if err != nil {
668	662	return c.fail(protocol.ExitFailure, "%v", err)
669	663	}
670	664	c.Store.RecordEvent(repo.ID, c.User.ID, "repo."+verb+"d", "{}")
		@@ -898,16 +892,19 @@ func setProtect(c *Ctx, args []string, protect bool) int {
898	892	return code
899	893	}
900	894	branch := args[1]
901		s := repo.Settings
902		has := slices.Contains(s.ProtectedBranches, branch)
903		if protect && !has {
904		s.ProtectedBranches = append(s.ProtectedBranches, branch)
905		slices.Sort(s.ProtectedBranches)
906		}
907		if !protect && has {
908		s.ProtectedBranches = slices.DeleteFunc(s.ProtectedBranches, func(b string) bool { return b == branch })
909		}
910		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	895	// The list is read and rewritten inside the update, so two admins
	896	// protecting different branches at once both land.
	897	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) {
	898	has := slices.Contains(s.ProtectedBranches, branch)
	899	if protect && !has {
	900	s.ProtectedBranches = append(s.ProtectedBranches, branch)
	901	slices.Sort(s.ProtectedBranches)
	902	}
	903	if !protect && has {
	904	s.ProtectedBranches = slices.DeleteFunc(s.ProtectedBranches, func(b string) bool { return b == branch })
	905	}
	906	})
	907	if err != nil {
911	908	return c.fail(protocol.ExitFailure, "%v", err)
912	909	}
913	910	verb := "protected"

internal/store/repos.go +57 −5

		@@ -1,6 +1,7 @@
1	1	package store
2	2
3	3	import (
	4	"context"
4	5	"database/sql"
5	6	"encoding/json"
6	7	"errors"
		@@ -92,13 +93,64 @@ func (s *Store) SetRepoVisibility(repoID int64, visibility string) error {
92	93	return err
93	94	}
94	95
95		func (s *Store) SetRepoSettings(repoID int64, settings RepoSettings) error {
96		raw, err := json.Marshal(settings)
	96	// UpdateRepoSettings applies mutate to the repository's settings and
	97	// stores the result, returning what was stored.
	98	//
	99	// settings_json is one blob, so changing one field means writing all of
	100	// them. Callers used to read the struct off a Repo they had loaded
	101	// earlier, change a field and write the whole blob back, which loses the
	102	// other admin's change whenever two ran at once — last write wins over a
	103	// value it never read. The read and the write happen here instead, inside
	104	// one transaction, and BEGIN IMMEDIATE takes the write lock up front: a
	105	// second updater waits at the start rather than discovering the conflict
	106	// after it has already read a stale blob.
	107	func (s Store) UpdateRepoSettings(repoID int64, mutate func(RepoSettings)) (RepoSettings, error) {
	108	ctx := context.Background()
	109	var out RepoSettings
	110	// The whole exchange must run on one connection for BEGIN to bracket
	111	// it; the pool would otherwise be free to hand the statements out
	112	// separately.
	113	conn, err := s.DB.Conn(ctx)
97	114	if err != nil {
98		return err
	115	return out, err
99	116	}
100		_, err = s.DB.Exec("UPDATE repos SET settings_json = ? WHERE id = ?", string(raw), repoID)
101		return err
	117	defer conn.Close()
	118	if _, err := conn.ExecContext(ctx, "BEGIN IMMEDIATE"); err != nil {
	119	return out, err
	120	}
	121	committed := false
	122	defer func() {
	123	if !committed {
	124	conn.ExecContext(ctx, "ROLLBACK")
	125	}
	126	}()
	127	var raw string
	128	if err := conn.QueryRowContext(ctx,
	129	"SELECT settings_json FROM repos WHERE id = ?", repoID).Scan(&raw); err != nil {
	130	if errors.Is(err, sql.ErrNoRows) {
	131	return out, ErrNotFound
	132	}
	133	return out, err
	134	}
	135	if raw != "" {
	136	if err := json.Unmarshal([]byte(raw), &out); err != nil {
	137	return out, err
	138	}
	139	}
	140	mutate(&out)
	141	next, err := json.Marshal(out)
	142	if err != nil {
	143	return out, err
	144	}
	145	if _, err := conn.ExecContext(ctx,
	146	"UPDATE repos SET settings_json = ? WHERE id = ?", string(next), repoID); err != nil {
	147	return out, err
	148	}
	149	if _, err := conn.ExecContext(ctx, "COMMIT"); err != nil {
	150	return out, err
	151	}
	152	committed = true
	153	return out, nil
102	154	}
103	155
104	156	// CreateFork is CreateRepo with fork_of set in the same insert, so a fork

internal/store/reposettings_test.go added +120

		@@ -0,0 +1,120 @@
	1	package store
	2
	3	import (
	4	"sync"
	5	"testing"
	6	)
	7
	8	func settingsFixture(t testing.T) (Store, int64) {
	9	t.Helper()
	10	s := open(t)
	11	if err := s.MigrateUp(); err != nil {
	12	t.Fatal(err)
	13	}
	14	uid, err := s.CreateUser("cmc", true)
	15	if err != nil {
	16	t.Fatal(err)
	17	}
	18	repoID, err := s.CreateRepo("user", uid, "lib", "public")
	19	if err != nil {
	20	t.Fatal(err)
	21	}
	22	return s, repoID
	23	}
	24
	25	func TestUpdateRepoSettingsRoundTrip(t *testing.T) {
	26	s, repoID := settingsFixture(t)
	27	got, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireApprovals = 2 })
	28	if err != nil {
	29	t.Fatal(err)
	30	}
	31	if got.RequireApprovals != 2 {
	32	t.Fatalf("returned %+v", got)
	33	}
	34	repo, err := s.RepoByID(repoID)
	35	if err != nil {
	36	t.Fatal(err)
	37	}
	38	if repo.Settings.RequireApprovals != 2 {
	39	t.Fatalf("stored %+v", repo.Settings)
	40	}
	41
	42	// Turning a flag back off must persist: the JSON tags are omitempty,
	43	// so "false" is an absent key, and a patch-style write would drop it.
	44	if _, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireChecks = true }); err != nil {
	45	t.Fatal(err)
	46	}
	47	got, err = s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireChecks = false })
	48	if err != nil {
	49	t.Fatal(err)
	50	}
	51	if got.RequireChecks {
	52	t.Fatal("require_checks stayed on")
	53	}
	54	repo, _ = s.RepoByID(repoID)
	55	if repo.Settings.RequireChecks \|\| repo.Settings.RequireApprovals != 2 {
	56	t.Fatalf("stored %+v", repo.Settings)
	57	}
	58	}
	59
	60	func TestUpdateRepoSettingsMissingRepo(t *testing.T) {
	61	s, _ := settingsFixture(t)
	62	if _, err := s.UpdateRepoSettings(9999, func(*RepoSettings) {}); err != ErrNotFound {
	63	t.Fatalf("err = %v, want ErrNotFound", err)
	64	}
	65	}
	66
	67	// Concurrent updates to different fields both survive. Read-modify-write
	68	// through the caller lost one of them: each read the same blob and the
	69	// later write put back what it had read for the other's field.
	70	func TestUpdateRepoSettingsConcurrent(t *testing.T) {
	71	s, repoID := settingsFixture(t)
	72	var wg sync.WaitGroup
	73	errs := make(chan error, 2)
	74	wg.Add(2)
	75	go func() {
	76	defer wg.Done()
	77	_, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireApprovals = 3 })
	78	errs <- err
	79	}()
	80	go func() {
	81	defer wg.Done()
	82	_, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireResolved = true })
	83	errs <- err
	84	}()
	85	wg.Wait()
	86	close(errs)
	87	for err := range errs {
	88	if err != nil {
	89	t.Fatal(err)
	90	}
	91	}
	92	repo, err := s.RepoByID(repoID)
	93	if err != nil {
	94	t.Fatal(err)
	95	}
	96	if repo.Settings.RequireApprovals != 3 \|\| !repo.Settings.RequireResolved {
	97	t.Fatalf("one update was lost: %+v", repo.Settings)
	98	}
	99	}
	100
	101	// The protected-branch list is read and rewritten inside the update, so
	102	// two admins protecting different branches at once both land.
	103	func TestProtectedBranchesConcurrent(t *testing.T) {
	104	s, repoID := settingsFixture(t)
	105	var wg sync.WaitGroup
	106	for _, branch := range []string{"main", "release"} {
	107	wg.Add(1)
	108	go func(b string) {
	109	defer wg.Done()
	110	s.UpdateRepoSettings(repoID, func(set *RepoSettings) {
	111	set.ProtectedBranches = append(set.ProtectedBranches, b)
	112	})
	113	}(branch)
	114	}
	115	wg.Wait()
	116	repo, _ := s.RepoByID(repoID)
	117	if len(repo.Settings.ProtectedBranches) != 2 {
	118	t.Fatalf("protected branches = %v, want both", repo.Settings.ProtectedBranches)
	119	}
	120	}