From 4e701bdae829bb02abd7f9acf5e6508e242b6977 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 05:54:47 -0400 Subject: [PATCH 01/90] Add missing assert keywords This turns two tuple expression statements--each of an equality comparison and message--that were not being checked or otherwise used, and that were intended to be assertions, into assertions. --- test/test_submodule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 4a9c9c582..0aa80e5ce 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -1031,8 +1031,8 @@ def test_branch_renames(self, rw_dir): # This doesn't fail as our own submodule binsha didn't change, and the reset is only triggered if # to latest revision is True. parent_repo.submodule_update(to_latest_revision=False) - sm_mod.head.ref.name == sm_pfb.name, "should have been switched to past head" - sm_mod.commit() == sm_fb.commit, "Head wasn't reset" + assert sm_mod.head.ref.name == sm_pfb.name, "should have been switched to past head" + assert sm_mod.commit() == sm_fb.commit, "Head wasn't reset" self.assertRaises(RepositoryDirtyError, parent_repo.submodule_update, to_latest_revision=True) parent_repo.submodule_update(to_latest_revision=True, force_reset=True) From 45773c27fa40fddf72b40970836b3649094cd994 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 7 Sep 2023 08:16:29 -0400 Subject: [PATCH 02/90] Instrument workflows to investigate skipped tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes jobs from both test workflows give more information relevant to examining which tests are skipped (and if any tests xfail, those too) in what environments: - Values of os.name and git.util.is_win. - The name of each test that runs, with its status. The latter doesn't increase the output length as much as might be expected, because due to the way the output is handled, the pytest-sugar pretty output format without -v looked like: test/test_actor.py ✓ 0% test/test_actor.py ✓✓ 0% ▏ test/test_actor.py ✓✓✓ 1% ▏ test/test_actor.py ✓✓✓✓ 1% ▏ When instead it was intended to fit on a single line. Still, the current output with -v has extra newlines, increasing length and worsening readability, so it should be improved on if possible. --- .github/workflows/cygwin-test.yml | 6 +++++- .github/workflows/pythonpackage.yml | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 962791ae7..3f93f6830 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -58,7 +58,11 @@ jobs: run: | /usr/bin/python -m pip install ".[test]" + - name: Check 'is_win' + run: | + /usr/bin/python -c 'import os, git; print(f"os.name={os.name}, is_win={git.compat.is_win}")' + - name: Test with pytest run: | set +x - /usr/bin/python -m pytest + /usr/bin/python -m pytest -v diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index a5467ef94..c57f31cdc 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -73,9 +73,13 @@ jobs: # so we have to ignore errors until that changes. continue-on-error: true + - name: Check 'is_win' + run: | + python -c 'import os, git; print(f"os.name={os.name}, is_win={git.compat.is_win}")' + - name: Test with pytest run: | - pytest + pytest -v continue-on-error: false - name: Documentation From 6fbe5118514618d34ea8912e99fbde3fd6d7a557 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 13 Sep 2023 16:09:01 -0400 Subject: [PATCH 03/90] Show version and platform info in one place Instead of splitting it in into two places where at least one of the places is highly likely to be missed, this puts it together just before the first steps that makes nontrivial use of the installed packages. Grouping it together, it can't really be shown earlier, because one of the pieces of information is obtained using the git module (to examine that behavior of the code). This also presents the information more clearly. "set -x" makes this easy, so the commands are rewritten to take advantage of it. --- .github/workflows/cygwin-test.yml | 13 ++++++------- .github/workflows/pythonpackage.yml | 17 ++++++++--------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 3f93f6830..1563afc95 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -29,11 +29,6 @@ jobs: with: packages: python39 python39-pip python39-virtualenv git - - name: Show python and git versions - run: | - /usr/bin/python --version - /usr/bin/git version - - name: Tell git to trust this repo run: | /usr/bin/git config --global --add safe.directory "$(pwd)" @@ -58,9 +53,13 @@ jobs: run: | /usr/bin/python -m pip install ".[test]" - - name: Check 'is_win' + - name: Show version and platform information run: | - /usr/bin/python -c 'import os, git; print(f"os.name={os.name}, is_win={git.compat.is_win}")' + /usr/bin/git version + /usr/bin/python --version + /usr/bin/python -c 'import sys; print(sys.platform)' + /usr/bin/python -c 'import os; print(os.name)' + /usr/bin/python -c 'import git; print(git.compat.is_win)' - name: Test with pytest run: | diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index c57f31cdc..696057ae2 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -36,11 +36,6 @@ jobs: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} - - name: Show python and git versions - run: | - python --version - git version - - name: Prepare this repo for tests run: | TRAVIS=yes ./init-tests-after-clone.sh @@ -66,6 +61,14 @@ jobs: run: | pip install ".[test]" + - name: Show version and platform information + run: | + git version + python --version + python -c 'import sys; print(sys.platform)' + python -c 'import os; print(os.name)' + python -c 'import git; print(git.compat.is_win)' + - name: Check types with mypy run: | mypy -p git @@ -73,10 +76,6 @@ jobs: # so we have to ignore errors until that changes. continue-on-error: true - - name: Check 'is_win' - run: | - python -c 'import os, git; print(f"os.name={os.name}, is_win={git.compat.is_win}")' - - name: Test with pytest run: | pytest -v From bd3307ad428025e72fb66a9ca4e8231aa0917f9e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 13 Sep 2023 16:29:43 -0400 Subject: [PATCH 04/90] Make "Update PyPA packages" step clearer --- .github/workflows/pythonpackage.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 696057ae2..a311798e2 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -50,12 +50,12 @@ jobs: - name: Update PyPA packages run: | - python -m pip install --upgrade pip + python -m pip install --upgrade pip wheel + + # Python prior to 3.12 ships setuptools. Upgrade it if present. if pip freeze --all | grep --quiet '^setuptools=='; then - # Python prior to 3.12 ships setuptools. Upgrade it if present. python -m pip install --upgrade setuptools fi - python -m pip install --upgrade wheel - name: Install project and test dependencies run: | From 680d7957373c6bf193388907e3dbb770f3867ffe Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 13 Sep 2023 17:13:01 -0400 Subject: [PATCH 05/90] Show all the failures Don't stop after the first 10. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index fa06458eb..0466ed4c4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "setuptools.build_meta" [tool.pytest.ini_options] python_files = 'test_*.py' testpaths = 'test' # space separated list of paths from root e.g test tests doc/testing -addopts = '--cov=git --cov-report=term --maxfail=10 --force-sugar --disable-warnings' +addopts = '--cov=git --cov-report=term --force-sugar --disable-warnings' filterwarnings = 'ignore::DeprecationWarning' # --cov coverage # --cov-report term # send report to terminal term-missing -> terminal with line numbers html xml From 75cf5402d0d6c6712c1b0f5bd114cc9fd8780edc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 14 Sep 2023 01:39:33 -0400 Subject: [PATCH 06/90] Keep sugar for local use, but use instafail on CI There are two benefits of the pytest-sugar plugin: 1. Pretty output. 2. Show details on each failure immediately instead of at the end. The first benefit is effectively local-only, because extra newlines are appearing when it runs on CI, both with and without -v. The second benefit applies both locally and on CI. So this adds the pytest-instafail plugin and uses it on CI to get the second benefit. It is not set up to run automatically, and pytest-sugar still is (though no longer forced), so local testing retains no benefit and we don't have a clash. The name "instafail" refers only to instantly *seeing* failures: it does not cause the pytest runner to stop earlier than otherwise. --- .github/workflows/cygwin-test.yml | 2 +- .github/workflows/pythonpackage.yml | 2 +- pyproject.toml | 2 +- test-requirements.txt | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 1563afc95..cae828099 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -64,4 +64,4 @@ jobs: - name: Test with pytest run: | set +x - /usr/bin/python -m pytest -v + /usr/bin/python -m pytest -p no:sugar -v --instafail diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index a311798e2..9ac1088f7 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -78,7 +78,7 @@ jobs: - name: Test with pytest run: | - pytest -v + pytest -v -p no:sugar --instafail continue-on-error: false - name: Documentation diff --git a/pyproject.toml b/pyproject.toml index 0466ed4c4..f4fc33fec 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "setuptools.build_meta" [tool.pytest.ini_options] python_files = 'test_*.py' testpaths = 'test' # space separated list of paths from root e.g test tests doc/testing -addopts = '--cov=git --cov-report=term --force-sugar --disable-warnings' +addopts = '--cov=git --cov-report=term --disable-warnings' filterwarnings = 'ignore::DeprecationWarning' # --cov coverage # --cov-report term # send report to terminal term-missing -> terminal with line numbers html xml diff --git a/test-requirements.txt b/test-requirements.txt index b00dd6f06..1c08c736f 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -5,4 +5,5 @@ mypy pre-commit pytest pytest-cov +pytest-instafail pytest-sugar From eb56e7bdf15344739d3c2d671c1ca7dc185b8abe Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 14 Sep 2023 02:01:25 -0400 Subject: [PATCH 07/90] Pass -v twice to see full skip reasons + Reorder pytest arguments so both workflows are consistent. --- .github/workflows/cygwin-test.yml | 2 +- .github/workflows/pythonpackage.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index cae828099..8d1145e76 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -64,4 +64,4 @@ jobs: - name: Test with pytest run: | set +x - /usr/bin/python -m pytest -p no:sugar -v --instafail + /usr/bin/python -m pytest -p no:sugar --instafail -vv diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 9ac1088f7..c0402e4bb 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -78,7 +78,7 @@ jobs: - name: Test with pytest run: | - pytest -v -p no:sugar --instafail + pytest -p no:sugar --instafail -vv continue-on-error: false - name: Documentation From 9c7ff1e4918128ff28ba02cb2771b440a392644c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 14 Sep 2023 02:04:52 -0400 Subject: [PATCH 08/90] Force pytest color output on CI While pytest-sugar output gets mangled with extra newlines on CI, colorized output seems to work fine and improves readability. --- .github/workflows/cygwin-test.yml | 2 +- .github/workflows/pythonpackage.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 8d1145e76..337c0a809 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -64,4 +64,4 @@ jobs: - name: Test with pytest run: | set +x - /usr/bin/python -m pytest -p no:sugar --instafail -vv + /usr/bin/python -m pytest --color=yes -p no:sugar --instafail -vv diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index c0402e4bb..392c894bc 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -78,7 +78,7 @@ jobs: - name: Test with pytest run: | - pytest -p no:sugar --instafail -vv + pytest --color=yes -p no:sugar --instafail -vv continue-on-error: false - name: Documentation From 0eb38bcedf3703c2e3aacae27ea4cbafce33e941 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 15 Sep 2023 06:33:10 -0400 Subject: [PATCH 09/90] Fix test_blocking_lock_file for cygwin This permits the longer delay in test_blocking_lock_file--which was already allowed for native Windows--on Cygwin, where it is also needed. That lets the xfail mark for Cygwin be removed. This also updates the comments to avoid implying that the need for the delay is AppVeyor-specific (it seems needed on CI and locally). --- test/test_util.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/test/test_util.py b/test/test_util.py index 42edc57cf..308ba311b 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -12,7 +12,6 @@ from unittest import mock, skipIf from datetime import datetime -import pytest import ddt from git.cmd import dashify @@ -156,11 +155,6 @@ def test_lock_file(self): lock_file._obtain_lock_or_raise() lock_file._release_lock() - @pytest.mark.xfail( - sys.platform == "cygwin", - reason="Cygwin fails here for some reason, always", - raises=AssertionError, - ) def test_blocking_lock_file(self): my_file = tempfile.mktemp() lock_file = BlockingLockFile(my_file) @@ -173,9 +167,8 @@ def test_blocking_lock_file(self): self.assertRaises(IOError, wait_lock._obtain_lock) elapsed = time.time() - start extra_time = 0.02 - if is_win: - # for Appveyor - extra_time *= 6 # NOTE: Indeterministic failures here... + if is_win or sys.platform == "cygwin": + extra_time *= 6 # NOTE: Indeterministic failures without this... self.assertLess(elapsed, wait_time + extra_time) def test_user_id(self): From 715dba473a202ef3631b6c4bd724b8ff4e6c6d0b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 14 Sep 2023 02:51:58 -0400 Subject: [PATCH 10/90] Run cygpath tests on Cygwin, not native Windows They were not running on Cygwin, because git.util.is_win is False on Cygwin. They were running on native Windows, with a number of them always failing; these failures had sometimes been obscured by the --maxfail=10 that had formerly been used (from pyproject.toml). Many of them (not all the same ones) fail on Cygwin, and it might be valuable for cygpath to work on other platforms, especially native Windows. But I think it still makes sense to limit the tests to Cygwin at this time, because all the uses of cygpath in the project are in code that only runs after a check that the platform is Cygwin. Part of that check, as it is implemented, explicitly excludes native Windows (is_win must be false). --- test/test_util.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/test_util.py b/test/test_util.py index 308ba311b..41d874678 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -9,7 +9,7 @@ import sys import tempfile import time -from unittest import mock, skipIf +from unittest import mock, skipUnless from datetime import datetime import ddt @@ -84,14 +84,14 @@ def setup(self): "array": [42], } - @skipIf(not is_win, "Paths specifically for Windows.") + @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.idata(_norm_cygpath_pairs + _unc_cygpath_pairs) def test_cygpath_ok(self, case): wpath, cpath = case cwpath = cygpath(wpath) self.assertEqual(cwpath, cpath, wpath) - @skipIf(not is_win, "Paths specifically for Windows.") + @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.data( (r"./bar", "bar"), (r".\bar", "bar"), @@ -104,7 +104,7 @@ def test_cygpath_norm_ok(self, case): cwpath = cygpath(wpath) self.assertEqual(cwpath, cpath or wpath, wpath) - @skipIf(not is_win, "Paths specifically for Windows.") + @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.data( r"C:", r"C:Relative", @@ -117,7 +117,7 @@ def test_cygpath_invalids(self, wpath): cwpath = cygpath(wpath) self.assertEqual(cwpath, wpath.replace("\\", "/"), wpath) - @skipIf(not is_win, "Paths specifically for Windows.") + @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.idata(_norm_cygpath_pairs) def test_decygpath(self, case): wpath, cpath = case From d6a2d2807de99715ce85887b8992dbcafcefcee9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 17 Sep 2023 04:08:02 -0400 Subject: [PATCH 11/90] Mark some cygpath tests xfail Two of the groups of cygpath tests in test_util.py generate tests that fail on Cygwin. There is no easy way to still run, but xfail, just the specific tests that fail, because the groups of tests are generated with `@ddt` parameterization, but neither the unittest nor pytest xfail mechanisms interact with that. If `@pytest.mark.parametrized` were used, this could be done. But that does not work on methods of test classes that derive from unittest.TestCase, including those in this project that indirectly derive from it by deriving from TestBase. The TestBase base class cannot be removed without overhauling many tests, due to fixtures it provides such as rorepo. So this marks too many tests as xfail, but in doing so allows test runs to pass while still exercising and showing status on all the tests, allowing result changes to be observed easily. --- test/test_util.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/test_util.py b/test/test_util.py index 41d874678..f2135a272 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -13,6 +13,7 @@ from datetime import datetime import ddt +import pytest from git.cmd import dashify from git.compat import is_win @@ -84,6 +85,10 @@ def setup(self): "array": [42], } + @pytest.mark.xfail( + reason="Many return paths prefixed /proc/cygdrive instead.", + raises=AssertionError, + ) @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.idata(_norm_cygpath_pairs + _unc_cygpath_pairs) def test_cygpath_ok(self, case): @@ -91,6 +96,10 @@ def test_cygpath_ok(self, case): cwpath = cygpath(wpath) self.assertEqual(cwpath, cpath, wpath) + @pytest.mark.xfail( + reason=r'2nd example r".\bar" -> "bar" fails, returns "./bar"', + raises=AssertionError, + ) @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.data( (r"./bar", "bar"), From 881456bdceb61d51fa84ea286e6ca0e3587e8dc5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 17 Sep 2023 06:54:56 -0400 Subject: [PATCH 12/90] Run test_commit_msg_hook_success on more systems This changes a default Windows skip of test_commit_msg_hook_success to an xfail, and makes it more specific, expecting failure only when either bash.exe is unavailable (definitely expected) or when bash.exe is the WSL bash wrapper in System32, which fails for some reason even though it's not at all clear it ought to. This showcases the failures rather than skipping, and also lets the test pass on Windows systems where bash.exe is something else, including the Git Bash bash.exe that native Windows CI would use. --- test/test_index.py | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index fba9c78ec..f4858a586 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -7,10 +7,14 @@ from io import BytesIO import os +import os.path as osp +from pathlib import Path from stat import S_ISLNK, ST_MODE +import shutil import tempfile from unittest import skipIf -import shutil + +import pytest from git import ( IndexFile, @@ -23,6 +27,7 @@ GitCommandError, CheckoutError, ) +from git.cmd import Git from git.compat import is_win from git.exc import HookExecutionError, InvalidGitRepositoryError from git.index.fun import hook_path @@ -34,15 +39,22 @@ from git.util import HIDE_WINDOWS_KNOWN_ERRORS, hex_to_bin from gitdb.base import IStream -import os.path as osp -from git.cmd import Git +HOOKS_SHEBANG = "#!/usr/bin/env sh\n" -from pathlib import Path -HOOKS_SHEBANG = "#!/usr/bin/env sh\n" +def _found_in(cmd, directory): + """Check if a command is resolved in a directory (without following symlinks).""" + path = shutil.which(cmd) + return path and Path(path).parent == Path(directory) + is_win_without_bash = is_win and not shutil.which("bash.exe") +is_win_with_wsl_bash = is_win and _found_in( + cmd="bash.exe", + directory=Path(os.getenv("WINDIR")) / "System32", +) + def _make_hook(git_dir, name, content, make_exec=True): """A helper to create a hook""" @@ -910,7 +922,11 @@ def test_pre_commit_hook_fail(self, rw_repo): else: raise AssertionError("Should have caught a HookExecutionError") - @skipIf(HIDE_WINDOWS_KNOWN_ERRORS, "TODO: fix hooks execution on Windows: #703") + @pytest.mark.xfail( + is_win_without_bash or is_win_with_wsl_bash, + reason="Specifically seems to fail on WSL bash (in spite of #1399)", + raises=AssertionError, + ) @with_rw_repo("HEAD", bare=True) def test_commit_msg_hook_success(self, rw_repo): commit_message = "commit default head by Frèderic Çaufl€" From c6a586ab4817a9dcb8c8290a6a3e7071b1834f32 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 17 Sep 2023 07:07:52 -0400 Subject: [PATCH 13/90] No longer skip test_index_mutation on Cygwin As it seems to be working now on Cygwin (maybe not native Windows). --- test/test_index.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/test/test_index.py b/test/test_index.py index f4858a586..06db3aedd 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -12,7 +12,6 @@ from stat import S_ISLNK, ST_MODE import shutil import tempfile -from unittest import skipIf import pytest @@ -27,16 +26,13 @@ GitCommandError, CheckoutError, ) -from git.cmd import Git from git.compat import is_win from git.exc import HookExecutionError, InvalidGitRepositoryError from git.index.fun import hook_path from git.index.typ import BaseIndexEntry, IndexEntry from git.objects import Blob -from test.lib import TestBase, fixture_path, fixture, with_rw_repo -from test.lib import with_rw_directory -from git.util import Actor, rmtree -from git.util import HIDE_WINDOWS_KNOWN_ERRORS, hex_to_bin +from test.lib import TestBase, fixture, fixture_path, with_rw_directory, with_rw_repo +from git.util import Actor, hex_to_bin, rmtree from gitdb.base import IStream HOOKS_SHEBANG = "#!/usr/bin/env sh\n" @@ -434,14 +430,6 @@ def _count_existing(self, repo, files): # END num existing helper - @skipIf( - HIDE_WINDOWS_KNOWN_ERRORS and Git.is_cygwin(), - """FIXME: File "C:\\projects\\gitpython\\git\\test\\test_index.py", line 642, in test_index_mutation - self.assertEqual(fd.read(), link_target) - AssertionError: '!\xff\xfe/\x00e\x00t\x00c\x00/\x00t\x00h\x00a\x00t\x00\x00\x00' - != '/etc/that' - """, - ) @with_rw_repo("0.1.6") def test_index_mutation(self, rw_repo): index = rw_repo.index From fc022304d2f4bc8770f75b0c5fc289dccab0ae5b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 24 Sep 2023 02:27:31 -0400 Subject: [PATCH 14/90] Report encoding error in test_add_unicode as error This makes the test explicitly error out, rather than skipping, if it appears the environment doesn't support encoding Unicode filenames. Platforms these days should be capable of that, and reporting it as an error lessens the risk of missing a bug in the test code (that method or a fixture) if one is ever introduced. Erroring out will also make it easier to see the details in the chained UnicodeDecodeError exception. This does not affect the behavior of GitPython itself. It only changes how a test reports an unusual condition that keeps the test\ from being usefully run. --- test/test_base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_base.py b/test/test_base.py index b77c8117d..90e701c4b 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -7,7 +7,7 @@ import os import sys import tempfile -from unittest import SkipTest, skipIf +from unittest import skipIf from git import Repo from git.objects import Blob, Tree, Commit, TagObject @@ -126,7 +126,7 @@ def test_add_unicode(self, rw_repo): try: file_path.encode(sys.getfilesystemencoding()) except UnicodeEncodeError as e: - raise SkipTest("Environment doesn't support unicode filenames") from e + raise RuntimeError("Environment doesn't support unicode filenames") from e with open(file_path, "wb") as fp: fp.write(b"something") From 203da23e5fe2ae5a0ae13d0f9b2a276ae584ea7b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 24 Sep 2023 03:19:23 -0400 Subject: [PATCH 15/90] Add a few FIXMEs re: better use of xfail --- test/test_config.py | 1 + test/test_util.py | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test/test_config.py b/test/test_config.py index 481e129c6..f805570d5 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -100,6 +100,7 @@ def test_includes_order(self): # values must be considered as soon as they get them assert r_config.get_value("diff", "tool") == "meld" try: + # FIXME: Split this assertion out somehow and mark it xfail (or fix it). assert r_config.get_value("sec", "var1") == "value1_main" except AssertionError as e: raise SkipTest("Known failure -- included values are not in effect right away") from e diff --git a/test/test_util.py b/test/test_util.py index f2135a272..2b1e518ed 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -85,6 +85,7 @@ def setup(self): "array": [42], } + # FIXME: Mark only the /proc-prefixing cases xfail, somehow (or fix them). @pytest.mark.xfail( reason="Many return paths prefixed /proc/cygdrive instead.", raises=AssertionError, @@ -103,7 +104,7 @@ def test_cygpath_ok(self, case): @skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.") @ddt.data( (r"./bar", "bar"), - (r".\bar", "bar"), + (r".\bar", "bar"), # FIXME: Mark only this one xfail, somehow (or fix it). (r"../bar", "../bar"), (r"..\bar", "../bar"), (r"../bar/.\foo/../chu", "../bar/chu"), From cf5f1dca139b809a744badb9f9740dd6d0a70c56 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 03:21:05 -0400 Subject: [PATCH 16/90] Report <2.5.1 in test_linked_worktree_traversal as error Although GitPython does not require git >=2.5.1 in general, and this does *not* change that, this makes the unavailability of git 2.5.1 or later an error in test_linked_worktree_traversal, where it is needed to exercise that test, rather than skipping the test. A few systems, such as CentOS 7, may have downstream patched versions of git that remain safe to use yet are numbered <2.5.1 and do not have the necesary feature to run this test. But by now, users of those systems likely anticipate that other software would rely on the presence of features added in git 2.5.1, which was released over 7 years ago. As such, I think it is more useful to give an error for that test, so the test's inability to be run on the system is clear, than to automatically skip the test, which is likely to go unnoticed. --- test/test_fun.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/test_fun.py b/test/test_fun.py index d76e189ed..f39955aa0 100644 --- a/test/test_fun.py +++ b/test/test_fun.py @@ -2,7 +2,6 @@ from stat import S_IFDIR, S_IFREG, S_IFLNK, S_IXUSR from os import stat import os.path as osp -from unittest import SkipTest from git import Git from git.index import IndexFile @@ -279,7 +278,7 @@ def test_linked_worktree_traversal(self, rw_dir): """Check that we can identify a linked worktree based on a .git file""" git = Git(rw_dir) if git.version_info[:3] < (2, 5, 1): - raise SkipTest("worktree feature unsupported") + raise RuntimeError("worktree feature unsupported (test needs git 2.5.1 or later)") rw_master = self.rorepo.clone(join_path_native(rw_dir, "master_repo")) branch = rw_master.create_head("aaaaaaaa") From 89232365fb0204c32d1f7c23b9eb39fe4401eb7f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 04:13:36 -0400 Subject: [PATCH 17/90] Change skipIf(not ...) to skipUnless(...) --- test/test_submodule.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 0aa80e5ce..432c02686 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -7,7 +7,7 @@ import tempfile from pathlib import Path import sys -from unittest import mock, skipIf +from unittest import mock, skipIf, skipUnless import pytest @@ -1039,7 +1039,7 @@ def test_branch_renames(self, rw_dir): assert sm_mod.commit() == sm_pfb.commit, "Now head should have been reset" assert sm_mod.head.ref.name == sm_pfb.name - @skipIf(not is_win, "Specifically for Windows.") + @skipUnless(is_win, "Specifically for Windows.") def test_to_relative_path_with_super_at_root_drive(self): class Repo(object): working_tree_dir = "D:\\" From b198bf1e7d9c9d9db675c6c4e94d2f136a0a7923 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 05:43:00 -0400 Subject: [PATCH 18/90] Express known test_depth failure with xfail Rather than skipping, so it becomes known if the situation changes. --- test/test_submodule.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 432c02686..54b1e796b 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -1050,9 +1050,8 @@ class Repo(object): msg = '_to_relative_path should be "submodule_path" but was "%s"' % relative_path assert relative_path == "submodule_path", msg - @skipIf( - True, - "for some unknown reason the assertion fails, even though it in fact is working in more common setup", + @pytest.mark.xfail( + reason="for some unknown reason the assertion fails, even though it in fact is working in more common setup", ) @with_rw_directory def test_depth(self, rwdir): From cd175a598ed457833bc06adba776e2bbb1d9014b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 07:09:43 -0400 Subject: [PATCH 19/90] Remove no-effect `@skipIf` on test_untracked_files It looked like test_untracked_files was sometimes skipped, and specifically that it would be skipped on Cygwin. But the `@skipIf` on it had the condition: HIDE_WINDOWS_KNOWN_ERRORS and Git.is_cygwin() HIDE_WINDOWS_KNOWN_ERRORS can only ever be true if it is set to a truthy value directly (not an intended use as it's a "constant"), or on native Windows systems: no matter how the environment variable related to it is set, it's only checked if is_win, which is set by checking os.name, which is only "nt" on native Windows systems, not Cygwin. So whenever HIDE_WINDOWS_KNOWN_ERRORS is true Git.is_cygwin() will be false. Thus this condition is never true and the test was never being skipped anyway: it was running and passing on Cygwin. --- test/test_repo.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/test/test_repo.py b/test/test_repo.py index 15899ec50..fe3419e33 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -13,7 +13,7 @@ import pickle import sys import tempfile -from unittest import mock, skipIf, SkipTest, skip +from unittest import mock, SkipTest, skip import pytest @@ -42,7 +42,7 @@ ) from git.repo.fun import touch from test.lib import TestBase, with_rw_repo, fixture -from git.util import HIDE_WINDOWS_KNOWN_ERRORS, cygpath +from git.util import cygpath from test.lib import with_rw_directory from git.util import join_path_native, rmtree, rmfile, bin_to_hex @@ -764,16 +764,6 @@ def test_blame_accepts_rev_opts(self, git): self.rorepo.blame("HEAD", "README.md", rev_opts=["-M", "-C", "-C"]) git.assert_called_once_with(*expected_args, **boilerplate_kwargs) - @skipIf( - HIDE_WINDOWS_KNOWN_ERRORS and Git.is_cygwin(), - """FIXME: File "C:\\projects\\gitpython\\git\\cmd.py", line 671, in execute - raise GitCommandError(command, status, stderr_value, stdout_value) - GitCommandError: Cmd('git') failed due to: exit code(128) - cmdline: git add 1__��ava verb��ten 1_test _myfile 1_test_other_file - 1_��ava-----verb��ten - stderr: 'fatal: pathspec '"1__çava verböten"' did not match any files' - """, - ) @with_rw_repo("HEAD", bare=False) def test_untracked_files(self, rwrepo): for run, repo_add in enumerate((rwrepo.index.add, rwrepo.git.add)): From f38cc000fe4d51f0f9d1bdedec86f6fcdb57f359 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 07:32:46 -0400 Subject: [PATCH 20/90] Make 2 more too-low git version skips into errors In the tests only, and not in any way affecting the feature set or requirements of GitPython itself. This is similar to, and with the same reasoning as, cf5f1dc. --- test/test_repo.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_repo.py b/test/test_repo.py index fe3419e33..4257d8a47 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -13,7 +13,7 @@ import pickle import sys import tempfile -from unittest import mock, SkipTest, skip +from unittest import mock, skip import pytest @@ -1235,7 +1235,7 @@ def test_merge_base(self): def test_is_ancestor(self): git = self.rorepo.git if git.version_info[:3] < (1, 8, 0): - raise SkipTest("git merge-base --is-ancestor feature unsupported") + raise RuntimeError("git merge-base --is-ancestor feature unsupported (test needs git 1.8.0 or later)") repo = self.rorepo c1 = "f6aa8d1" @@ -1283,7 +1283,7 @@ def test_git_work_tree_dotgit(self, rw_dir): based on it.""" git = Git(rw_dir) if git.version_info[:3] < (2, 5, 1): - raise SkipTest("worktree feature unsupported") + raise RuntimeError("worktree feature unsupported (test needs git 2.5.1 or later)") rw_master = self.rorepo.clone(join_path_native(rw_dir, "master_repo")) branch = rw_master.create_head("aaaaaaaa") From 8fd56e78366470e0b07db48daf623b188e7245ea Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 08:03:58 -0400 Subject: [PATCH 21/90] Update test_root_module Windows skip reason The current cause of failure is different from what is documented in the skip reason. --- test/test_submodule.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 54b1e796b..4c087caa1 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -477,11 +477,11 @@ def test_base_bare(self, rwrepo): @skipIf( HIDE_WINDOWS_KNOWN_ERRORS, """ - File "C:\\projects\\gitpython\\git\\cmd.py", line 559, in execute - raise GitCommandNotFound(command, err) - git.exc.GitCommandNotFound: Cmd('git') not found due to: OSError('[WinError 6] The handle is invalid') - cmdline: git clone -n --shared -v C:\\projects\\gitpython\\.git Users\\appveyor\\AppData\\Local\\Temp\\1\\tmplyp6kr_rnon_bare_test_root_module - """, # noqa E501 + E PermissionError: + [WinError 32] The process cannot access the file because it is being used by another process: + 'C:\\Users\\ek\\AppData\\Local\\Temp\\non_bare_test_root_modulep0eqt8_r\\git\\ext\\gitdb' + -> 'C:\\Users\\ek\\AppData\\Local\\Temp\\non_bare_test_root_modulep0eqt8_r\\path\\prefix\\git\\ext\\gitdb' + """, ) @with_rw_repo(k_subm_current, bare=False) def test_root_module(self, rwrepo): From c1798f5ead60f74de422732d5229244d00325f24 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 08:06:45 -0400 Subject: [PATCH 22/90] Change test_root_module Windows skip to xfail And rewrite the reason to give more useful information. (The new reason also doesn't state the exception type, because that is now specified, and checked by pytest, by being passed as "raises".) --- test/test_submodule.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 4c087caa1..256eb7dbf 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -474,14 +474,13 @@ def test_base_bare(self, rwrepo): reason="Cygwin GitPython can't find submodule SHA", raises=ValueError, ) - @skipIf( + @pytest.mark.xfail( HIDE_WINDOWS_KNOWN_ERRORS, - """ - E PermissionError: - [WinError 32] The process cannot access the file because it is being used by another process: - 'C:\\Users\\ek\\AppData\\Local\\Temp\\non_bare_test_root_modulep0eqt8_r\\git\\ext\\gitdb' - -> 'C:\\Users\\ek\\AppData\\Local\\Temp\\non_bare_test_root_modulep0eqt8_r\\path\\prefix\\git\\ext\\gitdb' - """, + reason=( + '"The process cannot access the file because it is being used by another process"' + + " on first call to rm.update" + ), + raises=PermissionError, ) @with_rw_repo(k_subm_current, bare=False) def test_root_module(self, rwrepo): From ba567521a5a01b93420cab4021d5663af66231ae Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 08:16:05 -0400 Subject: [PATCH 23/90] Update test_git_submodules_and_add_sm_with_new_commit skip reason This is working on Cygwin, so that old reason no longer applies. (The test was not being skipped on Cygwin, and was passing.) It is not working on native Windows, due to a PermissionError from attempting to move a file that is still open (which Windows doesn't allow). That may have been the original native Windows skip reason, but the old AppVeyor CI link for it is broken or not public. This makes the reason clear, though maybe I should add more details. --- test/test_submodule.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 256eb7dbf..25b0daa01 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -750,13 +750,12 @@ def test_list_only_valid_submodules(self, rwdir): @skipIf( HIDE_WINDOWS_KNOWN_ERRORS, - """FIXME on cygwin: File "C:\\projects\\gitpython\\git\\cmd.py", line 671, in execute - raise GitCommandError(command, status, stderr_value, stdout_value) - GitCommandError: Cmd('git') failed due to: exit code(128) - cmdline: git add 1__Xava verbXXten 1_test _myfile 1_test_other_file 1_XXava-----verbXXten - stderr: 'fatal: pathspec '"1__çava verböten"' did not match any files' - FIXME on appveyor: see https://ci.appveyor.com/project/Byron/gitpython/build/1.0.185 - """, + """ + E PermissionError: + [WinError 32] The process cannot access the file because it is being used by another process: + 'C:\\Users\\ek\\AppData\\Local\\Temp\\test_git_submodules_and_add_sm_with_new_commitu6d08von\\parent\\module' + -> 'C:\\Users\\ek\\AppData\\Local\\Temp\\test_git_submodules_and_add_sm_with_new_commitu6d08von\\parent\\module_moved' + """ # noqa: E501, ) @with_rw_directory @_patch_git_config("protocol.file.allow", "always") From 8704d1b81ba080cd4baa74968ac4cf84a84e4cbe Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 08:22:32 -0400 Subject: [PATCH 24/90] Change test_git_submodules_and_add_sm_with_new_commit Windows skip to xfail And improve details. The xfail is only for native Windows, not Cygwin (same as the old skip was, and still via checking HIDE_WINDOWS_KNOWN_ERRORS). This change is analogous to the change in c1798f5, but for test_git_submodules_and_add_sm_with_new_commit rather than test_root_module. --- test/test_submodule.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index 25b0daa01..72d7255b0 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -7,7 +7,7 @@ import tempfile from pathlib import Path import sys -from unittest import mock, skipIf, skipUnless +from unittest import mock, skipUnless import pytest @@ -748,14 +748,13 @@ def test_list_only_valid_submodules(self, rwdir): repo = git.Repo(repo_path) assert len(repo.submodules) == 0 - @skipIf( + @pytest.mark.xfail( HIDE_WINDOWS_KNOWN_ERRORS, - """ - E PermissionError: - [WinError 32] The process cannot access the file because it is being used by another process: - 'C:\\Users\\ek\\AppData\\Local\\Temp\\test_git_submodules_and_add_sm_with_new_commitu6d08von\\parent\\module' - -> 'C:\\Users\\ek\\AppData\\Local\\Temp\\test_git_submodules_and_add_sm_with_new_commitu6d08von\\parent\\module_moved' - """ # noqa: E501, + reason=( + '"The process cannot access the file because it is being used by another process"' + + " on first call to sm.move" + ), + raises=PermissionError, ) @with_rw_directory @_patch_git_config("protocol.file.allow", "always") From 1d6abdca134082bf0bce2e590a52d8c08bf04d9b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 08:34:54 -0400 Subject: [PATCH 25/90] Run the tests in test_tree on Windows This stops skipping them, as they are now working. --- test/test_tree.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/test/test_tree.py b/test/test_tree.py index e59705645..c5ac8d539 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -5,24 +5,14 @@ # the BSD License: https://opensource.org/license/bsd-3-clause/ from io import BytesIO -from unittest import skipIf from git.objects import Tree, Blob from test.lib import TestBase -from git.util import HIDE_WINDOWS_KNOWN_ERRORS import os.path as osp class TestTree(TestBase): - @skipIf( - HIDE_WINDOWS_KNOWN_ERRORS, - """ - File "C:\\projects\\gitpython\\git\\cmd.py", line 559, in execute - raise GitCommandNotFound(command, err) - git.exc.GitCommandNotFound: Cmd('git') not found due to: OSError('[WinError 6] The handle is invalid') - cmdline: git cat-file --batch-check""", - ) def test_serializable(self): # tree at the given commit contains a submodule as well roottree = self.rorepo.tree("6c1faef799095f3990e9970bc2cb10aa0221cf9c") @@ -51,14 +41,6 @@ def test_serializable(self): testtree._deserialize(stream) # END for each item in tree - @skipIf( - HIDE_WINDOWS_KNOWN_ERRORS, - """ - File "C:\\projects\\gitpython\\git\\cmd.py", line 559, in execute - raise GitCommandNotFound(command, err) - git.exc.GitCommandNotFound: Cmd('git') not found due to: OSError('[WinError 6] The handle is invalid') - cmdline: git cat-file --batch-check""", - ) def test_traverse(self): root = self.rorepo.tree("0.1.6") num_recursive = 0 From 5609faa5a1c22654dfc007f7bf229e1b08087aa8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 09:01:31 -0400 Subject: [PATCH 26/90] Add missing raises keyword for test_depth xfail I had forgotten to do this earlier when converting from skip to xfail. Besides consistency with the other uses of xfail in the test suite, the benefit of passing "raises" is that pytest checks that the failure gave the expected exception and makes it a non-expected failure if it didn't. --- test/test_submodule.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/test_submodule.py b/test/test_submodule.py index 72d7255b0..79ff2c5f2 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -1049,6 +1049,7 @@ class Repo(object): @pytest.mark.xfail( reason="for some unknown reason the assertion fails, even though it in fact is working in more common setup", + raises=AssertionError, ) @with_rw_directory def test_depth(self, rwdir): From ed95e8e72f6aa697c1ec69595335168e717da013 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 09:07:53 -0400 Subject: [PATCH 27/90] Consolidate test_repo module import statements --- test/test_repo.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/test_repo.py b/test/test_repo.py index 4257d8a47..364b895fb 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -41,10 +41,8 @@ UnsafeProtocolError, ) from git.repo.fun import touch -from test.lib import TestBase, with_rw_repo, fixture -from git.util import cygpath -from test.lib import with_rw_directory -from git.util import join_path_native, rmtree, rmfile, bin_to_hex +from git.util import bin_to_hex, cygpath, join_path_native, rmfile, rmtree +from test.lib import TestBase, fixture, with_rw_directory, with_rw_repo import os.path as osp From ceb4dd3a7e89ac281a6c0d4d815fc13c00cbdf9f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 10:33:23 -0400 Subject: [PATCH 28/90] Show more CI system information --- .github/workflows/cygwin-test.yml | 1 + .github/workflows/pythonpackage.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 337c0a809..d0be6bc39 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -55,6 +55,7 @@ jobs: - name: Show version and platform information run: | + /usr/bin/uname -a /usr/bin/git version /usr/bin/python --version /usr/bin/python -c 'import sys; print(sys.platform)' diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 392c894bc..0dc9d217a 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -63,6 +63,7 @@ jobs: - name: Show version and platform information run: | + uname -a git version python --version python -c 'import sys; print(sys.platform)' From 3276aac711186a5dd0bd74ba1be8bb6f4ad3d03a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 10:49:50 -0400 Subject: [PATCH 29/90] Use Cygwin's bash and git for more CI steps --- .github/workflows/cygwin-test.yml | 34 +++++++++++++++++------------ .github/workflows/pythonpackage.yml | 1 + 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index d0be6bc39..2269df0da 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -14,11 +14,13 @@ jobs: TEMP: "/tmp" defaults: run: - shell: bash.exe --noprofile --norc -exo pipefail -o igncr "{0}" + shell: C:\cygwin\bin\bash.exe --noprofile --norc -exo pipefail -o igncr "{0}" steps: - name: Force LF line endings - run: git config --global core.autocrlf input + run: | + git config --global core.autocrlf input + shell: bash - uses: actions/checkout@v4 with: @@ -29,9 +31,12 @@ jobs: with: packages: python39 python39-pip python39-virtualenv git + - name: Limit $PATH to Cygwin + run: echo 'C:\cygwin\bin' >"$GITHUB_PATH" + - name: Tell git to trust this repo run: | - /usr/bin/git config --global --add safe.directory "$(pwd)" + git config --global --add safe.directory "$(pwd)" - name: Prepare this repo for tests run: | @@ -39,30 +44,31 @@ jobs: - name: Further prepare git configuration for tests run: | - /usr/bin/git config --global user.email "travis@ci.com" - /usr/bin/git config --global user.name "Travis Runner" + git config --global user.email "travis@ci.com" + git config --global user.name "Travis Runner" # If we rewrite the user's config by accident, we will mess it up # and cause subsequent tests to fail cat test/fixtures/.gitconfig >> ~/.gitconfig - name: Update PyPA packages run: | - /usr/bin/python -m pip install --upgrade pip setuptools wheel + python -m pip install --upgrade pip setuptools wheel - name: Install project and test dependencies run: | - /usr/bin/python -m pip install ".[test]" + python -m pip install ".[test]" - name: Show version and platform information run: | - /usr/bin/uname -a - /usr/bin/git version - /usr/bin/python --version - /usr/bin/python -c 'import sys; print(sys.platform)' - /usr/bin/python -c 'import os; print(os.name)' - /usr/bin/python -c 'import git; print(git.compat.is_win)' + uname -a + command -v git python + git version + python --version + python -c 'import sys; print(sys.platform)' + python -c 'import os; print(os.name)' + python -c 'import git; print(git.compat.is_win)' - name: Test with pytest run: | set +x - /usr/bin/python -m pytest --color=yes -p no:sugar --instafail -vv + python -m pytest --color=yes -p no:sugar --instafail -vv diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 0dc9d217a..78d3ddf86 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -64,6 +64,7 @@ jobs: - name: Show version and platform information run: | uname -a + command -v git python git version python --version python -c 'import sys; print(sys.platform)' From 5d4097654c6498d56defcc98dc1611fbfba9b75a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 12:02:35 -0400 Subject: [PATCH 30/90] Try to work in all LF on Cygwin CI + Style tweak and comment to clarify the "Limit $PATH" step. --- .github/workflows/cygwin-test.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 2269df0da..b8f3efbba 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -9,7 +9,6 @@ jobs: fail-fast: false env: CHERE_INVOKING: 1 - SHELLOPTS: igncr TMP: "/tmp" TEMP: "/tmp" defaults: @@ -19,7 +18,7 @@ jobs: steps: - name: Force LF line endings run: | - git config --global core.autocrlf input + git config --global core.autocrlf false # Affects the non-Cygwin git. shell: bash - uses: actions/checkout@v4 @@ -32,11 +31,13 @@ jobs: packages: python39 python39-pip python39-virtualenv git - name: Limit $PATH to Cygwin - run: echo 'C:\cygwin\bin' >"$GITHUB_PATH" + run: | + echo 'C:\cygwin\bin' > "$GITHUB_PATH" # Overwrite it with just this. - - name: Tell git to trust this repo + - name: Special configuration for Cygwin's git run: | git config --global --add safe.directory "$(pwd)" + git config --global core.autocrlf false - name: Prepare this repo for tests run: | @@ -70,5 +71,4 @@ jobs: - name: Test with pytest run: | - set +x python -m pytest --color=yes -p no:sugar --instafail -vv From dda428640113d6a2c30225ea33f1387e784b7289 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Sep 2023 13:42:29 -0400 Subject: [PATCH 31/90] Consistent formatting style across all workflows --- .github/workflows/cygwin-test.yml | 3 +++ .github/workflows/lint.yml | 12 +++++++----- .github/workflows/pythonpackage.yml | 3 ++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index b8f3efbba..3cca4dd5f 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -5,12 +5,15 @@ on: [push, pull_request, workflow_dispatch] jobs: build: runs-on: windows-latest + strategy: fail-fast: false + env: CHERE_INVOKING: 1 TMP: "/tmp" TEMP: "/tmp" + defaults: run: shell: C:\cygwin\bin\bash.exe --noprofile --norc -exo pipefail -o igncr "{0}" diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5e79664a8..2204bb792 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -7,8 +7,10 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 - - uses: actions/setup-python@v4 - with: - python-version: "3.x" - - uses: pre-commit/action@v3.0.0 + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v4 + with: + python-version: "3.x" + + - uses: pre-commit/action@v3.0.0 diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 78d3ddf86..e9ccdd566 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -10,8 +10,8 @@ permissions: jobs: build: - runs-on: ubuntu-latest + strategy: fail-fast: false matrix: @@ -20,6 +20,7 @@ jobs: - experimental: false - python-version: "3.12" experimental: true + defaults: run: shell: /bin/bash --noprofile --norc -exo pipefail {0} From 3007abc6d229bcfe6643963f648597b7e231ab3d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Sep 2023 00:01:32 -0400 Subject: [PATCH 32/90] Remove the recently added "Limit $PATH" step I had put that step in the Cygwin workflow for purposes of experimentation, and it seemed to make clearer what is going on, but really it does the opposite: it's deceptive because Cygwin uses other logic to set its PATH. So this step is unnecessary and ineffective at doing what it appears to do. --- .github/workflows/cygwin-test.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 3cca4dd5f..35e9fde72 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -33,10 +33,6 @@ jobs: with: packages: python39 python39-pip python39-virtualenv git - - name: Limit $PATH to Cygwin - run: | - echo 'C:\cygwin\bin' > "$GITHUB_PATH" # Overwrite it with just this. - - name: Special configuration for Cygwin's git run: | git config --global --add safe.directory "$(pwd)" From 4860f701b96dc07ac7c489c74c06cae069ae3cd1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Sep 2023 00:14:29 -0400 Subject: [PATCH 33/90] Further reduce differences between test workflows This makes the two CI test workflows more similar in a couple of the remaining ways they differ unnecessarily. This could be extended, and otherwise improved upon, in the future. --- .github/workflows/cygwin-test.yml | 5 +++-- .github/workflows/pythonpackage.yml | 10 +++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 35e9fde72..e818803f1 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -42,7 +42,7 @@ jobs: run: | TRAVIS=yes ./init-tests-after-clone.sh - - name: Further prepare git configuration for tests + - name: Set git user identity and command aliases for the tests run: | git config --global user.email "travis@ci.com" git config --global user.name "Travis Runner" @@ -52,7 +52,8 @@ jobs: - name: Update PyPA packages run: | - python -m pip install --upgrade pip setuptools wheel + # Get the latest pip, wheel, and prior to Python 3.12, setuptools. + python -m pip install -U pip $(pip freeze --all | grep -oF setuptools) wheel - name: Install project and test dependencies run: | diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index e9ccdd566..1b049ba02 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -41,7 +41,7 @@ jobs: run: | TRAVIS=yes ./init-tests-after-clone.sh - - name: Prepare git configuration for tests + - name: Set git user identity and command aliases for the tests run: | git config --global user.email "travis@ci.com" git config --global user.name "Travis Runner" @@ -51,12 +51,8 @@ jobs: - name: Update PyPA packages run: | - python -m pip install --upgrade pip wheel - - # Python prior to 3.12 ships setuptools. Upgrade it if present. - if pip freeze --all | grep --quiet '^setuptools=='; then - python -m pip install --upgrade setuptools - fi + # Get the latest pip, wheel, and prior to Python 3.12, setuptools. + python -m pip install -U pip $(pip freeze --all | grep -oF setuptools) wheel - name: Install project and test dependencies run: | From 13b859745de3f5a610e9e6390d55da40c582e663 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Sep 2023 23:41:02 -0400 Subject: [PATCH 34/90] Fix new link to license in readme --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index dbec36024..c7c2992e3 100644 --- a/README.md +++ b/README.md @@ -267,7 +267,7 @@ gpg --edit-key 4C08421980C9 ### LICENSE -[New BSD License](https://opensource.org/license/bsd-3-clause/). See the [LICENSE file](https://github.com/gitpython-developers/GitPython/blob/main/license). +[New BSD License](https://opensource.org/license/bsd-3-clause/). See the [LICENSE file][license]. [contributing]: https://github.com/gitpython-developers/GitPython/blob/main/CONTRIBUTING.md -[license]: https://github.com/gitpython-developers/GitPython/blob/main/license +[license]: https://github.com/gitpython-developers/GitPython/blob/main/LICENSE From 388c7d1d70c2dabe1fe5e5eb1d453edd8f40a49d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 00:05:08 -0400 Subject: [PATCH 35/90] Clearer YAML style for flake8 extra plugin list --- .pre-commit-config.yaml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5a34b8af0..67aefb342 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -4,11 +4,9 @@ repos: hooks: - id: flake8 additional_dependencies: - [ - flake8-bugbear==23.9.16, - flake8-comprehensions==3.14.0, - flake8-typing-imports==1.14.0, - ] + - flake8-bugbear==23.9.16 + - flake8-comprehensions==3.14.0 + - flake8-typing-imports==1.14.0 exclude: ^doc|^git/ext/ - repo: https://github.com/pre-commit/pre-commit-hooks From 1a8f210a42c3f9c46ce3cfa9c25ea39b0a8ca6c6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 02:40:11 -0400 Subject: [PATCH 36/90] Drop flake8 suppressions that are no longer needed + Remove the comments that documented those old suppressions + Format the .flake8 file more readably --- .flake8 | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/.flake8 b/.flake8 index ed5d036bf..1cc049a69 100644 --- a/.flake8 +++ b/.flake8 @@ -1,38 +1,26 @@ [flake8] + show-source = True -count= True +count = True statistics = True -# E265 = comment blocks like @{ section, which it can't handle + # E266 = too many leading '#' for block comment # E731 = do not assign a lambda expression, use a def -# W293 = Blank line contains whitespace -# W504 = Line break after operator -# E704 = multiple statements in one line - used for @override # TC002 = move third party import to TYPE_CHECKING -# ANN = flake8-annotations # TC, TC2 = flake8-type-checking -# D = flake8-docstrings # select = C,E,F,W ANN, TC, TC2 # to enable code. Disabled if not listed, including builtin codes enable-extensions = TC, TC2 # only needed for extensions not enabled by default -ignore = E265,E266,E731,E704, - W293, W504, - ANN0 ANN1 ANN2, - TC002, - TC0, TC1, TC2 - # B, - A, - D, - RST, RST3 +ignore = E266, E731 -exclude = .tox,.venv,build,dist,doc,git/ext/ +exclude = .tox, .venv, build, dist, doc, git/ext/ rst-roles = # for flake8-RST-docstrings - attr,class,func,meth,mod,obj,ref,term,var # used by sphinx + attr, class, func, meth, mod, obj, ref, term, var # used by sphinx min-python-version = 3.7.0 # for `black` compatibility max-line-length = 120 -extend-ignore = E203,W503 +extend-ignore = E203, W503 From e07d91a6f67c1c4adb897096cb13d6cd1b98cf42 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Oct 2023 15:31:00 -0400 Subject: [PATCH 37/90] Drop claim about Cygwin not having git-daemon On a current Cygwin system with git 2.39.0 (the latest version offered by the Cygwin package manager), git-daemon is present, with the Cygwin path /usr/libexec/git-core/git-daemon.exe. In addition, the cygwin-test.yml workflow does not take any special steps to allow git-daemon to work, but all tests pass in it even without skipping or xfailing tests that seem related to git-daemon. The git_daemon_launched function in test/lib/helper.py only invokes git-daemon directly (rather than through "git daemon") when is_win evaluates true, which only happens on native Windows systems, not Cygwin, which is treated the same as (other) Unix-like systems and still works. So maybe Cygwin git-daemon was never a special case. Whether or not it was, the message about git-daemon needing to be findable in a PATH search is also under an is_win check, and thus is never shown on Cygwin. So I've removed the Cygwin part of that message. (Because the path shown is a MinGW-style path, I have kept the wording about that being for MinGW-git, even though it is no longer needed to disambiguate the Cygwin case.) --- README.md | 3 +-- test/lib/helper.py | 15 ++++++--------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index c7c2992e3..65c1e7bae 100644 --- a/README.md +++ b/README.md @@ -119,8 +119,7 @@ executed `git fetch --tags` followed by the `./init-tests-after-clone.sh` script in the repository root. Otherwise you will encounter test failures. On _Windows_, make sure you have `git-daemon` in your PATH. For MINGW-git, the `git-daemon.exe` -exists in `Git\mingw64\libexec\git-core\`; CYGWIN has no daemon, but should get along fine -with MINGW's. +exists in `Git\mingw64\libexec\git-core\`. #### Install test dependencies diff --git a/test/lib/helper.py b/test/lib/helper.py index e8464b7d4..9656345de 100644 --- a/test/lib/helper.py +++ b/test/lib/helper.py @@ -177,12 +177,10 @@ def git_daemon_launched(base_path, ip, port): gd = None try: if is_win: - ## On MINGW-git, daemon exists in .\Git\mingw64\libexec\git-core\, - # but if invoked as 'git daemon', it detaches from parent `git` cmd, - # and then CANNOT DIE! - # So, invoke it as a single command. - ## Cygwin-git has no daemon. But it can use MINGW's. - # + # On MINGW-git, daemon exists in Git\mingw64\libexec\git-core\, + # but if invoked as 'git daemon', it detaches from parent `git` cmd, + # and then CANNOT DIE! + # So, invoke it as a single command. daemon_cmd = [ "git-daemon", "--enable=receive-pack", @@ -217,12 +215,11 @@ def git_daemon_launched(base_path, ip, port): ) if is_win: msg += textwrap.dedent( - r""" + R""" On Windows, the `git-daemon.exe` must be in PATH. - For MINGW, look into .\Git\mingw64\libexec\git-core\), but problems with paths might appear. - CYGWIN has no daemon, but if one exists, it gets along fine (but has also paths problems).""" + For MINGW, look into \Git\mingw64\libexec\git-core\, but problems with paths might appear.""" ) log.warning(msg, ex, ip, port, base_path, base_path, exc_info=1) From 35e3875cb71913771885646f294cd25cd19c35f3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Oct 2023 17:06:47 -0400 Subject: [PATCH 38/90] Allow base_daemon_path to be normalized for Cygwin Since the Cygwin git-daemon can be used. --- test/lib/helper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lib/helper.py b/test/lib/helper.py index 9656345de..1de904610 100644 --- a/test/lib/helper.py +++ b/test/lib/helper.py @@ -302,7 +302,7 @@ def remote_repo_creator(self): cw.set("url", remote_repo_url) with git_daemon_launched( - Git.polish_url(base_daemon_path, is_cygwin=False), # No daemon in Cygwin. + Git.polish_url(base_daemon_path), "127.0.0.1", GIT_DAEMON_PORT, ): From 5e71c270b2ea0adfc5c0a103fce33ab6acf275b1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Oct 2023 22:43:36 -0400 Subject: [PATCH 39/90] Fix the name of the "executes git" test That test is not testing whether or not a shell is used (nor does it need to test that), but just whether the library actually runs git, passes an argument to it, and returns text from its stdout. --- test/test_git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_git.py b/test/test_git.py index 481309538..a21a3c78e 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -73,7 +73,7 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): res = self.git.transform_kwargs(**{"s": True, "t": True}) self.assertEqual({"-s", "-t"}, set(res)) - def test_it_executes_git_to_shell_and_returns_result(self): + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") def test_it_executes_git_not_from_cwd(self): From 59440607406873a28788ca38526871749c5549f9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 00:17:05 -0400 Subject: [PATCH 40/90] Test whether a shell is used In the Popen calls in Git.execute, for all combinations of allowed values for Git.USE_SHELL and the shell= keyword argument. --- test/test_git.py | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index a21a3c78e..6fd2c8268 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -4,23 +4,24 @@ # # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ +import contextlib import os +import os.path as osp import shutil import subprocess import sys from tempfile import TemporaryDirectory, TemporaryFile from unittest import mock, skipUnless -from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd -from test.lib import TestBase, fixture_path -from test.lib import with_rw_directory -from git.util import cwd, finalize_process - -import os.path as osp +import ddt +from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd from git.compat import is_win +from git.util import cwd, finalize_process +from test.lib import TestBase, fixture_path, with_rw_directory +@ddt.ddt class TestGit(TestBase): @classmethod def setUpClass(cls): @@ -73,6 +74,28 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): res = self.git.transform_kwargs(**{"s": True, "t": True}) self.assertEqual({"-s", "-t"}, set(res)) + @ddt.data( + (None, False, False), + (None, True, True), + (False, True, False), + (False, False, False), + (True, False, True), + (True, True, True), + ) + @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. + def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): + """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" + value_in_call, value_from_class, expected_popen_arg = case + # FIXME: Check what gets logged too! + with mock.patch.object(Git, "USE_SHELL", value_from_class): + with contextlib.suppress(GitCommandError): + self.git.execute( + "git", # No args, so it runs with or without a shell, on all OSes. + shell=value_in_call, + ) + mock_popen.assert_called_once() + self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") From aa5e2f6b24e36d2d38e84e7f2241b104318396c3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 01:45:17 -0400 Subject: [PATCH 41/90] Test if whether a shell is used is logged The log message shows "Popen(...)", not "execute(...)". So it should faithfully report what is about to be passed to Popen in cases where a user reading the log would otherwise be misled into thinking this is what has happened. Reporting the actual "shell=" argument passed to Popen is also more useful to log than the argument passed to Git.execute (at least if only one of them is to be logged). --- test/test_git.py | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 6fd2c8268..389e80552 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -5,8 +5,10 @@ # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ import contextlib +import logging import os import os.path as osp +import re import shutil import subprocess import sys @@ -74,7 +76,8 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): res = self.git.transform_kwargs(**{"s": True, "t": True}) self.assertEqual({"-s", "-t"}, set(res)) - @ddt.data( + _shell_cases = ( + # value_in_call, value_from_class, expected_popen_arg (None, False, False), (None, True, True), (False, True, False), @@ -82,20 +85,42 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): (True, False, True), (True, True, True), ) + + @ddt.idata(_shell_cases) @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" value_in_call, value_from_class, expected_popen_arg = case - # FIXME: Check what gets logged too! + with mock.patch.object(Git, "USE_SHELL", value_from_class): with contextlib.suppress(GitCommandError): self.git.execute( "git", # No args, so it runs with or without a shell, on all OSes. shell=value_in_call, ) + mock_popen.assert_called_once() self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) + @ddt.idata(full_case[:2] for full_case in _shell_cases) + @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. + def test_it_logs_if_it_uses_a_shell(self, case, mock_popen): + """``shell=`` in the log message agrees with what is passed to `Popen`.""" + value_in_call, value_from_class = case + + with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: + with mock.patch.object(Git, "USE_SHELL", value_from_class): + with contextlib.suppress(GitCommandError): + self.git.execute( + "git", # No args, so it runs with or without a shell, on all OSes. + shell=value_in_call, + ) + + popen_shell_arg = mock_popen.call_args.kwargs["shell"] + expected_message = re.compile(rf"DEBUG:git.cmd:Popen\(.*\bshell={popen_shell_arg}\b.*\)") + match_attempts = [expected_message.fullmatch(message) for message in log_watcher.output] + self.assertTrue(any(match_attempts), repr(log_watcher.output)) + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") From 0f19fb0be1bd3ccd3ff8f35dba9e924c9d379e41 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 02:21:10 -0400 Subject: [PATCH 42/90] Fix tests so they don't try to run "g" Both new shell-related tests were causing the code under test to split "git" into ["g", "i", "t"] and thus causing the code under test to attempt to execute a "g" command. This passes the command as a one-element list of strings rather than as a string, which avoids this on all operating systems regardless of whether the code under test has a bug being tested for. This would not occur on Windows, which does not iterate commands of type str character-by-character even when the command is run without a shell. But it did happen on other systems. Most of the time, the benefit of using a command that actually runs "git" rather than "g" is the avoidance of confusion in the error message. But this is also important because it is possible for the user who runs the tests to have a "g" command, in which case it could be very inconvenient, or even unsafe, to run "g". This should be avoided even when the code under test has a bug that causes a shell to be used when it shouldn't or not used when it should, so having separate commands (list and str) per test case parameters would not be a sufficient solution: it would only guard against running "g" when a bug in the code under test were absent. --- test/test_git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 389e80552..7e8e7c805 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -95,7 +95,7 @@ def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): with mock.patch.object(Git, "USE_SHELL", value_from_class): with contextlib.suppress(GitCommandError): self.git.execute( - "git", # No args, so it runs with or without a shell, on all OSes. + ["git"], # No args, so it runs with or without a shell, on all OSes. shell=value_in_call, ) @@ -112,7 +112,7 @@ def test_it_logs_if_it_uses_a_shell(self, case, mock_popen): with mock.patch.object(Git, "USE_SHELL", value_from_class): with contextlib.suppress(GitCommandError): self.git.execute( - "git", # No args, so it runs with or without a shell, on all OSes. + ["git"], # No args, so it runs with or without a shell, on all OSes. shell=value_in_call, ) From da3460c6cc3a7c6981dfd1d4675d167a7a5f2b0c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 02:56:37 -0400 Subject: [PATCH 43/90] Extract shared test logic to a helper This also helps mock Popen over a smaller scope, which may be beneficial (especially if it is mocked in the subprocess module, rather than the git.cmd module, in the future). --- test/test_git.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 7e8e7c805..343bf7a19 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -86,35 +86,33 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): (True, True, True), ) + def _do_shell_combo(self, value_in_call, value_from_class): + with mock.patch.object(Git, "USE_SHELL", value_from_class): + # git.cmd gets Popen via a "from" import, so patch it there. + with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen: + # Use a command with no arguments (besides the program name), so it runs + # with or without a shell, on all OSes, with the same effect. Since git + # errors out when run with no arguments, we swallow that error. + with contextlib.suppress(GitCommandError): + self.git.execute(["git"], shell=value_in_call) + + return mock_popen + @ddt.idata(_shell_cases) - @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. - def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): + def test_it_uses_shell_or_not_as_specified(self, case): """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" value_in_call, value_from_class, expected_popen_arg = case - - with mock.patch.object(Git, "USE_SHELL", value_from_class): - with contextlib.suppress(GitCommandError): - self.git.execute( - ["git"], # No args, so it runs with or without a shell, on all OSes. - shell=value_in_call, - ) - + mock_popen = self._do_shell_combo(value_in_call, value_from_class) mock_popen.assert_called_once() self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) @ddt.idata(full_case[:2] for full_case in _shell_cases) - @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. - def test_it_logs_if_it_uses_a_shell(self, case, mock_popen): + def test_it_logs_if_it_uses_a_shell(self, case): """``shell=`` in the log message agrees with what is passed to `Popen`.""" value_in_call, value_from_class = case with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: - with mock.patch.object(Git, "USE_SHELL", value_from_class): - with contextlib.suppress(GitCommandError): - self.git.execute( - ["git"], # No args, so it runs with or without a shell, on all OSes. - shell=value_in_call, - ) + mock_popen = self._do_shell_combo(value_in_call, value_from_class) popen_shell_arg = mock_popen.call_args.kwargs["shell"] expected_message = re.compile(rf"DEBUG:git.cmd:Popen\(.*\bshell={popen_shell_arg}\b.*\)") From 41294d578471f7f63c02bf59e8abc3459f9d6390 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 03:26:02 -0400 Subject: [PATCH 44/90] Use the mock backport on Python 3.7 Because mock.call.kwargs, i.e. the ability to examine m.call_args.kwargs where m is a Mock or MagicMock, was introduced in Python 3.8. Currently it is only in test/test_git.py that any use of mocks requires this, so I've put the conditional import logic to import mock (the top-level package) rather than unittest.mock only there. The mock library is added as a development (testing) dependency only when the Python version is lower than 3.8, so it is not installed when not needed. This fixes a problem in the new tests of whether a shell is used, and reported as used, in the Popen call in Git.execute. Those just-introduced tests need this, to be able to use mock_popen.call_args.kwargs on Python 3.7. --- test-requirements.txt | 3 ++- test/test_git.py | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/test-requirements.txt b/test-requirements.txt index 1c08c736f..9414da09c 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,6 +1,7 @@ black coverage[toml] -ddt>=1.1.1, !=1.4.3 +ddt >= 1.1.1, != 1.4.3 +mock ; python_version < "3.8" mypy pre-commit pytest diff --git a/test/test_git.py b/test/test_git.py index 343bf7a19..583c74fa3 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -13,7 +13,12 @@ import subprocess import sys from tempfile import TemporaryDirectory, TemporaryFile -from unittest import mock, skipUnless +from unittest import skipUnless + +if sys.version_info >= (3, 8): + from unittest import mock +else: + import mock # To be able to examine call_args.kwargs on a mock. import ddt From baf3457ec8f92c64d2481b812107e6acc4059ddd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 04:11:18 -0400 Subject: [PATCH 45/90] Fix Git.execute shell use and reporting bugs This updates the shell variable itself, only when it is None, from self.USE_SHELL. (That attribute is usually Git.USE_SHELL rather than an instance attribute. But although people probably shouldn't set it on instances, people will expect that this is permitted.) Now: - USE_SHELL is used as a fallback only. When shell=False is passed, USE_SHELL is no longer consuled. Thus shell=False always keeps a shell from being used, even in the non-default case where the USE_SHELL attribue has been set to True. - The debug message printed to the log shows the actual value that is being passed to Popen, because the updated shell variable is used both to produce that message and in the Popen call. --- git/cmd.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 9921dd6c9..53b8b9050 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -974,6 +974,8 @@ def execute( istream_ok = "None" if istream: istream_ok = "" + if shell is None: + shell = self.USE_SHELL log.debug( "Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)", redacted_command, @@ -992,7 +994,7 @@ def execute( stdin=istream or DEVNULL, stderr=PIPE, stdout=stdout_sink, - shell=shell is not None and shell or self.USE_SHELL, + shell=shell, close_fds=is_posix, # unsupported on windows universal_newlines=universal_newlines, creationflags=PROC_CREATIONFLAGS, From b79198a880982e6768fec4d0ef244338420efbdc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 04:58:13 -0400 Subject: [PATCH 46/90] Document Git.execute parameters in definition order - Reorder the items in the git.cmd.Git.execute docstring that document its parameters, to be in the same order the parameters are actually defined in. - Use consistent spacing, by having a blank line between successive items that document parameters. Before, most of them were separated this way, but some of them were not. - Reorder the elements of execute_kwargs (which list all those parameters except the first parameter, command) to be listed in that order as well. These were mostly in order, but a couple were out of order. This is just about the order they appear in the definition, since sets in Python (unlike dicts) have no key order guarantees. --- git/cmd.py | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 53b8b9050..4d772e909 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -66,10 +66,10 @@ "with_extended_output", "with_exceptions", "as_process", - "stdout_as_string", "output_stream", - "with_stdout", + "stdout_as_string", "kill_after_timeout", + "with_stdout", "universal_newlines", "shell", "env", @@ -883,6 +883,27 @@ def execute( decoded into a string using the default encoding (usually utf-8). The latter can fail, if the output contains binary data. + :param kill_after_timeout: + To specify a timeout in seconds for the git command, after which the process + should be killed. This will have no effect if as_process is set to True. It is + set to None by default and will let the process run until the timeout is + explicitly specified. This feature is not supported on Windows. It's also worth + noting that kill_after_timeout uses SIGKILL, which can have negative side + effects on a repository. For example, stale locks in case of git gc could + render the repository incapable of accepting changes until the lock is manually + removed. + + :param with_stdout: + If True, default True, we open stdout on the created process + + :param universal_newlines: + if True, pipes will be opened as text, and lines are split at + all known line endings. + + :param shell: + Whether to invoke commands through a shell (see `Popen(..., shell=True)`). + It overrides :attr:`USE_SHELL` if it is not `None`. + :param env: A dictionary of environment variables to be passed to `subprocess.Popen`. @@ -891,29 +912,14 @@ def execute( one invocation of write() method. If the given number is not positive then the default value is used. + :param strip_newline_in_stdout: + Whether to strip the trailing ``\\n`` of the command stdout. + :param subprocess_kwargs: Keyword arguments to be passed to subprocess.Popen. Please note that some of the valid kwargs are already set by this method, the ones you specify may not be the same ones. - :param with_stdout: If True, default True, we open stdout on the created process - :param universal_newlines: - if True, pipes will be opened as text, and lines are split at - all known line endings. - :param shell: - Whether to invoke commands through a shell (see `Popen(..., shell=True)`). - It overrides :attr:`USE_SHELL` if it is not `None`. - :param kill_after_timeout: - To specify a timeout in seconds for the git command, after which the process - should be killed. This will have no effect if as_process is set to True. It is - set to None by default and will let the process run until the timeout is - explicitly specified. This feature is not supported on Windows. It's also worth - noting that kill_after_timeout uses SIGKILL, which can have negative side - effects on a repository. For example, stale locks in case of git gc could - render the repository incapable of accepting changes until the lock is manually - removed. - :param strip_newline_in_stdout: - Whether to strip the trailing ``\\n`` of the command stdout. :return: * str(output) if extended_output = False (Default) * tuple(int(status), str(stdout), str(stderr)) if extended_output = True From 13e1593fc6a3c218451e29dd6b4a58b3a44afee3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 05:30:22 -0400 Subject: [PATCH 47/90] Don't say Git.execute uses a shell, in its summary The top line of the Git.execute docstring said that it used a shell, which is not necessarily the case (and is not usually the case, since the default is not to use one). This removes that claim while keeping the top-line wording otherwise the same. It also rephrases the description of the command parameter, in a way that does not change its meaning but reflects the more common practice of passing a sequence of arguments (since portable calls that do not use a shell must do that). --- git/cmd.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 4d772e909..e324db7e2 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -842,12 +842,12 @@ def execute( strip_newline_in_stdout: bool = True, **subprocess_kwargs: Any, ) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], AutoInterrupt]: - """Handles executing the command on the shell and consumes and returns - the returned information (stdout) + """Handles executing the command and consumes and returns the returned + information (stdout) :param command: The command argument list to execute. - It should be a string, or a sequence of program arguments. The + It should be a sequence of program arguments, or a string. The program to execute is the first item in the args sequence or string. :param istream: From 74b68e9b621a729a3407b2020b0a48d7921fb1e9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 05:55:54 -0400 Subject: [PATCH 48/90] Copyedit Git.execute docstring These are some small clarity and consistency revisions to the docstring of git.cmd.Git.execute that didn't specifically fit in the narrow topics of either of the preceding two commits. --- git/cmd.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index e324db7e2..f20cd77af 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -851,7 +851,7 @@ def execute( program to execute is the first item in the args sequence or string. :param istream: - Standard input filehandle passed to subprocess.Popen. + Standard input filehandle passed to `subprocess.Popen`. :param with_extended_output: Whether to return a (status, stdout, stderr) tuple. @@ -862,8 +862,7 @@ def execute( :param as_process: Whether to return the created process instance directly from which streams can be read on demand. This will render with_extended_output and - with_exceptions ineffective - the caller will have - to deal with the details himself. + with_exceptions ineffective - the caller will have to deal with the details. It is important to note that the process will be placed into an AutoInterrupt wrapper that will interrupt the process once it goes out of scope. If you use the command in iterators, you should pass the whole process instance @@ -876,25 +875,25 @@ def execute( always be created with a pipe due to issues with subprocess. This merely is a workaround as data will be copied from the output pipe to the given output stream directly. - Judging from the implementation, you shouldn't use this flag ! + Judging from the implementation, you shouldn't use this parameter! :param stdout_as_string: - if False, the commands standard output will be bytes. Otherwise, it will be - decoded into a string using the default encoding (usually utf-8). + If False, the command's standard output will be bytes. Otherwise, it will be + decoded into a string using the default encoding (usually UTF-8). The latter can fail, if the output contains binary data. :param kill_after_timeout: - To specify a timeout in seconds for the git command, after which the process + Specifies a timeout in seconds for the git command, after which the process should be killed. This will have no effect if as_process is set to True. It is set to None by default and will let the process run until the timeout is explicitly specified. This feature is not supported on Windows. It's also worth noting that kill_after_timeout uses SIGKILL, which can have negative side - effects on a repository. For example, stale locks in case of git gc could + effects on a repository. For example, stale locks in case of ``git gc`` could render the repository incapable of accepting changes until the lock is manually removed. :param with_stdout: - If True, default True, we open stdout on the created process + If True, default True, we open stdout on the created process. :param universal_newlines: if True, pipes will be opened as text, and lines are split at @@ -916,19 +915,19 @@ def execute( Whether to strip the trailing ``\\n`` of the command stdout. :param subprocess_kwargs: - Keyword arguments to be passed to subprocess.Popen. Please note that - some of the valid kwargs are already set by this method, the ones you + Keyword arguments to be passed to `subprocess.Popen`. Please note that + some of the valid kwargs are already set by this method; the ones you specify may not be the same ones. :return: * str(output) if extended_output = False (Default) * tuple(int(status), str(stdout), str(stderr)) if extended_output = True - if output_stream is True, the stdout value will be your output stream: + If output_stream is True, the stdout value will be your output stream: * output_stream if extended_output = False * tuple(int(status), output_stream, str(stderr)) if extended_output = True - Note git is executed with LC_MESSAGES="C" to ensure consistent + Note that git is executed with ``LC_MESSAGES="C"`` to ensure consistent output regardless of system language. :raise GitCommandError: From 133271bb3871baff3ed772fbdea8bc359f115fd6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 06:34:05 -0400 Subject: [PATCH 49/90] Other copyediting in the git.cmd module (Not specific to git.cmd.Git.execute.) --- git/cmd.py | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index f20cd77af..a6d287986 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -105,7 +105,7 @@ def handle_process_output( ) -> None: """Registers for notifications to learn that process output is ready to read, and dispatches lines to the respective line handlers. - This function returns once the finalizer returns + This function returns once the finalizer returns. :return: result of finalizer :param process: subprocess.Popen instance @@ -294,9 +294,7 @@ def __setstate__(self, d: Dict[str, Any]) -> None: @classmethod def refresh(cls, path: Union[None, PathLike] = None) -> bool: - """This gets called by the refresh function (see the top level - __init__). - """ + """This gets called by the refresh function (see the top level __init__).""" # discern which path to refresh with if path is not None: new_git = os.path.expanduser(path) @@ -446,9 +444,9 @@ def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None) -> PathLike: if is_cygwin: url = cygpath(url) else: - """Remove any backslahes from urls to be written in config files. + """Remove any backslashes from urls to be written in config files. - Windows might create config-files containing paths with backslashed, + Windows might create config files containing paths with backslashes, but git stops liking them as it will escape the backslashes. Hence we undo the escaping just to be sure. """ @@ -464,8 +462,8 @@ def check_unsafe_protocols(cls, url: str) -> None: Check for unsafe protocols. Apart from the usual protocols (http, git, ssh), - Git allows "remote helpers" that have the form `::
`, - one of these helpers (`ext::`) can be used to invoke any arbitrary command. + Git allows "remote helpers" that have the form ``::
``, + one of these helpers (``ext::``) can be used to invoke any arbitrary command. See: @@ -517,7 +515,7 @@ def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: self.status: Union[int, None] = None def _terminate(self) -> None: - """Terminate the underlying process""" + """Terminate the underlying process.""" if self.proc is None: return @@ -572,7 +570,7 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int: """Wait for the process and return its status code. :param stderr: Previously read value of stderr, in case stderr is already closed. - :warn: may deadlock if output or error pipes are used and not handled separately. + :warn: May deadlock if output or error pipes are used and not handled separately. :raise GitCommandError: if the return status is not 0""" if stderr is None: stderr_b = b"" @@ -605,13 +603,12 @@ def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> byte # END auto interrupt class CatFileContentStream(object): - """Object representing a sized read-only stream returning the contents of an object. It behaves like a stream, but counts the data read and simulates an empty stream once our sized content region is empty. - If not all data is read to the end of the objects's lifetime, we read the - rest to assure the underlying stream continues to work""" + If not all data is read to the end of the object's lifetime, we read the + rest to assure the underlying stream continues to work.""" __slots__: Tuple[str, ...] = ("_stream", "_nbr", "_size") @@ -740,11 +737,11 @@ def __getattr__(self, name: str) -> Any: def set_persistent_git_options(self, **kwargs: Any) -> None: """Specify command line options to the git executable - for subsequent subcommand calls + for subsequent subcommand calls. :param kwargs: is a dict of keyword arguments. - these arguments are passed as in _call_process + These arguments are passed as in _call_process but will be passed to the git command rather than the subcommand. """ @@ -775,7 +772,7 @@ def version_info(self) -> Tuple[int, int, int, int]: """ :return: tuple(int, int, int, int) tuple with integers representing the major, minor and additional version numbers as parsed from git version. - This value is generated on demand and is cached""" + This value is generated on demand and is cached.""" return self._version_info @overload @@ -843,7 +840,7 @@ def execute( **subprocess_kwargs: Any, ) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], AutoInterrupt]: """Handles executing the command and consumes and returns the returned - information (stdout) + information (stdout). :param command: The command argument list to execute. @@ -1213,7 +1210,7 @@ def _unpack_args(cls, arg_list: Sequence[str]) -> List[str]: def __call__(self, **kwargs: Any) -> "Git": """Specify command line options to the git executable - for a subcommand call + for a subcommand call. :param kwargs: is a dict of keyword arguments. @@ -1251,7 +1248,7 @@ def _call_process( self, method: str, *args: Any, **kwargs: Any ) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], "Git.AutoInterrupt"]: """Run the given git command with the specified arguments and return - the result as a String + the result as a string. :param method: is the command. Contained "_" characters will be converted to dashes, @@ -1260,7 +1257,7 @@ def _call_process( :param args: is the list of arguments. If None is included, it will be pruned. This allows your commands to call git more conveniently as None - is realized as non-existent + is realized as non-existent. :param kwargs: It contains key-values for the following: @@ -1390,7 +1387,7 @@ def get_object_header(self, ref: str) -> Tuple[str, str, int]: return self.__get_object_header(cmd, ref) def get_object_data(self, ref: str) -> Tuple[str, str, int, bytes]: - """As get_object_header, but returns object data as well + """As get_object_header, but returns object data as well. :return: (hexsha, type_string, size_as_int, data_string) :note: not threadsafe""" @@ -1400,10 +1397,10 @@ def get_object_data(self, ref: str) -> Tuple[str, str, int, bytes]: return (hexsha, typename, size, data) def stream_object_data(self, ref: str) -> Tuple[str, str, int, "Git.CatFileContentStream"]: - """As get_object_header, but returns the data as a stream + """As get_object_header, but returns the data as a stream. :return: (hexsha, type_string, size_as_int, stream) - :note: This method is not threadsafe, you need one independent Command instance per thread to be safe !""" + :note: This method is not threadsafe, you need one independent Command instance per thread to be safe!""" cmd = self._get_persistent_cmd("cat_file_all", "cat_file", batch=True) hexsha, typename, size = self.__get_object_header(cmd, ref) cmd_stdout = cmd.stdout if cmd.stdout is not None else io.BytesIO() From fc755dae6866b9c9e0aa347297b693fec2c5b415 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 06:38:54 -0400 Subject: [PATCH 50/90] Avoid having a local function seem to be a method The kill_process local function defined in the Git.execute method is a local function and not a method, but it was easy to misread as a method for two reasons: - Its docstring described it as a method. - It was named with a leading underscore, as though it were a nonpublic method. But this name is a local variable, and local variables are always nonpublic (except when they are function parameters, in which case they are in a sense public). A leading underscore in a local variable name usually means the variable is unused in the function. This fixes the docstring and drops the leading underscore from the name. If this is ever extracted from the Git.execute method and placed at class or module scope, then the name can be changed back. --- git/cmd.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index a6d287986..1d8c70f32 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1012,8 +1012,8 @@ def execute( if as_process: return self.AutoInterrupt(proc, command) - def _kill_process(pid: int) -> None: - """Callback method to kill a process.""" + def kill_process(pid: int) -> None: + """Callback to kill a process.""" p = Popen( ["ps", "--ppid", str(pid)], stdout=PIPE, @@ -1046,7 +1046,7 @@ def _kill_process(pid: int) -> None: if kill_after_timeout is not None: kill_check = threading.Event() - watchdog = threading.Timer(kill_after_timeout, _kill_process, args=(proc.pid,)) + watchdog = threading.Timer(kill_after_timeout, kill_process, args=(proc.pid,)) # Wait for the process to return status = 0 From 2d1efdca84e266a422f4298ee94ee9b8dae6c32e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 07:55:35 -0400 Subject: [PATCH 51/90] Test that git.cmd.execute_kwargs is correct --- test/test_git.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/test_git.py b/test/test_git.py index 583c74fa3..723bf624b 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -5,6 +5,7 @@ # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ import contextlib +import inspect import logging import os import os.path as osp @@ -364,3 +365,11 @@ def counter_stderr(line): self.assertEqual(count[1], line_count) self.assertEqual(count[2], line_count) + + def test_execute_kwargs_set_agrees_with_method(self): + parameter_names = inspect.signature(cmd.Git.execute).parameters.keys() + self_param, command_param, *most_params, extra_kwargs_param = parameter_names + self.assertEqual(self_param, "self") + self.assertEqual(command_param, "command") + self.assertEqual(set(most_params), cmd.execute_kwargs) # Most important. + self.assertEqual(extra_kwargs_param, "subprocess_kwargs") From a8a43fe6f8d6f0a7f9067149859634d624406bb1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 08:50:17 -0400 Subject: [PATCH 52/90] Simplify shell test helper with with_exceptions=False Instead of swallowing GitCommandError exceptions in the helper used by test_it_uses_shell_or_not_as_specified and test_it_logs_if_it_uses_a_shell, this modifies the helper so it prevents Git.execute from raising the exception in the first place. --- test/test_git.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 723bf624b..8d269f38a 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -4,7 +4,6 @@ # # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ -import contextlib import inspect import logging import os @@ -97,10 +96,8 @@ def _do_shell_combo(self, value_in_call, value_from_class): # git.cmd gets Popen via a "from" import, so patch it there. with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen: # Use a command with no arguments (besides the program name), so it runs - # with or without a shell, on all OSes, with the same effect. Since git - # errors out when run with no arguments, we swallow that error. - with contextlib.suppress(GitCommandError): - self.git.execute(["git"], shell=value_in_call) + # with or without a shell, on all OSes, with the same effect. + self.git.execute(["git"], with_exceptions=False, shell=value_in_call) return mock_popen From 9fa1ceef2c47c9f58e10d8925cc166fdfd6b5183 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 09:45:45 -0400 Subject: [PATCH 53/90] Extract a _assert_logged_for_popen method This extracts the logic of searching log messages, and asserting that (at least) one matches a pattern for the report of a Popen call with a given argument, from test_it_logs_if_it_uses_a_shell into a new nonpublic test helper method _assert_logged_for_popen. The extracted version is modified to make it slightly more general, and slightly more robust. This is still not extremely robust: the notation used to log Popen calls is informal, so it wouldn't make sense to really parse it as code. But this no longer assumes that the representation of a value ends at a word boundary, nor that the value is free of regular expression metacharacters. --- test/test_git.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 8d269f38a..10b346e4d 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -40,6 +40,13 @@ def tearDown(self): gc.collect() + def _assert_logged_for_popen(self, log_watcher, name, value): + re_name = re.escape(name) + re_value = re.escape(str(value)) + re_line = re.compile(fr"DEBUG:git.cmd:Popen\(.*\b{re_name}={re_value}[,)]") + match_attempts = [re_line.match(message) for message in log_watcher.output] + self.assertTrue(any(match_attempts), repr(log_watcher.output)) + @mock.patch.object(Git, "execute") def test_call_process_calls_execute(self, git): git.return_value = "" @@ -113,14 +120,9 @@ def test_it_uses_shell_or_not_as_specified(self, case): def test_it_logs_if_it_uses_a_shell(self, case): """``shell=`` in the log message agrees with what is passed to `Popen`.""" value_in_call, value_from_class = case - with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: mock_popen = self._do_shell_combo(value_in_call, value_from_class) - - popen_shell_arg = mock_popen.call_args.kwargs["shell"] - expected_message = re.compile(rf"DEBUG:git.cmd:Popen\(.*\bshell={popen_shell_arg}\b.*\)") - match_attempts = [expected_message.fullmatch(message) for message in log_watcher.output] - self.assertTrue(any(match_attempts), repr(log_watcher.output)) + self._assert_logged_for_popen(log_watcher, "shell", mock_popen.call_args.kwargs["shell"]) def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") From 790a790f49a2548c620532ee2b9705b09fb33855 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 09:59:28 -0400 Subject: [PATCH 54/90] Log stdin arg as such, and test that this is done This changes how the Popen call debug logging line shows the informal summary of what kind of thing is being passed as the stdin argument to Popen, showing it with stdin= rather than istream=. The new test, with "istream" in place of "stdin", passed before the code change in the git.cmd module, failed once "istream" was changed to "stdin" in the test, and then, as expected, passed again once "istream=" was changed to "stdin=" in the log.debug call in git.cmd.Git.execute. --- git/cmd.py | 2 +- test/test_git.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 1d8c70f32..e545ba160 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -979,7 +979,7 @@ def execute( if shell is None: shell = self.USE_SHELL log.debug( - "Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)", + "Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, stdin=%s)", redacted_command, cwd, universal_newlines, diff --git a/test/test_git.py b/test/test_git.py index 10b346e4d..1ee7b3642 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -124,6 +124,16 @@ def test_it_logs_if_it_uses_a_shell(self, case): mock_popen = self._do_shell_combo(value_in_call, value_from_class) self._assert_logged_for_popen(log_watcher, "shell", mock_popen.call_args.kwargs["shell"]) + @ddt.data( + ("None", None), + ("", subprocess.PIPE), + ) + def test_it_logs_istream_summary_for_stdin(self, case): + expected_summary, istream_argument = case + with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: + self.git.execute(["git", "version"], istream=istream_argument) + self._assert_logged_for_popen(log_watcher, "stdin", expected_summary) + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") From c3fde7fb8dcd48d17ee24b78db7b0dd25d2348ab Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 10:16:10 -0400 Subject: [PATCH 55/90] Log args in the order they are passed to Popen This is still not including all or even most of the arguments, nor are all the logged arguments literal (nor should either of those things likely be changed). It is just to facilitate easier comparison of what is logged to the Popen call in the code. --- git/cmd.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index e545ba160..c35e56703 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -979,12 +979,12 @@ def execute( if shell is None: shell = self.USE_SHELL log.debug( - "Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, stdin=%s)", + "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", redacted_command, cwd, - universal_newlines, - shell, istream_ok, + shell, + universal_newlines, ) try: with maybe_patch_caller_env: From ab958865d89b3146bb953f826f30da11dc59139a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 10:30:58 -0400 Subject: [PATCH 56/90] Eliminate istream_ok variable In Git.execute, the stdin argument to Popen is the only one where a compound expression (rather than a single term) is currently passed. So having that be the same in the log message makes it easier to understand what is going on, as well as to see how the information shown in the log corresponds to what Popen receives. --- git/cmd.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index c35e56703..7c448e3f2 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -973,16 +973,13 @@ def execute( # end handle stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") - istream_ok = "None" - if istream: - istream_ok = "" if shell is None: shell = self.USE_SHELL log.debug( "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", redacted_command, cwd, - istream_ok, + "" if istream else "None", shell, universal_newlines, ) From c3e926fbfda3bf5a1723258e990dfcfa45f2ef86 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 12:16:07 -0400 Subject: [PATCH 57/90] Fix a small YAML formatting style inconsistency --- .github/workflows/pythonpackage.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 1b049ba02..5564a526b 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -17,9 +17,9 @@ jobs: matrix: python-version: ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"] include: - - experimental: false - - python-version: "3.12" - experimental: true + - experimental: false + - python-version: "3.12" + experimental: true defaults: run: From b54a28a50b3e81e04b5b9ef61297fd12c62d09b3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 12:17:58 -0400 Subject: [PATCH 58/90] No longer allow CI to select a prerelease for 3.12 Since 3.12.0 stable is out, and available via setup-python, per: https://github.com/actions/python-versions/blob/main/versions-manifest.json --- .github/workflows/pythonpackage.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 5564a526b..e43317807 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -18,8 +18,6 @@ jobs: python-version: ["3.7", "3.8", "3.9", "3.10", "3.11", "3.12"] include: - experimental: false - - python-version: "3.12" - experimental: true defaults: run: From 8c4df3cfdca1eebd2f07c7be24ab5e2805ec2708 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 03:12:48 -0400 Subject: [PATCH 59/90] Add pre-commit hook to run shellcheck This also reorders the hooks from pre-commit/pre-commit-hooks so that the overall order of all hooks from all repositories is: lint Python, lint non-Python, non-lint. --- .pre-commit-config.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 67aefb342..c5973c9ea 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -9,9 +9,14 @@ repos: - flake8-typing-imports==1.14.0 exclude: ^doc|^git/ext/ + - repo: https://github.com/shellcheck-py/shellcheck-py + rev: v0.9.0.5 + hooks: + - id: shellcheck + - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 hooks: - - id: check-merge-conflict - id: check-toml - id: check-yaml + - id: check-merge-conflict From f3be76f474636f9805756bc9f05b22fb4aa8809d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 04:49:28 -0400 Subject: [PATCH 60/90] Force color when running shellcheck in pre-commit Its output is colorized normally, but on CI it is not colorized without this (pre-commit's own output is, but not shellcheck's). --- .pre-commit-config.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c5973c9ea..bacc90913 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -13,6 +13,7 @@ repos: rev: v0.9.0.5 hooks: - id: shellcheck + args: [--color] - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 From 7dd8added2b1695b1740f0d1d7d7b2858a49a88c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 04:17:04 -0400 Subject: [PATCH 61/90] Suppress SC2086 where word splitting is intended This suppresses ShellCheck SC2016, "Double quote to prevent globbing and word splitting," on the command in the version check script that expands $config_opts to build the "-c ..." arguments. It also moves the code repsonsible for getting the latest tag, which this is part of, into a function for that purpose, so it's clear that building config_opts is specifically for that, and so that the code is not made harder to read by adding the ShellCheck suppression comment. (The suppression applies only to the immediate next command.) --- check-version.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/check-version.sh b/check-version.sh index c50bf498b..46aa56173 100755 --- a/check-version.sh +++ b/check-version.sh @@ -10,6 +10,13 @@ trap 'echo "$0: Check failed. Stopping." >&2' ERR readonly version_path='VERSION' readonly changes_path='doc/source/changes.rst' +function get_latest_tag() { + local config_opts + config_opts="$(printf ' -c versionsort.suffix=-%s' alpha beta pre rc RC)" + # shellcheck disable=SC2086 # Deliberately word-splitting the arguments. + git $config_opts tag -l '[0-9]*' --sort=-v:refname | head -n1 +} + echo 'Checking current directory.' test "$(cd -- "$(dirname -- "$0")" && pwd)" = "$(pwd)" # Ugly, but portable. @@ -26,8 +33,7 @@ test -z "$(git status -s --ignore-submodules)" version_version="$(cat "$version_path")" changes_version="$(awk '/^[0-9]/ {print $0; exit}' "$changes_path")" -config_opts="$(printf ' -c versionsort.suffix=-%s' alpha beta pre rc RC)" -latest_tag="$(git $config_opts tag -l '[0-9]*' --sort=-v:refname | head -n1)" +latest_tag="$(get_latest_tag)" head_sha="$(git rev-parse HEAD)" latest_tag_sha="$(git rev-parse "${latest_tag}^{commit}")" From 21875b5a84c899a5b38c627e895a1bb58344b2a1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 04:31:18 -0400 Subject: [PATCH 62/90] Don't split and glob the interpreter name In the release building script, this changes $1 to "$1", because $1 without quotes means to perform word splitting and globbing (pathname expansion) on the parameter (unless otherwise disabled by the value of $IFS or "set -f", respectively) and use the result of doing so, which isn't the intent of the code. This function is only used from within the script, where it is not given values that would be changed by these additional expansions. So this is mainly about communicating intent. (If in the future it is intended that multiple arguments be usable, then they should be passed as separate arguments to release_with, which should be modified by replacing "$1" with "$@". I have not used "$@" at this time because it is not intuitively obvious that the arguments should be to the interpreter, rather than to the build module, so I don't think this should be supported unless or until a need for it determines that.) --- build-release.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build-release.sh b/build-release.sh index 5840e4472..4eb760ddd 100755 --- a/build-release.sh +++ b/build-release.sh @@ -6,7 +6,7 @@ set -eEu function release_with() { - $1 -m build --sdist --wheel + "$1" -m build --sdist --wheel } if test -n "${VIRTUAL_ENV:-}"; then From 0920371905561d7a242a8be158b79d1a8408a7c4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 04:40:08 -0400 Subject: [PATCH 63/90] Extract suggest_venv out of the else block I think this is easier to read, but this is admittedly subjective. This commit also makes the separate change of adjusting comment spacing for consistency within the script. (Two spaces before "#" is not widely regarded as better than one in shell scripting, so unlike Python where PEP-8 recommends that, it would be equally good to have changed all the other places in the shell scrips to have just one.) --- build-release.sh | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/build-release.sh b/build-release.sh index 4eb760ddd..4fd4a2251 100755 --- a/build-release.sh +++ b/build-release.sh @@ -9,6 +9,11 @@ function release_with() { "$1" -m build --sdist --wheel } +function suggest_venv() { + local venv_cmd='python -m venv env && source env/bin/activate' + printf "HELP: To avoid this error, use a virtual-env with '%s' instead.\n" "$venv_cmd" +} + if test -n "${VIRTUAL_ENV:-}"; then deps=(build twine) # Install twine along with build, as we need it later. echo "Virtual environment detected. Adding packages: ${deps[*]}" @@ -16,11 +21,7 @@ if test -n "${VIRTUAL_ENV:-}"; then echo 'Starting the build.' release_with python else - function suggest_venv() { - venv_cmd='python -m venv env && source env/bin/activate' - printf "HELP: To avoid this error, use a virtual-env with '%s' instead.\n" "$venv_cmd" - } trap suggest_venv ERR # This keeps the original exit (error) code. echo 'Starting the build.' - release_with python3 # Outside a venv, use python3. + release_with python3 # Outside a venv, use python3. fi From e973f527f92a932e833167c57cb4d4eeb3103429 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 05:06:59 -0400 Subject: [PATCH 64/90] Use some handy bash-isms in version check script The script is nonportable to other shells already because of its use of trap (and other features, such as implicit assumptions made about echo, and the function keyword), which its hashbang already clearly conveys. This uses: - $( Date: Wed, 27 Sep 2023 05:23:27 -0400 Subject: [PATCH 65/90] Have init script treat master unambiguously as a branch Because users may have an old version of git without "git switch", init-tests-after-clone.sh should continue to use "git checkout" to attempt to switch to master. But without "--", this suffers from the problem that it's ambiguous if master is a branch (so checkout behaves like switch) or a path (so checkout behaves like restore). There are two cases where this ambiguity can be a problem. The most common is on a fork with no master branch but also, fortunately, no file or directory named "master". Then the problem is just the error message (printed just before the script proceeds to redo the checkout with -b): error: pathspec 'master' did not match any file(s) known to git The real cause of the error is the branch being absent, as happens when a fork copies only the main branch and the upstream remote is not also set up. Adding the "--" improves the error message: fatal: invalid reference: master However, it is possible, though unlikely, for a file or directory called "master" to exist. In that case, if there is also no master branch, git discards unstaged changes made to the file or (much worse!) everywhere in that directory, potentially losing work. This commit adds "--" to the right of "master" so git never regards it as a path. This is not needed with -b, which is always followed by a symbolic name, so I have not added it there. (Note that the command is still imperfect because, for example, in rare cases there could be a master *tag*--and no master branch--in which case, as before, HEAD would be detached there and the script would attempt to continue.) --- init-tests-after-clone.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 95ced98b7..52c0c06aa 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -10,7 +10,7 @@ if [[ -z "$TRAVIS" ]]; then fi git tag __testing_point__ -git checkout master || git checkout -b master +git checkout master -- || git checkout -b master git reset --hard HEAD~1 git reset --hard HEAD~1 git reset --hard HEAD~1 From e604b469a1bdfb83f03d4c2ef1f79b45b8a5c3fa Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 05:44:02 -0400 Subject: [PATCH 66/90] Use 4-space indentation in all shell scripts This had been done everywhere except in init-tests-after-clone.sh. --- init-tests-after-clone.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 52c0c06aa..9d65570da 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -3,10 +3,10 @@ set -e if [[ -z "$TRAVIS" ]]; then - read -rp "This operation will destroy locally modified files. Continue ? [N/y]: " answer - if [[ ! $answer =~ [yY] ]]; then - exit 2 - fi + read -rp "This operation will destroy locally modified files. Continue ? [N/y]: " answer + if [[ ! $answer =~ [yY] ]]; then + exit 2 + fi fi git tag __testing_point__ From 19dfbd8ce4df9bde9b36eda12304ec4db71b084a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 07:33:56 -0400 Subject: [PATCH 67/90] Make the init script a portable POSIX shell script This translates init-tests-after-clone.sh from bash to POSIX sh, changing the hashbang and replacing all bash-specific features, so that it can be run on any POSIX-compatible ("Bourne style") shell, including but not limited to bash. This allows it to be used even on systems that don't have any version of bash installed anywhere. Only that script is modified. The other shell scripts make greater use of (and gain greater benefit from) bash features, and are also only used for building and releasing. In contrast, this script is meant to be used by most users who clone the repository. --- init-tests-after-clone.sh | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 9d65570da..19ff0fd28 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -1,12 +1,16 @@ -#!/usr/bin/env bash +#!/bin/sh set -e -if [[ -z "$TRAVIS" ]]; then - read -rp "This operation will destroy locally modified files. Continue ? [N/y]: " answer - if [[ ! $answer =~ [yY] ]]; then - exit 2 - fi +if test -z "$TRAVIS"; then + printf 'This operation will destroy locally modified files. Continue ? [N/y]: ' >&2 + read -r answer + case "$answer" in + [yY]) + ;; + *) + exit 2 ;; + esac fi git tag __testing_point__ From 7110bf8e04f96ffb20518ebf48803a50d1e3bf22 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 14:56:20 -0400 Subject: [PATCH 68/90] Move extra tag-fetching step into init script This makes three related changes: - Removes "git fetch --tags" from the instructions in the readme, because the goal of this command can be achieved in the init-tests-after-clone.sh script, and because this fetch command, as written, is probably only achieving that goal in narrow cases. In clones and fetches, tags on branches are fetched by default, and the tags needed by the tests are on branches. So the situations where "git fetch --tags" was helping were (a) when the remote recently gained the tags, and (b) when the original remote was cloned in an unusual way, not fetching all tags. In both cases, the "--tags" option is not what makes that fetch get the needed tags. - Adds "git fetch --all --tags" to init-tests-after-clone.sh. The "--all" option causes it to fetch from all remotes, and this is more significant than "--tags", since the tags needed for testing are on fetched branches. This achieves what "git fetch --tags" was achieving, and it also has the benefit of getting tags from remotes that have been added but not fetched from, as happens with an upstream remote that was manually added with no further action. (It also gets branches from those remotes, but if master is on multiple remotes but at different commits then "git checkout master" may not be able to pick one. So do this *after* rather than before that.) - Skips this extra fetch, and also the submodule cloning/updating step, when running on CI. CI jobs will already have taken care of cloning the repo with submodules recursively, and fetching all available tags. In forks without tags, the necessary tags for the test are not fetched--but the upstream remote is not set up on CI, so they wouldn't be obtained anyway, not even by refetching with "--all". --- README.md | 7 +++---- init-tests-after-clone.sh | 4 ++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 65c1e7bae..c17340f63 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,6 @@ To clone the [the GitHub repository](https://github.com/gitpython-developers/Git ```bash git clone https://github.com/gitpython-developers/GitPython cd GitPython -git fetch --tags ./init-tests-after-clone.sh ``` @@ -114,9 +113,9 @@ See [Issue #525](https://github.com/gitpython-developers/GitPython/issues/525). ### RUNNING TESTS -_Important_: Right after cloning this repository, please be sure to have -executed `git fetch --tags` followed by the `./init-tests-after-clone.sh` -script in the repository root. Otherwise you will encounter test failures. +_Important_: Right after cloning this repository, please be sure to have executed +the `./init-tests-after-clone.sh` script in the repository root. Otherwise +you will encounter test failures. On _Windows_, make sure you have `git-daemon` in your PATH. For MINGW-git, the `git-daemon.exe` exists in `Git\mingw64\libexec\git-core\`. diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 19ff0fd28..efa30a2dc 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -19,4 +19,8 @@ git reset --hard HEAD~1 git reset --hard HEAD~1 git reset --hard HEAD~1 git reset --hard __testing_point__ + +test -z "$TRAVIS" || exit 0 # CI jobs will already have taken care of the rest. + +git fetch --all --tags git submodule update --init --recursive From c7cdaf48865f4483bfd34e1f7527ab3e1d205dad Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 18:27:44 -0400 Subject: [PATCH 69/90] Reduce code duplication in version check script This extracts duplicated code to functions in check-version.sh. One effect is unfortunately that the specific commands being run are less explicitly clear when reading the script. However, small future changes, if accidentally made to one but not the other in either pair of "git status" commands, would create a much more confusing situation. So I think this change is justified overall. --- check-version.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/check-version.sh b/check-version.sh index d4200bd20..b47482d7e 100755 --- a/check-version.sh +++ b/check-version.sh @@ -10,6 +10,11 @@ trap 'echo "$0: Check failed. Stopping." >&2' ERR readonly version_path='VERSION' readonly changes_path='doc/source/changes.rst' +function check_status() { + git status -s "$@" + test -z "$(git status -s "$@")" +} + function get_latest_tag() { local config_opts printf -v config_opts ' -c versionsort.suffix=-%s' alpha beta pre rc RC @@ -23,13 +28,11 @@ test "$(cd -- "$(dirname -- "$0")" && pwd)" = "$(pwd)" # Ugly, but portable. echo "Checking that $version_path and $changes_path exist and have no uncommitted changes." test -f "$version_path" test -f "$changes_path" -git status -s -- "$version_path" "$changes_path" -test -z "$(git status -s -- "$version_path" "$changes_path")" +check_status -- "$version_path" "$changes_path" # This section can be commented out, if absolutely necessary. echo 'Checking that ALL changes are committed.' -git status -s --ignore-submodules -test -z "$(git status -s --ignore-submodules)" +check_status --ignore-submodules version_version="$(<"$version_path")" changes_version="$(awk '/^[0-9]/ {print $0; exit}' "$changes_path")" From f6dbba2ae4ad9eb3c470b30acce280a4fb6d0445 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 18:37:26 -0400 Subject: [PATCH 70/90] A couple more script tweaks for clarity - Because the substitition string after the hyphen is empty, "${VIRTUAL_ENV:-}" and "${VIRTUAL_ENV-}" have the same effect. However, the latter, which this changes it to, expresses the correct idea that the special case being handled is when the variable is unset: in this case, we expand an empty field rather than triggering an error due to set -u. When the variable is set but empty, it already expands to the substitution value, and including that in the special case with ":" is thus misleading. - Continuing in the vein of d18d90a (and 1e0b3f9), this removes another explicit newline by adding another echo command to print the leading blank line before the table. --- build-release.sh | 2 +- check-version.sh | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/build-release.sh b/build-release.sh index 4fd4a2251..49c13b93a 100755 --- a/build-release.sh +++ b/build-release.sh @@ -14,7 +14,7 @@ function suggest_venv() { printf "HELP: To avoid this error, use a virtual-env with '%s' instead.\n" "$venv_cmd" } -if test -n "${VIRTUAL_ENV:-}"; then +if test -n "${VIRTUAL_ENV-}"; then deps=(build twine) # Install twine along with build, as we need it later. echo "Virtual environment detected. Adding packages: ${deps[*]}" pip install --quiet --upgrade "${deps[@]}" diff --git a/check-version.sh b/check-version.sh index b47482d7e..dac386e46 100755 --- a/check-version.sh +++ b/check-version.sh @@ -41,7 +41,8 @@ head_sha="$(git rev-parse HEAD)" latest_tag_sha="$(git rev-parse "${latest_tag}^{commit}")" # Display a table of all the current version, tag, and HEAD commit information. -echo $'\nThe VERSION must be the same in all locations, and so must the HEAD and tag SHA' +echo +echo 'The VERSION must be the same in all locations, and so must the HEAD and tag SHA' printf '%-14s = %s\n' 'VERSION file' "$version_version" \ 'changes.rst' "$changes_version" \ 'Latest tag' "$latest_tag" \ From 5060c9dc42677c04af1b696e77228d17dac645a4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Sep 2023 19:05:25 -0400 Subject: [PATCH 71/90] Explain what each step in the init script achieves This adds comments to init-tests-after-clone.sh to explain what each of the steps is doing that is needed by some of the tests. It also refactors some recently added logic, in a way that lends itself to greater clarity when combined with these comments. --- init-tests-after-clone.sh | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index efa30a2dc..a53acbbef 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -13,14 +13,26 @@ if test -z "$TRAVIS"; then esac fi +# Stop if we have run this. (You can delete __testing_point__ to let it rerun.) +# This also keeps track of where we are, so we can get back here. git tag __testing_point__ + +# The tests need a branch called master. git checkout master -- || git checkout -b master + +# The tests need a reflog history on the master branch. git reset --hard HEAD~1 git reset --hard HEAD~1 git reset --hard HEAD~1 + +# Point the master branch where we started, so we test the correct code. git reset --hard __testing_point__ -test -z "$TRAVIS" || exit 0 # CI jobs will already have taken care of the rest. +# Do some setup that CI takes care of but that may not have been done locally. +if test -z "$TRAVIS"; then + # The tests needs some version tags. Try to get them even in forks. + git fetch --all --tags -git fetch --all --tags -git submodule update --init --recursive + # The tests need submodules, including a submodule with a submodule. + git submodule update --init --recursive +fi From d5479b206fd3a5815bad618d269ecb5e1577feb8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 01:40:33 -0400 Subject: [PATCH 72/90] Use set -u in init script This is already done in the other shell scripts. Although init-tests-after-clone.sh does not have as many places where a bug could slip through by an inadvertently nonexistent parameter, it does have $answer (and it may have more expansions in the future). --- init-tests-after-clone.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index a53acbbef..5ca767654 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -1,8 +1,8 @@ #!/bin/sh -set -e +set -eu -if test -z "$TRAVIS"; then +if test -z "${TRAVIS-}"; then printf 'This operation will destroy locally modified files. Continue ? [N/y]: ' >&2 read -r answer case "$answer" in @@ -29,7 +29,7 @@ git reset --hard HEAD~1 git reset --hard __testing_point__ # Do some setup that CI takes care of but that may not have been done locally. -if test -z "$TRAVIS"; then +if test -z "${TRAVIS-}"; then # The tests needs some version tags. Try to get them even in forks. git fetch --all --tags From 52f9a68d04233c3be9653a4a8b56a58afb9d7093 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 03:01:43 -0400 Subject: [PATCH 73/90] Make the "all" Makefile target more robust This is a minor improvement to the robustness of the command for "make all", in the face of plausible future target names. - Use [[:alpha:]] instead of [a-z] because, in POSIX BRE and ERE, whether [a-z] includes capital letters depends on the current collation order. (The goal here is greater consistency across systems, and for this it would also be okay to use [[:lower:]].) - Pass -x to the command that filters out the all target itself, so that the entire name must be "all", because a future target may contain the substring "all" as part of a larger word. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 38090244c..fe9d04a18 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ .PHONY: all clean release force_release all: - @grep -Ee '^[a-z].*:' Makefile | cut -d: -f1 | grep -vF all + @grep -E '^[[:alpha:]].*:' Makefile | cut -d: -f1 | grep -vxF all clean: rm -rf build/ dist/ .eggs/ .tox/ From b88d07e47667194e0668431e2a871926eb54d948 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 03:16:33 -0400 Subject: [PATCH 74/90] Use a single awk instead of two greps and a cut This seems simpler to me, but I admit it is subjective. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index fe9d04a18..d4f9acf87 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ .PHONY: all clean release force_release all: - @grep -E '^[[:alpha:]].*:' Makefile | cut -d: -f1 | grep -vxF all + @awk -F: '/^[[:alpha:]].*:/ && !/^all:/ {print $$1}' Makefile clean: rm -rf build/ dist/ .eggs/ .tox/ From d36818cf59d398e50cc6568f2239d69cd904883d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 03:33:59 -0400 Subject: [PATCH 75/90] Add a black check to pre-commit - Add the psf/black-pre-commit-mirror pre-commit hook but have it just check and report violations rather than fixing them automatically (to avoid inconsistency with all the other hooks, and also so that the linting instructions and CI lint workflow can remain the same and automatically do the black check). - Remove the "black" environment from tox.ini, since it is now part of the linting done in the "lint" environment. (But README.md remains the same, as it documented actually auto-formatting with black, which is still done the same way.) - Add missing "exclude" keys for the shellcheck and black pre-commit hooks to explicitly avoid traversing into the submodules. --- .pre-commit-config.yaml | 8 ++++++++ tox.ini | 7 +------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index bacc90913..d6b496a31 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,4 +1,11 @@ repos: + - repo: https://github.com/psf/black-pre-commit-mirror + rev: 23.9.1 + hooks: + - id: black + args: [--check, --diff] + exclude: ^git/ext/ + - repo: https://github.com/PyCQA/flake8 rev: 6.1.0 hooks: @@ -14,6 +21,7 @@ repos: hooks: - id: shellcheck args: [--color] + exclude: ^git/ext/ - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 diff --git a/tox.ini b/tox.ini index 82a41e22c..ed7896b59 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] requires = tox>=4 -env_list = py{37,38,39,310,311,312}, lint, mypy, black +env_list = py{37,38,39,310,311,312}, lint, mypy [testenv] description = Run unit tests @@ -20,11 +20,6 @@ base_python = py39 commands = mypy -p git ignore_outcome = true -[testenv:black] -description = Check style with black -base_python = py39 -commands = black --check --diff . - # Run "tox -e html" for this. It is deliberately excluded from env_list, as # unlike the other environments, this one writes outside the .tox/ directory. [testenv:html] From 4ba5ad107c4e91f62dacee9bfef277f2674fd90f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 04:27:07 -0400 Subject: [PATCH 76/90] Fix typo in comment --- init-tests-after-clone.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 5ca767654..5d1c16f0a 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -30,7 +30,7 @@ git reset --hard __testing_point__ # Do some setup that CI takes care of but that may not have been done locally. if test -z "${TRAVIS-}"; then - # The tests needs some version tags. Try to get them even in forks. + # The tests need some version tags. Try to get them even in forks. git fetch --all --tags # The tests need submodules, including a submodule with a submodule. From 5d8ddd9009ec38ecafddb55acfe7ad9919b1bb0d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 05:12:24 -0400 Subject: [PATCH 77/90] Use two hooks for black: to check, and format This splits the pre-commit hook for black into two hooks, both using the same repo and id but separately aliased: - black-check, which checks but does not modify files. This only runs when the manual stage is specified, and it is used by tox and on CI, with tox.ini and lint.yml modified accordingly. - black-format, which autoformats code. This provides the behavior most users will expect from a pre-commit hook for black. It runs automatically along with other hooks. But tox.ini and lint.yml, in addition to enabling the black-check hook, also explicitly skip the black-format hook. The effect is that in ordinary development the pre-commit hook for black does auto-formatting, but that pre-commit continues to be used effectively for running checks in which code should not be changed. --- .github/workflows/lint.yml | 4 ++++ .pre-commit-config.yaml | 8 ++++++++ README.md | 12 ++++++------ tox.ini | 4 +++- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 2204bb792..71e0a8853 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -14,3 +14,7 @@ jobs: python-version: "3.x" - uses: pre-commit/action@v3.0.0 + with: + extra_args: --hook-stage manual + env: + SKIP: black-format diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d6b496a31..5664fe980 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -3,8 +3,16 @@ repos: rev: 23.9.1 hooks: - id: black + alias: black-check + name: black (check) args: [--check, --diff] exclude: ^git/ext/ + stages: [manual] + + - id: black + alias: black-format + name: black (format) + exclude: ^git/ext/ - repo: https://github.com/PyCQA/flake8 rev: 6.1.0 diff --git a/README.md b/README.md index c17340f63..8cb6c88c2 100644 --- a/README.md +++ b/README.md @@ -142,22 +142,22 @@ To test, run: pytest ``` -To lint, run: +To lint, and apply automatic code formatting, run: ```bash pre-commit run --all-files ``` -To typecheck, run: +Code formatting can also be done by itself by running: -```bash -mypy -p git +``` +black . ``` -For automatic code formatting, run: +To typecheck, run: ```bash -black . +mypy -p git ``` Configuration for flake8 is in the `./.flake8` file. diff --git a/tox.ini b/tox.ini index ed7896b59..518577183 100644 --- a/tox.ini +++ b/tox.ini @@ -12,7 +12,9 @@ commands = pytest --color=yes {posargs} [testenv:lint] description = Lint via pre-commit base_python = py39 -commands = pre-commit run --all-files +set_env = + SKIP = black-format +commands = pre-commit run --all-files --hook-stage manual [testenv:mypy] description = Typecheck with mypy From a872d9c9c90b2a99c0b1a29e10aaecbe2fa387ed Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 15:29:10 -0400 Subject: [PATCH 78/90] Pass --all-files explicitly so it is retained In the lint workflow, passing extra-args replaced --all-files, rather than keeping it but adding the extra arguments after it. (But --show-diff-on-failure and --color=always were still passed.) --- .github/workflows/lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 71e0a8853..91dd919e0 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -15,6 +15,6 @@ jobs: - uses: pre-commit/action@v3.0.0 with: - extra_args: --hook-stage manual + extra_args: --all-files --hook-stage manual env: SKIP: black-format From 9b9de1133b85fd308c8795a4f312d3cfbf40b75f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 3 Oct 2023 15:34:28 -0400 Subject: [PATCH 79/90] Fix the formatting Now that the changes to lint.yml are fixed up, tested, and shown to be capable of revealing formatting errors, the formatting error I was using for testing (which is from 9fa1cee where I had introduced it inadvertently) can be fixed. --- test/test_git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_git.py b/test/test_git.py index 1ee7b3642..cf82d9ac7 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -43,7 +43,7 @@ def tearDown(self): def _assert_logged_for_popen(self, log_watcher, name, value): re_name = re.escape(name) re_value = re.escape(str(value)) - re_line = re.compile(fr"DEBUG:git.cmd:Popen\(.*\b{re_name}={re_value}[,)]") + re_line = re.compile(rf"DEBUG:git.cmd:Popen\(.*\b{re_name}={re_value}[,)]") match_attempts = [re_line.match(message) for message in log_watcher.output] self.assertTrue(any(match_attempts), repr(log_watcher.output)) From 5d1506352cdff5f3207c5942d82cdc628cb03f3c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 05:29:50 -0400 Subject: [PATCH 80/90] Add "make lint" to lint without auto-formatting As on CI and with tox (but not having to build and create and use a tox environment). This may not be the best way to do it, since currently the project's makefiles are otherwise used only for things directly related to building and publishing. However, this seemed like a readily available way to run the moderately complex command that produces the same behavior as on CI or with tox, but outside a tox environment. It may be possible to improve on this in the future. --- Makefile | 5 ++++- README.md | 7 ++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index d4f9acf87..839dc9f78 100644 --- a/Makefile +++ b/Makefile @@ -1,8 +1,11 @@ -.PHONY: all clean release force_release +.PHONY: all lint clean release force_release all: @awk -F: '/^[[:alpha:]].*:/ && !/^all:/ {print $$1}' Makefile +lint: + SKIP=black-format pre-commit run --all-files --hook-stage manual + clean: rm -rf build/ dist/ .eggs/ .tox/ diff --git a/README.md b/README.md index 8cb6c88c2..5ae43dc46 100644 --- a/README.md +++ b/README.md @@ -148,11 +148,8 @@ To lint, and apply automatic code formatting, run: pre-commit run --all-files ``` -Code formatting can also be done by itself by running: - -``` -black . -``` +- Linting without modifying code can be done with: `make lint` +- Auto-formatting without other lint checks can be done with: `black .` To typecheck, run: From 6de86a85e1d17e32cab8996aa66f10783e6beced Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 05:50:35 -0400 Subject: [PATCH 81/90] Update readme about most of the test/lint tools Including tox. --- README.md | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5ae43dc46..aac69e0df 100644 --- a/README.md +++ b/README.md @@ -157,12 +157,26 @@ To typecheck, run: mypy -p git ``` -Configuration for flake8 is in the `./.flake8` file. +#### CI (and tox) -Configurations for `mypy`, `pytest`, `coverage.py`, and `black` are in `./pyproject.toml`. +The same linting, and running tests on all the different supported Python versions, will be performed: -The same linting and testing will also be performed against different supported python versions -upon submitting a pull request (or on each push if you have a fork with a "main" branch and actions enabled). +- Upon submitting a pull request. +- On each push, *if* you have a fork with a "main" branch and GitHub Actions enabled. +- Locally, if you run [`tox`](https://tox.wiki/) (this skips any Python versions you don't have installed). + +#### Configuration files + +Specific tools: + +- Configurations for `mypy`, `pytest`, `coverage.py`, and `black` are in `./pyproject.toml`. +- Configuration for `flake8` is in the `./.flake8` file. + +Orchestration tools: + +- Configuration for `pre-commit` is in the `./.pre-commit-config.yaml` file. +- Configuration for `tox` is in `./tox.ini`. +- Configuration for GitHub Actions (CI) is in files inside `./.github/workflows/`. ### Contributions From f0949096f4a2dec466bce48d46a4bf2dd598d36f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 06:31:05 -0400 Subject: [PATCH 82/90] Add BUILDDIR var to doc/Makefile; have tox use it This adds BUILDDIR as a variable in the documentation generation makefile that, along SPHINXOPTS, SPHINXBUILD, and PAPER, defaults to the usual best value but can be set when invoking make. The main use for choosing a different build output directory is to test building without overwriting or otherwise interfering with any files from a build one may really want to use. tox.ini is thus modified to pass a path pointing inside its temporary directory as BUILDDIR, so the "html" tox environment now makes no changes outside the .tox directory. This is thus suitable for being run automatically, so the "html" environment is now in the envlist. --- doc/Makefile | 43 ++++++++++++++++++++++--------------------- tox.ini | 8 ++++---- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/doc/Makefile b/doc/Makefile index ef2d60e5f..ddeadbd7e 100644 --- a/doc/Makefile +++ b/doc/Makefile @@ -2,6 +2,7 @@ # # You can set these variables from the command line. +BUILDDIR = build SPHINXOPTS = -W SPHINXBUILD = sphinx-build PAPER = @@ -9,7 +10,7 @@ PAPER = # Internal variables. PAPEROPT_a4 = -D latex_paper_size=a4 PAPEROPT_letter = -D latex_paper_size=letter -ALLSPHINXOPTS = -d build/doctrees $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) source +ALLSPHINXOPTS = -d $(BUILDDIR)/doctrees $(PAPEROPT_$(PAPER)) $(SPHINXOPTS) source .PHONY: help clean html web pickle htmlhelp latex changes linkcheck @@ -24,52 +25,52 @@ help: @echo " linkcheck to check all external links for integrity" clean: - -rm -rf build/* + -rm -rf $(BUILDDIR)/* html: - mkdir -p build/html build/doctrees - $(SPHINXBUILD) -b html $(ALLSPHINXOPTS) build/html + mkdir -p $(BUILDDIR)/html $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html @echo - @echo "Build finished. The HTML pages are in build/html." + @echo "Build finished. The HTML pages are in $(BUILDDIR)/html." pickle: - mkdir -p build/pickle build/doctrees - $(SPHINXBUILD) -b pickle $(ALLSPHINXOPTS) build/pickle + mkdir -p $(BUILDDIR)/pickle $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b pickle $(ALLSPHINXOPTS) $(BUILDDIR)/pickle @echo @echo "Build finished; now you can process the pickle files." web: pickle json: - mkdir -p build/json build/doctrees - $(SPHINXBUILD) -b json $(ALLSPHINXOPTS) build/json + mkdir -p $(BUILDDIR)/json $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b json $(ALLSPHINXOPTS) $(BUILDDIR)/json @echo @echo "Build finished; now you can process the JSON files." htmlhelp: - mkdir -p build/htmlhelp build/doctrees - $(SPHINXBUILD) -b htmlhelp $(ALLSPHINXOPTS) build/htmlhelp + mkdir -p $(BUILDDIR)/htmlhelp $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b htmlhelp $(ALLSPHINXOPTS) $(BUILDDIR)/htmlhelp @echo @echo "Build finished; now you can run HTML Help Workshop with the" \ - ".hhp project file in build/htmlhelp." + ".hhp project file in $(BUILDDIR)/htmlhelp." latex: - mkdir -p build/latex build/doctrees - $(SPHINXBUILD) -b latex $(ALLSPHINXOPTS) build/latex + mkdir -p $(BUILDDIR)/latex $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b latex $(ALLSPHINXOPTS) $(BUILDDIR)/latex @echo - @echo "Build finished; the LaTeX files are in build/latex." + @echo "Build finished; the LaTeX files are in $(BUILDDIR)/latex." @echo "Run \`make all-pdf' or \`make all-ps' in that directory to" \ "run these through (pdf)latex." changes: - mkdir -p build/changes build/doctrees - $(SPHINXBUILD) -b changes $(ALLSPHINXOPTS) build/changes + mkdir -p $(BUILDDIR)/changes $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b changes $(ALLSPHINXOPTS) $(BUILDDIR)/changes @echo - @echo "The overview file is in build/changes." + @echo "The overview file is in $(BUILDDIR)/changes." linkcheck: - mkdir -p build/linkcheck build/doctrees - $(SPHINXBUILD) -b linkcheck $(ALLSPHINXOPTS) build/linkcheck + mkdir -p $(BUILDDIR)/linkcheck $(BUILDDIR)/doctrees + $(SPHINXBUILD) -b linkcheck $(ALLSPHINXOPTS) $(BUILDDIR)/linkcheck @echo @echo "Link check complete; look for any errors in the above output " \ - "or in build/linkcheck/output.txt." + "or in $(BUILDDIR)/linkcheck/output.txt." diff --git a/tox.ini b/tox.ini index 518577183..47a7a6209 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] requires = tox>=4 -env_list = py{37,38,39,310,311,312}, lint, mypy +env_list = py{37,38,39,310,311,312}, lint, mypy, html [testenv] description = Run unit tests @@ -22,11 +22,11 @@ base_python = py39 commands = mypy -p git ignore_outcome = true -# Run "tox -e html" for this. It is deliberately excluded from env_list, as -# unlike the other environments, this one writes outside the .tox/ directory. [testenv:html] description = Build HTML documentation base_python = py39 deps = -r doc/requirements.txt allowlist_externals = make -commands = make -C doc html +commands = + make BUILDDIR={env_tmp_dir}/doc/build -C doc clean + make BUILDDIR={env_tmp_dir}/doc/build -C doc html From fc969807086d4483c4c32b80d2c2b67a6c6813e7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 15:27:11 -0400 Subject: [PATCH 83/90] Have init script check for GitHub Actions As well as still checking for Travis, for backward compatibility and because experience shows that this is safe. The check can be much broader, and would be more accurate, with fewer false negatives. But a false positive can result in local data loss because the script does hard resets on CI without prompting for confirmation. So for now, this just checks $TRAVIS and $GITHUB_ACTIONS. Now that GHA is included, the CI workflows no longer need to set $TRAVIS when running the script, so that is removed. --- .github/workflows/cygwin-test.yml | 2 +- .github/workflows/pythonpackage.yml | 2 +- init-tests-after-clone.sh | 9 +++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index e818803f1..cd913385f 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -40,7 +40,7 @@ jobs: - name: Prepare this repo for tests run: | - TRAVIS=yes ./init-tests-after-clone.sh + ./init-tests-after-clone.sh - name: Set git user identity and command aliases for the tests run: | diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index e43317807..2a82e0e03 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -37,7 +37,7 @@ jobs: - name: Prepare this repo for tests run: | - TRAVIS=yes ./init-tests-after-clone.sh + ./init-tests-after-clone.sh - name: Set git user identity and command aliases for the tests run: | diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 5d1c16f0a..4697c2ecc 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -2,7 +2,12 @@ set -eu -if test -z "${TRAVIS-}"; then +ci() { + # For now, check just these, as a false positive could lead to data loss. + test -n "${TRAVIS-}" || test -n "${GITHUB_ACTIONS-}" +} + +if ! ci; then printf 'This operation will destroy locally modified files. Continue ? [N/y]: ' >&2 read -r answer case "$answer" in @@ -29,7 +34,7 @@ git reset --hard HEAD~1 git reset --hard __testing_point__ # Do some setup that CI takes care of but that may not have been done locally. -if test -z "${TRAVIS-}"; then +if ! ci; then # The tests need some version tags. Try to get them even in forks. git fetch --all --tags From b98f15e46a7d5f343b1303b55bc4dae2d18bd621 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Sep 2023 23:59:14 -0400 Subject: [PATCH 84/90] Get tags for tests from original repo as fallback This extends the init script so it tries harder to get version tags: - As before, locally, "git fetch --all --tags" is always run. (This also fetches other objects, and the developer experience would be undesirably inconsistent if that were only sometimes done.) - On CI, run it if version tags are absent. The criterion followed here and in subsequent steps is that if any existing tag starts with a digit, or with the letter v followed by a digit, we regard version tags to be present. This is to balance the benefit of getting the tags (to make the tests work) against the risk of creating a very confusing situation in clones of forks that publish packages or otherwise use their own separate versioning scheme (especially if those tags later ended up being pushed). - Both locally and on CI, after that, if version tags are absent, try to copy them from the original gitpython-developers/GitPython repo, without copying other tags or adding that repo as a remote. Copy only tags that start with a digit, since v+digit tags aren't currently used in this project (though forks may use them). This is a fallback option and it always displays a warning. If it fails, another message is issued for that. Unexpected failure to access the repo terminates the script with a failing exit status, but the absence of version tags in the fallback remote does not, so CI jobs can continue and reveal which tests fail as a result. On GitHub Actions CI specifically, the Actions syntax for creating a warning annotation on the workflow is used, but the warning is still also written to stderr (as otherwise). GHA workflow annotations don't support multi-line warnings, so the message is adjusted into a single line where needed. --- init-tests-after-clone.sh | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 4697c2ecc..5e31e3315 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -2,11 +2,25 @@ set -eu +fallback_repo_for_tags='https://github.com/gitpython-developers/GitPython.git' + ci() { # For now, check just these, as a false positive could lead to data loss. test -n "${TRAVIS-}" || test -n "${GITHUB_ACTIONS-}" } +no_version_tags() { + test -z "$(git tag -l '[0-9]*' 'v[0-9]*')" +} + +warn() { + printf '%s\n' "$@" >&2 # Warn in step output. + + if test -n "${GITHUB_ACTIONS-}"; then + printf '::warning ::%s\n' "$*" >&2 # Annotate workflow. + fi +} + if ! ci; then printf 'This operation will destroy locally modified files. Continue ? [N/y]: ' >&2 read -r answer @@ -33,11 +47,27 @@ git reset --hard HEAD~1 # Point the master branch where we started, so we test the correct code. git reset --hard __testing_point__ -# Do some setup that CI takes care of but that may not have been done locally. +# The tests need submodules. (On CI, they would already have been checked out.) if ! ci; then - # The tests need some version tags. Try to get them even in forks. + git submodule update --init --recursive +fi + +# The tests need some version tags. Try to get them even in forks. This fetch +# gets other objects too, so for a consistent experience, always do it locally. +if ! ci || no_version_tags; then git fetch --all --tags +fi - # The tests need submodules, including a submodule with a submodule. - git submodule update --init --recursive +# If we still have no version tags, try to get them from the original repo. +if no_version_tags; then + warn 'No local or remote version tags found. Trying fallback remote:' \ + "$fallback_repo_for_tags" + + # git fetch supports * but not [], and --no-tags means no *other* tags, so... + printf 'refs/tags/%d*:refs/tags/%d*\n' 0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8 9 9 | + xargs git fetch --no-tags "$fallback_repo_for_tags" + + if no_version_tags; then + warn 'No version tags found anywhere. Some tests will fail.' + fi fi From 7cca7d2245a504047188943623cc58c4c2e0c35f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Sep 2023 10:28:17 -0400 Subject: [PATCH 85/90] Don't print the exact same warning twice In the step output, the warning that produces a workflow annotation is fully readable (and even made to stand out), so it doesn't need to be printed in the plain way as well, which can be reserved for when GitHub Actions is not detected. --- init-tests-after-clone.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 5e31e3315..49df49daa 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -14,10 +14,10 @@ no_version_tags() { } warn() { - printf '%s\n' "$@" >&2 # Warn in step output. - if test -n "${GITHUB_ACTIONS-}"; then printf '::warning ::%s\n' "$*" >&2 # Annotate workflow. + else + printf '%s\n' "$@" >&2 fi } From e4e009d03b890d456b4c1ff2411591d3a50dcdd0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Sep 2023 16:59:58 -0400 Subject: [PATCH 86/90] Reword comment to fix ambiguity --- init-tests-after-clone.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index 49df49daa..21d1f86d8 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -52,8 +52,8 @@ if ! ci; then git submodule update --init --recursive fi -# The tests need some version tags. Try to get them even in forks. This fetch -# gets other objects too, so for a consistent experience, always do it locally. +# The tests need some version tags. Try to get them even in forks. This fetches +# other objects too. So, locally, we always do it, for a consistent experience. if ! ci || no_version_tags; then git fetch --all --tags fi From e16e4c0099cd0197b5c80ce6ec9a6b4bca41845e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Sep 2023 18:58:08 -0400 Subject: [PATCH 87/90] Format all YAML files in the same style --- .github/dependabot.yml | 2 +- .pre-commit-config.yaml | 68 ++++++++++++++++++++--------------------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 203f3c889..8c139c7be 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -3,4 +3,4 @@ updates: - package-ecosystem: "github-actions" directory: "/" schedule: - interval: "weekly" + interval: "weekly" diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5664fe980..be97d5f9b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,39 +1,39 @@ repos: - - repo: https://github.com/psf/black-pre-commit-mirror - rev: 23.9.1 - hooks: - - id: black - alias: black-check - name: black (check) - args: [--check, --diff] - exclude: ^git/ext/ - stages: [manual] +- repo: https://github.com/psf/black-pre-commit-mirror + rev: 23.9.1 + hooks: + - id: black + alias: black-check + name: black (check) + args: [--check, --diff] + exclude: ^git/ext/ + stages: [manual] - - id: black - alias: black-format - name: black (format) - exclude: ^git/ext/ + - id: black + alias: black-format + name: black (format) + exclude: ^git/ext/ - - repo: https://github.com/PyCQA/flake8 - rev: 6.1.0 - hooks: - - id: flake8 - additional_dependencies: - - flake8-bugbear==23.9.16 - - flake8-comprehensions==3.14.0 - - flake8-typing-imports==1.14.0 - exclude: ^doc|^git/ext/ +- repo: https://github.com/PyCQA/flake8 + rev: 6.1.0 + hooks: + - id: flake8 + additional_dependencies: + - flake8-bugbear==23.9.16 + - flake8-comprehensions==3.14.0 + - flake8-typing-imports==1.14.0 + exclude: ^doc|^git/ext/ - - repo: https://github.com/shellcheck-py/shellcheck-py - rev: v0.9.0.5 - hooks: - - id: shellcheck - args: [--color] - exclude: ^git/ext/ +- repo: https://github.com/shellcheck-py/shellcheck-py + rev: v0.9.0.5 + hooks: + - id: shellcheck + args: [--color] + exclude: ^git/ext/ - - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.4.0 - hooks: - - id: check-toml - - id: check-yaml - - id: check-merge-conflict +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v4.4.0 + hooks: + - id: check-toml + - id: check-yaml + - id: check-merge-conflict From 62c024e277820b2fd3764a0499a71f03d4aa432d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Sep 2023 19:33:07 -0400 Subject: [PATCH 88/90] Let tox run lint, mypy, and html envs without 3.9 The tox environments that are not duplicated per Python version were set to run on Python 3.9, for consistency with Cygwin, where 3.9 is the latest available (through official Cygwin packages), so output can be compared between Cygwin and other platforms while troubleshooting problems. However, this prevented them from running altogether on systems where Python 3.9 was not installed. So I've added all the other Python versions the project supports as fallback versions for those tox environments to use, in one of the reasonable precedence orders, while keeping 3.9 as the first choice for the same reasons as before. --- tox.ini | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tox.ini b/tox.ini index 47a7a6209..f9ac25b78 100644 --- a/tox.ini +++ b/tox.ini @@ -11,20 +11,20 @@ commands = pytest --color=yes {posargs} [testenv:lint] description = Lint via pre-commit -base_python = py39 +base_python = py{39,310,311,312,38,37} set_env = SKIP = black-format commands = pre-commit run --all-files --hook-stage manual [testenv:mypy] description = Typecheck with mypy -base_python = py39 +base_python = py{39,310,311,312,38,37} commands = mypy -p git ignore_outcome = true [testenv:html] description = Build HTML documentation -base_python = py39 +base_python = py{39,310,311,312,38,37} deps = -r doc/requirements.txt allowlist_externals = make commands = From 9e245d0561ca2f6249e9435a04761da2c64977f1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Oct 2023 00:03:16 -0400 Subject: [PATCH 89/90] Update readme: CI jobs not just for "main" branch This changed a while ago but README.md still listed having a main branch as a condition for automatic CI linting and testing in a fork. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index aac69e0df..021aab15f 100644 --- a/README.md +++ b/README.md @@ -162,7 +162,7 @@ mypy -p git The same linting, and running tests on all the different supported Python versions, will be performed: - Upon submitting a pull request. -- On each push, *if* you have a fork with a "main" branch and GitHub Actions enabled. +- On each push, *if* you have a fork with GitHub Actions enabled. - Locally, if you run [`tox`](https://tox.wiki/) (this skips any Python versions you don't have installed). #### Configuration files From c2472e9b57afde98bb18ada55ae978721a27713d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Oct 2023 00:11:37 -0400 Subject: [PATCH 90/90] Note that the init script can be run from Git Bash This is probably the *only* way anyone should run that script on Windows, but I don't know of specific bad things that happen if it is run in some other way, such as with WSL bash, aside from messing up line endings, which users are likely to notice anyway. This commit also clarifies the instructions by breaking up another paragraph that really represented two separate steps. --- README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 021aab15f..69fb54c9f 100644 --- a/README.md +++ b/README.md @@ -79,13 +79,17 @@ cd GitPython ./init-tests-after-clone.sh ``` +On Windows, `./init-tests-after-clone.sh` can be run in a Git Bash shell. + If you are cloning [your own fork](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-forks), then replace the above `git clone` command with one that gives the URL of your fork. Or use this [`gh`](https://cli.github.com/) command (assuming you have `gh` and your fork is called `GitPython`): ```bash gh repo clone GitPython ``` -Having cloned the repo, create and activate your [virtual environment](https://docs.python.org/3/tutorial/venv.html). Then make an [editable install](https://pip.pypa.io/en/stable/topics/local-project-installs/#editable-installs): +Having cloned the repo, create and activate your [virtual environment](https://docs.python.org/3/tutorial/venv.html). + +Then make an [editable install](https://pip.pypa.io/en/stable/topics/local-project-installs/#editable-installs): ```bash pip install -e ".[test]"