-
-
Notifications
You must be signed in to change notification settings - Fork 317
Issue #412 #700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Issue #412 #700
Changes from all commits
ec5e4c9
1d8880f
8cd6df2
831dd93
38a24fb
bcd2a9a
7124dce
07ee23a
e10f510
09a6941
73e93f4
4e13e31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,9 @@ import ( | |||||||||||||||||||||
| // needs to be sync.Map as it potentially could be called by many GoRoutines | ||||||||||||||||||||||
| var extensionCache sync.Map | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Added as a way to track files per run. | ||||||||||||||||||||||
| var visitedPaths sync.Map | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+16
to
+18
|
||||||||||||||||||||||
| // Added as a way to track files per run. | |
| var visitedPaths sync.Map | |
| // Added as a way to track files per run. | |
| // This must be reset at the start of each processing run so state does not | |
| // leak across repeated library calls or tests executed in the same process. | |
| var visitedPaths sync.Map | |
| func resetVisitedPaths() { | |
| visitedPaths = sync.Map{} | |
| } |
Copilot
AI
Apr 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplicate-detection key uses path/symPath directly. However, newFileJob is called in this repo with path sometimes being a directory (and name the filename), which will cause every file in that directory to be treated as a duplicate and skipped. Use a stable key that always represents the actual file path being processed (e.g., if path != name and filepath.Base(path) != name then filepath.Join(path, name), otherwise path; still prefer the resolved symPath when present).
Copilot
AI
Apr 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Load then Store sequence on visitedPaths is not atomic. If newFileJob is ever called concurrently (the code already uses goroutines heavily), two goroutines can both observe “not visited” and both process the same file. Use visitedPaths.LoadOrStore(realPath, true) to make the check+set atomic.
| if _, exists := visitedPaths.Load(realPath); exists { | |
| printWarnF("skipping already processed file: %s", realPath) | |
| return nil | |
| } | |
| visitedPaths.Store(realPath, true) | |
| if _, loaded := visitedPaths.LoadOrStore(realPath, true); loaded { | |
| printWarnF("skipping already processed file: %s", realPath) | |
| return nil | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,9 @@ package processor | |||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||
| "math/rand/v2" | ||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||
| "path/filepath" | ||||||||||||||||||||||||||||||||||
| "runtime" | ||||||||||||||||||||||||||||||||||
| "sync" | ||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -183,6 +186,38 @@ func TestNewFileJobSize(t *testing.T) { | |||||||||||||||||||||||||||||||||
| LargeByteCount = 1000000 | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| func TestNewFileJobBrokenSymlink(t *testing.T) { | ||||||||||||||||||||||||||||||||||
| if runtime.GOOS == "windows" { | ||||||||||||||||||||||||||||||||||
| t.Skip("skipping symlink test on Windows due to privilege requirements") | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| ProcessConstants() | ||||||||||||||||||||||||||||||||||
| IncludeSymLinks = true | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
Comment on lines
+194
to
+196
|
||||||||||||||||||||||||||||||||||
| // Create a temp directory to work in | ||||||||||||||||||||||||||||||||||
| dir, err := os.MkdirTemp("", "scc-broken-symlink-test") | ||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||
| t.Fatal("Failed to create temp dir:", err) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| defer os.RemoveAll(dir) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Create a symlink that points to a path that doesn't exist | ||||||||||||||||||||||||||||||||||
| symPath := dir + "/broken.go" | ||||||||||||||||||||||||||||||||||
| err = os.Symlink("/this/path/does/not/exist.go", symPath) | ||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||
| t.Fatal("Failed to create broken symlink:", err) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| fi, _ := os.Lstat(symPath) | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| fi, _ := os.Lstat(symPath) | |
| fi, err := os.Lstat(symPath) | |
| if err != nil { | |
| t.Fatal("Failed to lstat broken symlink:", err) | |
| } |
Copilot
AI
Apr 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors from os.Lstat are ignored when creating fi1/fi2. If either fails, newFileJob can panic due to a nil fileInfo. Handle both errors explicitly (and fail the test) so failures are actionable.
| fi1, _ := os.Lstat(testFile) | |
| job1 := newFileJob(testFile, "file.go", fi1) | |
| // Process the symlink (same target) | |
| fi2, _ := os.Lstat(linkFile) | |
| fi1, err := os.Lstat(testFile) | |
| if err != nil { | |
| t.Fatalf("os.Lstat(%q) failed: %v", testFile, err) | |
| } | |
| job1 := newFileJob(testFile, "file.go", fi1) | |
| // Process the symlink (same target) | |
| fi2, err := os.Lstat(linkFile) | |
| if err != nil { | |
| t.Fatalf("os.Lstat(%q) failed: %v", linkFile, err) | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1833,3 +1833,51 @@ func TestToJSON2Keys(t *testing.T) { | |||||||||||||||||||||
| t.Error("JSON2 estimatedPeople check failed") | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func TestFileSummarizeLongComplexityLines(t *testing.T) { | ||||||||||||||||||||||
| inputChan := make(chan *FileJob, 1000) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Creating 3 dummy files to test the Complexity/Lines | ||||||||||||||||||||||
| inputChan <- &FileJob{ | ||||||||||||||||||||||
| Language: "C Header", | ||||||||||||||||||||||
| Filename: "File1", | ||||||||||||||||||||||
| Location: "File1", | ||||||||||||||||||||||
| Lines: 1000, | ||||||||||||||||||||||
| Code: 2000, | ||||||||||||||||||||||
| Comment: 43, | ||||||||||||||||||||||
| Blank: 42, | ||||||||||||||||||||||
| Complexity: 100, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| inputChan <- &FileJob{ | ||||||||||||||||||||||
| Language: "C Header", | ||||||||||||||||||||||
| Filename: "File2", | ||||||||||||||||||||||
| Location: "File2", | ||||||||||||||||||||||
| Lines: 200, | ||||||||||||||||||||||
| Code: 300, | ||||||||||||||||||||||
| Comment: 23, | ||||||||||||||||||||||
| Blank: 21, | ||||||||||||||||||||||
| Complexity: 20, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| inputChan <- &FileJob{ | ||||||||||||||||||||||
| Language: "C Header", | ||||||||||||||||||||||
| Filename: "File3", | ||||||||||||||||||||||
| Location: "File3", | ||||||||||||||||||||||
| Lines: 10, | ||||||||||||||||||||||
| Code: 90, | ||||||||||||||||||||||
| Comment: 5, | ||||||||||||||||||||||
| Blank: 4, | ||||||||||||||||||||||
| Complexity: 8, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| close(inputChan) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Files = true | ||||||||||||||||||||||
| res := fileSummarizeLong(inputChan) | ||||||||||||||||||||||
| Files = false | ||||||||||||||||||||||
|
Comment on lines
+1873
to
+1875
|
||||||||||||||||||||||
| Files = true | |
| res := fileSummarizeLong(inputChan) | |
| Files = false | |
| originalFiles := Files | |
| t.Cleanup(func() { | |
| Files = originalFiles | |
| }) | |
| Files = true | |
| res := fileSummarizeLong(inputChan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description focuses on fixing
Complexity/Linesaggregation, but this change set also introduces new global symlink de-duplication behavior viavisitedPaths. Please update the PR description (or split into a separate PR) so reviewers/users understand the additional behavior change and its rationale.