diff --git a/src/git/src/mcp_server_git/server.py b/src/git/src/mcp_server_git/server.py index f7bfc1c20b..fded30cf40 100644 --- a/src/git/src/mcp_server_git/server.py +++ b/src/git/src/mcp_server_git/server.py @@ -73,18 +73,47 @@ class GitTools(str, Enum): BRANCH = "git_branch" +def _git_text(repo: git.Repo, command: str, *args: str) -> str: + """Run `git ` for a tool that hands git's output to the client. + + Two things about that output are fixed here, for git_status and the three + diff tools alike: + + - Paths (#5129). Under git's default `core.quotepath=true`, every byte of a + path at or above 0x80 prints as an octal escape (`"\\346\\227\\245.txt"`), + a name the client cannot pass back to git_add. `-c core.quotepath=false` + prints such names as written, for this one command only: nothing is + written to the repository's config. Git still C-quotes a name holding a + control character (a tab, say) or a double quote or backslash; the flag + governs only the high-bit bytes. + - Content (#5193). GitPython decodes output with surrogateescape, so a + non-UTF-8 file (Latin-1, say) leaves lone surrogates that cannot be + serialized as UTF-8. Take the bytes and replace what does not decode + with U+FFFD, as git_show does. + + The `-c` has to come before the subcommand, so it goes through + `repo.git(c=...)`; `repo.git.status(c=...)` would put it after and fail. + That call sets the option on the shared `repo.git` until its next command + runs, which is the one made here. + """ + output = getattr(repo.git(c="core.quotepath=false"), command)( + *args, stdout_as_string=False + ) + return output.decode("utf-8", errors="replace") + + def git_status(repo: git.Repo) -> str: - return repo.git.status() + return _git_text(repo, "status") def git_diff_unstaged( repo: git.Repo, context_lines: int = DEFAULT_CONTEXT_LINES ) -> str: - return repo.git.diff(f"--unified={context_lines}") + return _git_text(repo, "diff", f"--unified={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") + return _git_text(repo, "diff", f"--unified={context_lines}", "--cached") def git_diff( @@ -95,7 +124,7 @@ def git_diff( if target.startswith("-"): raise FlagInjectionError(f"Invalid target: '{target}' - cannot start with '-'") repo.rev_parse(target) # Validates target is a real git ref, throws BadName if not - return repo.git.diff(f"--unified={context_lines}", target) + return _git_text(repo, "diff", f"--unified={context_lines}", target) def _has_staged_changes(repo: git.Repo) -> bool: diff --git a/src/git/tests/test_protocol.py b/src/git/tests/test_protocol.py index e235f0dbcf..c6220df2c4 100644 --- a/src/git/tests/test_protocol.py +++ b/src/git/tests/test_protocol.py @@ -315,6 +315,42 @@ async def test_git_diff_rejects_flag_injection(repo: git.Repo): ) +async def test_git_diff_of_non_utf8_content_is_a_normal_result(repo: git.Repo): + """A Latin-1 file's diff reaches the client with U+FFFD in place of the + undecodable bytes, as git_show's does, and serializes as UTF-8 (#5193).""" + root = root_of(repo) + (root / "latin1.txt").write_bytes(b"caf\xe9\n") + repo.index.add(["latin1.txt"]) + repo.index.commit("add Latin-1 file") + (root / "latin1.txt").write_bytes(b"caf\xe9 na\xefve\n") + result = await call(None, "git_diff_unstaged", {"repo_path": str(root)}) + assert result["isError"] is False + text = result["content"][0]["text"] + assert text.startswith("Unstaged changes:\ndiff --git a/latin1.txt b/latin1.txt") + assert "-caf�\n+caf� na�ve" in text + json.dumps(result, ensure_ascii=False).encode("utf-8") # no lone surrogates + + +async def test_git_status_non_ascii_path_round_trips_through_git_add( + repo: git.Repo, +): + """Under git's default core.quotepath=true the name still comes back as + written, so a client can pass it straight to git_add (#5129).""" + root = root_of(repo) + with repo.config_writer() as config: + config.set_value("core", "quotepath", "true") + (root / "日本語.txt").write_text("new\n", newline="\n") + status = await call(None, "git_status", {"repo_path": str(root)}) + assert "\t日本語.txt\n" in status["content"][0]["text"] + added = await call( + None, "git_add", {"repo_path": str(root), "files": ["日本語.txt"]} + ) + assert added["isError"] is False + # Read the index directly: `staged_paths` lists through git, which would + # octal-escape the name under the pinned core.quotepath. + assert "日本語.txt" in {path for path, _stage in repo.index.entries} + + # -------------------------------------------------------------------------- # git_commit # -------------------------------------------------------------------------- diff --git a/src/git/tests/test_server.py b/src/git/tests/test_server.py index 4b3815e717..538d168a80 100644 --- a/src/git/tests/test_server.py +++ b/src/git/tests/test_server.py @@ -268,6 +268,101 @@ def test_git_diff(test_repository): assert "feature changes" in result +# A non-ASCII name, and the octal escape git prints for its first character +# under its default core.quotepath=true (#5129). +NON_ASCII_FILENAME = "日本語.txt" +NON_ASCII_OCTAL = "\\346\\227\\245" + + +def pin_quotepath_true(repo: git.Repo) -> None: + """Pin git's default repo-locally, so a global config cannot hide the bug.""" + with repo.config_writer() as config: + config.set_value("core", "quotepath", "true") + + +def commit_non_ascii_file(repo: git.Repo) -> None: + """Commit NON_ASCII_FILENAME, then leave an unstaged change to it.""" + pin_quotepath_true(repo) + file_path = Path(repo.working_dir) / NON_ASCII_FILENAME + file_path.write_text("original\n") + repo.index.add([NON_ASCII_FILENAME]) + repo.index.commit("add non-ASCII file") + file_path.write_text("changed\n") + + +def test_git_status_prints_non_ascii_path_unquoted(test_repository): + pin_quotepath_true(test_repository) + (Path(test_repository.working_dir) / NON_ASCII_FILENAME).write_text("new\n") + + result = git_status(test_repository) + + assert f"\t{NON_ASCII_FILENAME}\n" in result + assert NON_ASCII_OCTAL not in result + + +def test_git_status_non_ascii_path_round_trips_through_git_add(test_repository): + pin_quotepath_true(test_repository) + (Path(test_repository.working_dir) / NON_ASCII_FILENAME).write_text("new\n") + + untracked = [ + line.strip() + for line in git_status(test_repository).splitlines() + if line.startswith("\t") + ] + git_add(test_repository, untracked) + + staged = [path for path, _stage in test_repository.index.entries] + assert NON_ASCII_FILENAME in staged + + +@pytest.mark.parametrize("tool", ["unstaged", "staged", "target"]) +def test_git_diff_tools_print_non_ascii_path_unquoted(test_repository, tool): + commit_non_ascii_file(test_repository) + if tool == "unstaged": + result = git_diff_unstaged(test_repository) + elif tool == "staged": + test_repository.index.add([NON_ASCII_FILENAME]) + result = git_diff_staged(test_repository) + else: + result = git_diff(test_repository, "HEAD") + + assert f"diff --git a/{NON_ASCII_FILENAME} b/{NON_ASCII_FILENAME}" in result + assert NON_ASCII_OCTAL not in result + + +def test_quotepath_override_is_not_written_to_repo_config(test_repository): + commit_non_ascii_file(test_repository) + config_path = Path(test_repository.git_dir) / "config" + before = config_path.read_bytes() + + git_status(test_repository) + git_diff_unstaged(test_repository) + + assert config_path.read_bytes() == before + # The override was for that one command: a plain call still escapes. + assert NON_ASCII_OCTAL in test_repository.git.status() + + +@pytest.mark.parametrize("tool", ["unstaged", "staged", "target"]) +def test_git_diff_tools_replace_undecodable_bytes(test_repository, tool): + """A Latin-1 file's diff comes back with U+FFFD, not lone surrogates (#5193).""" + file_path = Path(test_repository.working_dir) / "latin1.txt" + file_path.write_bytes(b"caf\xe9\n") + test_repository.index.add(["latin1.txt"]) + test_repository.index.commit("add Latin-1 file") + file_path.write_bytes(b"caf\xe9 na\xefve\n") + if tool == "unstaged": + result = git_diff_unstaged(test_repository) + elif tool == "staged": + test_repository.index.add(["latin1.txt"]) + result = git_diff_staged(test_repository) + else: + result = git_diff(test_repository, "HEAD") + + assert "+caf� na�ve" in result + result.encode("utf-8") # raises UnicodeEncodeError on a lone surrogate + + def test_git_commit(test_repository): file_path = Path(test_repository.working_dir) / "commit_test.txt" file_path.write_text("content to commit")