cli: warn user if they try to jj bookmark set bookmark@remote
#5423
Conversation
That would also catch cases like |
0xdeafbeef
force-pushed
the
0xdeafbeef/push-wxxxnkvrpxns
branch
2 times, most recently
from
df4e93a to
88ed179
Compare
i've added revset parsing |
| fn parse(name: &str, helper: &WorkspaceCommandHelper) -> Result<Self, CommandError> { | ||
| let node = helper.parse_revset_expression(name)?; |
There was a problem hiding this comment.
Perhaps, we can add public revset::parse_symbol() -> Result<String, ..> function that does parse_program() and expect_literal() (or reexport these functions.) Revset aliases shouldn't be expanded, and the other context parameters are unneeded.
Maybe we can add error hint by inspecting the source string? It might be easier if parsed tree (= result of parse_program()) is available, but I'm not sure.
cli/src/commands/bookmark/mod.rs
Outdated
| writeln!( | ||
| hint, | ||
| "To track remote bookmarks, use: jj bookmark --track {}", | ||
| remote_like.join(" ") | ||
| )?; |
There was a problem hiding this comment.
Unparsable name can be just an error, not a warning. I think that's the idea of using revset parser. User can use escape syntax to specify weird bookmark name.
There was a problem hiding this comment.
Unparsable name can be just an error, not a warning. I think that's the idea of using revset parser. User can use escape syntax to specify weird bookmark name.
Sorry, I didn't get it. The only reason to use a revset parser is not to reinvent my own :)
You want to return an error for cases when name was sucessfully parsed as revset(except legit cases) and when it was not parsed?
There was a problem hiding this comment.
Yes, I think Yuya just meant the since we're using the real revset parser, we can make it an error. If we had some naive check instead, we might have wanted to make it a warning so we don't prevent valid names. But I'm not 100% sure that's what he meant.
There was a problem hiding this comment.
Yes.
without shell escaping:
foo@bar-> error"foo@bar"-> local bookmarkfoo@barfoo--> error"foo-"-> local bookmarkfoo-(foo)-> could be an error (but might be okay to parse as local bookmarkfoo)
CHANGELOG.md
Outdated
| @@ -115,6 +115,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|
|||
| * New `subject(pattern)` revset function that matches first line of commit | |||
| descriptions. | |||
| * Warn user if he tries to use `jj boookmark set branch@remote` | |||
There was a problem hiding this comment.
nit: "he tries" -> "they try"
cli/src/commands/bookmark/mod.rs
Outdated
| } | ||
| drop(writer); | ||
|
|
||
| let mut hint = ui.hint_default(); |
There was a problem hiding this comment.
nit: we usually inline these where they're used, so just call ui.hint_default() on line 235 etc
0xdeafbeef
force-pushed
the
0xdeafbeef/push-wxxxnkvrpxns
branch
from
88ed179 to
aa06bfd
Compare
I'm not sure, maybe there is a legit use case for having
@in branch name, but most likely it's a mistake.It's also not very consistent with other commands.
You can
jj rebase -d br@origin, but you can'tjj bookmark set br@originafter rebasing(I was lazy importing bookmark).Checklist
If applicable:
CHANGELOG.md