Skip to content

Commit f32f62b

Browse files
Matovidloclaude
andcommitted
fix(.kbcignore): address Copilot review comments on branch ignore (PSGO-198)
- Fix correctness bug: branch names containing "/" (e.g. "branch/feature/foo") were split into 3 parts and misrouted to the config-row case; now handled via HasPrefix("branch/") guard before the generic split - Add NullIgnoredBranchStates() to registry: single-pass over All() to null local+remote state for every ignored branch and all its configs/rows, avoiding repeated O(n²) ConfigsFrom/ConfigRowsFrom sorts in nested loops - Replace triple-nested loops in push and pull operations with a call to NullIgnoredBranchStates(), eliminating the duplication between the two callers - Add tests: branch-with-slash correctness test, NullIgnoredBranchStates unit test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent a57181a commit f32f62b

7 files changed

Lines changed: 105 additions & 33 deletions

File tree

internal/pkg/project/ignore/file_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,16 @@ func Test_loadFile(t *testing.T) {
2929
assert.Equal(t, "keboola.bar/678/34\nkeboola.foo/345", file.rawStringPattern)
3030
}
3131

32+
func newTestRegistryWithSlashBranch(t *testing.T) *registry.Registry {
33+
t.Helper()
34+
r := registry.New(knownpaths.NewNop(t.Context()), naming.NewRegistry(), model.NewComponentsMap(nil), model.SortByPath)
35+
require.NoError(t, r.Set(&model.BranchState{
36+
BranchManifest: &model.BranchManifest{BranchKey: model.BranchKey{ID: 789}},
37+
Local: &model.Branch{Name: "feature/foo"},
38+
}))
39+
return r
40+
}
41+
3242
func newTestRegistry(t *testing.T) *registry.Registry {
3343
t.Helper()
3444

internal/pkg/project/ignore/ignore.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,18 +36,19 @@ func (f *File) parseIgnoredPatterns() []string {
3636

3737
// applyIgnorePattern applies a single ignore pattern, marking the appropriate config or row as ignored.
3838
func (f *File) applyIgnorePattern(ignoreConfig string) error {
39-
parts := strings.Split(ignoreConfig, "/")
39+
// Branch pattern: "branch/<name>" — name may itself contain "/".
40+
if strings.HasPrefix(ignoreConfig, "branch/") {
41+
branchName := strings.TrimPrefix(ignoreConfig, "branch/")
42+
f.state.IgnoreBranch(branchName)
43+
return nil
44+
}
4045

46+
parts := strings.Split(ignoreConfig, "/")
4147
switch len(parts) {
4248
case 2:
43-
if parts[0] == "branch" {
44-
// Ignore entire branch by name.
45-
f.state.IgnoreBranch(parts[1])
46-
} else {
47-
// Ignore config by componentID/configID.
48-
configID, componentID := parts[1], parts[0]
49-
f.state.IgnoreConfig(configID, componentID)
50-
}
49+
// Ignore config by componentID/configID.
50+
configID, componentID := parts[1], parts[0]
51+
f.state.IgnoreConfig(configID, componentID)
5152
case 3:
5253
// Ignore specific config row.
5354
configID, rowID := parts[1], parts[2]

internal/pkg/project/ignore/ignore_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,27 @@ func TestFile_IgnoreConfigsOrRows_Branch(t *testing.T) {
5050
assert.Equal(t, "123", ignored[0].ID.String())
5151
}
5252

53+
func TestFile_IgnoreConfigsOrRows_BranchWithSlashInName(t *testing.T) {
54+
t.Parallel()
55+
56+
ctx := t.Context()
57+
r := newTestRegistryWithSlashBranch(t)
58+
fs := aferofs.NewMemoryFs()
59+
60+
require.NoError(t, fs.WriteFile(ctx, filesystem.NewRawFile(`foo/bar1`, "branch/feature/foo")))
61+
62+
file, err := LoadFile(ctx, fs, r, "foo/bar1")
63+
require.NoError(t, err)
64+
65+
require.NoError(t, file.IgnoreConfigsOrRows())
66+
67+
// Branch "feature/foo" should be ignored, not misidentified as a config-row pattern.
68+
ignored := r.IgnoredBranches()
69+
require.Len(t, ignored, 1)
70+
assert.Equal(t, "789", ignored[0].ID.String())
71+
assert.Empty(t, r.IgnoredConfigRows())
72+
}
73+
5374
func Test_applyIgnoredPatterns(t *testing.T) {
5475
t.Parallel()
5576
projectState := newTestRegistry(t)
@@ -109,6 +130,13 @@ func Test_applyIgnoredPatterns(t *testing.T) {
109130
},
110131
wantErr: assert.NoError,
111132
},
133+
{
134+
name: "branch pattern with slash in name",
135+
args: args{
136+
pattern: "branch/feature/foo",
137+
},
138+
wantErr: assert.NoError,
139+
},
112140
}
113141
for _, tt := range tests {
114142
t.Run(tt.name, func(t *testing.T) {

internal/pkg/state/registry/registry.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,42 @@ func (s *Registry) IgnoredBranches() (branches []*model.BranchState) {
167167
return branches
168168
}
169169

170+
// NullIgnoredBranchStates nulls local+remote state for every ignored branch and for all
171+
// configs/rows belonging to those branches, using a single pass over All() to avoid
172+
// repeated sorts.
173+
func (s *Registry) NullIgnoredBranchStates() {
174+
ignored := s.IgnoredBranches()
175+
if len(ignored) == 0 {
176+
return
177+
}
178+
179+
// Build set of ignored branch keys for O(1) lookup.
180+
ignoredKeys := make(map[model.BranchKey]struct{}, len(ignored))
181+
for _, b := range ignored {
182+
ignoredKeys[b.BranchKey] = struct{}{}
183+
}
184+
185+
for _, obj := range s.All() {
186+
switch v := obj.(type) {
187+
case *model.BranchState:
188+
if _, ok := ignoredKeys[v.BranchKey]; ok {
189+
v.SetLocalState(nil)
190+
v.SetRemoteState(nil)
191+
}
192+
case *model.ConfigState:
193+
if _, ok := ignoredKeys[model.BranchKey{ID: v.BranchID}]; ok {
194+
v.SetLocalState(nil)
195+
v.SetRemoteState(nil)
196+
}
197+
case *model.ConfigRowState:
198+
if _, ok := ignoredKeys[model.BranchKey{ID: v.BranchID}]; ok {
199+
v.SetLocalState(nil)
200+
v.SetRemoteState(nil)
201+
}
202+
}
203+
}
204+
}
205+
170206
func (s *Registry) IgnoreConfig(ignoreID string, componentID string) {
171207
for _, object := range s.All() {
172208
if v, ok := object.(*model.ConfigState); ok {

internal/pkg/state/registry/registry_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,25 @@ func TestRegistry_GetByPath(t *testing.T) {
228228
assert.True(t, found)
229229
}
230230

231+
func TestNullIgnoredBranchStates(t *testing.T) {
232+
t.Parallel()
233+
s := newTestState(t, knownpaths.NewNop(t.Context()))
234+
235+
// Initially all 6 objects (2 branches, 2 configs, 2 rows) are present.
236+
assert.Len(t, s.All(), 6)
237+
238+
// Ignore the "Main" branch (ID 123).
239+
s.IgnoreBranch("Main")
240+
s.NullIgnoredBranchStates()
241+
242+
// Branch 123, its 2 configs, and 2 config rows are now invisible (both states nil).
243+
// Only branch 567 remains.
244+
assert.Len(t, s.All(), 1)
245+
assert.Equal(t, BranchKey{ID: 567}, s.Branches()[0].BranchKey)
246+
assert.Empty(t, s.Configs())
247+
assert.Empty(t, s.ConfigRows())
248+
}
249+
231250
func TestIgnoreBranch(t *testing.T) {
232251
t.Parallel()
233252
s := newTestState(t, knownpaths.NewNop(t.Context()))

pkg/lib/operation/project/sync/pull/operation.go

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -166,16 +166,5 @@ func ignoreBranchesConfigsAndRows(projectState *project.State) {
166166
}
167167

168168
// Null both states for ignored branches so they are invisible to the diff.
169-
for _, branch := range projectState.IgnoredBranches() {
170-
branch.SetLocalState(nil)
171-
branch.SetRemoteState(nil)
172-
for _, config := range projectState.ConfigsFrom(branch.BranchKey) {
173-
config.SetLocalState(nil)
174-
config.SetRemoteState(nil)
175-
for _, row := range projectState.ConfigRowsFrom(config.ConfigKey) {
176-
row.SetLocalState(nil)
177-
row.SetRemoteState(nil)
178-
}
179-
}
180-
}
169+
projectState.NullIgnoredBranchStates()
181170
}

pkg/lib/operation/project/sync/push/operation.go

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -76,18 +76,7 @@ func Run(ctx context.Context, projectState *project.State, o Options, d dependen
7676
}
7777

7878
// Make ignored branches invisible to the push diff.
79-
for _, branch := range projectState.IgnoredBranches() {
80-
branch.SetLocalState(nil)
81-
branch.SetRemoteState(nil)
82-
for _, config := range projectState.ConfigsFrom(branch.BranchKey) {
83-
config.SetLocalState(nil)
84-
config.SetRemoteState(nil)
85-
for _, row := range projectState.ConfigRowsFrom(config.ConfigKey) {
86-
row.SetLocalState(nil)
87-
row.SetRemoteState(nil)
88-
}
89-
}
90-
}
79+
projectState.NullIgnoredBranchStates()
9180
}
9281

9382
// Diff

0 commit comments

Comments
 (0)