Fix abbr --command conflicts #12021
Fix abbr --command conflicts #12021
Conversation
| if !self.matches_position(position) { | ||
| return false; | ||
| } | ||
| if !self.commands.is_empty() && !self.commands.contains(&command.to_owned()) { |
There was a problem hiding this comment.
would have been nice to add a test case to the commit message,
or (even better) to tests/checks/abbr.fish,
so reviewers don't have to read through the issue.
src/abbrs.rs
Outdated
| return false; | ||
| if self.commands.is_empty() { | ||
| if !command.is_empty() { | ||
| return false; |
There was a problem hiding this comment.
I don't really know this code but this feels like a bit hard to follow,
can we make it easier?
I think what the existing code is trying to do is,
return false if all of
- the abbr has any
-cargument - the actual command does not match any of the
-carguments
and you return false if all of
- there is no
-cargument - there is no command detected.
I don't understand why,
do you have a test case.
src/abbrs.rs
Outdated
| return false; | ||
| } | ||
| } else { | ||
| if command.is_empty() || !self.commands.contains(&command.to_owned()) { |
There was a problem hiding this comment.
I'm also not sure why we also special-case "command.is_empty()" in the other branch,
because why would self.commands ever contain an empty command?
If it doesn't that's probably a case of GIGO
src/abbrs.rs
Outdated
| .abbrs | ||
| .iter() | ||
| .any(|a| a.name == new_name && a.commands == commands), | ||
| "Target abbreviation name and commands already exist." |
There was a problem hiding this comment.
this assertion is no longer needed, because we already do
.expect("Abbreviation to rename was not found.")
below, which has the same meaning
src/abbrs.rs
Outdated
| .iter() | ||
| .position(|a| a.name == name && a.commands == commands); | ||
|
|
||
| if let Some(idx) = index { |
There was a problem hiding this comment.
use let-else here to reduce indentation
let Some(idx) = self.abbrs.position(...) else {
return false;
};
src/abbrs.rs
Outdated
| if !self.abbrs.iter().any(|a| a.name == name) { | ||
| assert!( | ||
| self.used_names.remove(name), | ||
| "Name was not in used_names but should have been" |
There was a problem hiding this comment.
this code ("remove from used_names if necessary") is duplicated across both rename() erase(),
can we extract a simple function like fn on_remove(name: &wstr) to do it?
| if abbrs | ||
| .list() | ||
| .iter() | ||
| .any(|a| a.name == *new_name && a.commands == opts.commands) |
There was a problem hiding this comment.
this should probably be stricter.
Consider
abbr f --command=foo ...
abbr fb --command=foo --command=bar ...
abbr --rename f --command=foo fb
It's probably not a good idea for this to be allowed, because they do conflict.
There was a problem hiding this comment.
Thanks for the review. I'm not sure if I agree here as the resulting (name = "fb", commands = ["foo"]) doesn't conflict with (name = "fb", commands = ["foo", "bar"]). With the new logic we consider an abbr unique by their (name, commands) pair, instead of just their name.
There was a problem hiding this comment.
it doesn't conflict in the data structure but it does when trying to use it. Hmm.
src/builtins/abbr.rs
Outdated
| { | ||
| streams.err.append(wgettext_fmt!( | ||
| "%s %s: Abbreviation %s already exists, cannot rename %s\n", | ||
| "%s %s: Abbreviation %s with the specified command restrictions already exists, cannot rename %s\n", |
There was a problem hiding this comment.
probably better not to change the output if there are no command restrictions?
And if there are, we can mention the concrete ones:
Abbreviation dst already exists for commands 'foo', 'bar' and 'baz', cannot rename src
| } | ||
| abbrs.rename(old_name, new_name); | ||
|
|
||
| abbrs.rename(old_name, new_name, &opts.commands); |
There was a problem hiding this comment.
The synopsis in doc_src/cmds/abbr.rst is
abbrs --rename OLD_WORD NEW_WORD
so it doesn't mention "--command" at all.
So we should either document this behavior,
or have "abbr --rename" error if OLD_WORD is ambiguous.
Either is fine by me.
| if ret == EnvStackSetResult::Ok { | ||
| result = Ok(SUCCESS) | ||
| }; | ||
| if opts.commands.is_empty() { |
There was a problem hiding this comment.
Wait, do abbreviations with commands have no _fish_abbr_ materialization?
I guess that might be true because that's a legacy thing.
But this change feels like an independent fix,
if that's true, it should go into a separate commit if possible.
2 participants
Description
Fixes #11184
TODOs: