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: Fixed path strip panic on Windows #36

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

marc2332
Copy link
Contributor

Closes #35

The problem was that repo_path was "canonicalized" but abs_path was not, and this broke it because paths on Window get the \\?\ (UNC I think it's called?) thing added at the begining, this small difference made strip_prefix return an error. Now, both paths are "canonicalized" before used.

image

@adriencaccia
Copy link
Member

@marc2332 Thanks for the PR!

I added a commit to ensure that we return the canonicalized path even if no git repo is found, with a test.

Can you test that it still works on windows? And it would be great to have a unit test for the specific case you had on windows as well if possible.

After that I will merge it 😉

Copy link

codspeed-hq bot commented Feb 15, 2024

CodSpeed Performance Report

Merging #36 will degrade performances by 12.44%

Comparing marc2332:fix/path-strip-panic-windows (c2db3e7) with main (69c8309)

Summary

❌ 3 regressions
✅ 54 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main marc2332:fix/path-strip-panic-windows Change
Auto 195.6 ns 223.3 ns -12.44%
Flat 700.6 ns 728.3 ns -3.81%
Linear 195.6 ns 223.3 ns -12.44%

@art049 art049 merged commit db3d58c into CodSpeedHQ:main Feb 26, 2024
3 checks passed
@marc2332
Copy link
Contributor Author

test that it still works on windows? And it would be great to h

Sorry, I missed this comment, I have too many notifications, just tried and it works well!

@marc2332 marc2332 deleted the fix/path-strip-panic-windows branch February 26, 2024 10:41
@adriencaccia
Copy link
Member

Sorry, I missed this comment, I have too many notifications, just tried and it works well!

No worries, thank you again for the contribution!

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.

CLI panics when running benchmarks
3 participants