fix: correct add logic for all vs specific files and add tests (#2373)

[tajbaba999]

Description

This PR updates the git_add function to correctly differentiate between staging all files (["."]) and specific files. Previously, using ["."] could unintentionally include the .git/ directory, potentially corrupting the repository. This fix ensures only appropriate files are staged.

It also includes two additional test cases:

• One for adding all files using ["."]
• One for adding specific files only

Server Details

• Server: github
• Changes to: tools

Motivation and Context

Fixes #2373

The issue identified that git_add(files=["."]) was tracking files inside the .git/ directory. This caused repository integrity issues. This PR corrects the logic to avoid touching internal Git files and ensures safe staging behavior.

How Has This Been Tested?

• Verified manually that .git/ is excluded when staging all files
• Added and ran unit tests for both general and specific file staging
• Confirmed behavior works correctly with an LLM client environment

Breaking Changes

None

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Protocol Documentation
• My changes follow MCP security best practices
• I have updated the server's README accordingly
• I have tested this with an LLM client
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have documented all environment variables and configuration options

Additional context

Let me know if there are any changes or improvements you'd like to see. Happy to revise as needed.

[tajbaba999]

Hello @olaservo,
Please let me know if any changes or improvements are needed. I’ll be happy to make the necessary updates.

[domdomegg]

forgive my ignorance here, but is there a reason we don't just use repo.git.add always? e.g. could we get away with not making . a special case?

[tajbaba999]

forgive my ignorance here, but is there a reason we don't just use repo.git.add always? e.g. could we get away with not making . a special case?

The issue was caused by using repo.index.add(["."]), which doesn't behave like git add . and ends up including .git/ files, corrupting the repo. I added a check to use repo.git.add(".") when ["."] is passed, which correctly mimics Git’s behavior. This keeps things safe and avoids staging internal Git files.

[domdomegg]

I meant why not just repo.git.add everything (i.e. never use repo.index.add)?

Or @claude if you're around are you able to chime in?

[domdomegg]

LGTM - would like to understand why we use repo.index at all (see comment above) before merging.

[tajbaba999]

Hey @domdomegg — fixed a small typo ("successfully") that caused the build to fail. Also shared a quick note above on why this approach was used — could you please re-review the PR?

[domdomegg]

From Claude:

---

Hi @domdomegg,

Regarding your question about why not always use repo.git.add instead of repo.index.add:

Both approaches work, but there are trade-offs:

1. Performance: repo.index.add(files) is more direct and efficient when adding specific files because it works with GitPython's index object directly, avoiding the overhead of spawning a git subprocess.

2. Consistency with GitPython patterns: Using repo.index.add() for specific files follows GitPython's intended API patterns - using the Python API for most operations and falling back to git commands only when needed.

3. **The ." special case**: The issue that prompted this PR is that repo.index.add(["."])doesn't behave the same asgit add .. The git command has special logic to exclude the .git/` directory, while the index method doesn't have this safeguard.

You could indeed simplify the code to always use repo.git.add(), which would work correctly. The current approach is a balance between:

• Using the more efficient Python API for specific file operations
• Using the git command only for the special `." case where the behavior differs

If you prefer consistency over the minor performance benefit, switching everything to repo.git.add() would be a valid approach and would eliminate the need for special-case handling.

---

To me, this suggests we could simplify the code by always using repo.git.add - I think I'd have a slight preference for this. I don't think performance is what we're optimising for here.

But that said it sounds like this PR will work and is an improvement, so I will accept it.

[domdomegg]

Approving - will merge in a bit if we don't want to change the add/index thing