In handle_process_output don't forward finalizer result

[EliahKagan]

The git.cmd.handle_process_output function is non-public (git.cmd.__all__ only lists Git) but used throughout GitPython and referenced in the git.util.RemoteProgress.new_message_handler docstring. Its finalizer argument is annotated to accept an optional callable that always returns None. It is always used this way in GitPython and RemoteProcess.new_message_handler is annotated accordingly. However, the handle_process_output docstring and implementation had documented the return value as the result of the finalizer, and the implementation had forwarded that result if passed.

This modifies the docstring and implementation to disregard any result, in accordance with the everywhere-annotated assumption that the finalizer is conceptually void and the absence of any code in GitPython that uses the result of calling handle_process_output. None is now implicitly returned, simplifying the implementation and bringing it and the docstring in line with annotations and usage.

This would be a breaking change if done on a public function, but because handle_process_output is nonpublic (and documentation for it is omitted by Sphinx, so users probably don't wrongly think it is public), I believe this is safe and non-breaking.

---

This builds on #1755, which mentioned this issue but did not fix it.

However, there is another way to fix it, which is what would have to be done instead of this if the function were public, and which I think is what should be done if, for any reason, it is intended that it continue to forward the result of a finalizer in the event that a finalizer ever returns one. The implementation could be put back to what it was before or something similar to it, and the annotations could be updated to allow the function to return a value.

It occurs to me that this may have been what was intended all along, and that the annotations might not have reflected it due to the absence of @overload support in some type checker that was in use at the time, or perhaps unawareness of it. (GitPython is using @overload elsewhere at present.) I was not sure which approach should be taken to fix the inconsistency in handle_process_output, so I chose the simpler approach of not forwarding the result.

However, if you would prefer to retain the old behavior (even aspects of it that are not in use in GitPython) and instead fix the annotations, I'd be pleased to do so. In that case, either this pull request could be closed and a new one created, or I could add another commit to it for the change.

[Byron]

That's a great catch, thanks for digging in!

I am sure you checked the observable history to see how this came to be without finding hints of this being intentional, maybe its even 'lost' in history of it ever was.

Let's go with this simple version of dealing with this rightfully assuming that nobody will be affected by this, but consider rolling it back (and make it official) if there is an outcry following a release.

[EliahKagan]

I am sure you checked the observable history to see how this came to be without finding hints of this being intentional, maybe its even 'lost' in history of it ever was.

I haven't looked at the history of all uses ever of handle_process_output; perhaps I should have. But as for the origin of forwarding the finalizer's result, that goes back to when handle_process_output was first introduced in 4914405. I presume this was done with the idea that it might eventually be useful to examine the forwarded result, but at that time, although the same commit added a few uses of handle_process_output, none of them examined the return value.

Another big moment in the history of handle_process_output was 85f38a1 (#530), when the signature changed to permit None in place of a callable for finalizer. There were no uses of handle_process_output at that time that used the result either (at least so far as git grep -EB1 '\bhandle_process_output\(' 85f38a1 could reveal).

When type annotations were later added to that code in f62c8d8 (#1234), there were likewise no uses of handle_process_output that used its return value (going by inspection with the analogous git grep command). That may be part of why it was annotated the way it was. That may have been the first point at which the finalizer always returning None was really formalized.

Beyond looking at those few points in the history, though, I have not investigated further. A deeper investigation might well reveal more.

Let's go with this simple version of dealing with this rightfully assuming that nobody will be affected by this, but consider rolling it back (and make it official) if there is an outcry following a release.

Sounds good.

[Byron]

I do remember that when pulling, the stderr stream is analysed line by line and vaguely recall that there 'was something' about how callbacks where handled. However, this finalizer doesn't seem to be used to analyse stderr to obtain a remotes progress messages. So probably it's something I miss-associate and thus what we are doing is indeed non-breaking.

[EliahKagan]

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:

GitPython/git/remote.py

Lines 828 to 855 in 32c02d1

	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:

GitPython/git/util.py

Lines 674 to 686 in 32c02d1

	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:

GitPython/git/util.py

Line 571 in 32c02d1

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:
-            return self._parse_progress_line(line.rstrip())
+            self._parse_progress_line(line.rstrip())
 
         # END handler
 
         return handler

[Byron]

Thanks for investigating!

The patch above looks like the part that was at some point added because someone had a need for it, and what I vaguely remember might not be without its merit. Whether that is true or not should be in the history of that return self._parse_progress_line(…) though, as it would mean somebody has made this a return earlier.
But if not, it should be safe to assure there is no return value.

[EliahKagan]

The line return self._parse_progress_line(line.rstrip()) was introduced in the initial version of the git.util.RemoteProgress.new_message_handler method in 4914405.

Prior to that commit, there was a git.remote.digest_process_messages function:

GitPython/git/remote.py

Lines 43 to 64 in d83f6e8

	def digest_process_messages(fh, progress):
	"""Read progress messages from file-like object fh, supplying the respective
	progress messages to the progress instance.
	:param fh: File handle to read from
	:return: list(line, ...) list of lines without linebreaks that did
	not contain progress information"""
	line_so_far = b''
	dropped_lines = list()
	while True:
	char = fh.read(1) # reads individual single byte strings
	if not char:
	break
	if char in (b'\r', b'\n') and line_so_far:
	dropped_lines.extend(progress._parse_progress_line(line_so_far.decode(defenc)))
	line_so_far = b''
	else:
	line_so_far += char
	# END process parsed line
	# END while file is not done reading
	return dropped_lines

At that time, git.util.RemoteProgress._parse_progress_line did return a value--a list of any lines that failed to be process and were thus dropped while parsing:

GitPython/git/util.py

Lines 166 to 170 in d83f6e8

	def _parse_progress_line(self, line):
	"""Parse progress information from the given line as retrieved by git-push
	or git-fetch
	:return: list(line, ...) list of lines that could not be processed"""

git.remote.digest_process_messages had used this functionality to populate dropped_lines (which it returned):

GitPython/git/remote.py

Line 58 in d83f6e8

dropped_lines.extend(progress._parse_progress_line(line_so_far.decode(defenc)))

Examining the current behavior, the git.util.RemoteProgress.new_message_handler method defines and returns a local handler function (discussed above) that itself calls the RemoteProgress instance's _parse_progress_line method. The implementation of _parse_progress_line in RemoteProgress stores lines in lists on the instance:

GitPython/git/util.py

Lines 582 to 589 in 867305d

	def _parse_progress_line(self, line: AnyStr) -> None:
	"""Parse progress information from the given line as retrieved by git-push
	or git-fetch.
	- Lines that do not contain progress info are stored in :attr:`other_lines`.
	- Lines that seem to contain an error (i.e. start with ``error:`` or ``fatal:``)
	are stored in :attr:`error_lines`.
	"""

GitPython/git/util.py

Lines 601 to 603 in 867305d

	if self._cur_line.startswith(("error:", "fatal:")):
	self.error_lines.append(self._cur_line)
	return

GitPython/git/util.py

Lines 624 to 627 in 867305d

	if not match:
	self.line_dropped(line_str)
	self.other_lines.append(line_str)
	return

RemoteProgress._parse_progress_line is annotated to return None, its implementation always implicitly returns None (either by returning without an operand or falling off the end), and CallableRemoteProgress does not override that method.

There is also a subclass in the test suite. test_remote.TestRemoteProgress overrides _parse_progress_line. It delegates most of its work to the superclass implementation and forwards its result:

GitPython/test/test_remote.py

Lines 54 to 59 in 867305d

	def _parse_progress_line(self, line):
	# We may remove the line later if it is dropped.
	# Keep it for debugging.
	self._seen_lines.append(line)
	rval = super()._parse_progress_line(line)
	return rval

That result is always None these days, so this could and should be changed to:

    def _parse_progress_line(self, line):
        # We may remove the line later if it is dropped.
        # Keep it for debugging.
        self._seen_lines.append(line)
        super()._parse_progress_line(line)

The tests don't do anything with that returned result. They do not themselves call _parse_progress_line. So I am inclined to say the handler created by new_message_handler could also call _parse_progress_line with no return keyword, in accordance with the annotations and current behavior.

But it's possible that, based on code in test_remote.py, a project using the GitPython library may be subclassing RemoteProgress and defining _parse_progress_line to return something. So that should be considered.

[Byron]

Thanks for the update!

But it's possible that, based on code in test_remote.py, a project using the GitPython library may be subclassing RemoteProgress and defining _parse_progress_line to return something. So that should be considered.

Yes, APIs that support inheritance would essentially be locked-in to certain behaviours, and it is definitely allowed (and maybe useful) to let subclasses override this method and return something where the base-implementation would only return None, implicitly or explicitly.

Due to the vague memory I have, I'd be hesitant to change the return statements outside of tests to keep supporting such sub-class implementations, as otherwise it also doesn't hurt the code to do as it does now.

Maybe this helps with clarifying documentation.

[EliahKagan]

Yes, I think that information will help.

If it is considered reasonable for new code to override _parse_progress_line with an implementation that returns a non-None value, then I think it may be worthwhile to broaden the type annotations.

If it is not considered reasonable, then maybe a docstring could caution that it is unexpected, or recommend against it, or describe it as deprecated.

[Byron]

If it is considered reasonable for new code to override _parse_progress_line with an implementation that returns a non-None value, then I think it may be worthwhile to broaden the type annotations.

I'd definitely go with this and broaden the type-annotations specifically to represent the status quo, assuming that it is or at some point was reasonable. With that said, I also wouldn't specifically advertise it.

[EliahKagan]

In hindsight, I should've opened an issue for this so it would not be forgotten about, because a substantial amount of useful information has accumulated here, and unfortunately I may not end up opening a PR for it for some time. I don't want to discourage anyone else from doing so, but I think my approach would be to wait until:

1. Most of the mypy errors are solved, so it is easier to make subjective judgments about clarity of annotations, as well as easier to notice if one has introduced an actual error or made it harder to solve one that already exists.

2. All actual deprecations are clearly indicated, so it is easy to make clear the distinction between a deprecation and something that is just documented as not being recommended to use in most situations.

   (The PEP 702 @warnings.deprecated decorator may help with this, since it is meant to work with @overload, though I don't think it's necessary to wait for that to come in before fixing this. The way it would help is that if it is used for all actual deprecations where a function or some overloads are deprecated, then its absence here will help clarify the distinction.)

I can make an issue that describes this and links here for more information. Or you can remind me of this in the future when something related to it comes up, if you think of it. Or both.

[Byron]

Thanks for bringing this up.

I can make an issue that describes this and links here for more information. Or you can remind me of this in the future when something related to it comes up, if you think of it. Or both.

I think it would be best to have a reminder-feature with the summary and back-link here for details, as I cannot vouch for the 'me of the future' - judging by my current track record, chances of me remembering would be way lower than yours 😅.

[EliahKagan]

I think it would be best to have a reminder

I've opened #1834 for this.