git: reject remote names that are empty or contain whitespace#9155
git: reject remote names that are empty or contain whitespace#9155eyupcanakman wants to merge 1 commit into
Conversation
eyupcanakman
force-pushed
the
fix/remote-name-whitespace-9099
branch
from
f98ed63 to
597f07a
Compare
|
|
||
| fn validate_remote_name(name: &RemoteName) -> Result<(), GitRemoteNameError> { | ||
| if name == REMOTE_NAME_FOR_LOCAL_GIT_REPO { | ||
| if name.as_str().is_empty() { |
There was a problem hiding this comment.
Can we use gix function such as https://docs.rs/gix/latest/gix/remote/name/fn.validated.html ?
There was a problem hiding this comment.
Done, using gix::remote::name::validated() now.
| } | ||
|
|
||
| #[test] | ||
| fn test_git_remote_name_with_whitespace() { |
There was a problem hiding this comment.
CLI tests are slow. If we need to test various error conditions, it's better to add jj-lib tests.
There was a problem hiding this comment.
Moved the detailed error condition tests to lib/tests/test_git.rs. The CLI test now has a single case with a comment pointing to the lib tests.
|
nit: Please squash the two commits (see https://docs.jj-vcs.dev/latest/contributing/#commit-guidelines) |
|
Thanks for doing this! I know I self-assigned the issue but haven't had the time to get around to it... |
eyupcanakman
force-pushed
the
fix/remote-name-whitespace-9099
branch
from
62a9a7c to
1f335dd
Compare
|
Squashed. |
| #[error("Invalid Git remote name: {0}")] | ||
| InvalidName(#[from] gix::remote::name::Error), |
There was a problem hiding this comment.
nit: remove : {0} which should be printed as a source error.
|
Hey @eyupcanakman, any updates on this? |
Use `gix::remote::name::validated()` to reject invalid git remote names (empty strings, whitespace, and other names that git itself would reject) at the jj-lib level before they propagate to git operations. Fixes jj-vcs#9099
eyupcanakman
force-pushed
the
fix/remote-name-whitespace-9099
branch
from
1f335dd to
265b27a
Compare
|
Sorry for the lag on this. Pushed yuja's last nit just now, dropping the |
| "https://example.com/", | ||
| None, | ||
| gix::remote::fetch::Tags::default(), | ||
| &StringExpression::all(), |
There was a problem hiding this comment.
Can you rebase this PR and update the tests accordingly?
4 participants
Passing a remote name with spaces (e.g.
jj git remote add "my remote" url) causes a panic in libgit2. Empty names also panic.Added
EmptyandWithWhitespacevariants toGitRemoteNameErrorsovalidate_remote_namerejects these with a clear message before they reach libgit2.Fixes #9099