Skip to content

Commit 72525b6

Browse files
pfrederiksenclaude
andcommitted
fix: Prevent early exit in directory comparison with --exit-code flag
**Issue**: When using --exit-code flag with directory comparison, the program would exit immediately upon finding the first file with differences, preventing comparison of remaining files and display of the summary. **Root Cause**: Both compareFiles() and compareDirectories() were calling os.Exit(1) directly when --exit-code was set and changes were found. When compareFiles() was called from compareDirectories(), this caused premature termination. **Previous Behavior**: 1. Start comparing directory files 2. First file with differences triggers os.Exit(1) in compareFiles() 3. Program terminates immediately 4. Remaining files never compared 5. Summary never displayed **Fixed Behavior**: 1. All files are compared 2. Summary is displayed 3. Then exit with code 1 if any changes were found **Changes**: - Refactored compareFiles() to return bool indicating if changes found - Refactored compareDirectories() to return bool indicating if changes found - Moved exit code handling to compare() function (main entry point) - Both functions now return results instead of calling os.Exit directly - The compare() function aggregates results and handles os.Exit **Tests Added**: - TestCompareFilesReturnValue: Verifies compareFiles returns correct boolean - TestDirectoryComparisonDoesNotExitEarly: Ensures all files are compared before exit, even with --exit-code flag set **Verification**: - All existing tests pass - New tests verify the fix - Manual testing confirms all files compared before exit - Coverage remains above threshold (84.1%) Fixes issue #2 from PR review comments. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 9d34e3f commit 72525b6

File tree

2 files changed

+154
-28
lines changed

2 files changed

+154
-28
lines changed

cmd/configdiff/compare.go

Lines changed: 44 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,17 @@ func compare(oldFile, newFile string) error {
2121
if !recursive {
2222
return fmt.Errorf("comparing directories requires --recursive flag")
2323
}
24-
return compareDirectories(oldFile, newFile)
24+
hasChanges, err := compareDirectories(oldFile, newFile)
25+
if err != nil {
26+
return err
27+
}
28+
29+
// Handle exit code mode for directory comparison
30+
if exitCode && hasChanges {
31+
os.Exit(1)
32+
}
33+
34+
return nil
2535
}
2636

2737
// One is a directory and one isn't
@@ -33,11 +43,22 @@ func compare(oldFile, newFile string) error {
3343
}
3444

3545
// Both are files (or stdin), proceed with normal comparison
36-
return compareFiles(oldFile, newFile)
46+
hasChanges, err := compareFiles(oldFile, newFile)
47+
if err != nil {
48+
return err
49+
}
50+
51+
// Handle exit code mode for single file comparison
52+
if exitCode && hasChanges {
53+
os.Exit(1)
54+
}
55+
56+
return nil
3757
}
3858

