Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/git/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ Please note that mcp-server-git is currently in early development. The functiona
- Shows differences between branches or commits
- Inputs:
- `repo_path` (string): Path to Git repository
- `target` (string): Target branch or commit to compare with
- `target` (string): Target branch, commit, or revision range (e.g. `main..feature`) to compare with
- `context_lines` (number, optional): Number of context lines to show (default: 3)
- Returns: Diff output comparing current state with target
- Returns: Diff output comparing current state with target, or comparing the two endpoints with each other when target is a revision range

5. `git_commit`
- Records changes to the repository
Expand Down
45 changes: 44 additions & 1 deletion src/git/src/mcp_server_git/server.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
import re
from pathlib import Path
from typing import Any, Optional, Sequence
from mcp.server import Server
Expand Down Expand Up @@ -117,12 +118,54 @@ def git_diff_unstaged(repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES
def git_diff_staged(repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES) -> str:
return repo.git.diff(f"--unified={context_lines}", "--cached")

def _resolves(repo: git.Repo, revision: str) -> bool:
"""Whether ``revision`` names a git object, treating every refusal as one.

``rev_parse`` raises ``BadName`` for a revision that does not resolve,
``ValueError`` for a spec its own parser cannot tokenize (e.g.
``HEAD~1..HEAD``), and ``KeyError`` when a ``rev:path`` target names a path
the tree does not contain. Only the first is a deliberate "not a revision"
signal, but all three mean the target is unusable as one.
"""
try:
repo.rev_parse(revision)
except (BadName, ValueError, KeyError):
return False
return True

def git_diff(repo: git.Repo, target: str, context_lines: int = DEFAULT_CONTEXT_LINES) -> str:
# Defense in depth: reject targets starting with '-' to prevent flag injection,
# even if a malicious ref with that name exists (e.g. via filesystem manipulation)
if target.startswith("-"):
raise BadName(f"Invalid target: '{target}' - cannot start with '-'")
repo.rev_parse(target) # Validates target is a real git ref, throws BadName if not
# target may be a revision range, so the endpoints have to be validated
# individually: rev_parse rejects 'main..feature' as a whole. Try the target
# unchanged first, because a single revision is allowed to contain '..'
# itself, as the commit-message selector ':/fix..bug' does.
if not _resolves(repo, target):
# Only '..' and '...' separate endpoints, so a run of four or more dots
# is a malformed range rather than a range with an odd endpoint.
if re.search(r"\.\.\.\.", target):
raise BadName(
f"Invalid target: '{target}' - expected a revision or a single "
f"'..' or '...' range"
)
revisions = re.split(r"\.\.\.?", target)
if len(revisions) > 2:
raise BadName(
f"Invalid target: '{target}' - expected a revision or a single range"
)
for revision in revisions:
if not revision:
raise BadName(f"Invalid target: '{target}' - empty range endpoint")
# Same flag-injection guard as the whole target above: an endpoint
# reaching git's option parser is no safer than the target doing so.
if revision.startswith("-"):
raise BadName(f"Invalid target: '{target}' - cannot start with '-'")
if not _resolves(repo, revision):
raise BadName(
f"Invalid target: '{target}' - '{revision}' is not a revision"
)
return repo.git.diff(f"--unified={context_lines}", target)

def git_commit(repo: git.Repo, message: str) -> str:
Expand Down
135 changes: 135 additions & 0 deletions src/git/tests/test_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,141 @@ def test_git_diff_allows_valid_refs(test_repository):
assert result is not None


def test_git_diff_allows_revision_ranges(test_repository):
"""git_diff should accept revision ranges such as 'A..B' and 'A...B'."""
default_branch = test_repository.active_branch.name

test_repository.git.checkout("-b", "range-feature")
file_path = Path(test_repository.working_dir) / "test.txt"
file_path.write_text("first range change")
test_repository.index.add(["test.txt"])
first_commit = test_repository.index.commit("first range commit")
file_path.write_text("second range change")
test_repository.index.add(["test.txt"])
test_repository.index.commit("second range commit")

# Branch ranges, in both directions and with both range syntaxes
test_repository.git.checkout(default_branch)
for target in (
f"{default_branch}..range-feature",
f"range-feature..{default_branch}",
f"{default_branch}...range-feature",
):
result = git_diff(test_repository, target)
assert "test.txt" in result
assert "range change" in result

# Commit ranges reachable from HEAD
test_repository.git.checkout("range-feature")
result = git_diff(test_repository, "HEAD~1..HEAD")
assert "second range change" in result

result = git_diff(test_repository, f"{first_commit.hexsha}..HEAD")
assert "second range change" in result

# Endpoints that are not real refs are still rejected
with pytest.raises(BadName):
git_diff(test_repository, "nonexistent..HEAD")

with pytest.raises(BadName):
git_diff(test_repository, f"{default_branch}..--output=/tmp/evil")


def test_git_diff_rejects_ranges_without_two_endpoints(test_repository):
"""A range has to name two revisions.

`..target`, `target..` and `...` are ranges with an empty endpoint, and
`....` is not a range at all. None of them resolve to two real refs, so
they are rejected here rather than reaching `git diff` and failing there
with a raw git error.
"""
for target in ("..HEAD", "HEAD..", "...", "...."):
with pytest.raises(BadName):
git_diff(test_repository, target)

# A range may not carry more than one separator either
default_branch = test_repository.active_branch.name
with pytest.raises(BadName):
git_diff(test_repository, f"{default_branch}..{default_branch}...HEAD")


def test_git_diff_allows_single_revisions_containing_dots(test_repository):
"""A single revision may contain '..' without being a revision range.

`:/text` matches commit messages, and the text is a regular expression, so
a selector such as ':/fix..bug' resolves to one revision. Splitting every
target on '..' before resolving it would validate ':/fix' and 'bug'
separately and reject a target git accepts, so the whole target is tried
first and the range split is only the fallback.
"""
file_path = Path(test_repository.working_dir) / "test.txt"
file_path.write_text("dotted selector change")
test_repository.index.add(["test.txt"])
test_repository.index.commit("fix..bug in the subject")

# Leave a change behind so the diff against that revision is not empty
file_path.write_text("dotted selector change plus worktree edit")

assert test_repository.rev_parse(":/fix..bug") is not None

result = git_diff(test_repository, ":/fix..bug")
assert "worktree edit" in result

# A dotted selector is still rejected when it matches no commit
with pytest.raises(BadName):
git_diff(test_repository, ":/no..such..subject")


def test_git_diff_rejects_ranges_with_more_than_two_endpoints(test_repository):
"""Only '..' and '...' separate endpoints, so extra dots are malformed.

`HEAD....` splits into `HEAD` and `.`, which is not a range with two real
endpoints; it has to be rejected explicitly rather than by whichever
endpoint happens to fail to resolve.
"""
default_branch = test_repository.active_branch.name

for target in ("HEAD....", f"{default_branch}....HEAD", "HEAD.....", "HEAD..--all"):
with pytest.raises(BadName):
git_diff(test_repository, target)


def test_git_diff_reports_an_unresolvable_rev_path_as_bad_name(test_repository):
"""A 'rev:path' target naming a missing path is a BadName, not a KeyError.

`rev_parse` resolves the revision and then raises `KeyError` from the tree
lookup, so probing a target for being a single revision has to treat that
the same way as a revision that does not resolve at all.
"""
for target in ("HEAD:missing/path", "HEAD:missing..path"):
with pytest.raises(BadName):
git_diff(test_repository, target)

default_branch = test_repository.active_branch.name
with pytest.raises(BadName):
git_diff(test_repository, f"{default_branch}:missing..path..HEAD")


def test_git_diff_accepts_a_peeled_message_selector(test_repository):
"""'HEAD^{/text}' is one revision; wrapping it in a range is not a range.

The selector is accepted on its own, and a range naming it is rejected
because git cannot parse one either -- it splits on the first '..' too.
"""
file_path = Path(test_repository.working_dir) / "test.txt"
file_path.write_text("peeled selector change")
test_repository.index.add(["test.txt"])
test_repository.index.commit("fix..bug in the subject")

file_path.write_text("peeled selector change plus worktree edit")

assert test_repository.rev_parse("HEAD^{/fix..bug}") is not None
assert "worktree edit" in git_diff(test_repository, "HEAD^{/fix..bug}")

with pytest.raises(BadName):
git_diff(test_repository, "HEAD^{/fix..bug}..HEAD")


def test_git_checkout_allows_valid_branches(test_repository):
"""git_checkout should work normally with valid branch names."""
# Get the default branch name
Expand Down