Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fish_add_path: set --erase incorrectly handles indices #7776

Closed
branchvincent opened this issue Mar 4, 2021 · 1 comment
Closed

fish_add_path: set --erase incorrectly handles indices #7776

branchvincent opened this issue Mar 4, 2021 · 1 comment
Labels
bug Something that's not working as intended
Milestone

Comments

@branchvincent
Copy link
Contributor

branchvincent commented Mar 4, 2021

While testing out the new fish_add_path --move, I noticed incorrect paths being removed/moved stemming from

set -e newvar[$indexes]

A simple repro is:

> set arr 1 2 3
> set -e arr[1] arr[2] # expanded form of arr[$indexes] above
> echo $arr
2 # expected 3

As you can see, set -e arr[1] works fine but presumably when set -e arr[2] occurs, arr = 2 3. The elements should be erased in index-descending order as set -e arr[1 2] correctly handles. Thus, a workaround for my initial issue is

diff --git a/share/functions/fish_add_path.fish b/share/functions/fish_add_path.fish
index cac0d9d4f..11e1017a0 100644
--- a/share/functions/fish_add_path.fish
+++ b/share/functions/fish_add_path.fish
@@ -63,7 +63,7 @@ function fish_add_path --description "Add paths to the PATH"
     set -l newvar $$var
     if set -q _flag_move; and set -q indexes[1]
         # We remove in one step, so the indexes don't move.
-        set -e newvar[$indexes]
+        set -e newvar[(echo $indexes)]
     end
     set $mode newvar $newpaths
System info
> fish --version
fish, version 3.2.0

> uname -a
Darwin Branchs-MacBook-Pro.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64

> echo $TERM
xterm-256color

> sh -c 'env HOME=$(mktemp -d) fish'
# reproduces

Btw, amazing work on fish! It is such a joy to use!

@faho
Copy link
Member

faho commented Mar 4, 2021

Ooh, this is a bit subtle.

Since set is a "normal" builtin and not a piece of syntax, this runs the normal expansions. So, because $indexes is a multi-element list, it expands as a cartesian product ("everything with everything"), so

set -l indexes 1 2 3 # for example
set -e newvar[$indexes]

becomes something like

set -e newvar[1] newvar[2] newvar[3]

and then set just goes through its arguments and runs everything immediately - removes the first element, removes the second element (but on the new list!), removes the third element, ...

The simplest fix here is to just quote the "$indexes", and that's what I'm going to do.

@faho faho added the bug Something that's not working as intended label Mar 4, 2021
@faho faho added this to the fish 3.2.1 milestone Mar 4, 2021
@faho faho closed this as completed in d85bdf1 Mar 4, 2021
@faho faho changed the title set --erase incorrectly handles indices fish_add_path: set --erase incorrectly handles indices Mar 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

No branches or pull requests

2 participants