39-
// compareFiles performs the diff operation between two files
40-
func compareFiles(oldFile, newFile string) error {
59+
// compareFiles performs the diff operation between two files.
60+
// Returns true if changes were found, false otherwise.
61+
func compareFiles(oldFile, newFile string) (bool, error) {
4162
// Build CLI options from flags
4263
cliOpts := cli.CLIOptions{
4364
OldFile: oldFile,
@@ -64,25 +85,25 @@ func compareFiles(oldFile, newFile string) error {
6485

6586
// Validate options
6687
if err := cliOpts.Validate(); err != nil {
67-
return err
88+
return false, err
6889
}
6990

7091
// Read old file
7192
oldInput, err := cli.ReadInput(oldFile, cliOpts.GetOldFormat())
7293
if err != nil {
73-
return err
94+
return false, err
7495
}
7596

7697
// Read new file
7798
newInput, err := cli.ReadInput(newFile, cliOpts.GetNewFormat())
7899
if err != nil {
79-
return err
100+
return false, err
80101
}
81102

82103
// Convert CLI options to library options
83104
diffOpts, err := cliOpts.ToLibraryOptions()
84105
if err != nil {
85-
return err
106+
return false, err
86107
}
87108

88109
// Perform the diff
@@ -92,7 +113,7 @@ func compareFiles(oldFile, newFile string) error {
92113
diffOpts,
93114
)
94115
if err != nil {
95-
return fmt.Errorf("diff failed: %w", err)
116+
return false, fmt.Errorf("diff failed: %w", err)
96117
}
97118

98119
// Format and output results (unless quiet mode)
@@ -105,31 +126,28 @@ func compareFiles(oldFile, newFile string) error {
105126
NewFile: newFile,
106127
})
107128
if err != nil {
108-
return err
129+
return false, err
109130
}
110131

111132
fmt.Println(output)
112133
}
113134

114-
// Handle exit code mode
115-
if exitCode && cli.HasChanges(result) {
116-
os.Exit(1)
117-
}
118-
119-
return nil
135+
// Return whether changes were found
136+
return cli.HasChanges(result), nil
120137
}
121138

122-
// compareDirectories recursively compares two directories
123-
func compareDirectories(oldDir, newDir string) error {
139+
// compareDirectories recursively compares two directories.
140+
// Returns true if any changes were found, false otherwise.
141+
func compareDirectories(oldDir, newDir string) (bool, error) {
124142
// Collect all config files from both directories
125143
oldFiles, err := collectConfigFiles(oldDir)
126144
if err != nil {
127-
return fmt.Errorf("failed to scan old directory: %w", err)
145+
return false, fmt.Errorf("failed to scan old directory: %w", err)
128146
}
129147

130148
newFiles, err := collectConfigFiles(newDir)
131149
if err != nil {
132-
return fmt.Errorf("failed to scan new directory: %w", err)
150+
return false, fmt.Errorf("failed to scan new directory: %w", err)
133151
}
134152

135153
// Build set of all relative paths
@@ -163,14 +181,17 @@ func compareDirectories(oldDir, newDir string) error {
163181
fmt.Printf("\n=== %s ===\n", relPath)
164182
}
165183

166-
err := compareFiles(oldPath, newPath)
184+
fileHasChanges, err := compareFiles(oldPath, newPath)
167185
if err != nil {
168186
if !quiet {
169187
fmt.Printf("Error: %v\n", err)
170188
}
171189
continue
172190
}
173191
filesCompared++
192+
if fileHasChanges {
193+
hasAnyChanges = true
194+
}
174195
} else if newExists && !oldExists {
175196
// File added
176197
filesAdded++
@@ -195,12 +216,8 @@ func compareDirectories(oldDir, newDir string) error {
195216
filesCompared, filesAdded, filesRemoved)
196217
}
197218

198-
// Handle exit code mode
199-
if exitCode && hasAnyChanges {
200-
os.Exit(1)
201-
}
202-
203-
return nil
219+
// Return whether any changes were found
220+
return hasAnyChanges, nil
204221
}
205222

206223
// collectConfigFiles recursively finds all config files in a directory

cmd/configdiff/main_test.go

Lines changed: 110 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package main
22

33
import (
4+
"fmt"
45
"os"
56
"path/filepath"
67
"testing"
@@ -226,7 +227,7 @@ func TestCompareDirectories(t *testing.T) {
226227
quiet = true // Suppress output during test
227228
exitCode = false
228229

229-
err := compareDirectories(oldDir, newDir)
230+
_, err := compareDirectories(oldDir, newDir)
230231
if err != nil {
231232
t.Errorf("compareDirectories() error = %v", err)
232233
}
@@ -306,3 +307,111 @@ func findSubstring(s, substr string) bool {
306307
}
307308
return false
308309
}
310+
311+
func TestCompareFilesReturnValue(t *testing.T) {
312+
tmpDir := t.TempDir()
313+
314+
// Create files with no changes
315+
sameFile1 := filepath.Join(tmpDir, "same1.yaml")
316+
sameFile2 := filepath.Join(tmpDir, "same2.yaml")
317+
if err := os.WriteFile(sameFile1, []byte("value: 1"), 0644); err != nil {
318+
t.Fatalf("Failed to write file: %v", err)
319+
}
320+
if err := os.WriteFile(sameFile2, []byte("value: 1"), 0644); err != nil {
321+
t.Fatalf("Failed to write file: %v", err)
322+
}
323+
324+
// Create files with changes
325+
diffFile1 := filepath.Join(tmpDir, "diff1.yaml")
326+
diffFile2 := filepath.Join(tmpDir, "diff2.yaml")
327+
if err := os.WriteFile(diffFile1, []byte("value: 1"), 0644); err != nil {
328+
t.Fatalf("Failed to write file: %v", err)
329+
}
330+
if err := os.WriteFile(diffFile2, []byte("value: 2"), 0644); err != nil {
331+
t.Fatalf("Failed to write file: %v", err)
332+
}
333+
334+
tests := []struct {
335+
name string
336+
oldFile string
337+
newFile string
338+
wantChanges bool
339+
wantErr bool
340+
}{
341+
{
342+
name: "no changes",
343+
oldFile: sameFile1,
344+
newFile: sameFile2,
345+
wantChanges: false,
346+
wantErr: false,
347+
},
348+
{
349+
name: "with changes",
350+
oldFile: diffFile1,
351+
newFile: diffFile2,
352+
wantChanges: true,
353+
wantErr: false,
354+
},
355+
}
356+
357+
for _, tt := range tests {
358+
t.Run(tt.name, func(t *testing.T) {
359+
quiet = true
360+
exitCode = false
361+
362+
hasChanges, err := compareFiles(tt.oldFile, tt.newFile)
363+
if (err != nil) != tt.wantErr {
364+
t.Errorf("compareFiles() error = %v, wantErr %v", err, tt.wantErr)
365+
}
366+
if hasChanges != tt.wantChanges {
367+
t.Errorf("compareFiles() hasChanges = %v, want %v", hasChanges, tt.wantChanges)
368+
}
369+
})
370+
}
371+
}
372+
373+
func TestDirectoryComparisonDoesNotExitEarly(t *testing.T) {
374+
tmpDir := t.TempDir()
375+
376+
oldDir := filepath.Join(tmpDir, "old")
377+
newDir := filepath.Join(tmpDir, "new")
378+
379+
if err := os.MkdirAll(oldDir, 0755); err != nil {
380+
t.Fatalf("Failed to create old dir: %v", err)
381+
}
382+
if err := os.MkdirAll(newDir, 0755); err != nil {
383+
t.Fatalf("Failed to create new dir: %v", err)
384+
}
385+
386+
// Create multiple files with changes
387+
// This tests that compareDirectories doesn't exit early when --exit-code is set
388+
files := []string{"file1.yaml", "file2.yaml", "file3.yaml"}
389+
for i, f := range files {
390+
oldContent := fmt.Sprintf("value: %d", i)
391+
newContent := fmt.Sprintf("value: %d", i+10)
392+
if err := os.WriteFile(filepath.Join(oldDir, f), []byte(oldContent), 0644); err != nil {
393+
t.Fatalf("Failed to write old file: %v", err)
394+
}
395+
if err := os.WriteFile(filepath.Join(newDir, f), []byte(newContent), 0644); err != nil {
396+
t.Fatalf("Failed to write new file: %v", err)
397+
}
398+
}
399+
400+
// Run with quiet mode and exit-code flag
401+
// The function should compare all files and return normally (not call os.Exit)
402+
quiet = true
403+
exitCode = true // This used to cause early exit, now it should work correctly
404+
405+
hasChanges, err := compareDirectories(oldDir, newDir)
406+
if err != nil {
407+
t.Errorf("compareDirectories() error = %v", err)
408+
}
409+
410+
// Verify that changes were detected
411+
if !hasChanges {
412+
t.Error("compareDirectories() should have detected changes but didn't")
413+
}
414+
415+
// If we get here, the function completed successfully without os.Exit
416+
// The os.Exit would happen in the caller (compare function), not in compareDirectories
417+
}

0 commit comments

Comments
 (0)