In handle_process_output don't forward finalizer result by EliahKagan · Pull Request #1788 · gitpython-developers/GitPython (original) (raw)

handle_process_output has another parameter, stderr_handler, which can also be a callable or None, which handle_process_output calls when non-None, both directly and through its pump_stream local function. However, when it is a callable, it is also expected to return None, and handle_process_output does not forward or otherwise use a result from it (and has not, at least not recently--I haven't looked into its history).

That parameter is used, including when pulling, for reporting progress:

def _get_fetch_info_from_stderr(
self,
proc: "Git.AutoInterrupt",
progress: Union[Callable[..., Any], RemoteProgress, None],
kill_after_timeout: Union[None, float] = None,
) -> IterableList["FetchInfo"]:
progress = to_progress_instance(progress)
# Skip first line as it is some remote info we are not interested in.
output: IterableList["FetchInfo"] = IterableList("name")
# Lines which are no progress are fetch info lines.
# This also waits for the command to finish.
# Skip some progress lines that don't provide relevant information.
fetch_info_lines = []
# Basically we want all fetch info lines which appear to be in regular form, and
# thus have a command character. Everything else we ignore.
cmds = set(FetchInfo._flag_map.keys())
progress_handler = progress.new_message_handler()
handle_process_output(
proc,
None,
progress_handler,
finalizer=None,
decode_streams=False,
kill_after_timeout=kill_after_timeout,
)

(The function doesn't end there, but that's the relevant part.)

to_progress_instance returns a RemoteProgress object (it is annotated with a return type of a union of RemoteProgress and CallableRemoteProgress, but the latter inherits from the former). Calling new_message_handler on that returns a function annotated always to return None... yet, its implementation inn RemoteProgress (it is not overridden in CallableRemoteProgress) forwards a result from the _parse_progress_line method:

def new_message_handler(self) -> Callable[[str], None]:
"""
:return:
A progress handler suitable for handle_process_output(), passing lines on to
this Progress handler in a suitable format
"""
def handler(line: AnyStr) -> None:
return self._parse_progress_line(line.rstrip())
# END handler
return handler

But I believe this is always just forwarding an implicitly returned None (often due to a return statement, but never with an operand). The _parse_progress_line method has this signature, which it implementation appears completely true to, both in behavior and in style:

def _parse_progress_line(self, line: AnyStr) -> None:

Rather than return a result or status from parsing to the caller, it mutates the state of self.

It seems to me that the git.util.RemoteProgress.new_message_handler method should therefore have its handler local function, which it returns, return None implicitly rather than forwarding the implicit None from RemoteProgress._parse_progress_line. That change would be:

 def new_message_handler(self) -> Callable[[str], None]:
     """
     :return:
         A progress handler suitable for handle_process_output(), passing lines on to
         this Progress handler in a suitable format
     """

     def handler(line: AnyStr) -> None: