Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Update and adjust pre-commit hooks#1953

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Byron merged 10 commits intogitpython-developers:mainfromEliahKagan:pre-commit
Aug 17, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKaganEliahKagan commentedAug 16, 2024
edited
Loading

This updates pre-commit hooks to their latest stable versions and adjusts exclusions and tool configuration to avoid false positives, as well as adding one more hook one of whose benefits is to catch problems from false positives of an existing hook.

Bumping versions

Ruff 0.6 has come out, with some improvements, so we may as well use it. Updating the hook for it achieves that, and while doing so, I think makes sense to update the others.

Over time, hooks that are not updated will become more out of date, so that the number of breakages that have to be solved when a bug or feature consideration requires an update is greater. No changes were needed to accommodate Ruff 0.6. But more false positives occur with the newer version of codespell. This addresses them by adding some more explicitly ignored words in itspyproject.toml configuration.

Avoiding symlink path corruption

Separate from any changes introduced by those updates, there is a problem with the way theend-of-file-fixer operates. As covered in more detail in commit messages, on Windows, symlinks are often checked out as regular files. But these files should not have newlines appended to the end of them, because when such a change is committed, it is adding a newline to the end of the path the repository stores for the symlink. Then, when the symlink is actually checked out, it is broken.

This excludes files named like licenses from being scanned and "fixed" by end-of-file-fixer, since that is both the current situation where the problem happens and the most likely way it would arise in the future without being detected through use of the repository.

It also adds a pre-commit hook (not used in this repository before) to check all symlinks. This will never find that error in a local repository where it arises, because it only checks actual symlinks, but it will find it on CI after it arises if it ever does again. There may also be a further benefit to it, since dangling symlinks could be accidentally introduced in the future for other reasons.

On Windows, when `core.symlinks` is `false` or unset (since itdefaults to `false` on Windows), Git checks out symbolic links asregular files whose contents are symlinks' target paths. Modifyingthose regular files and committing the changes alters the symlinktarget in the repository, and when they are checked out as actualsymlinks, the targets are different.But the `end-of-file-fixer` pre-commit hook automatically addsnewlines to the end of regular files that lack them. It doesn't dothis on actual symlinks, but it does do it to regular files thatstand in for symlinks. This causes it to carry a risk of breakingsymlinks if it is run on Windows and the changes committed, and itis easy to miss that this will happen because `git diff` outputshows it the same way as other additions of absent newlines.This deliberately commits the change that end-of-file-fixer makesto the `LICENSE-BSD` symlink, in order to allow a mitigation beyondjust excluding that symlink (or replacing it with a regular file)to be tested. This change must be undone, of course.
Rationale:- Small but likely benefit in general, since there are no currently  foreseen intentional use cases of committing of broken/dangling  symlinks in this project. So such symlinks that arise are likely  unintentional.- If the end-of-file-fixer hook has run on a Windows system where  `core.symlinks` has *not* been set to `true`, and symlinks' paths  have not been excluded, then a newline character is added to the  end of the path held in the regular file Git checks out to stand  in for the symlink. Because it is not actually a symlink, this  will not detect the problem at that time (regardless of the order  in which this and that hook run relative to each other). But when  it is then run on CI on a system where symlinks are checked out,  it will detect the problem.
The unanchored `LICENSE` and `COPYING` alternatives match thepattern anywhere, and therefore exclude the currently used path`fuzzing/LICENSE-BSD`.License files are more likely than other files in this project tobe introduced as symlinks, and less likely to be noticedimmediately if they break. Symlinks can be checked out as regularfiles when `core.symlinks` is set to `false`, which is rare outsideof Windows but is the default behavior when unset on Windows.This exclusion fixes the current problem that end-of-file-fixerbreaks those links by adding a newline character to the end (thesymlinks are checked out broken if that is committed). It alsoguards against most future cases involving licenses, thoughpossibly not all, and not other unrelated cases where symlinks maybe used for other purposes.Although the pre-commit-hooks repository also provides adestroyed-symlinks hook that detects the situation of a symlinkthat has been replaced by a regular file, this does not add thathook, because this situation is not inherently a problem. The codehere does not require symlinks to be checked out to work, andadding that would break significant uses of the repository onWindows.Note that this leaves the situation where a license file may be asymlink to another license file and may thus be checked out as aregular file containing that file's path. However, it is easy tounderstand that situation and manually follow the path. Thatdiffers from the scenario where a symlink is created but broken,because attempting to open it gives an error, and the error messageis often non-obvious, reporting that a file is not found but givingthe name of the symlink rather than its target.
@EliahKaganEliahKagan marked this pull request as ready for reviewAugust 16, 2024 01:04
Copy link
Member

