Conversation
| assert_eq!( | ||
| is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "--show-current"])), | ||
| true | ||
| false |
There was a problem hiding this comment.
I know you have to update this to make the test pass, but I don't think this illustrated what this test was trying to prove anymore. I think the idea is that git branch --show-current should be safe/read-only whereas git branch -d feature is not.
There was a problem hiding this comment.
yeah good call, let me rewrite this to preserve that intention
There was a problem hiding this comment.
Incidentally, Rust prefers using assert!(thing) to assert_eq!(thing, true) and using assert!(!thing) to assert_eq!(!thing, false) and pretty_assertions::assert_eq isn't doing much for us here AFAICT so it's weird that it's imported just for this function.
| matches!( | ||
| arg, | ||
| "-c" | "--config-env" | ||
| "-C" | "-c" |
There was a problem hiding this comment.
Somewhere in here we should comment on why we reject when we see -C.
2 participants
It's safer to make sure these commands go through approval flows.