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.

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

Add the .git subdir as another safe.directory on Cygwin CI #1916

Merged
merged 2 commits into from
May 27, 2024

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented May 26, 2024

As shown in this run on my fork, the CI test job for Cygwin is failing now. This fixes that.

The first commit in this pull request just demonstrates that the failure is due to the upgrade of the Cygwin git package from 2.43.0-1 to 2.45.1-1. Although that commit makes test pass, I recommend against following that approach, mainly because the new version contains multiple security updates (coming in with the upstream 2.45.1 version, not with any downstream patches), but also because the older version will eventually be dropped from the Cygwin repositories.

The better workaround is in the second commit here, which adds the .git subdirectory of the cloned GitPython directory as a value of the multi-valued safe.directory Git configuration variable. Its parent directory is already added, which was previously sufficient, but not anymore.

I suspect that, rather than this being any bug in the downstream package, this is actually the correct behavior due to one of the several security fixes in the new version of Git, though I have not verified that. So maybe this is really not a workaround, but a permanent fix.

I believe the reason there is no need to modify any other workflow is that the other workflows don't need to add any safe.directory paths at all: they either clone the repository with the necessary ownership in the first place or, in the case of the Alpine Linux job, set the ownership with chown.

Using this older version is not in general secure, since the new
version is a security update. It is sometimes acceptable to run
software with security bugs in CI workflows, but the intent of this
change is just to check if the version of the Cygwin `git` package
is the cause of the failures. If so, they can probably be fixed or
worked around in a better way than downgrading. (Furthermore, the
lower version of the `git` package will not always be avaialable
from Cygwin's repositories.)
This undoes the change of pinning Git to an earlier version (before
the recent security update) on Cygwin, and instead adds the `.git`
subdirectory of the `GitPython` directory as an additional value of
the multi-valued `safe.directory` Git configuration variable.
@EliahKagan EliahKagan marked this pull request as ready for review May 26, 2024 21:28
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Seeing the change to safe.directory made me wonder if this is indeed a change in how it works and if gitoxide is affected.

And indeed, 22 months ago I see no evidence of it.

This line tells us it will check the git-dir/repository-directory if there is no worktree, but it's certainly strange that a wt/.git is a clone that is bare, it's an unusual name.

Anyway, it's good this is fixed and I don't worry about it.

@Byron Byron merged commit 9fbfb71 into gitpython-developers:main May 27, 2024
26 checks passed
@EliahKagan EliahKagan deleted the dubious branch May 27, 2024 11:57
@EliahKagan
Copy link
Contributor Author

I'm still not totally clear what's going on here. Some of the changes in 2.45.1 were judged overzealous and undone in 2.45.2. But don't know if that has anything to do with this. Anyway, git 2.45.2 is not yet in the Cygwin repositories, and I'm not sure it will be since it's not a security fix (so it might be skipped over, with some future version being packaged next).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants