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

fix: secret scan pre-commit crashing on big merges #1048

Conversation

salome-voltz
Copy link
Collaborator

Context

ggshield secret scan pre-commit crashes on Windows when committing a merge with many long file names with [WinError 206] The filename or extension is too long.
This happens because ggshield is building a git command that is longer than the maximum path length on Windows. (see #1032 ).

What has been done

Functions that execute git commands on all files in a merge were updated to process files in batches, respecting the limit defined by MAX_FILES_PER_GIT_COMMAND.

Validation

On Windows:

  • Runs the script provided in the issue description. ggshield should crash
  • Test with the updated ggshield version. ggshield should proceed as expected.

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@salome-voltz salome-voltz self-assigned this Jan 10, 2025
@salome-voltz salome-voltz requested a review from a team as a code owner January 10, 2025 15:36
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.76%. Comparing base (28f5153) to head (4af42f9).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1048      +/-   ##
==========================================
- Coverage   92.10%   91.76%   -0.34%     
==========================================
  Files         181      143      -38     
  Lines        7762     6048    -1714     
==========================================
- Hits         7149     5550    -1599     
+ Misses        613      498     -115     
Flag Coverage Δ
unittests 91.76% <100.00%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sevbch sevbch left a comment

Choose a reason for hiding this comment

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

It's looking good to me!

As our CI is running tests in Windows environment, is it possible to design a test which would crash before and now pass? Or are there blockers for such a test?

@salome-voltz
Copy link
Collaborator Author

I added a test that crashed on Windows before the change and passes now. The test is a bit slow, however, as it generates a lot of files, and I can't use cassettes to speed it up so I don't know if we want to keep it. WDYT @sevbch ?

@salome-voltz salome-voltz requested a review from sevbch January 15, 2025 08:23
@sevbch
Copy link
Contributor

sevbch commented Jan 15, 2025

I added a test that crashed on Windows before the change and passes now. The test is a bit slow, however, as it generates a lot of files, and I can't use cassettes to speed it up so I don't know if we want to keep it. WDYT @sevbch ?

I tried locally launching your test and it took 0.2s. It's indeed a bit slow for a unit test but it seems acceptable to me. I'd rather keep the test to prevent us from future regressions.

@salome-voltz salome-voltz force-pushed the salomevoltz/scrt-5247-ggshield-crashed-on-windows-because-of-git-command-too-long branch from 4e75c15 to 4af42f9 Compare January 16, 2025 08:42
@salome-voltz salome-voltz disabled auto-merge January 16, 2025 08:45
@salome-voltz salome-voltz merged commit c405695 into main Jan 16, 2025
31 of 32 checks passed
@salome-voltz salome-voltz deleted the salomevoltz/scrt-5247-ggshield-crashed-on-windows-because-of-git-command-too-long branch January 16, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants