René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Git.refresh GitCommandNotFound indicates \"git\" even for other commands","articleBody":"### The bug\r\n\r\nEven when the Git command that `Git.refresh` runs and checks is not `git`, its command line is reported as `git` in the `GitCommandNotFound` exception, when such an exception is raised:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d28c20b1dbf18dc60a2b1859e818801bd8d742f2/git/cmd.py#L483-L485\r\n\r\n### Steps to reproduce\r\n\r\nThe effect happens when the exception is raised due to a subsequent call to `Git.refresh`, since that is when `Git.refresh` raises `GitCommandNotFound` rather than `ImportError`. The `command` attribute is directly part of the traceback message, presented as `cmdline`. This can be produced by running a shell one-liner:\r\n\r\n```text\r\n$ GIT_PYTHON_GIT_EXECUTABLE=git-2.43 GIT_PYTHON_REFRESH=quiet python -c 'import git; git.refresh()'\r\nTraceback (most recent call last):\r\n  File \"\u003cstring\u003e\", line 1, in \u003cmodule\u003e\r\n  File \"/home/ek/repos-wsl/GitPython/git/__init__.py\", line 127, in refresh\r\n    if not Git.refresh(path=path):\r\n           ^^^^^^^^^^^^^^^^^^^^^^\r\n  File \"/home/ek/repos-wsl/GitPython/git/cmd.py\", line 485, in refresh\r\n    raise GitCommandNotFound(\"git\", err)\r\ngit.exc.GitCommandNotFound: Cmd('git') not found due to: 'Bad git executable.\r\nThe git executable must be specified in one of the following ways:\r\n    - be included in your $PATH\r\n    - be set via $GIT_PYTHON_GIT_EXECUTABLE\r\n    - explicitly set via git.refresh()\r\n'\r\n  cmdline: git\r\n```\r\n\r\nAlthough often a custom Git command may be a full path whose last component is exactly `git` (or sometimes `git.exe` on Windows), the above shows a realistic scenario in which even that may not be the case. I do not have a command named `git-2.43` installed, so it is correct that I get an error, but the error should not state the name of the command as `git`.\r\n\r\n### Verification\r\n\r\nVerbose debugging confirms that the command given in `GIT_PYTHON_GIT_EXECUTABLE` really is running, and that the only problem is that the hard-coded command name `git` is printed instead:\r\n\r\n```text\r\n$ GIT_PYTHON_GIT_EXECUTABLE=git-2.43 GIT_PYTHON_REFRESH=quiet GIT_PYTHON_TRACE=full python -c 'import logging; logging.basicConfig(level=logging.DEBUG); import git; git.refresh()'\r\nDEBUG:git.cmd:Popen(['git-2.43', 'version'], cwd=/home/ek/repos-wsl/GitPython, stdin=None, shell=False, universal_newlines=False)\r\nDEBUG:git.cmd:Popen(['git-2.43', 'version'], cwd=/home/ek/repos-wsl/GitPython, stdin=None, shell=False, universal_newlines=False)\r\nTraceback (most recent call last):\r\n  ...\r\n  cmdline: git\r\n```\r\n\r\nCapturing the `subprocess.Popen` [audit event](https://docs.python.org/3/library/audit_events.html) verifies that no other logging bug in GitPython is confusing the analysis:\r\n\r\n```text\r\n$ GIT_PYTHON_GIT_EXECUTABLE=git-2.43 GIT_PYTHON_REFRESH=quiet python -c 'import sys, git; sys.addaudithook(lambda event, args: event == \"subprocess.Popen\" and print(args[:2])); git.refresh()'\r\n('git-2.43', ['git-2.43', 'version'])\r\nTraceback (most recent call last):\r\n  ...\r\n  cmdline: git\r\n```\r\n\r\n(For both the above runs, I replaced most of the traceback with `...`.)\r\n\r\n### Why this is a bug\r\n\r\nSince it's possible, however unlikely, that someone has come to rely on the exception object's `command` attribute having the exact value `\"git\"` in this situation, it should *arguably* only be changed if it really is a bug for it to hold that value.\r\n\r\nI believe it is a bug, for three reasons:\r\n\r\n#### 1. It is presented as the command line\r\n\r\nWhen users see this in the `GitCommandNotFound` exception message, I believe they will assume it is the command that was run:\r\n\r\n```text\r\n  cmdline: git\r\n```\r\n\r\n#### 2. It differs from any other `GitCommandNotFound`\r\n\r\nIn other situations that raise `GitCommandNotFound`, it comes from `Git.execute`, and the exception is constructed with the actual command line as `command` (except that redactions are performed, such as for passwords):\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d28c20b1dbf18dc60a2b1859e818801bd8d742f2/git/cmd.py#L1065-L1079\r\n\r\n#### 3. The superclass documents the `command` argument\r\n\r\n`GitCommandNotFound` 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:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d28c20b1dbf18dc60a2b1859e818801bd8d742f2/git/exc.py#L83-L88\r\n\r\nThis 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.\r\n\r\n### Possible fixes\r\n\r\nThis 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`.\r\n\r\nAnother approach, though, would be to keep a reference to the `GitCommandNotFound` exception that occurred occurred due to running `cls().version()`:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d28c20b1dbf18dc60a2b1859e818801bd8d742f2/git/cmd.py#L381-L385\r\n\r\nThen that same exception object, with its original information, could be reraised instead of constructing and raising a new `GitCommandNotFound` object.\r\n\r\nIf 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:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/d28c20b1dbf18dc60a2b1859e818801bd8d742f2/git/cmd.py#L476\r\n\r\nThus, in addition to the other changes, that statement could be changed to the `raise ... from ...` form.\r\n\r\nThere may be reasons to avoid this approach, in whole or in part, that I am not aware of.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2024-01-24T03:25:48.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":1},"url":"https://github.com/1809/GitPython/issues/1809"}