Skip to content

Harden commit trailer subprocess handling and align trailer I/O paths #2125

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

Merged
Byron merged 6 commits into from
Apr 13, 2026
Merged

Harden commit trailer subprocess handling and align trailer I/O paths #2125

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bd58716
Use consistent interpret-trailers encoding
Copilot Apr 12, 2026
7cdf9c7
Normalize interpret-trailers subprocess IO
Copilot Apr 12, 2026
1e2a895
Handle mypy CI typing failure
Copilot Apr 12, 2026
34ec40d
Use commit encoding for trailer parsing
Copilot Apr 13, 2026
4aa8157
Refine trailer encoding review follow-up
Copilot Apr 13, 2026
633abdb
Fix trailer subprocess lifetime
Copilot Apr 13, 2026
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
38 changes: 21 additions & 17 deletions git/objects/commit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,14 +450,7 @@ def trailers_list(self) -> List[Tuple[str, str]]:
:return:
List containing key-value tuples of whitespace stripped trailer information.
"""
cmd = ["git", "interpret-trailers", "--parse"]
proc: Git.AutoInterrupt = self.repo.git.execute( # type: ignore[call-overload]
cmd,
as_process=True,
istream=PIPE,
)
trailer: str = proc.communicate(str(self.message).encode())[0].decode("utf8")
trailer = trailer.strip()
trailer = self._interpret_trailers(self.repo, self.message, ["--parse"], encoding=self.encoding).strip()

if not trailer:
return []
Expand All @@ -469,6 +462,25 @@ def trailers_list(self) -> List[Tuple[str, str]]:

return trailer_list

@classmethod
def _interpret_trailers(
cls,
repo: "Repo",
message: Union[str, bytes],
trailer_args: Sequence[str],
encoding: str = default_encoding,
) -> str:
cmd = [repo.git.GIT_PYTHON_GIT_EXECUTABLE, "interpret-trailers", *trailer_args]
proc: Git.AutoInterrupt = repo.git.execute( # type: ignore[call-overload]
cmd,
as_process=True,
istream=PIPE,
)
message_bytes = message if isinstance(message, bytes) else message.encode(encoding, errors="strict")
stdout_bytes, _ = proc.communicate(message_bytes)
finalize_process(proc)
return stdout_bytes.decode(encoding, errors="strict")
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

_interpret_trailers() starts the subprocess before encoding message to bytes. If message.encode(...) raises (e.g., UnicodeEncodeError under a legacy commitencoding), the git process is left running until GC/AutoInterrupt finalization. Compute message_bytes before launching the process, or wrap subprocess lifetime in try/finally to ensure it is waited/terminated on all failure paths.

Suggested change
cmd = [repo.git.GIT_PYTHON_GIT_EXECUTABLE, "interpret-trailers", *trailer_args]
proc: Git.AutoInterrupt = repo.git.execute( # type: ignore[call-overload]
cmd,
as_process=True,
istream=PIPE,
)
message_bytes = message if isinstance(message, bytes) else message.encode(encoding, errors="strict")
stdout_bytes, _ = proc.communicate(message_bytes)
finalize_process(proc)
return stdout_bytes.decode(encoding, errors="strict")
message_bytes = message if isinstance(message, bytes) else message.encode(encoding, errors="strict")
cmd = [repo.git.GIT_PYTHON_GIT_EXECUTABLE, "interpret-trailers", *trailer_args]
proc: Git.AutoInterrupt = repo.git.execute( # type: ignore[call-overload]
cmd,
as_process=True,
istream=PIPE,
)
try:
stdout_bytes, _ = proc.communicate(message_bytes)
return stdout_bytes.decode(encoding, errors="strict")
finally:
finalize_process(proc)

Copilot uses AI. Check for mistakes.

@property
def trailers_dict(self) -> Dict[str, List[str]]:
"""Get the trailers of the message as a dictionary.
Expand Down Expand Up @@ -699,15 +711,7 @@ def create_from_tree(
trailer_args.append("--trailer")
trailer_args.append(f"{key}: {val}")

cmd = [repo.git.GIT_PYTHON_GIT_EXECUTABLE, "interpret-trailers"] + trailer_args
proc: Git.AutoInterrupt = repo.git.execute( # type: ignore[call-overload]
cmd,
as_process=True,
istream=PIPE,
)
stdout_bytes, _ = proc.communicate(str(message).encode())
finalize_process(proc)
message = stdout_bytes.decode("utf8")
message = cls._interpret_trailers(repo, str(message), trailer_args)
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

create_from_tree has already computed conf_encoding from i18n.commitencoding, but trailer application calls _interpret_trailers without using it. To keep trailer I/O aligned with how the commit will be serialized, pass conf_encoding (or have _interpret_trailers derive it from the repo) when applying trailers.

Suggested change
message = cls._interpret_trailers(repo, str(message), trailer_args)
message = cls._interpret_trailers(
repo,
str(message),
trailer_args,
conf_encoding=conf_encoding,
)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

In create_from_tree(), trailers are applied via _interpret_trailers() without passing the repo-configured commit encoding (conf_encoding). This means interpret-trailers I/O is always UTF-8 even when the commit will be serialized using a different encoding, which is inconsistent with trailers_list() and can produce different on-disk bytes. Pass encoding=conf_encoding when calling _interpret_trailers here (and avoid double str() conversion if possible).

Suggested change
message = cls._interpret_trailers(repo, str(message), trailer_args)
if not isinstance(message, str):
message = str(message)
message = cls._interpret_trailers(repo, message, trailer_args, encoding=conf_encoding)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

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

create_from_tree reads i18n.commitencoding into conf_encoding, but the trailer application path calls _interpret_trailers(...) without passing that encoding (so it always encodes/decodes as UTF-8). To keep trailer I/O aligned with the configured commit encoding (and avoid byte-length differences affecting wrapping/formatting), pass encoding=conf_encoding when calling _interpret_trailers here.

Suggested change
message = cls._interpret_trailers(repo, str(message), trailer_args)
message = cls._interpret_trailers(repo, str(message), trailer_args, encoding=conf_encoding)

Copilot uses AI. Check for mistakes.
# END apply trailers

# CREATE NEW COMMIT
Expand Down
54 changes: 54 additions & 0 deletions test/test_commit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,60 @@ def test_create_from_tree_with_trailers_list(self, rw_dir):
"Issue": ["456"],
}

@with_rw_directory
def test_create_from_tree_with_non_utf8_trailers(self, rw_dir):
"""Test that trailer creation and parsing respect the configured commit encoding."""
rw_repo = Repo.init(osp.join(rw_dir, "test_trailers_non_utf8"))
with rw_repo.config_writer() as writer:
writer.set_value("i18n", "commitencoding", "ISO-8859-1")

path = osp.join(str(rw_repo.working_tree_dir), "hello.txt")
touch(path)
rw_repo.index.add([path])
tree = rw_repo.index.write_tree()

commit = Commit.create_from_tree(
rw_repo,
tree,
"Résumé",
head=True,
trailers={"Reviewed-by": "André <andre@example.com>"},
)

assert commit.encoding == "ISO-8859-1"
assert "Résumé" in commit.message
assert "Reviewed-by: André <andre@example.com>" in commit.message
assert commit.trailers_list == [("Reviewed-by", "André <andre@example.com>")]

@with_rw_directory
def test_trailers_list_with_non_utf8_message_bytes(self, rw_dir):
"""Test that trailer parsing handles non-UTF-8 commit message bytes."""
rw_repo = Repo.init(osp.join(rw_dir, "test_trailers_non_utf8_bytes"))
with rw_repo.config_writer() as writer:
writer.set_value("i18n", "commitencoding", "ISO-8859-1")

path = osp.join(str(rw_repo.working_tree_dir), "hello.txt")
touch(path)
rw_repo.index.add([path])
tree = rw_repo.index.write_tree()

commit = Commit.create_from_tree(
rw_repo,
tree,
"Résumé",
head=True,
trailers={"Reviewed-by": "André <andre@example.com>"},
)

bytes_commit = Commit(
rw_repo,
commit.binsha,
message=commit.message.encode(commit.encoding),
encoding=commit.encoding,
)

assert bytes_commit.trailers_list == [("Reviewed-by", "André <andre@example.com>")]

@with_rw_directory
def test_index_commit_with_trailers(self, rw_dir):
"""Test that IndexFile.commit() supports adding trailers."""
Expand Down
Loading