CodSpeedHQ / codspeed-rust

Rust crates to create CodSpeed benchmarks
https://codspeed.io
Apache License 2.0
23 stars 9 forks source link

fix: Fixed path strip panic on Windows #36

Closed marc2332 closed 8 months ago

marc2332 commented 9 months ago

Closes https://github.com/CodSpeedHQ/codspeed-rust/issues/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 commented 8 months ago

@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 😉

codspeed-hq[bot] commented 8 months ago

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

:warning: 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%
marc2332 commented 8 months ago

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!

adriencaccia commented 8 months ago

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

No worries, thank you again for the contribution!