Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions internal/setup/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package setup
import (
"errors"
"fmt"
"maps"
"os"
"path/filepath"
"slices"
Expand Down Expand Up @@ -273,14 +274,47 @@ func (r *Release) validate() error {
}

// Check for glob and generate conflicts.
//
// This is the most time-consuming part of every command because checking
// each pair of paths uses an n^2 algorithm; specifically due to glob
// handling.
Comment thread
letFunny marked this conversation as resolved.
Outdated
//
// We can speed up the search by looking at the prefixes of each path that
// don't contain globs. Specifically two paths a and b conflict if and only
// if they both start with the prefix of a or with the prefix of b. We can
// discard directly all other parts that do not share either of the
// prefixes. This can be done efficiently with a radix tree or a trie or a
// simple list of sorted paths and using binary search (see below).
Comment thread
letFunny marked this conversation as resolved.
Outdated
//
// Note: to check both prefixes we have to check `(a,b)` and `(b,a)` as the
// code below is not symmetric.
allPaths := slices.Collect(maps.Keys(paths))
slices.Sort(allPaths)
Comment thread
letFunny marked this conversation as resolved.
Outdated
for oldPath, oldSlices := range paths {
for _, old := range oldSlices {
oldInfo := old.Contents[oldPath]
if oldInfo.Kind != GeneratePath && oldInfo.Kind != GlobPath {
break
}
for newPath, newSlices := range paths {
if oldPath == newPath {

prefixLen := strings.IndexAny(oldPath, "*?")
if prefixLen == -1 {
return fmt.Errorf("internal error: invalid path: generate or glob path does not contain ' ?' or '*': %q", oldPath)
Comment thread
letFunny marked this conversation as resolved.
Outdated
}
searchKey := oldPath[:prefixLen]
// startIndex is the position of the prefix or the position where
// the prefix would have been found.
startIndex, _ := slices.BinarySearch(allPaths, searchKey)
for i := startIndex; i < len(allPaths); i++ {
newPath := allPaths[i]
if !strings.HasPrefix(newPath, searchKey) {
// Iterate until the prefix no longer matches, which means
// all other paths will fail the comparison below.
break
}
newSlices := paths[newPath]
// We know prefixes match, we can skip that part of the string.
if oldPath[len(searchKey)-1:] == newPath[len(searchKey)-1:] {
// Identical paths have been filtered earlier.
continue
}
Expand All @@ -291,13 +325,16 @@ func (r *Release) validate() error {
continue
}
}
if strdist.GlobPath(newPath, oldPath) {
// We know prefixes match, we can skip that part of the string.
if strdist.GlobPath(newPath[len(searchKey)-1:], oldPath[len(searchKey)-1:]) {
if (old.Package > new.Package) || (old.Package == new.Package && old.Name > new.Name) ||
(old.Package == new.Package && old.Name == new.Name && oldPath > newPath) {
old, new = new, old
oldPath, newPath = newPath, oldPath
}
return fmt.Errorf("slices %s and %s conflict on %s and %s", old, new, oldPath, newPath)
} else {
break
Comment on lines +335 to +341

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot confidently tell if this is changing anything. Is that to speed things up by skipping remaining slices in newSlices?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, once GlobDistance fails we can skip the rest of the loop. This obviously does not have a very big impact, compared to the other change, but it was a low hanging fruit for a 1 line change so I decided to add it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks! Maybe the comment could mention why this is fine to stop the rest of the loop?

@letFunny letFunny Apr 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did and I added a TODO comment saying that the loops can be reordered. I didn't do it in these patches because:

  1. It did not have a measurable impact.
  2. It made the diff much bigger and harder to review.

Let me know what you think.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is much clearer, thanks!

}
}
}
Expand Down
20 changes: 20 additions & 0 deletions internal/setup/setup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,26 @@ var setupTests = []setupTest{{
`,
},
relerror: "slices mypkg1_myslice1 and mypkg2_myslice1 conflict on /path1",
}, {
summary: "When multiple prefixes match all are tested",
input: map[string]string{
"slices/mydir/mypkg1.yaml": `
package: mypkg1
slices:
myslice1:
contents:
/path/*/foo:
`,
"slices/mydir/mypkg2.yaml": `
package: mypkg2
slices:
myslice1:
contents:
/path/bar/foo1:
/path/bar/f*:
`,
},
relerror: "slices mypkg1_myslice1 and mypkg2_myslice1 conflict on /path/\\*/foo and /path/bar/f\\*",
}, {
summary: "Directories must be suffixed with /",
input: map[string]string{
Expand Down
Loading