Remove bash dependency for Windows #112
Conversation
In windows bash was required because a pipe was used in GetGrowthStats. removes the pipe usage, so we no longer require bash for windows. Now uses native go piping.
There was a problem hiding this comment.
Pull request overview
This PR removes the bash dependency for Windows by refactoring the Git command execution in GetGrowthStats to use native Go process management and pipes instead of shell-based command invocation.
Changes:
- Replaced shell-based git command execution with direct process piping using Go's
os/execandiopackages - Removed the
ShellToUsehelper function that selected bash on Windows and sh on Unix - Removed the Windows-specific bash requirement check from the requirements validation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/requirements/requirements.go | Removed bash availability check for Windows and the now-unused runtime import |
| pkg/git/git.go | Refactored GetGrowthStats to use direct command piping instead of shell invocation, removed ShellToUse function, and added io import |
Comments suppressed due to low confidence (2)
pkg/git/git.go:175
- The cleanup logic when io.ReadAll fails has a potential issue. Using Process.Kill() directly doesn't wait for the processes to terminate, which can leave zombie processes. Additionally, if revListCommand has already finished successfully by the time this error occurs, calling Kill on it could fail. Consider using a more robust cleanup approach that checks process state and uses Wait() after Kill() to properly reap the processes.
revListCommand.Process.Kill()
catFileCommand.Process.Kill()
return currentStatistics, fmt.Errorf("failed to read cat-file output: %w", err)
pkg/git/git.go:167
- When catFileCommand fails to start at line 165, the revListCommand is killed but Wait() is never called on it. This will leave a zombie process. After killing a process, you should call Wait() to properly reap it and release system resources.
revListCommand.Process.Kill()
return currentStatistics, fmt.Errorf("failed to start cat-file: %w", err)
…re gracefully and potential race condition if buffers fill
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
pkg/git/git.go:198
- When
io.ReadAll()fails, the code kills both processes but doesn't close the stdout pipes before waiting. The pipes should be closed after reading fails to ensure proper cleanup and prevent potential deadlocks where processes might be blocked on writing to the pipes.
if result.err != nil {
revListCommand.Process.Kill()
catFileCommand.Process.Kill()
revListCommand.Wait()
catFileCommand.Wait()
return currentStatistics, fmt.Errorf("failed to read cat-file output: %w", result.err)
}
pkg/git/git.go:210
- When
revListCommand.Wait()fails, the code immediately kills and waits on catFileCommand without closing the stdout pipe from catFileCommand. The pipe should be closed before or after killing the process to ensure proper cleanup.
Additionally, calling Wait() immediately after Kill() without checking the Kill() error is a race condition pattern that could lead to unexpected behavior.
if err := revListCommand.Wait(); err != nil {
catFileCommand.Process.Kill()
catFileCommand.Wait()
stderrMsg := strings.TrimSpace(revListStderr.String())
if stderrMsg != "" {
return currentStatistics, fmt.Errorf("rev-list command failed: %w, stderr: %s", err, stderrMsg)
}
return currentStatistics, fmt.Errorf("rev-list command failed: %w", err)
}
pkg/git/git.go:198
- The goroutine reading from
catFileStdoutmay be blocked if the catFile process is killed while it's reading. When processes are killed in error paths (lines 193-194, 203), the goroutine might be stuck onio.ReadAll()waiting for EOF, but the pipe may not be properly closed. This could lead to goroutine leaks.
Consider closing catFileStdout after killing the processes to ensure the goroutine can complete, or use a context with timeout to bound the read operation.
go func() {
output, err := io.ReadAll(catFileStdout)
resultChannel <- readResult{output, err}
}()
// Wait for the read to complete
result := <-resultChannel
if result.err != nil {
revListCommand.Process.Kill()
catFileCommand.Process.Kill()
revListCommand.Wait()
catFileCommand.Wait()
return currentStatistics, fmt.Errorf("failed to read cat-file output: %w", result.err)
}
pkg/git/git.go:166
- The pipe cleanup at line 164 only closes
revListStdout, but there's no corresponding cleanup forcatFileStdoutwhen the second StdoutPipe() call fails. This creates a resource leak. Both pipes should be properly closed in error paths.
Consider adding catFileStdout.Close() alongside the existing revListStdout.Close() call when needed.
if err != nil {
revListStdout.Close()
return currentStatistics, fmt.Errorf("failed to create cat-file stdout pipe: %w", err)
}
| if err := revListCommand.Start(); err != nil { | ||
| return currentStatistics, fmt.Errorf("failed to start rev-list: %w", err) | ||
| } | ||
| if err := catFileCommand.Start(); err != nil { |
There was a problem hiding this comment.
There's no cleanup of the pipes when revListCommand.Start() fails. The revListStdout and catFileStdout pipes should be closed to avoid resource leaks. This is particularly important since the pipes were already created before starting the commands.
This issue also appears in the following locations of the same file:
- line 192
- line 185
- line 163
| if err := revListCommand.Start(); err != nil { | |
| return currentStatistics, fmt.Errorf("failed to start rev-list: %w", err) | |
| } | |
| if err := catFileCommand.Start(); err != nil { | |
| if err := revListCommand.Start(); err != nil { | |
| revListStdout.Close() | |
| catFileStdout.Close() | |
| return currentStatistics, fmt.Errorf("failed to start rev-list: %w", err) | |
| } | |
| if err := catFileCommand.Start(); err != nil { | |
| revListStdout.Close() | |
| catFileStdout.Close() |
| revListCommand.Process.Kill() | ||
| revListCommand.Wait() |
There was a problem hiding this comment.
When catFileCommand.Start() fails, revListCommand.Wait() is called immediately after Process.Kill(). This is a race condition risk because Kill() is asynchronous and may not have completed before Wait() is called. Additionally, there's no cleanup of the catFileStdout pipe.
Consider checking the error from Kill() and ensuring proper cleanup of both pipes before returning.
This issue also appears in the following locations of the same file:
- line 202
| revListCommand.Process.Kill() | |
| revListCommand.Wait() | |
| var killError error | |
| if revListCommand.Process != nil { | |
| killError = revListCommand.Process.Kill() | |
| _ = revListCommand.Wait() | |
| } | |
| _ = revListStdout.Close() | |
| _ = catFileStdout.Close() | |
| if killError != nil && !errors.Is(killError, os.ErrProcessDone) { | |
| return currentStatistics, fmt.Errorf("failed to start cat-file: %w (failed to kill rev-list: %v)", err, killError) | |
| } |
| // Run git rev-list to get objects | ||
| revListCommand := exec.Command("git", "rev-list", "--objects", "--all", | ||
| fmt.Sprintf("--before=%d-01-01", year+1), | ||
| fmt.Sprintf("--after=%d-12-31", year-1)) | ||
|
|
||
| // Run git cat-file to get object details | ||
| catFileCommand := exec.Command("git", "cat-file", "--batch-check=%(objecttype) %(objectname) %(objectsize) %(objectsize:disk) %(rest)") | ||
|
|
||
| // Capture stderr from both commands for debugging | ||
| var revListStderr, catFileStderr bytes.Buffer | ||
| revListCommand.Stderr = &revListStderr | ||
| catFileCommand.Stderr = &catFileStderr | ||
|
|
||
| // Connect rev-list stdout to cat-file stdin using a pipe | ||
| revListStdout, err := revListCommand.StdoutPipe() | ||
| if err != nil { | ||
| return currentStatistics, err | ||
| return currentStatistics, fmt.Errorf("failed to create rev-list stdout pipe: %w", err) | ||
| } | ||
| catFileCommand.Stdin = revListStdout | ||
|
|
||
| // Capture cat-file output | ||
| catFileStdout, err := catFileCommand.StdoutPipe() | ||
| if err != nil { | ||
| revListStdout.Close() | ||
| return currentStatistics, fmt.Errorf("failed to create cat-file stdout pipe: %w", err) | ||
| } | ||
|
|
||
| // Start both commands | ||
| if err := revListCommand.Start(); err != nil { | ||
| return currentStatistics, fmt.Errorf("failed to start rev-list: %w", err) | ||
| } | ||
| if err := catFileCommand.Start(); err != nil { | ||
| revListCommand.Process.Kill() | ||
| revListCommand.Wait() | ||
| return currentStatistics, fmt.Errorf("failed to start cat-file: %w", err) | ||
| } | ||
|
|
||
| // Read output from cat-file in a goroutine to avoid deadlock | ||
| // (if rev-list fills the pipe buffer while we're blocked reading) | ||
| type readResult struct { | ||
| output []byte | ||
| err error | ||
| } | ||
| resultChannel := make(chan readResult, 1) | ||
| go func() { | ||
| output, err := io.ReadAll(catFileStdout) | ||
| resultChannel <- readResult{output, err} | ||
| }() | ||
|
|
||
| // Wait for the read to complete | ||
| result := <-resultChannel | ||
| if result.err != nil { | ||
| revListCommand.Process.Kill() | ||
| catFileCommand.Process.Kill() | ||
| revListCommand.Wait() | ||
| catFileCommand.Wait() | ||
| return currentStatistics, fmt.Errorf("failed to read cat-file output: %w", result.err) | ||
| } | ||
| output := result.output | ||
|
|
||
| // Wait for both commands to complete | ||
| if err := revListCommand.Wait(); err != nil { | ||
| catFileCommand.Process.Kill() | ||
| catFileCommand.Wait() | ||
| stderrMsg := strings.TrimSpace(revListStderr.String()) | ||
| if stderrMsg != "" { | ||
| return currentStatistics, fmt.Errorf("rev-list command failed: %w, stderr: %s", err, stderrMsg) | ||
| } | ||
| return currentStatistics, fmt.Errorf("rev-list command failed: %w", err) | ||
| } | ||
| if err := catFileCommand.Wait(); err != nil { | ||
| stderrMsg := strings.TrimSpace(catFileStderr.String()) | ||
| if stderrMsg != "" { | ||
| return currentStatistics, fmt.Errorf("cat-file command failed: %w, stderr: %s", err, stderrMsg) | ||
| } | ||
| return currentStatistics, fmt.Errorf("cat-file command failed: %w", err) | ||
| } |
There was a problem hiding this comment.
The refactored GetGrowthStats function introduces significant new process management and piping logic, but there are no tests covering this functionality. Given that other functions in this package have tests in git_test.go (e.g., TestGetGitVersion, TestGetGitDirectory), this function should also have test coverage to ensure the pipe communication between git commands works correctly across platforms.
Consider adding tests that verify the piping behavior works correctly, and tests that validate proper error handling when commands fail.
1 participant
This pull request refactors how the application gathers Git object statistics for a given year, replacing a shell-based approach with direct use of Go's process management and pipes. The main goal is to improve cross-platform compatibility and reliability by eliminating the dependency on external shells (like bash on windows) and simplifying requirement checks.
Refactoring Git command execution:
GetGrowthStatswith direct execution ofgitcommands and piping using Go'sos/execandiopackages, eliminating the need for a shell and making the process more robust and platform-independent.ShellToUsehelper function and all related logic, as shell invocation is no longer necessary.Requirements and platform checks:
bashon Windows fromCheckRequirementsinpkg/requirements/requirements.go, since the code no longer relies on shell pipes that require Bash.runtimefrom bothpkg/git/git.goandpkg/requirements/requirements.go, reflecting the removal of platform-specific logic. [1] [2]