GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Git.refresh GitCommandNotFound indicates "git" even for other commands · Issue #1809 · gitpython-developers/GitPython Closed Labels DescriptionThe bugEven when the Git command that Lines 483 to 485 in d28c20b
Steps to reproduceThe effect happens when the exception is raised due to a subsequent call to Although often a custom Git command may be a full path whose last component is exactly VerificationVerbose debugging confirms that the command given in Capturing the (For both the above runs, I replaced most of the traceback with Why this is a bugSince it's possible, however unlikely, that someone has come to rely on the exception object's I believe it is a bug, for three reasons: 1. It is presented as the command lineWhen users see this in the 2. It differs from any other
|
| try: | |
| proc = safer_popen( | |
| command, | |
| env=env, | |
| cwd=cwd, | |
| bufsize=-1, | |
| stdin=(istream or DEVNULL), | |
| stderr=PIPE, | |
| stdout=stdout_sink, | |
| shell=shell, | |
| universal_newlines=universal_newlines, | |
| **subprocess_kwargs, | |
| ) | |
| except cmd_not_found_exception as err: | |
| raise GitCommandNotFound(redacted_command, err) from err |
command argumentGitCommandNotFound inherits its command attribute from its CommandError superclass. Although CommandError does not document the command attribute directly, it does document the command argument from which the value of that attribute is derived:
Lines 83 to 88 in d28c20b
| class CommandError(GitError): | |
| """Base class for exceptions thrown at every stage of `Popen()` execution. | |
| :param command: | |
| A non-empty list of argv comprising the command-line. | |
| """ |
This does not really guarantee the specific meaning of the command attribute. (Likskov substitutability is usually a goal for objects that have already been constructed/initialized; subclass __new__ and __init__ are not expected to treat arguments the same ways as the superclass.) But it is a further reason to consider the current behavior in Git.refresh unexpected.
This could be fixed by having Git.refresh construct the GitCommandNotFoundError with the same command line that its cls().version() call causes Git.execute to use, which is given by GIT_PYTHON_GIT_EXECUTABLE.
Another approach, though, would be to keep a reference to the GitCommandNotFound exception that occurred occurred due to running cls().version():
Lines 381 to 385 in d28c20b
| try: | |
| cls().version() | |
| has_git = True | |
| except (GitCommandNotFound, PermissionError): | |
| pass |
Then that same exception object, with its original information, could be reraised instead of constructing and raising a new GitCommandNotFound object.
If this is to be done, then some refactoring may be in order, and a decision about whether that GitCommandNotFound object should be used as the cause of an ImportError in the case that is raised should probably also be made:
Line 476 in d28c20b
| raise ImportError(err) |
Thus, in addition to the other changes, that statement could be changed to the raise ... from ... form.
There may be reasons to avoid this approach, in whole or in part, that I am not aware of.
That's a great catch, thanks for reporting!
I agree that the exception should show the actual git command invoked, instead of guessing it.
As for the best way to do this, I can't tell. As always, it would be great if that wasn't a breaking change, and if the resulting code wouldn't be (much) harder to understand.