geiger-rs / cargo-geiger

Detects usage of unsafe Rust in a Rust crate and its dependencies.
https://crates.io/crates/cargo-geiger
1.41k stars 66 forks source link

Fix panic when git dependency does not declare revision #462

Closed ginger51011 closed 1 year ago

ginger51011 commented 1 year ago

Fixes #461

Is it something like this we want? This is the simplest fix I can come up with, but perhaps it messes with resolution?

Also, do went a debug message, or should this be a silent solution (after all, if there is no rev it should have a sane default)

I have yet to figure out the integration tests

pinkforest commented 1 year ago

Yeah this needs integration tests - see how we do the existing with insta crate

https://github.com/rust-secure-code/cargo-geiger/tree/master/cargo-geiger/tests

Would like to confirm it works in empty and non-empty rev if we can ?

ginger51011 commented 1 year ago

Yeah this needs integration tests - see how we do the existing with insta crate

https://github.com/rust-secure-code/cargo-geiger/tree/master/cargo-geiger/tests

Would like to confirm it works in empty and non-empty rev if we can ?

I think I figured it out, but I am having issues with the snapshots, and I have some failing integration tests (I have these on master as well). insta wants to add artifact error messages for me, but these include paths local to my machine so that's no good. Any special setup that I have missed?

ginger51011 commented 1 year ago

I realized that this may cause issues if ref_slice is updated, but looking at the repo that will probably be an uncommon issue...

Also sorry for slamming the CI, I got in an argument with insta locally and outputs don't agree with CI

pinkforest commented 1 year ago

Hey thanks for the great effort on fixing this - looks really good :partying_face:

That's the CI is here for :) need to do some overall alignment everywhere later to ensure all flows local / CI better