-
-
Notifications
You must be signed in to change notification settings - Fork 971
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.
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
Changes from all commits
bd58716
7cdf9c7
1e2a895
34ec40d
4aa8157
633abdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 [] | ||||||||||||||||||||||||||||
|
|
@@ -469,6 +462,27 @@ 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: | ||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @property | ||||||||||||||||||||||||||||
| def trailers_dict(self) -> Dict[str, List[str]]: | ||||||||||||||||||||||||||||
| """Get the trailers of the message as a dictionary. | ||||||||||||||||||||||||||||
|
|
@@ -699,15 +713,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) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
| message = cls._interpret_trailers(repo, str(message), trailer_args) | |
| message = cls._interpret_trailers( | |
| repo, | |
| str(message), | |
| trailer_args, | |
| conf_encoding=conf_encoding, | |
| ) |
Copilot
AI
Apr 13, 2026
There was a problem hiding this comment.
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).
| 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
AI
Apr 13, 2026
There was a problem hiding this comment.
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.
| message = cls._interpret_trailers(repo, str(message), trailer_args) | |
| message = cls._interpret_trailers(repo, str(message), trailer_args, encoding=conf_encoding) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
_interpret_trailers,proc.communicate()captures stderr but it’s discarded, and thenfinalize_process(proc)may raiseGitCommandErrorwithout including the subprocess stderr (pipes may already be drained/closed). Capturestderr_bytesfromcommunicate()and pass it intofinalize_process(proc, stderr=stderr_bytes)so failures surface with a useful error message.