@ByronByron 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 for your help, much appreciated.

@EliahKagan
Copy link
MemberAuthor

No problem! Should anything further be done on it before it is ready to be merged?

@Byron
Copy link
Member

I might have failed to press the button 🤦‍♂️

@ByronByron merged commit900fc33 intogitpython-developers:mainAug 17, 2024
22 checks passed
@EliahKagan
Copy link
MemberAuthor

No problem!

@EliahKaganEliahKagan deleted the pre-commit branchAugust 17, 2024 18:32
@EliahKagan
Copy link
MemberAuthor

I've noticed that the merge commitfailed CI on the main branch. But this appears unrelated to any of the changes here, because it is a random failure due to#1676. I expect that the failed check would succeed if re-run.

Byron reacted with thumbs up emoji

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestJun 8, 2025
This resolves two warnings about Ruff configuration, by:- No longer setting `ignore-init-module-imports = true` explicitly,  which was deprecated since `ruff` 0.4.4. We primarily use `ruff`  via `pre-commit`, for which this deprecation has applied since we  upgraded the version in `.pre-commit-config.yaml` from 0.4.3 to  0.6.0 ind1582d1 (gitpython-developers#1953).  We continue to list `F401` ("Module imported but unused") as not  automatically fixable, to avoid inadvertently removing imports  that may be needed.  See also:https://docs.astral.sh/ruff/settings/#lint_ignore-init-module-imports- Rename the rule `TCH004` to `TC004`, since `TCH004` is the old  name that may eventually be removed and that is deprecated since  0.8.0. We upgraded `ruff` in `.pre-commit-config.yml` again inb7ce712 (gitpython-developers#2031), from 0.6.0 to 0.11.12, at which point this  deprecation applied.  See alsohttps://astral.sh/blog/ruff-v0.8.0.These changes make those configuration-related warnings go away,and no new diagnostics (errors/warnings) are produced when running`ruff check` or `pre-commit --all-files`. No F401-relateddiagnostics are triggered when testing with explicit`ignore-init-module-imports = false`, in preview mode or otherwise.This commit also adds the version lower bound `>=0.8` for `ruff` in`requirements-dev.txt`. (That file is rarely used, as noted ina8a73ff (gitpython-developers#1871), but as long as we have it, there may be a benefitto excluding dependency versions for which our configuration is nolonger compatible.)
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull requestJun 8, 2025
This resolves two warnings about Ruff configuration, by:- No longer setting `ignore-init-module-imports = true` explicitly,  which was deprecated since `ruff` 0.4.4. We primarily use `ruff`  via `pre-commit`, for which this deprecation has applied since we  upgraded the version in `.pre-commit-config.yaml` from 0.4.3 to  0.6.0 ind1582d1 (gitpython-developers#1953).  We continue to list `F401` ("Module imported but unused") as not  automatically fixable, to avoid inadvertently removing imports  that may be needed.  See also:https://docs.astral.sh/ruff/settings/#lint_ignore-init-module-imports- Rename the rule `TCH004` to `TC004`, since `TCH004` is the old  name that may eventually be removed and that is deprecated since  0.8.0. We upgraded `ruff` in `.pre-commit-config.yml` again inb7ce712 (gitpython-developers#2031), from 0.6.0 to 0.11.12, at which point this  deprecation applied.  See alsohttps://astral.sh/blog/ruff-v0.8.0.These changes make those configuration-related warnings go away,and no new diagnostics (errors/warnings) are produced when running`ruff check` or `pre-commit run --all-files`. No F401-relateddiagnostics are triggered when testing with explicit`ignore-init-module-imports = false`, in preview mode or otherwise.In addition, this commit makes two changes that are not needed toresolve warnings:- Stop excluding `E203` ("Whitespace before ':'"). That diagnostic  is no longer failing with the current code here in the current  version of `ruff`, and code changes that would cause it to fail  would likely be accidentally mis-st- Add the version lower bound `>=0.8` for `ruff` in  `requirements-dev.txt`. That file is rarely used, as noted ina8a73ff (gitpython-developers#1871), but as long as we have it, there may be a  benefit to excluding dependency versions for which our  configuration is no longer compatible. This is the only change in  this commit outside of `pyproject.toml`.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ByronByronByron approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@EliahKagan@Byron

[8]ページ先頭

©2009-2025 Movatter.jp