From 150783767ddb66b3802078c31b4e4cca605c5300 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sat, 4 Jul 2026 16:34:11 -0400 Subject: [PATCH 1/2] Suppress noisy ERROR logs from git.is_worktree non-repo probe is_worktree() probes cwd with 'git rev-parse --show-toplevel' and expects that command to fail (retcode 128) when cwd is not a git repository, which it already handles by catching CommandExecutionError and returning False. However _get_toplevel() ran the probe without ignore_retcode, so cmd.run_all logged the expected failure at ERROR level, producing noise such as: [ERROR ] Command 'git rev-parse --show-toplevel' failed with return code: 128 Add an ignore_retcode parameter to _get_toplevel (default False, preserving behaviour for other callers) and pass ignore_retcode=True from is_worktree's probe so the expected failure no longer logs at ERROR level. Fixes #51157 --- changelog/51157.fixed.md | 1 + salt/modules/git.py | 11 ++++++++-- tests/pytests/unit/modules/test_git.py | 29 ++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 changelog/51157.fixed.md diff --git a/changelog/51157.fixed.md b/changelog/51157.fixed.md new file mode 100644 index 000000000000..1b40b814d34d --- /dev/null +++ b/changelog/51157.fixed.md @@ -0,0 +1 @@ +Suppress noisy ERROR log messages when git.is_worktree probes a directory that is not a git repository. diff --git a/salt/modules/git.py b/salt/modules/git.py index 7bde2c65bde3..d588d6275c43 100644 --- a/salt/modules/git.py +++ b/salt/modules/git.py @@ -408,7 +408,9 @@ def _git_run( return result -def _get_toplevel(path, user=None, password=None, output_encoding=None): +def _get_toplevel( + path, user=None, password=None, ignore_retcode=False, output_encoding=None +): """ Use git rev-parse to return the top level of a repo """ @@ -417,6 +419,7 @@ def _get_toplevel(path, user=None, password=None, output_encoding=None): cwd=path, user=user, password=password, + ignore_retcode=ignore_retcode, output_encoding=output_encoding, )["stdout"] @@ -2419,7 +2422,11 @@ def is_worktree(cwd, user=None, password=None, output_encoding=None): cwd = _expand_path(cwd, user) try: toplevel = _get_toplevel( - cwd, user=user, password=password, output_encoding=output_encoding + cwd, + user=user, + password=password, + ignore_retcode=True, + output_encoding=output_encoding, ) except CommandExecutionError: return False diff --git a/tests/pytests/unit/modules/test_git.py b/tests/pytests/unit/modules/test_git.py index dbbff722025b..d8113e64e080 100644 --- a/tests/pytests/unit/modules/test_git.py +++ b/tests/pytests/unit/modules/test_git.py @@ -285,3 +285,32 @@ def test_tag_rejects_message_in_opts(tmp_path): git_mod.tag(str(tmp_path), "v1.2", opts="-m 'sneaky'") git_run_mock.assert_not_called() + + +def test_is_worktree_probe_ignores_retcode(): + """ + Regression guard for #51157. + + ``git.is_worktree`` probes ``cwd`` with ``git rev-parse --show-toplevel`` + and expects that command to fail (retcode 128) when ``cwd`` is not a git + repository. That expected failure must be run with ``ignore_retcode=True`` + so the noisy ERROR-level logging is suppressed while still returning False. + """ + cmd_run_mock = MagicMock( + return_value={ + "stdout": "", + "stderr": ( + "fatal: not a git repository (or any of the parent " + "directories): .git" + ), + "retcode": 128, + "pid": 12345, + } + ) + with patch.dict(git_mod.__salt__, {"cmd.run_all": cmd_run_mock}), patch.object( + git_mod, "_expand_path", lambda cwd, user: str(cwd) + ): + assert git_mod.is_worktree("/not/a/repo") is False + + cmd_run_mock.assert_called_once() + assert cmd_run_mock.call_args.kwargs.get("ignore_retcode") is True From 52d980e5a8403eb10b75ead8ef5071704fc193f5 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sun, 5 Jul 2026 23:22:16 -0400 Subject: [PATCH 2/2] Add direct and inverse regression tests for git.is_worktree rev-parse probe The direct test calls _get_toplevel itself with ignore_retcode=True, the exact flag its production caller is_worktree passes when probing a path that may not be a repo, and asserts the flag is forwarded to cmd.run_all so the expected failure is not logged at ERROR level. The inverse test guards against overcorrection: callers like list_worktrees invoke _get_toplevel without the flag, and a genuine rev-parse failure there must still reach cmd.run_all with ignore_retcode=False and raise CommandExecutionError. --- tests/pytests/unit/modules/test_git.py | 59 ++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/pytests/unit/modules/test_git.py b/tests/pytests/unit/modules/test_git.py index d8113e64e080..3d77b419c7de 100644 --- a/tests/pytests/unit/modules/test_git.py +++ b/tests/pytests/unit/modules/test_git.py @@ -314,3 +314,62 @@ def test_is_worktree_probe_ignores_retcode(): cmd_run_mock.assert_called_once() assert cmd_run_mock.call_args.kwargs.get("ignore_retcode") is True + + +def test_get_toplevel_forwards_ignore_retcode_51157(): + """ + Regression test for #51157. + + ``_get_toplevel`` must accept ``ignore_retcode`` and forward it to + ``cmd.run_all`` so that an expected rev-parse failure is not logged at + ERROR level. This calls the helper directly with ``ignore_retcode=True``, + which is exactly what its production caller ``is_worktree`` passes when + probing a path that may not be a git repository. + """ + cmd_run_mock = MagicMock( + return_value={ + "stdout": "/some/repo", + "stderr": "", + "retcode": 0, + "pid": 12345, + } + ) + with patch.dict(git_mod.__salt__, {"cmd.run_all": cmd_run_mock}): + # ignore_retcode=True is the decisive flag; is_worktree passes it + # because the probe is expected to fail on non-repo paths. + result = git_mod._get_toplevel("/some/repo", ignore_retcode=True) + + assert result == "/some/repo" + cmd_run_mock.assert_called_once() + assert cmd_run_mock.call_args.kwargs.get("ignore_retcode") is True + + +def test_get_toplevel_default_stays_loud_51157(): + """ + Overcorrection guard for #51157. + + Only the ``is_worktree`` probe opts in to ``ignore_retcode``. Other + production callers such as ``list_worktrees`` invoke ``_get_toplevel`` + without it, and a genuine rev-parse failure there must NOT be silenced + by this fix: ``cmd.run_all`` must still receive ``ignore_retcode=False`` + (so the failure is logged) and ``_git_run`` must still raise + ``CommandExecutionError``. This test passes both with and without the + fix applied; it guards against the default flipping to True. + """ + cmd_run_mock = MagicMock( + return_value={ + "stdout": "", + "stderr": ( + "fatal: not a git repository (or any of the parent " + "directories): .git" + ), + "retcode": 128, + "pid": 12345, + } + ) + with patch.dict(git_mod.__salt__, {"cmd.run_all": cmd_run_mock}): + with pytest.raises(git_mod.CommandExecutionError): + git_mod._get_toplevel("/some/repo") + + cmd_run_mock.assert_called_once() + assert cmd_run_mock.call_args.kwargs.get("ignore_retcode") is False