diff --git a/.github/workflows/alpine-test.yml b/.github/workflows/alpine-test.yml index b7de7482e..a5b63f423 100644 --- a/.github/workflows/alpine-test.yml +++ b/.github/workflows/alpine-test.yml @@ -1,6 +1,10 @@ name: test-alpine -on: [push, pull_request, workflow_dispatch] +on: + push: + branches: [main] + pull_request: + workflow_dispatch: permissions: contents: read @@ -26,7 +30,7 @@ jobs: adduser runner docker shell: sh -exo pipefail {0} # Run this as root, not the "runner" user. - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -61,6 +65,27 @@ jobs: . .venv/bin/activate pip install '.[test]' + - name: Show POSIX file ownership + run: | + ls -ld -- \ + "$(pwd)" \ + "$(pwd)/.git" \ + "$(pwd)/git/ext/gitdb" \ + "$(pwd)/git/ext/gitdb/.git" \ + "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \ + "$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \ + "${HOME:?HOME is not set}/.gitconfig" \ + 2>&1 || true + + - name: Show safe.directory entries + # `actions/checkout`'s safe.directory add is only durable for the + # checkout itself (it writes under a throwaway HOME override and + # then discards it), so by the time this step runs the runner + # user's `~/.gitconfig` has no entries -- and the Alpine container + # chowns the workspace to runner:docker to match the test user, so + # git accepts the ownership without one. Expected: `(none)`. + run: git config --global --get-all safe.directory || echo "(none)" + - name: Show version and platform information run: | . .venv/bin/activate diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index e243416a8..1ad1a612c 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -13,6 +13,7 @@ name: "CodeQL" on: push: + branches: [main] pull_request: schedule: - cron: '27 10 * * 3' @@ -47,7 +48,7 @@ jobs: # your codebase is analyzed, see https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages steps: - name: Checkout repository - uses: actions/checkout@v6 + uses: actions/checkout@v7 # Add any setup steps before running the `github/codeql-action/init` action. # This includes steps like installing compilers or runtimes (`actions/setup-node` diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 327e1f10c..6f9f347a9 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -1,6 +1,10 @@ name: test-cygwin -on: [push, pull_request, workflow_dispatch] +on: + push: + branches: [main] + pull_request: + workflow_dispatch: permissions: contents: read @@ -34,7 +38,7 @@ jobs: git config --global core.autocrlf false # Affects the non-Cygwin git. shell: pwsh # Do this outside Cygwin, to affect actions/checkout. - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -53,6 +57,8 @@ jobs: run: | git config --global --add safe.directory "$(pwd)" git config --global --add safe.directory "$(pwd)/.git" + git config --global --add safe.directory "$(pwd)/git/ext/gitdb" + git config --global --add safe.directory "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" git config --global core.autocrlf false - name: Prepare this repo for tests @@ -80,6 +86,58 @@ jobs: run: | pip install '.[test]' + - name: Show POSIX file ownership + # Cygwin's `ls -ld` reports the NTFS Owner SID via Cygwin's SID-to-uid + # mapping (well-known SIDs by their RID, machine-local accounts by + # 0x30000+RID). That mapping is what Cygwin git's + # `is_path_owned_by_current_user` reduces to, so this is the view that + # determines whether `safe.directory` is consulted. + run: | + ls -ld -- \ + "$(pwd)" \ + "$(pwd)/.git" \ + "$(pwd)/git/ext/gitdb" \ + "$(pwd)/git/ext/gitdb/.git" \ + "$(pwd)/.git/modules/gitdb" \ + "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \ + "$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \ + "$(pwd)/.git/modules/gitdb/modules/smmap" \ + "${HOME:?HOME is not set}/.gitconfig" \ + 2>&1 || true + + - name: Show NTFS file ownership + # Authoritative NTFS Owner via Get-Acl, with no Cygwin SID-to-uid layer + # in between -- useful for confirming what the Cygwin view reports as + # "Administrators" is the BUILTIN\Administrators SID (S-1-5-32-544). + shell: pwsh + run: | + $paths = @( + "$pwd", + "$pwd\.git", + "$pwd\git\ext\gitdb", + "$pwd\git\ext\gitdb\.git", + "$pwd\.git\modules\gitdb", + "$pwd\git\ext\gitdb\gitdb\ext\smmap", + "$pwd\git\ext\gitdb\gitdb\ext\smmap\.git", + "$pwd\.git\modules\gitdb\modules\smmap", + "$env:USERPROFILE\.gitconfig" + ) + foreach ($p in $paths) { + if (Test-Path -LiteralPath $p) { + try { + $owner = (Get-Acl -LiteralPath $p).Owner + } catch { + $owner = "ERROR: $($_.Exception.Message)" + } + "{0,-44} {1}" -f $owner, $p + } else { + "(missing: $p)" + } + } + + - name: Show safe.directory entries + run: git config --global --get-all safe.directory + - name: Show version and platform information run: | uname -a diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index e32e946c8..79d786669 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -1,6 +1,10 @@ name: Lint -on: [push, pull_request, workflow_dispatch] +on: + push: + branches: [main] + pull_request: + workflow_dispatch: permissions: contents: read @@ -10,7 +14,7 @@ jobs: runs-on: ubuntu-slim steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - uses: actions/setup-python@v6 with: diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 874e18a8f..fe3e26236 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -3,7 +3,11 @@ name: Python package -on: [push, pull_request, workflow_dispatch] +on: + push: + branches: [main] + pull_request: + workflow_dispatch: permissions: contents: read @@ -50,7 +54,7 @@ jobs: shell: bash --noprofile --norc -exo pipefail {0} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -87,6 +91,62 @@ jobs: run: | pip install '.[test]' + - name: Show POSIX file ownership + # Linux and macOS only. On Windows, Git Bash's `ls -ld` reports a + # uniform uid+gid for every path regardless of NTFS Owner (MSYS2's + # SID-to-uid mapping doesn't have Cygwin's fidelity), so it would + # not be informative here. The NTFS Owner check below covers Windows. + if: matrix.os-type != 'windows' + run: | + ls -ld -- \ + "$(pwd)" \ + "$(pwd)/.git" \ + "$(pwd)/git/ext/gitdb" \ + "$(pwd)/git/ext/gitdb/.git" \ + "$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \ + "$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \ + "${HOME:?HOME is not set}/.gitconfig" \ + 2>&1 || true + + - name: Show NTFS file ownership + # Windows only. Reads NTFS Owner directly via Get-Acl, which is the + # authoritative view for Windows-side ownership questions; the POSIX + # view via Git Bash's MSYS2 layer is not a reliable proxy here. + if: matrix.os-type == 'windows' + shell: pwsh + run: | + $paths = @( + "$pwd", + "$pwd\.git", + "$pwd\git\ext\gitdb", + "$pwd\git\ext\gitdb\.git", + "$pwd\git\ext\gitdb\gitdb\ext\smmap", + "$pwd\git\ext\gitdb\gitdb\ext\smmap\.git", + "$env:USERPROFILE\.gitconfig" + ) + foreach ($p in $paths) { + if (Test-Path -LiteralPath $p) { + try { + $owner = (Get-Acl -LiteralPath $p).Owner + } catch { + $owner = "ERROR: $($_.Exception.Message)" + } + "{0,-44} {1}" -f $owner, $p + } else { + "(missing: $p)" + } + } + + - name: Show safe.directory entries + # `actions/checkout`'s safe.directory add is only durable for the + # checkout itself (it writes under a throwaway HOME override and + # then discards it), so by the time this step runs the runner + # user's `~/.gitconfig` has no entries -- and git accepts the + # workspace's ownership anyway: Git for Windows via its + # Admins-group exemption on the windows matrix; on Linux/macOS + # the workspace is owned by the test user. Expected: `(none)`. + run: git config --global --get-all safe.directory || echo "(none)" + - name: Show version and platform information run: | uname -a diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f3ab67035..a13e85d40 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -7,7 +7,7 @@ repos: exclude: ^test/fixtures/ - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.15.12 + rev: v0.15.20 hooks: - id: ruff-check args: ["--fix"] diff --git a/AUTHORS b/AUTHORS index 15333e1e5..95205f439 100644 --- a/AUTHORS +++ b/AUTHORS @@ -57,5 +57,6 @@ Contributors are: -Jonas Scharpf -Gordon Marx -Enji Cooper +-Harshita Yadav Portions derived from other open source works and are clearly marked. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8536d7f73..60e34a651 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -9,6 +9,37 @@ The following is a short step-by-step rundown of what one typically would do to - Feel free to add yourself to AUTHORS file. - Create a pull request. +## Quality expectations + +Contributions must be made with care and meet the quality bar of the surrounding code. +That means a change should not leave GitPython worse than it was before: it should be +readable, maintainable, tested where practical, documented and consistent with the +existing style and behavior. + +A contribution that works only narrowly but lowers the quality of the +codebase may be declined. The maintainers may not always be able to provide +detailed feedback. + +## AI-assisted contributions + +If AI edits files for you, disclose it in the pull request description and commit +metadata. Prefer making the agent identity part of the commit, for example by using +an AI author such as `$agent $version ` or a co-author via +a `Co-authored-by: ` trailer. + +Agents operating through a person's GitHub account must identify themselves. For +example, comments posted by an agent should say so directly with phrases like +`AI agent on behalf of : ...`. + +Fully AI-generated comments on pull requests or issues must also be disclosed. +Undisclosed AI-generated comments may lead to the pull request or issue being closed. + +AI-assisted proofreading or wording polish does not need disclosure, but it is still +courteous to mention it when the AI materially influenced the final text. + +Automated or "full-auto" AI contributions without a human responsible for reviewing +and standing behind the work may be closed. + ## Fuzzing Test Specific Documentation For details related to contributing to the fuzzing test suite and OSS-Fuzz integration, please diff --git a/VERSION b/VERSION index 0bc461141..9e5b44197 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.1.50 +3.1.51 diff --git a/doc/source/changes.rst b/doc/source/changes.rst index b5152b3c5..8337478dd 100644 --- a/doc/source/changes.rst +++ b/doc/source/changes.rst @@ -2,6 +2,23 @@ Changelog ========= +3.1.51 +====== + +This is primarily a security release. It prevents additional argument-injection +paths that could allow execution of arbitrary commands or writing to arbitrary +files through unsafe Git options (GHSA-956x-8gvw-wg5v), and closes bypasses of +the existing protections using abbreviated long options or joined short options +(GHSA-2f96-g7mh-g2hx). + +The release also improves support for relative worktree paths and diffs against +the empty tree, preserves stderr from failed diff processes, supports relative +configuration include paths, and includes assorted documentation, typing, test, +and CI improvements. + +See the following for all changes. +https://github.com/gitpython-developers/GitPython/releases/tag/3.1.51 + 3.1.50 ====== diff --git a/git/cmd.py b/git/cmd.py index 096900819..2a4f36024 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -649,6 +649,11 @@ class Git(metaclass=_GitMeta): re_unsafe_protocol = re.compile(r"(.+)::.+") + unsafe_git_ls_remote_options = [ + # This option allows arbitrary command execution in git-ls-remote. + "--upload-pack", + ] + def __getstate__(self) -> Dict[str, Any]: return slots_to_dict(self, exclude=self._excluded_) @@ -961,17 +966,80 @@ def _canonicalize_option_name(cls, option: str) -> str: @classmethod def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> None: - """Check for unsafe options. - - Some options that are passed to ``git `` can be used to execute - arbitrary commands. These are blocked by default. + """Raise :class:`~git.exc.UnsafeOptionError` for blocked option spellings. + + In addition to exact matches, this rejects abbreviated long options accepted + by Git (for example, ``--upl`` for ``--upload-pack``) and unsafe short options + whose values are joined to the same token, including after clusterable flags + (for example, ``-uVALUE`` and ``-fuVALUE``). + + A list containing only bare names is treated as normalized keyword arguments, + so multi-character names such as ``upload_p`` are checked as long-option + abbreviations. If any item starts with ``-``, the list is treated as tokenized + command-line input: bare items can be option values and are not checked as + abbreviations. Thus ``["--origin", "upload"]`` is allowed. Single-dash options + use short-option parsing rather than broad prefix matching, preserving safe + attached values such as ``-oupstream`` and ``-bcurrent``. + + Some options passed to ``git `` can execute arbitrary commands and + are therefore blocked by default unless the caller explicitly allows them. """ # Options can be of the form `foo`, `--foo`, `--foo bar`, or `--foo=bar`. + # Git accepts any unambiguous prefix of a long option, so an abbreviated + # spelling such as `--upl` for `--upload-pack` must be rejected too. An + # option is unsafe if its canonical name is a prefix of any blocked + # option's canonical name. Only long options and multi-character kwargs + # can be abbreviations; single-character short options remain exact-match + # only. canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options} + unsafe_short_options = { + canonical: option + for canonical, option in canonical_unsafe_options.items() + if option.startswith("-") and not option.startswith("--") and len(canonical) == 1 + } + # These value-less Git flags can be clustered before another short option + # (for example, ``-fuVALUE``). Stop at any other character because it may + # begin an attached value, as ``o`` does in the safe option ``-oupstream``. + clusterable_short_options = frozenset("46flnqsv") + options_are_kwargs = all(not option.startswith("-") for option in options) for option in options: - unsafe_option = canonical_unsafe_options.get(cls._canonicalize_option_name(option)) + candidate = cls._canonicalize_option_name(option) + if not candidate: + continue + unsafe_option = canonical_unsafe_options.get(candidate) if unsafe_option is not None: raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.") + option_token = option.split("=", 1)[0].split(None, 1)[0] + if option_token.startswith("-") and not option_token.startswith("--"): + for option_char in option_token[1:]: + unsafe_option = unsafe_short_options.get(option_char) + if unsafe_option is not None: + raise UnsafeOptionError( + f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it." + ) + if option_char not in clusterable_short_options: + break + if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)): + continue + for canonical, unsafe_option in canonical_unsafe_options.items(): + if canonical.startswith(candidate): + raise UnsafeOptionError( + f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it." + ) + + @classmethod + def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[str, Any]] = None) -> List[str]: + """Collect possible option spellings before command-line transformation.""" + options = [ + option for option in cls._unpack_args([arg for arg in args if arg is not None]) if option.startswith("-") + ] + if kwargs: + for key, value in kwargs.items(): + values = value if isinstance(value, (list, tuple)) else (value,) + if any(value is True or (value is not False and value is not None) for value in values): + key = str(key) + options.append(f"-{key}" if len(key) == 1 else f"--{dashify(key)}") + return options AutoInterrupt: TypeAlias = _AutoInterrupt @@ -1030,6 +1098,22 @@ def set_persistent_git_options(self, **kwargs: Any) -> None: self._persistent_git_options = self.transform_kwargs(split_single_char_options=True, **kwargs) + def ls_remote( + self, + *args: Any, + allow_unsafe_options: bool = False, + **kwargs: Any, + ) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], "Git.AutoInterrupt"]: + """List references in a remote repository. + + :param allow_unsafe_options: + Allow unsafe options, like ``--upload-pack``. + """ + if not allow_unsafe_options: + candidate_options = self._option_candidates(args, kwargs) + Git.check_unsafe_options(options=candidate_options, unsafe_options=self.unsafe_git_ls_remote_options) + return self._call_process("ls_remote", *args, **kwargs) + @property def working_dir(self) -> Union[None, PathLike]: """:return: Git directory we are working on""" @@ -1131,9 +1215,28 @@ def execute( information (stdout). :param command: - The command argument list to execute. - 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. + The command to execute. A sequence of program arguments is recommended. + A string is also accepted, but its meaning is strongly platform-dependent. + + By default, a shell is not used. On Unix-like systems, a string is the whole + program name (so ``"git log -n 1"`` raises :class:`GitCommandNotFound`). On + Windows, the program parses the arguments itself, so multi-word strings can + work but are not portable. + + Avoid ``shell=True`` (and :attr:`Git.USE_SHELL`): this runs the command in + a shell, which is generally unsafe. The shell interprets metacharacters + such as ``;``, ``|``, ``&``, ``$(...)``, ``$VAR``, ``%VAR%``, and ``^`` + (depending on the platform) as syntax. Any untrusted text in the command + can then execute arbitrary OS commands. See :attr:`Git.USE_SHELL`. + + Producing a sequence automatically by :func:`shlex.split` and passing it + as the command is far safer than ``shell=True``. But :func:`shlex.split` + parses POSIX shell syntax on all systems, and the result is still unsafe + for anything but *fixed, fully trusted* strings. Do not use it on strings + built by interpolating values: whitespace or quoting in an untrusted value + can still inject arguments. For input derived in any way from untrusted + data, build the argument sequence yourself, while ensuring each argument + is fully sanitized. :param istream: Standard input filehandle passed to :class:`subprocess.Popen`. @@ -1201,6 +1304,11 @@ def execute( needed (nor useful) to work around any known operating system specific issues. + On Unix-like systems, when migrating away from passing string commands with + ``shell=True``, :func:`shlex.split` may serve as a transitional step in rare + cases, with extreme care. (Drop ``shell=True`` and pass the resulting + sequence as the command.) See the `command` parameter above on the risks. + :param env: A dictionary of environment variables to be passed to :class:`subprocess.Popen`. @@ -1512,7 +1620,7 @@ def transform_kwargs(self, split_single_char_options: bool = True, **kwargs: Any return args @classmethod - def _unpack_args(cls, arg_list: Sequence[str]) -> List[str]: + def _unpack_args(cls, arg_list: Sequence[Any]) -> List[str]: outlist = [] if isinstance(arg_list, (list, tuple)): for arg in arg_list: @@ -1588,7 +1696,7 @@ def _call_process( turns into:: - git rev-list --max-count=10 --header=master + git rev-list --max-count=10 --header master :return: Same as :meth:`execute`. If no args are given, used :meth:`execute`'s diff --git a/git/config.py b/git/config.py index 82747eadd..64f501424 100644 --- a/git/config.py +++ b/git/config.py @@ -634,6 +634,8 @@ def read(self) -> None: # type: ignore[override] files_to_read = list(self._file_or_files) # END ensure we have a copy of the paths to handle + files_to_read = [osp.abspath(path) if isinstance(path, (str, os.PathLike)) else path for path in files_to_read] + seen = set(files_to_read) num_read_include_files = 0 while files_to_read: diff --git a/git/diff.py b/git/diff.py index 23cb5675e..5af53e556 100644 --- a/git/diff.py +++ b/git/diff.py @@ -3,7 +3,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ -__all__ = ["DiffConstants", "NULL_TREE", "INDEX", "Diffable", "DiffIndex", "Diff"] +__all__ = ["DiffConstants", "NULL_TREE", "NULL_TREE_SHA", "INDEX", "Diffable", "DiffIndex", "Diff"] import enum import re @@ -84,6 +84,9 @@ class DiffConstants(enum.Enum): :const:`git.NULL_TREE` and :const:`Diffable.NULL_TREE`. """ +NULL_TREE_SHA = "4b825dc642cb6eb9a060e54bf8d69288fbee4904" +"""SHA of Git's canonical empty tree object.""" + INDEX: Literal[DiffConstants.INDEX] = DiffConstants.INDEX """Stand-in indicating you want to diff against the index. @@ -599,7 +602,14 @@ def _index_from_patch_format(cls, repo: "Repo", proc: Union["Popen", "Git.AutoIn # FIXME: Here SLURPING raw, need to re-phrase header-regexes linewise. text_list: List[bytes] = [] - handle_process_output(proc, text_list.append, None, finalize_process, decode_streams=False) + stderr_list: List[bytes] = [] + + def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> None: + finalize_process(proc, stderr=b"".join(stderr_list)) + + handle_process_output( + proc, text_list.append, stderr_list.append, finalize_process_with_stderr, decode_streams=False + ) # For now, we have to bake the stream. text = b"".join(text_list) @@ -765,11 +775,16 @@ def _index_from_raw_format(cls, repo: "Repo", proc: "Popen") -> "DiffIndex[Diff] # :100644 100644 687099101... 37c5e30c8... M .gitignore index: "DiffIndex" = DiffIndex() + stderr_list: List[bytes] = [] + + def finalize_process_with_stderr(proc: Union["Popen", "Git.AutoInterrupt"]) -> None: + finalize_process(proc, stderr=b"".join(stderr_list)) + handle_process_output( proc, lambda byt: cls._handle_diff_line(byt, repo, index), - None, - finalize_process, + stderr_list.append, + finalize_process_with_stderr, decode_streams=False, ) diff --git a/git/index/base.py b/git/index/base.py index 2276343f2..f03b452dc 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -1480,12 +1480,11 @@ def reset( return self - # FIXME: This is documented to accept the same parameters as Diffable.diff, but this - # does not handle NULL_TREE for `other`. (The suppressed mypy error is about this.) def diff( self, - other: Union[ # type: ignore[override] + other: Union[ Literal[git_diff.DiffConstants.INDEX], + Literal[git_diff.DiffConstants.NULL_TREE], "Tree", "Commit", str, @@ -1512,6 +1511,44 @@ def diff( if other is self.INDEX: return git_diff.DiffIndex() + if other == git_diff.NULL_TREE or other == git_diff.NULL_TREE_SHA: + args: List[Union[PathLike, str]] = [ + "--cached", + git_diff.NULL_TREE_SHA, + "--abbrev=40", + "--full-index", + ] + + if not any(x in kwargs for x in ("find_renames", "no_renames", "M")): + args.append("-M") + + if create_patch: + args.append("-p") + args.append("--no-ext-diff") + else: + args.append("--raw") + args.append("-z") + + args.append("--no-color") + + if paths is not None and not isinstance(paths, (tuple, list)): + paths = [paths] + + if paths: + args.append("--") + args.extend(paths) + + kwargs["as_process"] = True + proc = self.repo.git.diff(*args, **kwargs) + + diff_method = ( + git_diff.Diff._index_from_patch_format if create_patch else git_diff.Diff._index_from_raw_format + ) + index = diff_method(self.repo, proc) + + proc.wait() + return index + # Index against anything but None is a reverse diff with the respective item. # Handle existing -R flags properly. # Transform strings to the object so that we can call diff on it. diff --git a/git/objects/commit.py b/git/objects/commit.py index da7677ee0..1d8e8f071 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -86,6 +86,12 @@ class Commit(base.Object, TraversableIterableObj, Diffable, Serializable): # INVARIANTS default_encoding = "UTF-8" + # Options to :manpage:`git-rev-list(1)` that can overwrite files. + unsafe_git_rev_options = [ + "--output", + "-o", + ] + type: Literal["commit"] = "commit" __slots__ = ( @@ -302,6 +308,7 @@ def iter_items( repo: "Repo", rev: Union[str, "Commit", "SymbolicReference"], paths: Union[PathLike, Sequence[PathLike]] = "", + allow_unsafe_options: bool = False, **kwargs: Any, ) -> Iterator["Commit"]: R"""Find all commits matching the given criteria. @@ -330,6 +337,11 @@ def iter_items( raise ValueError("--pretty cannot be used as parsing expects single sha's only") # END handle pretty + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([rev], kwargs), unsafe_options=cls.unsafe_git_rev_options + ) + # Use -- in all cases, to prevent possibility of ambiguous arguments. # See https://github.com/gitpython-developers/GitPython/issues/264. @@ -374,6 +386,11 @@ def stats(self) -> Stats: """Create a git stat from changes between this commit and its first parent or from all changes done if this is the very first commit. + :note: + If this commit is at the boundary of a shallow clone, this will + raise :exc:`~git.exc.GitCommandError`, since the parent object + was never fetched and only exists as a reference on this commit. + :return: :class:`Stats` """ diff --git a/git/remote.py b/git/remote.py index 20e42b412..0c3dbfe15 100644 --- a/git/remote.py +++ b/git/remote.py @@ -517,6 +517,9 @@ def iter_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> NoReturn: # -> raise NotImplementedError +Progress = Union[RemoteProgress, "UpdateProgress", Callable[..., RemoteProgress], None] + + class Remote(LazyMixin, IterableObj): """Provides easy read and write access to a git remote. @@ -872,7 +875,7 @@ def update(self, **kwargs: Any) -> "Remote": def _get_fetch_info_from_stderr( self, proc: "Git.AutoInterrupt", - progress: Union[Callable[..., Any], RemoteProgress, None], + progress: Progress, kill_after_timeout: Union[None, float] = None, ) -> IterableList["FetchInfo"]: progress = to_progress_instance(progress) @@ -1000,7 +1003,7 @@ def _assert_refspec(self) -> None: def fetch( self, refspec: Union[str, List[str], None] = None, - progress: Union[RemoteProgress, None, "UpdateProgress"] = None, + progress: Progress = None, verbose: bool = True, kill_after_timeout: Union[None, float] = None, allow_unsafe_protocols: bool = False, @@ -1068,7 +1071,10 @@ def fetch( Git.check_unsafe_protocols(ref) if not allow_unsafe_options: - Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_fetch_options) + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=self.unsafe_git_fetch_options, + ) proc = self.repo.git.fetch( "--", self, *args, as_process=True, with_stdout=False, universal_newlines=True, v=verbose, **kwargs @@ -1081,7 +1087,7 @@ def fetch( def pull( self, refspec: Union[str, List[str], None] = None, - progress: Union[RemoteProgress, "UpdateProgress", None] = None, + progress: Progress = None, kill_after_timeout: Union[None, float] = None, allow_unsafe_protocols: bool = False, allow_unsafe_options: bool = False, @@ -1122,7 +1128,10 @@ def pull( Git.check_unsafe_protocols(ref) if not allow_unsafe_options: - Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_pull_options) + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=self.unsafe_git_pull_options, + ) proc = self.repo.git.pull( "--", self, refspec, with_stdout=False, as_process=True, universal_newlines=True, v=True, **kwargs @@ -1135,7 +1144,7 @@ def pull( def push( self, refspec: Union[str, List[str], None] = None, - progress: Union[RemoteProgress, "UpdateProgress", Callable[..., RemoteProgress], None] = None, + progress: Progress = None, kill_after_timeout: Union[None, float] = None, allow_unsafe_protocols: bool = False, allow_unsafe_options: bool = False, @@ -1195,7 +1204,10 @@ def push( Git.check_unsafe_protocols(ref) if not allow_unsafe_options: - Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=self.unsafe_git_push_options) + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=self.unsafe_git_push_options, + ) proc = self.repo.git.push( "--", diff --git a/git/repo/base.py b/git/repo/base.py index 7579e326f..913984975 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -161,6 +161,20 @@ class Repo: https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---configltkeygtltvaluegt """ + unsafe_git_archive_options = [ + # Allows arbitrary command execution through the remote git-upload-archive command. + "--exec", + # Writes output to a caller-controlled filesystem path. + "--output", + "-o", + ] + + unsafe_git_revision_options = [ + # This option allows output to be written to arbitrary files before revision parsing. + "--output", + "-o", + ] + # Invariants config_level: ConfigLevels_Tup = ("system", "user", "global", "repository") """Represents the configuration level of a configuration file.""" @@ -295,7 +309,8 @@ def __init__( sm_gitpath = find_worktree_git_dir(dotgit) if sm_gitpath is not None: - git_dir = expand_path(sm_gitpath, expand_vars) + # worktrees can use relative paths as of Git 2.48, so we join to curpath + git_dir = osp.normpath(osp.join(curpath, sm_gitpath)) self._working_tree_dir = curpath break @@ -774,6 +789,7 @@ def iter_commits( self, rev: Union[str, Commit, "SymbolicReference", None] = None, paths: Union[PathLike, Sequence[PathLike]] = "", + allow_unsafe_options: bool = False, **kwargs: Any, ) -> Iterator[Commit]: """An iterator of :class:`~git.objects.commit.Commit` objects representing the @@ -791,6 +807,9 @@ def iter_commits( Arguments to be passed to :manpage:`git-rev-list(1)`. Common ones are ``max_count`` and ``skip``. + :param allow_unsafe_options: + Allow unsafe options in the revision argument, like ``--output``. + :note: To receive only commits between two named revisions, use the ``"revA...revB"`` revision specifier. @@ -801,7 +820,18 @@ def iter_commits( if rev is None: rev = self.head.commit - return Commit.iter_items(self, rev, paths, **kwargs) + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options + ) + + return Commit.iter_items( + self, + rev, + paths, + allow_unsafe_options=allow_unsafe_options, + **kwargs, + ) def merge_base(self, *rev: TBD, **kwargs: Any) -> List[Commit]: R"""Find the closest common ancestor for the given revision @@ -1078,7 +1108,9 @@ def active_branch(self) -> Head: ) return active_branch - def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) -> Iterator["BlameEntry"]: + def blame_incremental( + self, rev: str | HEAD | None, file: str, allow_unsafe_options: bool = False, **kwargs: Any + ) -> Iterator["BlameEntry"]: """Iterator for blame information for the given file at the given revision. Unlike :meth:`blame`, this does not return the actual file's contents, only a @@ -1089,6 +1121,9 @@ def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) -> uncommitted changes. Otherwise, anything successfully parsed by :manpage:`git-rev-parse(1)` is a valid option. + :param allow_unsafe_options: + Allow unsafe options in revision argument, like ``--output``. + :return: Lazy iterator of :class:`BlameEntry` tuples, where the commit indicates the commit to blame for the line, and range indicates a span of line numbers in @@ -1097,6 +1132,10 @@ def blame_incremental(self, rev: str | HEAD | None, file: str, **kwargs: Any) -> If you combine all line number ranges outputted by this command, you should get a continuous range spanning all line numbers in the file. """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([rev], kwargs), unsafe_options=self.unsafe_git_revision_options + ) data: bytes = self.git.blame(rev, "--", file, p=True, incremental=True, stdout_as_string=False, **kwargs) commits: Dict[bytes, Commit] = {} @@ -1175,7 +1214,8 @@ def blame( rev: Union[str, HEAD, None], file: str, incremental: bool = False, - rev_opts: Optional[List[str]] = None, + rev_opts: Optional[Sequence[str]] = None, + allow_unsafe_options: bool = False, **kwargs: Any, ) -> List[List[Commit | List[str | bytes] | None]] | Iterator[BlameEntry] | None: """The blame information for the given file at the given revision. @@ -1185,6 +1225,9 @@ def blame( uncommitted changes. Otherwise, anything successfully parsed by :manpage:`git-rev-parse(1)` is a valid option. + :param allow_unsafe_options: + Allow unsafe options in revision argument, like ``--output``. + :return: list: [git.Commit, list: []] @@ -1194,9 +1237,14 @@ def blame( appearance. """ if incremental: - return self.blame_incremental(rev, file, **kwargs) - rev_opts = rev_opts or [] - data: bytes = self.git.blame(rev, *rev_opts, "--", file, p=True, stdout_as_string=False, **kwargs) + return self.blame_incremental(rev, file, allow_unsafe_options=allow_unsafe_options, **kwargs) + rev_opts_list = list(rev_opts or []) + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([rev, rev_opts_list], kwargs), + unsafe_options=self.unsafe_git_revision_options, + ) + data: bytes = self.git.blame(rev, *rev_opts_list, "--", file, p=True, stdout_as_string=False, **kwargs) commits: Dict[str, Commit] = {} blames: List[List[Commit | List[str | bytes] | None]] = [] @@ -1407,7 +1455,10 @@ def _clone( if not allow_unsafe_protocols: Git.check_unsafe_protocols(url) if not allow_unsafe_options: - Git.check_unsafe_options(options=list(kwargs.keys()), unsafe_options=cls.unsafe_git_clone_options) + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=cls.unsafe_git_clone_options, + ) if not allow_unsafe_options and multi: Git.check_unsafe_options(options=multi, unsafe_options=cls.unsafe_git_clone_options) @@ -1582,6 +1633,8 @@ def archive( ostream: Union[TextIO, BinaryIO], treeish: Optional[str] = None, prefix: Optional[str] = None, + allow_unsafe_options: bool = False, + allow_unsafe_protocols: bool = False, **kwargs: Any, ) -> Repo: """Archive the tree at the given revision. @@ -1604,6 +1657,12 @@ def archive( repository-relative path to a directory or file to place into the archive, or a list or tuple of multiple paths. + :param allow_unsafe_options: + Allow unsafe options, like ``--exec`` or ``--output``. + + :param allow_unsafe_protocols: + Allow unsafe protocols to be used in ``remote``, like ``ext``. + :raise git.exc.GitCommandError: If something went wrong. @@ -1614,6 +1673,14 @@ def archive( treeish = self.head.commit if prefix and "prefix" not in kwargs: kwargs["prefix"] = prefix + remote = kwargs.get("remote") + if not allow_unsafe_protocols and remote is not None: + Git.check_unsafe_protocols(str(remote)) + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=self.unsafe_git_archive_options, + ) kwargs["output_stream"] = ostream path = kwargs.pop("path", []) path = cast(Union[PathLike, List[PathLike], Tuple[PathLike, ...]], path) diff --git a/init-tests-after-clone.sh b/init-tests-after-clone.sh index bfada01b0..a88f983fc 100755 --- a/init-tests-after-clone.sh +++ b/init-tests-after-clone.sh @@ -40,6 +40,11 @@ fi git tag __testing_point__ # The tests need a branch called master. +# +# If master is locally absent but more than one remote has it, checkout fails +# by default even if all remotes agree, and we fall back to creating it at +# HEAD. The reflog we populate below then traces HEAD's history rather than +# a remote master's, but master is reset to __testing_point__ either way. git checkout master -- || git checkout -b master # The tests need a reflog history on the master branch. diff --git a/test/lib/helper.py b/test/lib/helper.py index 6a8b714e6..1c110e103 100644 --- a/test/lib/helper.py +++ b/test/lib/helper.py @@ -18,6 +18,7 @@ "skipIf", "GIT_REPO", "GIT_DAEMON_PORT", + "xfail_if_raises", ] import contextlib @@ -35,8 +36,10 @@ import time import unittest import venv +from typing import Union, Type, Tuple import gitdb +import pytest from git.util import rmtree, cwd @@ -87,7 +90,7 @@ def __init__(self, input_string): self.stdout = io.BytesIO(input_string) self.stderr = io.BytesIO() - def wait(self): + def wait(self, stderr=None): return 0 poll = wait @@ -465,3 +468,27 @@ def _executable(self, basename): if osp.isfile(path) or osp.islink(path): return path raise RuntimeError(f"no regular file or symlink {path!r}") + + +@contextlib.contextmanager +def xfail_if_raises( + condition: bool, + *, + raises: Union[Type[BaseException], Tuple[Type[BaseException], ...]], + reason: str = "", + strict: bool = False, +): + """Approximates the behavior of @pytest.mark.xfail(..., raises=...) as a context + manager that can be used within a test, such as when the condition is complex or has + side effects + + One difference is it will not report XPASS if the test passes, but setting `strict` + simulates it by raising an exception""" + try: + yield + except raises: + if condition: + pytest.xfail(reason) + raise + if strict and condition: + pytest.fail("[XPASS(strict)] " + reason) diff --git a/test/test_clone.py b/test/test_clone.py index 653d50aa3..79d63dfdc 100644 --- a/test/test_clone.py +++ b/test/test_clone.py @@ -117,9 +117,16 @@ def test_clone_unsafe_options(self, rw_repo): tmp_file = tmp_dir / "pwn" unsafe_options = [ f"--upload-pack='touch {tmp_file}'", + f"--upl='touch {tmp_file}'", f"-u 'touch {tmp_file}'", + f"-utouch {tmp_file}; false", + f"-futouch${{IFS}}{tmp_file}; false", + f"-qutouch${{IFS}}{tmp_file}; false", "--config=protocol.ext.allow=always", + "--conf=protocol.ext.allow=always", "-c protocol.ext.allow=always", + "-cprotocol.ext.allow=always", + "-vcprotocol.ext.allow=always", ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): @@ -129,8 +136,10 @@ def test_clone_unsafe_options(self, rw_repo): unsafe_options = [ {"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}, + {"upl": f"touch {tmp_file}"}, {"u": f"touch {tmp_file}"}, {"config": "protocol.ext.allow=always"}, + {"conf": "protocol.ext.allow=always"}, {"c": "protocol.ext.allow=always"}, ] for unsafe_option in unsafe_options: @@ -138,6 +147,31 @@ def test_clone_unsafe_options(self, rw_repo): rw_repo.clone(tmp_dir, **unsafe_option) assert not tmp_file.exists() + @with_rw_repo("HEAD") + def test_clone_unsafe_options_abbreviated(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + tmp_dir = pathlib.Path(tdir) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upl='touch {tmp_file}'", + f"--upload-pac='touch {tmp_file}'", + "--conf=protocol.ext.allow=always", + ] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + rw_repo.clone(tmp_dir, multi_options=[unsafe_option]) + assert not tmp_file.exists() + + unsafe_kwargs = [ + {"upl": f"touch {tmp_file}"}, + {"upload_pac": f"touch {tmp_file}"}, + {"conf": "protocol.ext.allow=always"}, + ] + for unsafe_option in unsafe_kwargs: + with self.assertRaises(UnsafeOptionError): + rw_repo.clone(tmp_dir, **unsafe_option) + assert not tmp_file.exists() + @with_rw_repo("HEAD") def test_clone_unsafe_options_are_checked_after_splitting_multi_options(self, rw_repo): with tempfile.TemporaryDirectory() as tdir: @@ -191,7 +225,9 @@ def test_clone_safe_options(self, rw_repo): options = [ "--depth=1", "--single-branch", + "--origin upload", "-q", + "-oupstream", ] for option in options: destination = tmp_dir / option @@ -207,8 +243,13 @@ def test_clone_from_unsafe_options(self, rw_repo): unsafe_options = [ f"--upload-pack='touch {tmp_file}'", f"-u 'touch {tmp_file}'", + f"-utouch {tmp_file}; false", + f"-futouch${{IFS}}{tmp_file}; false", + f"-qutouch${{IFS}}{tmp_file}; false", "--config=protocol.ext.allow=always", "-c protocol.ext.allow=always", + "-cprotocol.ext.allow=always", + "-vcprotocol.ext.allow=always", ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): diff --git a/test/test_commit.py b/test/test_commit.py index b56ad3a18..74b7078f5 100644 --- a/test/test_commit.py +++ b/test/test_commit.py @@ -6,6 +6,7 @@ import copy from datetime import datetime from io import BytesIO +import tempfile import os.path as osp import re import sys @@ -15,6 +16,7 @@ from gitdb import IStream from git import Actor, Commit, Repo +from git.exc import UnsafeOptionError from git.objects.util import tzoffset, utc from git.repo.fun import touch @@ -288,6 +290,17 @@ def test_iter_items(self): # pretty not allowed. self.assertRaises(ValueError, Commit.iter_items, self.rorepo, "master", pretty="raw") + def test_iter_items_rejects_unsafe_revision(self): + with tempfile.TemporaryDirectory() as tdir: + marker = osp.join(tdir, "pwn") + self.assertRaises(UnsafeOptionError, Commit.iter_items, self.rorepo, f"--output={marker}") + + def test_iter_items_rejects_unsafe_options(self): + with tempfile.TemporaryDirectory() as tdir: + marker = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeOptionError): + list(Commit.iter_items(self.rorepo, "HEAD", output=marker)) + def test_rev_list_bisect_all(self): """ 'git rev-list --bisect-all' returns additional information diff --git a/test/test_config.py b/test/test_config.py index 3ddaf0a4b..897caba4c 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -310,6 +310,20 @@ def check_test_value(cr, value): with GitConfigParser(fpa, read_only=True) as cr: check_test_value(cr, tv) + @with_rw_directory + def test_config_relative_path_include(self, rw_dir): + included_path = osp.join(rw_dir, "included") + with GitConfigParser(included_path, read_only=False) as cw: + cw.set_value("included", "value", "included") + + config_path = osp.join(rw_dir, "config") + with GitConfigParser(config_path, read_only=False) as cw: + cw.set_value("include", "path", "included") + + relative_config_path = osp.relpath(config_path) + with GitConfigParser(relative_config_path, read_only=True) as cr: + assert cr.get_value("included", "value") == "included" + @with_rw_directory def test_multiple_include_paths_with_same_key(self, rw_dir): """Test that multiple 'path' entries under [include] are all respected. diff --git a/test/test_docs.py b/test/test_docs.py index cc0bbf26a..c3426a807 100644 --- a/test/test_docs.py +++ b/test/test_docs.py @@ -6,9 +6,6 @@ import gc import os import os.path -import sys - -import pytest from test.lib import TestBase from test.lib.helper import with_rw_directory @@ -478,11 +475,6 @@ def test_references_and_objects(self, rw_dir): repo.git.clear_cache() - @pytest.mark.xfail( - sys.platform == "cygwin", - reason="Cygwin GitPython can't find SHA for submodule", - raises=ValueError, - ) def test_submodules(self): # [1-test_submodules] repo = self.rorepo diff --git a/test/test_fixture_health.py b/test/test_fixture_health.py new file mode 100644 index 000000000..b18d5e8f9 --- /dev/null +++ b/test/test_fixture_health.py @@ -0,0 +1,131 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + +"""Verify that fixture directories are usable by git. + +If a fixture directory is missing, isn't an initialized git repository, +or is rejected by git for "dubious ownership", dependent tests +elsewhere in the suite fail in opaque ways. The checks here name the +preconditions directly so a misconfigured environment is recognizable +from the test output rather than from a cascade of unrelated-seeming +failures. + +These tests do not exercise GitPython's production code. They verify +the conditions under which production code is exercised are valid. +""" + +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent + +# Directories git must trust for the test suite to operate normally. The +# current set is the GitPython working tree plus the working trees of its +# gitdb submodule and the smmap submodule nested inside gitdb. New entries +# should be added here whenever the test suite gains a dependency on git +# accepting another directory. +FIXTURE_DIRS = [ + pytest.param(REPO_ROOT, id="repo_root"), + pytest.param(REPO_ROOT / "git" / "ext" / "gitdb", id="gitdb"), + pytest.param( + REPO_ROOT / "git" / "ext" / "gitdb" / "gitdb" / "ext" / "smmap", + id="smmap", + ), +] + +# Submodule working trees that must be present and initialized for the +# test suite to operate normally: gitdb at `git/ext/gitdb`, and smmap +# nested inside gitdb at `git/ext/gitdb/gitdb/ext/smmap`. The paths +# below are anchored at REPO_ROOT (the GitPython source tree), not at +# any rorepo redirection target. +SUBMODULE_DIRS = [ + pytest.param(REPO_ROOT / "git" / "ext" / "gitdb", id="gitdb"), + pytest.param( + REPO_ROOT / "git" / "ext" / "gitdb" / "gitdb" / "ext" / "smmap", + id="smmap", + ), +] + + +@pytest.mark.parametrize("fixture_dir", FIXTURE_DIRS) +def test_fixture_dir_is_trusted_by_git(fixture_dir: Path) -> None: + """git accepts ``fixture_dir`` as its own repository owned by a trusted user. + + Run ``git -C rev-parse --show-toplevel`` and assert it + succeeds and reports ``fixture_dir`` itself as the toplevel. Failure + typically means the directory's on-disk ownership doesn't match the + running user and the CI workflow's ``safe.directory`` list is missing + an entry that would override the check. + """ + if not fixture_dir.exists(): + pytest.skip(f"{fixture_dir} not present (run `git submodule update --init --recursive` from the repo root)") + if not (fixture_dir / ".git").exists(): + pytest.skip( + f"{fixture_dir} has no .git marker " + "(submodule not initialized; run " + "`git submodule update --init --recursive` from the repo root)" + ) + try: + result = subprocess.run( + ["git", "-C", str(fixture_dir), "rev-parse", "--show-toplevel"], + capture_output=True, + text=True, + check=False, + ) + except FileNotFoundError: + pytest.skip("git is not installed or not on PATH") + assert result.returncode == 0, ( + f"git refuses to operate in {fixture_dir}.\n" + f"stderr: {result.stderr.strip()}\n" + "The directory's owner doesn't match the running user and no " + "`safe.directory` entry overrides the check. On CI, the " + "workflow's `safe.directory` list typically needs an entry for " + "this path. Locally, this is unexpected and usually indicates " + "an ownership problem worth investigating." + ) + reported = Path(result.stdout.strip()) + assert reported.samefile(fixture_dir), ( + f"git reports the toplevel as {reported}, " + f"not as {fixture_dir} itself. " + "This usually means the directory is not an initialized git " + "repository (its `.git` marker may be stale or pointing elsewhere)." + ) + + +@pytest.mark.parametrize("submodule_dir", SUBMODULE_DIRS) +def test_required_submodule_is_initialized(submodule_dir: Path) -> None: + """The submodule's working tree is present and initialized. + + Failure means the source tree is a git clone but the submodule's + working tree hasn't been populated. Skipped when the source tree + itself isn't a git clone (e.g. an extracted release tarball), since + ``git submodule update`` cannot operate there; setups that handle + submodules in a separately-prepared tree (via + ``GIT_PYTHON_TEST_GIT_REPO_BASE``) are exempted from this check. + """ + if not (REPO_ROOT / ".git").exists(): + pytest.skip( + "Source tree is not a git clone (no .git in REPO_ROOT); submodules " + "cannot be initialized via `git submodule update` here. Setups " + "that prepare submodules in a separately-pointed tree (via " + "GIT_PYTHON_TEST_GIT_REPO_BASE) are exempted from this check." + ) + # The assertion messages below recommend `git submodule update --init + # --recursive` rather than `init-tests-after-clone.sh`, even though the + # latter is the documented entry point for first-time test setup. Two + # reasons: the script performs `git reset --hard` operations that can + # destroy local work, and #1713 showed the script itself can carry + # submodule-init regressions, in which case recommending it would lead + # developers in a circle. The direct git command is a safe minimal fix + # for this test's specific failure mode and bypasses any such regression. + assert submodule_dir.is_dir(), ( + f"Submodule working tree missing: {submodule_dir}.\n" + "Run `git submodule update --init --recursive` from the repo root." + ) + assert (submodule_dir / ".git").exists(), ( + f"Submodule directory exists but has no .git marker: {submodule_dir}.\n" + "The submodule hasn't been initialized. " + "Run `git submodule update --init --recursive` from the repo root." + ) diff --git a/test/test_git.py b/test/test_git.py index 24b60af9d..e95992ba8 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -164,12 +164,56 @@ def test_check_unsafe_options_normalizes_kwargs(self): (["c"], ["-c"]), (["--upload-pack=/tmp/helper"], ["--upload-pack"]), (["--config core.filemode=false"], ["--config"]), + (["--upl=/tmp/helper"], ["--upload-pack"]), + (["conf"], ["--config"]), + (["--out=/tmp/output"], ["--output"]), ] for options, unsafe_options in cases: with self.assertRaises(UnsafeOptionError): Git.check_unsafe_options(options=options, unsafe_options=unsafe_options) + def test_check_unsafe_options_does_not_treat_short_options_as_abbreviations(self): + unsafe_options = ["--upload-pack"] + + Git.check_unsafe_options(options=["-u", "u", "-upl"], unsafe_options=unsafe_options) + with self.assertRaises(UnsafeOptionError): + Git.check_unsafe_options(options=["--u"], unsafe_options=unsafe_options) + + def test_check_unsafe_options_does_not_treat_option_values_as_abbreviations(self): + Git.check_unsafe_options(options=["--branch", "conf"], unsafe_options=["--config"]) + + def test_check_unsafe_options_rejects_joined_unsafe_short_options(self): + cases = [ + (["-utouch /tmp/pwn"], ["-u"]), + (["-futouch /tmp/pwn"], ["-u"]), + (["-qutouch${IFS}/tmp/pwn"], ["-u"]), + (["-cprotocol.ext.allow=always"], ["-c"]), + (["-vcprotocol.ext.allow=always"], ["-c"]), + ] + + for options, unsafe_options in cases: + with self.assertRaises(UnsafeOptionError): + Git.check_unsafe_options(options=options, unsafe_options=unsafe_options) + + def test_check_unsafe_options_allows_attached_safe_short_option_values(self): + unsafe_options = ["--upload-pack", "-u", "--config", "-c"] + + Git.check_unsafe_options(options=["-oupstream", "-bcurrent"], unsafe_options=unsafe_options) + + def test_option_candidates_ignore_untransformed_kwargs(self): + options = Git._option_candidates( + kwargs={ + "output": None, + "upload_pack": False, + "exec": [], + "config": [None, False], + "max_count": 1, + } + ) + + self.assertEqual(options, ["--max-count"]) + _shell_cases = ( # value_in_call, value_from_class, expected_popen_arg (None, False, False), diff --git a/test/test_index.py b/test/test_index.py index f8280450a..3be750dbb 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -23,7 +23,8 @@ import ddt import pytest -from git import BlobFilter, Diff, Git, IndexFile, Object, Repo, Tree +from git import BlobFilter, Diff, Git, IndexFile, NULL_TREE, Object, Repo, Tree +from git.diff import NULL_TREE_SHA from git.exc import ( CheckoutError, GitCommandError, @@ -38,6 +39,7 @@ from git.util import Actor, cwd, hex_to_bin, rmtree from test.lib import TestBase, VirtualEnvironment, fixture, fixture_path, with_rw_directory, with_rw_repo, PathLikeMock +from test.lib.helper import xfail_if_raises HOOKS_SHEBANG = "#!/usr/bin/env sh\n" @@ -554,6 +556,39 @@ def test_index_file_diffing(self, rw_repo): rval = index.checkout("lib") assert len(list(rval)) > 1 + @with_rw_directory + def test_index_file_diff_null_tree_with_initial_index(self, rw_dir): + repo = Repo.init(rw_dir) + filename = ".gitkeep" + file_path = osp.join(repo.working_tree_dir, filename) + with open(file_path, "w") as fp: + fp.write("# Initial file\n") + + index = repo.index + index.add([filename]) + index.write() + + index = IndexFile(repo) + self.assertEqual(len(index.diff(None)), 0) + + diff = index.diff(NULL_TREE) + self.assertEqual(len(diff), 1) + self.assertEqual(diff[0].change_type, "A") + assert diff[0].new_file + self.assertEqual(diff[0].b_path, filename) + + self.assertEqual(len(index.diff(NULL_TREE, paths=filename)), 1) + self.assertEqual(len(index.diff(NULL_TREE_SHA, paths=filename)), 1) + self.assertEqual(len(index.diff(NULL_TREE, paths="missing")), 0) + + patch = index.diff(NULL_TREE, create_patch=True) + self.assertEqual(len(patch), 1) + self.assertIn(b"+# Initial file", patch[0].diff) + + with self.assertRaises(GitCommandError) as exc_info: + index.diff(NULL_TREE, bogus_option=True) + self.assertIn("usage: git diff", exc_info.exception.stderr) + def _count_existing(self, repo, files): """Return count of files that actually exist in the repository directory.""" existing = 0 @@ -565,369 +600,369 @@ def _count_existing(self, repo, files): # END num existing helper - @pytest.mark.xfail( - sys.platform == "win32" and (Git().config("core.symlinks") == "true" or _windows_supports_symlinks()), - reason="Assumes symlinks are not created on Windows and opens a symlink to a nonexistent target.", - raises=(FileNotFoundError, GitCommandError), - ) @with_rw_repo("0.1.6") def test_index_mutation(self, rw_repo): - index = rw_repo.index - num_entries = len(index.entries) - cur_head = rw_repo.head - - uname = "Thomas Müller" - umail = "sd@company.com" - with rw_repo.config_writer() as writer: - writer.set_value("user", "name", uname) - writer.set_value("user", "email", umail) - self.assertEqual(writer.get_value("user", "name"), uname) - - # Remove all of the files, provide a wild mix of paths, BaseIndexEntries, - # IndexEntries. - def mixed_iterator(): - count = 0 - for entry in index.entries.values(): - type_id = count % 5 - if type_id == 0: # path (str) - yield entry.path - elif type_id == 1: # path (PathLike) - yield Path(entry.path) - elif type_id == 2: # path mock (PathLike) - yield PathLikeMock(entry.path) - elif type_id == 3: # path mock in a blob - yield Blob(rw_repo, entry.binsha, entry.mode, entry.path) - elif type_id == 4: # blob - yield Blob(rw_repo, entry.binsha, entry.mode, entry.path) - elif type_id == 5: # BaseIndexEntry - yield BaseIndexEntry(entry[:4]) - elif type_id == 6: # IndexEntry - yield entry - else: - raise AssertionError("Invalid Type") - count += 1 - # END for each entry - - # END mixed iterator - deleted_files = index.remove(mixed_iterator(), working_tree=False) - assert deleted_files - self.assertEqual(self._count_existing(rw_repo, deleted_files), len(deleted_files)) - self.assertEqual(len(index.entries), 0) - - # Reset the index to undo our changes. - index.reset() - self.assertEqual(len(index.entries), num_entries) - - # Remove with working copy. - deleted_files = index.remove(mixed_iterator(), working_tree=True) - assert deleted_files - self.assertEqual(self._count_existing(rw_repo, deleted_files), 0) - - # Reset everything. - index.reset(working_tree=True) - self.assertEqual(self._count_existing(rw_repo, deleted_files), len(deleted_files)) - - # Invalid type. - self.assertRaises(TypeError, index.remove, [1]) - - # Absolute path. - deleted_files = index.remove([osp.join(rw_repo.working_tree_dir, "lib")], r=True) - assert len(deleted_files) > 1 - self.assertRaises(ValueError, index.remove, ["/doesnt/exists"]) - - # TEST COMMITTING - # Commit changed index. - cur_commit = cur_head.commit - commit_message = "commit default head by Frèderic Çaufl€" - - new_commit = index.commit(commit_message, head=False) - assert cur_commit != new_commit - self.assertEqual(new_commit.author.name, uname) - self.assertEqual(new_commit.author.email, umail) - self.assertEqual(new_commit.committer.name, uname) - self.assertEqual(new_commit.committer.email, umail) - self.assertEqual(new_commit.message, commit_message) - self.assertEqual(new_commit.parents[0], cur_commit) - self.assertEqual(len(new_commit.parents), 1) - self.assertEqual(cur_head.commit, cur_commit) - - # Commit with other actor. - cur_commit = cur_head.commit - - my_author = Actor("Frèderic Çaufl€", "author@example.com") - my_committer = Actor("Committing Frèderic Çaufl€", "committer@example.com") - commit_actor = index.commit(commit_message, author=my_author, committer=my_committer) - assert cur_commit != commit_actor - self.assertEqual(commit_actor.author.name, "Frèderic Çaufl€") - self.assertEqual(commit_actor.author.email, "author@example.com") - self.assertEqual(commit_actor.committer.name, "Committing Frèderic Çaufl€") - self.assertEqual(commit_actor.committer.email, "committer@example.com") - self.assertEqual(commit_actor.message, commit_message) - self.assertEqual(commit_actor.parents[0], cur_commit) - self.assertEqual(len(new_commit.parents), 1) - self.assertEqual(cur_head.commit, commit_actor) - self.assertEqual(cur_head.log()[-1].actor, my_committer) - - # Commit with author_date and commit_date. - cur_commit = cur_head.commit - commit_message = "commit with dates by Avinash Sajjanshetty" - - new_commit = index.commit( - commit_message, - author_date="2006-04-07T22:13:13", - commit_date="2005-04-07T22:13:13", - ) - assert cur_commit != new_commit - print(new_commit.authored_date, new_commit.committed_date) - self.assertEqual(new_commit.message, commit_message) - self.assertEqual(new_commit.authored_date, 1144447993) - self.assertEqual(new_commit.committed_date, 1112911993) - - # Same index, no parents. - commit_message = "index without parents" - commit_no_parents = index.commit(commit_message, parent_commits=[], head=True) - self.assertEqual(commit_no_parents.message, commit_message) - self.assertEqual(len(commit_no_parents.parents), 0) - self.assertEqual(cur_head.commit, commit_no_parents) - - # same index, multiple parents. - commit_message = "Index with multiple parents\n commit with another line" - commit_multi_parent = index.commit(commit_message, parent_commits=(commit_no_parents, new_commit)) - self.assertEqual(commit_multi_parent.message, commit_message) - self.assertEqual(len(commit_multi_parent.parents), 2) - self.assertEqual(commit_multi_parent.parents[0], commit_no_parents) - self.assertEqual(commit_multi_parent.parents[1], new_commit) - self.assertEqual(cur_head.commit, commit_multi_parent) - - # Re-add all files in lib. - # Get the lib folder back on disk, but get an index without it. - index.reset(new_commit.parents[0], working_tree=True).reset(new_commit, working_tree=False) - lib_file_path = osp.join("lib", "git", "__init__.py") - assert (lib_file_path, 0) not in index.entries - assert osp.isfile(osp.join(rw_repo.working_tree_dir, lib_file_path)) - - # Directory. - entries = index.add(["lib"], fprogress=self._fprogress_add) - self._assert_entries(entries) - self._assert_fprogress(entries) - assert len(entries) > 1 - - # Glob. - entries = index.reset(new_commit).add([osp.join("lib", "git", "*.py")], fprogress=self._fprogress_add) - self._assert_entries(entries) - self._assert_fprogress(entries) - self.assertEqual(len(entries), 14) - - # Same file. - entries = index.reset(new_commit).add( - [osp.join(rw_repo.working_tree_dir, "lib", "git", "head.py")] * 2, - fprogress=self._fprogress_add, - ) - self._assert_entries(entries) - self.assertEqual(entries[0].mode & 0o644, 0o644) - # Would fail, test is too primitive to handle this case. - # self._assert_fprogress(entries) - self._reset_progress() - self.assertEqual(len(entries), 2) - - # Missing path. - self.assertRaises(OSError, index.reset(new_commit).add, ["doesnt/exist/must/raise"]) - - # Blob from older revision overrides current index revision. - old_blob = new_commit.parents[0].tree.blobs[0] - entries = index.reset(new_commit).add([old_blob], fprogress=self._fprogress_add) - self._assert_entries(entries) - self._assert_fprogress(entries) - self.assertEqual(index.entries[(old_blob.path, 0)].hexsha, old_blob.hexsha) - self.assertEqual(len(entries), 1) - - # Mode 0 not allowed. - null_hex_sha = Diff.NULL_HEX_SHA - null_bin_sha = b"\0" * 20 - self.assertRaises( - ValueError, - index.reset(new_commit).add, - [BaseIndexEntry((0, null_bin_sha, 0, "doesntmatter"))], - ) - - # Add new file. - new_file_relapath = "my_new_file" - self._make_file(new_file_relapath, "hello world", rw_repo) - entries = index.reset(new_commit).add( - [BaseIndexEntry((0o10644, null_bin_sha, 0, new_file_relapath))], - fprogress=self._fprogress_add, - ) - self._assert_entries(entries) - self._assert_fprogress(entries) - self.assertEqual(len(entries), 1) - self.assertNotEqual(entries[0].hexsha, null_hex_sha) - - # Add symlink. - if sys.platform != "win32": - for target in ("/etc/nonexisting", "/etc/passwd", "/etc"): - basename = "my_real_symlink" - - link_file = osp.join(rw_repo.working_tree_dir, basename) - os.symlink(target, link_file) - entries = index.reset(new_commit).add([link_file], fprogress=self._fprogress_add) - self._assert_entries(entries) - self._assert_fprogress(entries) - self.assertEqual(len(entries), 1) - self.assertTrue(S_ISLNK(entries[0].mode)) - self.assertTrue(S_ISLNK(index.entries[index.entry_key("my_real_symlink", 0)].mode)) - - # We expect only the target to be written. - self.assertEqual( - index.repo.odb.stream(entries[0].binsha).read().decode("ascii"), - target, - ) - - os.remove(link_file) - # END for each target - # END real symlink test - - # Add fake symlink and assure it checks out as a symlink. - fake_symlink_relapath = "my_fake_symlink" - link_target = "/etc/that" - fake_symlink_path = self._make_file(fake_symlink_relapath, link_target, rw_repo) - fake_entry = BaseIndexEntry((0o120000, null_bin_sha, 0, fake_symlink_relapath)) - entries = index.reset(new_commit).add([fake_entry], fprogress=self._fprogress_add) - self._assert_entries(entries) - self._assert_fprogress(entries) - assert entries[0].hexsha != null_hex_sha - self.assertEqual(len(entries), 1) - self.assertTrue(S_ISLNK(entries[0].mode)) - - # Check that this also works with an alternate method. - full_index_entry = IndexEntry.from_base(BaseIndexEntry((0o120000, entries[0].binsha, 0, entries[0].path))) - entry_key = index.entry_key(full_index_entry) - index.reset(new_commit) - - assert entry_key not in index.entries - index.entries[entry_key] = full_index_entry - index.write() - index.update() # Force reread of entries. - new_entry = index.entries[entry_key] - assert S_ISLNK(new_entry.mode) + with xfail_if_raises( + sys.platform == "win32" and (Git().config("core.symlinks") == "true" or _windows_supports_symlinks()), + raises=(FileNotFoundError, GitCommandError), + reason="Assumes symlinks are not created on Windows and opens a symlink to a nonexistent target.", + ): + index = rw_repo.index + num_entries = len(index.entries) + cur_head = rw_repo.head + + uname = "Thomas Müller" + umail = "sd@company.com" + with rw_repo.config_writer() as writer: + writer.set_value("user", "name", uname) + writer.set_value("user", "email", umail) + self.assertEqual(writer.get_value("user", "name"), uname) + + # Remove all of the files, provide a wild mix of paths, BaseIndexEntries, + # IndexEntries. + def mixed_iterator(): + count = 0 + for entry in index.entries.values(): + type_id = count % 5 + if type_id == 0: # path (str) + yield entry.path + elif type_id == 1: # path (PathLike) + yield Path(entry.path) + elif type_id == 2: # path mock (PathLike) + yield PathLikeMock(entry.path) + elif type_id == 3: # path mock in a blob + yield Blob(rw_repo, entry.binsha, entry.mode, entry.path) + elif type_id == 4: # blob + yield Blob(rw_repo, entry.binsha, entry.mode, entry.path) + elif type_id == 5: # BaseIndexEntry + yield BaseIndexEntry(entry[:4]) + elif type_id == 6: # IndexEntry + yield entry + else: + raise AssertionError("Invalid Type") + count += 1 + # END for each entry + + # END mixed iterator + deleted_files = index.remove(mixed_iterator(), working_tree=False) + assert deleted_files + self.assertEqual(self._count_existing(rw_repo, deleted_files), len(deleted_files)) + self.assertEqual(len(index.entries), 0) + + # Reset the index to undo our changes. + index.reset() + self.assertEqual(len(index.entries), num_entries) + + # Remove with working copy. + deleted_files = index.remove(mixed_iterator(), working_tree=True) + assert deleted_files + self.assertEqual(self._count_existing(rw_repo, deleted_files), 0) + + # Reset everything. + index.reset(working_tree=True) + self.assertEqual(self._count_existing(rw_repo, deleted_files), len(deleted_files)) + + # Invalid type. + self.assertRaises(TypeError, index.remove, [1]) + + # Absolute path. + deleted_files = index.remove([osp.join(rw_repo.working_tree_dir, "lib")], r=True) + assert len(deleted_files) > 1 + self.assertRaises(ValueError, index.remove, ["/doesnt/exists"]) + + # TEST COMMITTING + # Commit changed index. + cur_commit = cur_head.commit + commit_message = "commit default head by Frèderic Çaufl€" + + new_commit = index.commit(commit_message, head=False) + assert cur_commit != new_commit + self.assertEqual(new_commit.author.name, uname) + self.assertEqual(new_commit.author.email, umail) + self.assertEqual(new_commit.committer.name, uname) + self.assertEqual(new_commit.committer.email, umail) + self.assertEqual(new_commit.message, commit_message) + self.assertEqual(new_commit.parents[0], cur_commit) + self.assertEqual(len(new_commit.parents), 1) + self.assertEqual(cur_head.commit, cur_commit) + + # Commit with other actor. + cur_commit = cur_head.commit + + my_author = Actor("Frèderic Çaufl€", "author@example.com") + my_committer = Actor("Committing Frèderic Çaufl€", "committer@example.com") + commit_actor = index.commit(commit_message, author=my_author, committer=my_committer) + assert cur_commit != commit_actor + self.assertEqual(commit_actor.author.name, "Frèderic Çaufl€") + self.assertEqual(commit_actor.author.email, "author@example.com") + self.assertEqual(commit_actor.committer.name, "Committing Frèderic Çaufl€") + self.assertEqual(commit_actor.committer.email, "committer@example.com") + self.assertEqual(commit_actor.message, commit_message) + self.assertEqual(commit_actor.parents[0], cur_commit) + self.assertEqual(len(new_commit.parents), 1) + self.assertEqual(cur_head.commit, commit_actor) + self.assertEqual(cur_head.log()[-1].actor, my_committer) + + # Commit with author_date and commit_date. + cur_commit = cur_head.commit + commit_message = "commit with dates by Avinash Sajjanshetty" + + new_commit = index.commit( + commit_message, + author_date="2006-04-07T22:13:13", + commit_date="2005-04-07T22:13:13", + ) + assert cur_commit != new_commit + print(new_commit.authored_date, new_commit.committed_date) + self.assertEqual(new_commit.message, commit_message) + self.assertEqual(new_commit.authored_date, 1144447993) + self.assertEqual(new_commit.committed_date, 1112911993) + + # Same index, no parents. + commit_message = "index without parents" + commit_no_parents = index.commit(commit_message, parent_commits=[], head=True) + self.assertEqual(commit_no_parents.message, commit_message) + self.assertEqual(len(commit_no_parents.parents), 0) + self.assertEqual(cur_head.commit, commit_no_parents) + + # same index, multiple parents. + commit_message = "Index with multiple parents\n commit with another line" + commit_multi_parent = index.commit(commit_message, parent_commits=(commit_no_parents, new_commit)) + self.assertEqual(commit_multi_parent.message, commit_message) + self.assertEqual(len(commit_multi_parent.parents), 2) + self.assertEqual(commit_multi_parent.parents[0], commit_no_parents) + self.assertEqual(commit_multi_parent.parents[1], new_commit) + self.assertEqual(cur_head.commit, commit_multi_parent) + + # Re-add all files in lib. + # Get the lib folder back on disk, but get an index without it. + index.reset(new_commit.parents[0], working_tree=True).reset(new_commit, working_tree=False) + lib_file_path = osp.join("lib", "git", "__init__.py") + assert (lib_file_path, 0) not in index.entries + assert osp.isfile(osp.join(rw_repo.working_tree_dir, lib_file_path)) + + # Directory. + entries = index.add(["lib"], fprogress=self._fprogress_add) + self._assert_entries(entries) + self._assert_fprogress(entries) + assert len(entries) > 1 + + # Glob. + entries = index.reset(new_commit).add([osp.join("lib", "git", "*.py")], fprogress=self._fprogress_add) + self._assert_entries(entries) + self._assert_fprogress(entries) + self.assertEqual(len(entries), 14) + + # Same file. + entries = index.reset(new_commit).add( + [osp.join(rw_repo.working_tree_dir, "lib", "git", "head.py")] * 2, + fprogress=self._fprogress_add, + ) + self._assert_entries(entries) + self.assertEqual(entries[0].mode & 0o644, 0o644) + # Would fail, test is too primitive to handle this case. + # self._assert_fprogress(entries) + self._reset_progress() + self.assertEqual(len(entries), 2) + + # Missing path. + self.assertRaises(OSError, index.reset(new_commit).add, ["doesnt/exist/must/raise"]) + + # Blob from older revision overrides current index revision. + old_blob = new_commit.parents[0].tree.blobs[0] + entries = index.reset(new_commit).add([old_blob], fprogress=self._fprogress_add) + self._assert_entries(entries) + self._assert_fprogress(entries) + self.assertEqual(index.entries[(old_blob.path, 0)].hexsha, old_blob.hexsha) + self.assertEqual(len(entries), 1) + + # Mode 0 not allowed. + null_hex_sha = Diff.NULL_HEX_SHA + null_bin_sha = b"\0" * 20 + self.assertRaises( + ValueError, + index.reset(new_commit).add, + [BaseIndexEntry((0, null_bin_sha, 0, "doesntmatter"))], + ) - # A tree created from this should contain the symlink. - tree = index.write_tree() - assert fake_symlink_relapath in tree - index.write() # Flush our changes for the checkout. - - # Check out the fake link, should be a link then. - assert not S_ISLNK(os.stat(fake_symlink_path)[ST_MODE]) - os.remove(fake_symlink_path) - index.checkout(fake_symlink_path) - - # On Windows, we currently assume we will never get symlinks. - if sys.platform == "win32": - # Symlinks should contain the link as text (which is what a - # symlink actually is). - with open(fake_symlink_path, "rt") as fd: - self.assertEqual(fd.read(), link_target) - else: - self.assertTrue(S_ISLNK(os.lstat(fake_symlink_path)[ST_MODE])) - - # TEST RENAMING - def assert_mv_rval(rval): - for source, dest in rval: - assert not osp.exists(source) and osp.exists(dest) - # END for each renamed item - - # END move assertion utility - - self.assertRaises(ValueError, index.move, ["just_one_path"]) - # Try to move a file onto an existing file. - files = ["AUTHORS", "LICENSE"] - self.assertRaises(GitCommandError, index.move, files) - - # Again, with force. - assert_mv_rval(index.move(files, f=True)) - - # Move files into a directory - dry run. - paths = ["LICENSE", "VERSION", "doc"] - rval = index.move(paths, dry_run=True) - self.assertEqual(len(rval), 2) - assert osp.exists(paths[0]) - - # Again, no dry run. - rval = index.move(paths) - assert_mv_rval(rval) - - # Move dir into dir. - rval = index.move(["doc", "test"]) - assert_mv_rval(rval) - - # TEST PATH REWRITING - ###################### - count = [0] - - def rewriter(entry): - rval = str(count[0]) - count[0] += 1 - return rval - - # END rewriter - - def make_paths(): - """Help out the test by yielding two existing paths and one new path.""" - yield "CHANGES" - yield "ez_setup.py" - yield index.entries[index.entry_key("README", 0)] - yield index.entries[index.entry_key(".gitignore", 0)] - - for fid in range(3): - fname = "newfile%i" % fid - with open(fname, "wb") as fd: - fd.write(b"abcd") - yield Blob(rw_repo, Blob.NULL_BIN_SHA, 0o100644, fname) - # END for each new file - - # END path producer - paths = list(make_paths()) - self._assert_entries(index.add(paths, path_rewriter=rewriter)) - - for filenum in range(len(paths)): - assert index.entry_key(str(filenum), 0) in index.entries - - # TEST RESET ON PATHS - ###################### - arela = "aa" - brela = "bb" - afile = self._make_file(arela, "adata", rw_repo) - bfile = self._make_file(brela, "bdata", rw_repo) - akey = index.entry_key(arela, 0) - bkey = index.entry_key(brela, 0) - keys = (akey, bkey) - absfiles = (afile, bfile) - files = (arela, brela) - - for fkey in keys: - assert fkey not in index.entries - - index.add(files, write=True) - nc = index.commit("2 files committed", head=False) - - for fkey in keys: - assert fkey in index.entries - - # Just the index. - index.reset(paths=(arela, afile)) - assert akey not in index.entries - assert bkey in index.entries - - # Now with working tree - files on disk as well as entries must be recreated. - rw_repo.head.commit = nc - for absfile in absfiles: - os.remove(absfile) - - index.reset(working_tree=True, paths=files) - - for fkey in keys: - assert fkey in index.entries - for absfile in absfiles: - assert osp.isfile(absfile) + # Add new file. + new_file_relapath = "my_new_file" + self._make_file(new_file_relapath, "hello world", rw_repo) + entries = index.reset(new_commit).add( + [BaseIndexEntry((0o10644, null_bin_sha, 0, new_file_relapath))], + fprogress=self._fprogress_add, + ) + self._assert_entries(entries) + self._assert_fprogress(entries) + self.assertEqual(len(entries), 1) + self.assertNotEqual(entries[0].hexsha, null_hex_sha) + + # Add symlink. + if sys.platform != "win32": + for target in ("/etc/nonexisting", "/etc/passwd", "/etc"): + basename = "my_real_symlink" + + link_file = osp.join(rw_repo.working_tree_dir, basename) + os.symlink(target, link_file) + entries = index.reset(new_commit).add([link_file], fprogress=self._fprogress_add) + self._assert_entries(entries) + self._assert_fprogress(entries) + self.assertEqual(len(entries), 1) + self.assertTrue(S_ISLNK(entries[0].mode)) + self.assertTrue(S_ISLNK(index.entries[index.entry_key("my_real_symlink", 0)].mode)) + + # We expect only the target to be written. + self.assertEqual( + index.repo.odb.stream(entries[0].binsha).read().decode("ascii"), + target, + ) + + os.remove(link_file) + # END for each target + # END real symlink test + + # Add fake symlink and assure it checks out as a symlink. + fake_symlink_relapath = "my_fake_symlink" + link_target = "/etc/that" + fake_symlink_path = self._make_file(fake_symlink_relapath, link_target, rw_repo) + fake_entry = BaseIndexEntry((0o120000, null_bin_sha, 0, fake_symlink_relapath)) + entries = index.reset(new_commit).add([fake_entry], fprogress=self._fprogress_add) + self._assert_entries(entries) + self._assert_fprogress(entries) + assert entries[0].hexsha != null_hex_sha + self.assertEqual(len(entries), 1) + self.assertTrue(S_ISLNK(entries[0].mode)) + + # Check that this also works with an alternate method. + full_index_entry = IndexEntry.from_base(BaseIndexEntry((0o120000, entries[0].binsha, 0, entries[0].path))) + entry_key = index.entry_key(full_index_entry) + index.reset(new_commit) + + assert entry_key not in index.entries + index.entries[entry_key] = full_index_entry + index.write() + index.update() # Force reread of entries. + new_entry = index.entries[entry_key] + assert S_ISLNK(new_entry.mode) + + # A tree created from this should contain the symlink. + tree = index.write_tree() + assert fake_symlink_relapath in tree + index.write() # Flush our changes for the checkout. + + # Check out the fake link, should be a link then. + assert not S_ISLNK(os.stat(fake_symlink_path)[ST_MODE]) + os.remove(fake_symlink_path) + index.checkout(fake_symlink_path) + + # On Windows, we currently assume we will never get symlinks. + if sys.platform == "win32": + # Symlinks should contain the link as text (which is what a + # symlink actually is). + with open(fake_symlink_path, "rt") as fd: + self.assertEqual(fd.read(), link_target) + else: + self.assertTrue(S_ISLNK(os.lstat(fake_symlink_path)[ST_MODE])) + + # TEST RENAMING + def assert_mv_rval(rval): + for source, dest in rval: + assert not osp.exists(source) and osp.exists(dest) + # END for each renamed item + + # END move assertion utility + + self.assertRaises(ValueError, index.move, ["just_one_path"]) + # Try to move a file onto an existing file. + files = ["AUTHORS", "LICENSE"] + self.assertRaises(GitCommandError, index.move, files) + + # Again, with force. + assert_mv_rval(index.move(files, f=True)) + + # Move files into a directory - dry run. + paths = ["LICENSE", "VERSION", "doc"] + rval = index.move(paths, dry_run=True) + self.assertEqual(len(rval), 2) + assert osp.exists(paths[0]) + + # Again, no dry run. + rval = index.move(paths) + assert_mv_rval(rval) + + # Move dir into dir. + rval = index.move(["doc", "test"]) + assert_mv_rval(rval) + + # TEST PATH REWRITING + ###################### + count = [0] + + def rewriter(entry): + rval = str(count[0]) + count[0] += 1 + return rval + + # END rewriter + + def make_paths(): + """Help out the test by yielding two existing paths and one new path.""" + yield "CHANGES" + yield "ez_setup.py" + yield index.entries[index.entry_key("README", 0)] + yield index.entries[index.entry_key(".gitignore", 0)] + + for fid in range(3): + fname = "newfile%i" % fid + with open(fname, "wb") as fd: + fd.write(b"abcd") + yield Blob(rw_repo, Blob.NULL_BIN_SHA, 0o100644, fname) + # END for each new file + + # END path producer + paths = list(make_paths()) + self._assert_entries(index.add(paths, path_rewriter=rewriter)) + + for filenum in range(len(paths)): + assert index.entry_key(str(filenum), 0) in index.entries + + # TEST RESET ON PATHS + ###################### + arela = "aa" + brela = "bb" + afile = self._make_file(arela, "adata", rw_repo) + bfile = self._make_file(brela, "bdata", rw_repo) + akey = index.entry_key(arela, 0) + bkey = index.entry_key(brela, 0) + keys = (akey, bkey) + absfiles = (afile, bfile) + files = (arela, brela) + + for fkey in keys: + assert fkey not in index.entries + + index.add(files, write=True) + nc = index.commit("2 files committed", head=False) + + for fkey in keys: + assert fkey in index.entries + + # Just the index. + index.reset(paths=(arela, afile)) + assert akey not in index.entries + assert bkey in index.entries + + # Now with working tree - files on disk as well as entries must be recreated. + rw_repo.head.commit = nc + for absfile in absfiles: + os.remove(absfile) + + index.reset(working_tree=True, paths=files) + + for fkey in keys: + assert fkey in index.entries + for absfile in absfiles: + assert osp.isfile(absfile) @with_rw_repo("HEAD") def test_compare_write_tree(self, rw_repo): diff --git a/test/test_remote.py b/test/test_remote.py index 1c627127a..64c41290e 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -9,7 +9,7 @@ import random import sys import tempfile -from unittest import skipIf +from unittest import mock, skipIf import pytest @@ -687,7 +687,12 @@ def test_multiple_urls(self, rw_repo): def test_fetch_error(self): rem = self.rorepo.remote("origin") - with self.assertRaisesRegex(GitCommandError, "[Cc]ouldn't find remote ref __BAD_REF__"): + msg = ( + r"[Cc]ouldn't find remote ref __BAD_REF__|" + r"could not read Username|" + r"expected flush after ref listing" + ) + with self.assertRaisesRegex(GitCommandError, msg): rem.fetch("__BAD_REF__") @with_rw_repo("0.1.6", bare=False) @@ -827,7 +832,11 @@ def test_fetch_unsafe_options(self, rw_repo): remote = rw_repo.remote("origin") tmp_dir = Path(tdir) tmp_file = tmp_dir / "pwn" - unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}] + unsafe_options = [ + {"upload-pack": f"touch {tmp_file}"}, + {"upload_pack": f"touch {tmp_file}"}, + {"upload_p": f"touch {tmp_file}"}, + ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): remote.fetch(**unsafe_option) @@ -895,7 +904,11 @@ def test_pull_unsafe_options(self, rw_repo): remote = rw_repo.remote("origin") tmp_dir = Path(tdir) tmp_file = tmp_dir / "pwn" - unsafe_options = [{"upload-pack": f"touch {tmp_file}"}, {"upload_pack": f"touch {tmp_file}"}] + unsafe_options = [ + {"upload-pack": f"touch {tmp_file}"}, + {"upload_pack": f"touch {tmp_file}"}, + {"upload_p": f"touch {tmp_file}"}, + ] for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): remote.pull(**unsafe_option) @@ -966,7 +979,9 @@ def test_push_unsafe_options(self, rw_repo): unsafe_options = [ {"receive-pack": f"touch {tmp_file}"}, {"receive_pack": f"touch {tmp_file}"}, + {"receive_p": f"touch {tmp_file}"}, {"exec": f"touch {tmp_file}"}, + {"exe": f"touch {tmp_file}"}, ] for unsafe_option in unsafe_options: assert not tmp_file.exists() @@ -1002,6 +1017,47 @@ def test_push_unsafe_options_allowed(self, rw_repo): assert tmp_file.exists() tmp_file.unlink() + @with_rw_repo("HEAD") + def test_ls_remote_unsafe_options(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + tmp_dir = Path(tdir) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + {"upload-pack": f"touch {tmp_file}"}, + {"upload_pack": f"touch {tmp_file}"}, + {"upl": f"touch {tmp_file}"}, + ] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote(".", **unsafe_option) + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote([f"--upload-pack={tmp_file}"], ".") + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote([f"--upl={tmp_file}"], ".") + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote(f"--upload-pack={tmp_file}", ".") + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote(f"--upl={tmp_file}", ".") + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote("--upload-pack", "touch", ".") + with self.assertRaises(UnsafeOptionError): + rw_repo.git.ls_remote("--refs", ".", upl=f"touch {tmp_file}") + + def test_ls_remote_allows_operand_named_like_unsafe_option(self): + with mock.patch.object(Git, "_call_process") as git: + Git().ls_remote("upload-pack") + git.assert_called_once() + + @with_rw_repo("HEAD") + def test_ls_remote_unsafe_options_allowed(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + tmp_dir = Path(tdir) + tmp_file = tmp_dir / "pwn" + unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] + for unsafe_option in unsafe_options: + with self.assertRaises(GitCommandError): + rw_repo.git.ls_remote(".", **unsafe_option, allow_unsafe_options=True) + @with_rw_and_rw_remote_repo("0.1.6") def test_fetch_unsafe_branch_name(self, rw_repo, remote_repo): # Create branch with a name containing a NBSP diff --git a/test/test_repo.py b/test/test_repo.py index fae3dc0b9..cfad73489 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -37,6 +37,8 @@ Submodule, Tree, ) +from git.exc import UnsafeOptionError +from git.exc import UnsafeProtocolError from git.exc import BadObject from git.repo.fun import touch from git.util import bin_to_hex, cwd, cygpath, join_path_native, rmfile, rmtree @@ -422,6 +424,54 @@ def test_archive(self): assert stream.tell() os.remove(stream.name) # Do it this way so we can inspect the file on failure. + def test_archive_rejects_unsafe_options(self): + with tempfile.TemporaryDirectory() as tdir: + output_marker = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeOptionError): + self.rorepo.archive(io.BytesIO(), "0.1.6", exec=f"touch {output_marker}") + assert not osp.exists(output_marker) + with self.assertRaises(UnsafeOptionError): + self.rorepo.archive(io.BytesIO(), "0.1.6", output=output_marker) + assert not osp.exists(output_marker) + + def test_archive_rejects_unsafe_remote_protocol(self): + with tempfile.TemporaryDirectory() as tdir: + output_marker = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeProtocolError): + self.rorepo.archive(io.BytesIO(), "HEAD", remote=f"ext::sh -c touch% {output_marker}") + assert not osp.exists(output_marker) + + def test_archive_preserves_positional_allow_unsafe_options(self): + with mock.patch.object(Git, "_call_process") as git: + self.rorepo.archive(io.BytesIO(), "HEAD", None, True, exec="git-upload-archive") + git.assert_called_once() + + def test_archive_accepts_stringifiable_remote(self): + class StringifiableRemote: + def __str__(self): + return "origin" + + with mock.patch.object(Git, "_call_process") as git: + self.rorepo.archive(io.BytesIO(), "HEAD", remote=StringifiableRemote()) + git.assert_called_once() + + def test_archive_rejects_unsafe_falsey_remote_protocol(self): + class FalseyRemote: + def __bool__(self): + return False + + def __str__(self): + return "ext::sh -c true" + + with self.assertRaises(UnsafeProtocolError): + self.rorepo.archive(io.BytesIO(), "HEAD", remote=FalseyRemote()) + + def test_iter_commits_rejects_unsafe_revision(self): + with tempfile.TemporaryDirectory() as tdir: + target = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeOptionError): + list(self.rorepo.iter_commits(f"--output={target}", max_count=1)) + @mock.patch.object(Git, "_call_process") def test_should_display_blame_information(self, git): git.return_value = fixture("blame") @@ -471,6 +521,27 @@ def test_blame_real(self): assert c, "Should have executed at least one blame command" assert nml, "There should at least be one blame commit that contains multiple lines" + def test_blame_rejects_unsafe_revision(self): + with tempfile.TemporaryDirectory() as tdir: + output_marker = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeOptionError): + self.rorepo.blame(f"--output={output_marker}", "README.md") + assert not osp.exists(output_marker) + + def test_blame_rejects_unsafe_options(self): + with tempfile.TemporaryDirectory() as tdir: + output_marker = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeOptionError): + self.rorepo.blame("HEAD", "README.md", output=output_marker) + assert not osp.exists(output_marker) + + def test_blame_rejects_unsafe_rev_opts(self): + with tempfile.TemporaryDirectory() as tdir: + output_marker = osp.join(tdir, "pwn") + with self.assertRaises(UnsafeOptionError): + self.rorepo.blame("HEAD", "README.md", rev_opts=(f"--output={output_marker}",)) + assert not osp.exists(output_marker) + @mock.patch.object(Git, "_call_process") def test_blame_incremental(self, git): # Loop over two fixtures, create a test fixture for 2.11.1+ syntax. @@ -877,11 +948,6 @@ def test_repo_odbtype(self): target_type = GitCmdObjectDB self.assertIsInstance(self.rorepo.odb, target_type) - @pytest.mark.xfail( - sys.platform == "cygwin", - reason="Cygwin GitPython can't find submodule SHA", - raises=ValueError, - ) def test_submodules(self): self.assertEqual(len(self.rorepo.submodules), 1) # non-recursive self.assertGreaterEqual(len(list(self.rorepo.iter_submodules())), 2) @@ -1094,7 +1160,7 @@ def test_is_valid_object(self): self.assertFalse(repo.is_valid_object(tag_sha, "commit")) @with_rw_directory - def test_git_work_tree_dotgit(self, rw_dir): + def test_git_work_tree_dotgit(self, rw_dir, use_relative_paths=False): """Check that we find .git as a worktree file and find the worktree based on it.""" git = Git(rw_dir) @@ -1106,7 +1172,11 @@ def test_git_work_tree_dotgit(self, rw_dir): worktree_path = join_path_native(rw_dir, "worktree_repo") if Git.is_cygwin(): worktree_path = cygpath(worktree_path) - rw_master.git.worktree("add", worktree_path, branch.name) + wt_add_kwargs = {"insert_kwargs_after": "add"} + # relative worktree paths introduced in git 2.48.0 + if use_relative_paths and git.version_info[:3] >= (2, 48, 0): + wt_add_kwargs["relative_paths"] = True + rw_master.git.worktree("add", worktree_path, branch.name, **wt_add_kwargs) # This ensures that we can read the repo's gitdir correctly. repo = Repo(worktree_path) @@ -1124,6 +1194,15 @@ def test_git_work_tree_dotgit(self, rw_dir): self.assertIsInstance(repo.heads["aaaaaaaa"], Head) + def test_git_work_tree_dotgit_relative(self): + """Check that we find .git as a worktree file containing a relative path + and find the worktree based on it.""" + if Git().version_info[:3] < (2, 48, 0): + pytest.skip("relative worktree feature unsupported, needs git 2.48.0 or later") + # this class inherits from TestCase so we can't use pytest.mark.parametrize on + # test_git_work_tree_dotgit; delegate instead + self.test_git_work_tree_dotgit(use_relative_paths=True) + @with_rw_directory def test_git_work_tree_env(self, rw_dir): """Check that we yield to GIT_WORK_TREE.""" diff --git a/test/test_submodule.py b/test/test_submodule.py index 63bb007de..778d22e3f 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -480,11 +480,6 @@ def test_base_rw(self, rwrepo): def test_base_bare(self, rwrepo): self._do_base_tests(rwrepo) - @pytest.mark.xfail( - sys.platform == "cygwin", - reason="Cygwin GitPython can't find submodule SHA", - raises=ValueError, - ) @pytest.mark.xfail( HIDE_WINDOWS_KNOWN_ERRORS, reason=( @@ -513,9 +508,9 @@ def test_root_module(self, rwrepo): with rm.config_writer(): pass - # Deep traversal gitdb / async. + # Deep traversal yields gitdb and its nested smmap. rsmsp = [sm.path for sm in rm.traverse()] - assert len(rsmsp) >= 2 # gitdb and async [and smmap], async being a child of gitdb. + assert rsmsp == ["git/ext/gitdb", "gitdb/ext/smmap"] # Cannot set the parent commit as root module's path didn't exist. self.assertRaises(ValueError, rm.set_parent_commit, "HEAD")