Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only make config more permissive in tests that need it #1648

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Only make config more permissive in tests that need it #1648

merged 4 commits into from
Sep 7, 2023

Commits on Sep 6, 2023

  1. Only set safe.directory on Cygwin (which needs it) 

    This stops setting the current directory as an explicit safe
directory on CI for non-Windows systems, where this is not needed
because the repository has the ownership Git expects. The step name
is updated accordingly to reflect its now narrower purpose.

This also adds shell quoting to $(pwd) in the Cygwin workflow. In
practice, on CI, the path is very unlikely to contain whitespace,
but double-quoting $ expansions on which splitting and globbing are
unwanted is more robust and better expresses intent. This also has
the benefit that users who use the CI workflows as a guide to
commands they run locally, where on Windows they may very well have
spaces somewhere in this absolute path, will use a correct command.
    @EliahKagan
    EliahKagan committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    537af83 View commit details
    Browse the repository at this point in the history

  2. Use env vars on CI to set protocol.file.allow 

    Instead of setting a global git configuration.

This makes no significant difference for security on CI, but it is
an iterative step toward a more specific way of setting them that
will apply on CI and locally and require less configuration.

In addition, this shows an approach more similar to what users who
do not want to carefully review the security impact of changing
the global setting can use locally (and which is more secure).
    @EliahKagan
    EliahKagan committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    92d9ae2 View commit details
    Browse the repository at this point in the history

  3. Set protocol.file.allow only in tests that need it 

    Instead of setting environment variables just on CI and for the
the entire pytest command, this has the two test cases that need
protocol.file.allow to be set to "always" (instead of "user") set
them, via a shared fixture, just while those tests are running.

Both on CI and for local test runs, this makes it no longer
necessary to set this in a global configuration or through
environment variables, reducing the setup needed to run the tests.
    @EliahKagan
    EliahKagan committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    4f594cd View commit details
    Browse the repository at this point in the history

  4. Redesign new decorator to better separate concerns 

    _allow_file_protocol was effectively a _patch_git_config fixture,
being no no shorter, simpler, or clearer by hard-coding the
specific name and value to patch. So this changes it to be that.

As a secondary issue, it previously was called with no arguments,
then that would be used as a decorator. That was unintutive and it
was easy to omit the parentheses accidentally. This resolves that.
    @EliahKagan
    EliahKagan committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    f6c3262 View commit details
    Browse the repository at this point in the history