Skip to content

Refactor testCloneRepository function and update .gitignore #85

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.

Sign up for GitHub

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Refactor testCloneRepository function and update .gitignore #85

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
18c57de
refactor: enhance testCloneRepository function with git executable ha…
Apr 29, 2025
209fe0a
chore: add .idea and .devcontainer to .gitignore
Apr 29, 2025
6aa90ce
fix: replace cp command with mv in testCloneRepository for better fil…
ryan-gang Apr 29, 2025
90fee20
refactor: improve testCloneRepository by updating git executable hand…
ryan-gang Apr 29, 2025
7eb2f24
chore: set global git user configuration in your_git.sh
ryan-gang Apr 29, 2025
b1c5d9e
chore: set default branch name to 'main' in your_git.sh
ryan-gang Apr 29, 2025
8a0a2a1
fix: improve logging for git executable movement in testCloneRepository
Apr 29, 2025
ce98ee6
fix: remove unused debug subproject reference
ryan-gang May 20, 2025
448311b
fix: update git executable handling in testCloneRepository to use sud…
May 20, 2025
366d42e
fix: ensure temporary git directory is removed after cloning
May 20, 2025
6754885
fix: improve logging in testCloneRepository and clean up deferred fun…
ryan-gang May 20, 2025
5318018
fix: add logging for git command location in testCloneRepository
ryan-gang May 20, 2025
f8ae2ff
fix: comment out unused git command execution in testCloneRepository
ryan-gang May 20, 2025
eccd922
feat: add support for Rust and Python setup in GitHub Actions workflo…
ryan-gang May 20, 2025
88b1597
fix: comment out git configuration commands in your_git.sh
ryan-gang May 20, 2025
07a50d6
fix: uncomment git configuration commands in your_git.sh
ryan-gang May 20, 2025
87a489c
feat: add test_with_gix step to GitHub Actions workflow
ryan-gang May 20, 2025
5f9d8fc
fix: add export PATH command for cargo binaries in GitHub Actions wor…
ryan-gang May 20, 2025
6d68173
fix: update PATH handling for cargo binaries in GitHub Actions workflow
ryan-gang May 20, 2025
1a8c8b7
fix: add --force flag to cargo binstall for gitoxide
ryan-gang May 20, 2025
06a9732
fix: uncomment cargo binstall command for gitoxide in GitHub Actions …
ryan-gang May 20, 2025
46be12b
fix: add init command handling in your_git.sh script
May 20, 2025
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
1 change: 1 addition & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ jobs:
with:
python-version: '3.12'
- run: make test
- run: make test_with_git
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ dist
__pycache__
vendor
.history/
.idea/
.devcontainer/
62 changes: 61 additions & 1 deletion internal/stage_clone_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package internal

import (
"fmt"
"io"
"os"
"os/exec"
"path"

"github.com/codecrafters-io/tester-utils/random"
Expand Down Expand Up @@ -31,7 +33,7 @@ func (r TestRepo) randomFile() TestFile {
return r.exampleFiles[random.RandomInt(0, len(r.exampleFiles))]
}

var testRepos []TestRepo = []TestRepo{
var testRepos = []TestRepo{
{
url: "https://github.com/codecrafters-io/git-sample-1",
exampleCommits: []string{
Expand Down Expand Up @@ -90,6 +92,46 @@ func testCloneRepository(harness *test_case_harness.TestCaseHarness) error {
testRepo := randomRepo()

logger.Infof("$ ./%s clone %s <testDir>", path.Base(executable.Path), testRepo.url)

oldGitPath, err := exec.LookPath("git")
if err != nil {
return fmt.Errorf("git executable not found: %v", err)
}
oldGitDir := path.Dir(oldGitPath)
logger.Debugf("Found git executable at: %s", oldGitPath)

tmpGitDir, err := os.MkdirTemp("/tmp", "git-*")
if err != nil {
return err
}
logger.Debugf("Created temporary directory for git clone: %s", tmpGitDir)
tmpGitPath := path.Join(tmpGitDir, "git")
defer os.RemoveAll(tmpGitDir)

// Copy the custom_executable to the output path
command := fmt.Sprintf("sudo mv %s %s", oldGitPath, tmpGitDir)
logger.Debugf(command)
copyCmd := exec.Command("sh", "-c", command)
copyCmd.Stdout = os.Stdout
copyCmd.Stderr = os.Stderr
if err := copyCmd.Run(); err != nil {
return fmt.Errorf("CodeCrafters Internal Error: mv1 failed: %w", err)
}
logger.Debugf("mv-ed git to temp directory: %s", tmpGitDir)

logger.Infof("$ git clone %s %s", testRepo.url, "test_dir")

//////// which git
command = fmt.Sprint("which git")
logger.Debugf(command)
cmd := exec.Command("sh", "-c", command)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
return fmt.Errorf("CodeCrafters Internal Error: mv1 failed: %w", err)
}
logger.Debugf("mv-ed git to temp directory: %s", tmpGitDir)

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify checking git path and improve error message

This code has several issues:

  1. Uses fmt.Sprint unnecessarily (flagged by static analysis)
  2. Reuses the same error message from the previous command (incorrect error text)
  3. Duplicates the log message from earlier
-//////// which git
-command = fmt.Sprint("which git")
-logger.Debugf(command)
-cmd := exec.Command("sh", "-c", command)
-cmd.Stdout = os.Stdout
-cmd.Stderr = os.Stderr
-if err := cmd.Run(); err != nil {
-	return fmt.Errorf("CodeCrafters Internal Error: mv1 failed: %w", err)
-}
-logger.Debugf("mv-ed git to temp directory: %s", tmpGitDir)
+// Verify current git path
+cmd := exec.Command("which", "git")
+output, err := cmd.CombinedOutput()
+if err != nil {
+	return fmt.Errorf("failed to locate git after moving executable: %w", err)
+}
+logger.Debugf("Current git path: %s", string(output))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//////// which git
command = fmt.Sprint("which git")
logger.Debugf(command)
cmd := exec.Command("sh", "-c", command)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
return fmt.Errorf("CodeCrafters Internal Error: mv1 failed: %w", err)
}
logger.Debugf("mv-ed git to temp directory: %s", tmpGitDir)
// Verify current git path
cmd := exec.Command("which", "git")
output, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("failed to locate git after moving executable: %w", err)
}
logger.Debugf("Current git path: %s", string(output))
🧰 Tools
🪛 golangci-lint (1.64.8)

125-125: S1039: unnecessary use of fmt.Sprint

(gosimple)

🤖 Prompt for AI Agents 
In internal/stage_clone_repository.go around lines 124 to 134, simplify the git
path check by removing the unnecessary use of fmt.Sprint and directly assign the
command string. Update the error message to accurately reflect the failure
context instead of reusing the previous command's error text. Remove the
duplicated debug log message to avoid redundancy.

result, err := executable.Run("clone", testRepo.url, "test_dir")
if err != nil {
return err
Expand Down Expand Up @@ -136,5 +178,23 @@ func testCloneRepository(harness *test_case_harness.TestCaseHarness) error {
}
logger.Successf("File contents verified")

defer func() error {
// Copy the custom_executable to the output path
command := fmt.Sprintf("sudo mv %s %s", tmpGitPath, oldGitDir)
logger.Debugf(command)
copyCmd := exec.Command("sh", "-c", command)
copyCmd.Stdout = io.Discard
copyCmd.Stderr = io.Discard
if err := copyCmd.Run(); err != nil {
return fmt.Errorf("CodeCrafters Internal Error: mv2 failed: %w", err)
}
logger.Debugf("mv-ed git to og directory: %s", oldGitDir)

if err := os.RemoveAll(tmpGitDir); err != nil {
return fmt.Errorf("CodeCrafters Internal Error: delete directory failed: %s", tmpGitDir)
}
return nil
}()
Comment on lines +181 to +197
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fix error handling in deferred function and use Go's file operations

The deferred function has several issues:

  1. It returns an error that isn't captured by the caller
  2. Uses shell commands with sudo, which isn't portable
  3. Contains duplicate code to remove the temporary directory
-defer func() error {
+defer func() {
-	// Copy the custom_executable to the output path
-	command := fmt.Sprintf("sudo mv %s %s", tmpGitPath, oldGitDir)
-	logger.Debugf(command)
-	copyCmd := exec.Command("sh", "-c", command)
-	copyCmd.Stdout = io.Discard
-	copyCmd.Stderr = io.Discard
-	if err := copyCmd.Run(); err != nil {
-		return fmt.Errorf("CodeCrafters Internal Error: mv2 failed: %w", err)
-	}
-	logger.Debugf("mv-ed git to og directory: %s", oldGitDir)
+	// Restore the git executable to its original location
+	originalPath := path.Join(oldGitDir, "git")
+	logger.Debugf("Restoring git executable to: %s", originalPath)
+	
+	// Copy the git executable back to its original location
+	srcFile, err := os.Open(tmpGitPath)
+	if err != nil {
+		logger.Errorf("Failed to open temporary git executable: %v", err)
+		return
+	}
+	defer srcFile.Close()
+	
+	destFile, err := os.Create(originalPath)
+	if err != nil {
+		logger.Errorf("Failed to create original git executable: %v", err)
+		return
+	}
+	defer destFile.Close()
+	
+	if _, err := io.Copy(destFile, srcFile); err != nil {
+		logger.Errorf("Failed to copy git executable back: %v", err)
+		return
+	}
+	
+	if err := os.Chmod(originalPath, 0755); err != nil {
+		logger.Errorf("Failed to make original git executable: %v", err)
+		return
+	}
+	
+	logger.Debugf("Restored git executable to original location: %s", originalPath)

-	if err := os.RemoveAll(tmpGitDir); err != nil {
-		return fmt.Errorf("CodeCrafters Internal Error: delete directory failed: %s", tmpGitDir)
-	}
-	return nil
}()
+	// Verify git is available after restoration
+	if _, err := exec.LookPath("git"); err != nil {
+		logger.Errorf("Git executable was not restored correctly: %v", err)
+	}
+}()

Note: The os.RemoveAll(tmpGitDir) is already called in the earlier defer statement on line 109, so it's removed here to avoid duplication.

🧰 Tools
🪛 golangci-lint (1.64.8)

197-197: Error return value is not checked

(errcheck)

🤖 Prompt for AI Agents 
In internal/stage_clone_repository.go around lines 181 to 197, the deferred
function incorrectly returns an error that is not handled, uses a non-portable
shell command with sudo to move files, and duplicates the removal of the
temporary directory already handled by an earlier defer on line 109. To fix
this, remove the error return from the deferred function and handle errors
internally by logging them. Replace the shell command with Go's os.Rename
function to move the directory without sudo for portability. Also, remove the
os.RemoveAll call from this defer to avoid duplicate directory removal.


return nil
}
4 changes: 4 additions & 0 deletions internal/test_helpers/pass_all/your_git.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
#!/bin/sh

git config --global user.email "[email protected]"
git config --global user.name "Your Name"
Comment on lines +3 to +4
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid mutating the user's global Git config
Running git config --global … here will permanently overwrite ~/.gitconfig for anyone executing these tests. Instead, confine the settings to this invocation or to the repo under test. For example:

- git config --global user.email "[email protected]"
- git config --global user.name "Your Name"
- exec git "$@"
+ # Apply email/name only for this invocation, without touching ~/.gitconfig
+ exec git \
+   -c user.email="[email protected]" \
+   -c user.name="Your Name" \
+   "$@"

This keeps your test setup isolated and avoids side-effects on the developer’s environment.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
git config --global user.email "[email protected]"
git config --global user.name "Your Name"
#!/usr/bin/env bash
# Apply email/name only for this invocation, without touching ~/.gitconfig
exec git \
-c user.email="[email protected]" \
-c user.name="Your Name" \
"$@"

git config --global init.defaultBranch "main"

if [ "$1" = "write-tree" ]
then
git add .
Expand Down
Loading