fix: Alternate approach to sorting paths #629
Conversation
robinbowes
changed the title
robinbowes
changed the title
MaxymVlasov
changed the title
|
That's... interesting And it okay with local -a dir_paths_unique
# Note: This can break if any elements of dir_path contain glob characters.
# However, all elements of dir_path are generated from the output of dirname
# Which means all glob characters will already have been expanded.
# shellcheck disable=SC2207 # Can't use mapfile in bash 3
IFS=$'\n' dir_paths_unique=($(sort -u <<< "${dir_paths[*]}"))
unset IFS |
shfmt uses a static parser, so it can't cope with all valid bash syntax. |
| local -a dir_paths_unique | ||
| # Note: This can break if any elements of dir_path contain glob characters. | ||
| # However, all elements of dir_path are generated from the output of dirname | ||
| # Which means all glob characters will already have been expanded. | ||
| # shellcheck disable=SC2207 # Can't use mapfile in bash 3 | ||
| IFS=$'\n' dir_paths_unique=($(sort -u <<< "${dir_paths[*]}")) | ||
| unset IFS |
There was a problem hiding this comment.
| local -a dir_paths_unique | |
| # Note: This can break if any elements of dir_path contain glob characters. | |
| # However, all elements of dir_path are generated from the output of dirname | |
| # Which means all glob characters will already have been expanded. | |
| # shellcheck disable=SC2207 # Can't use mapfile in bash 3 | |
| IFS=$'\n' dir_paths_unique=($(sort -u <<< "${dir_paths[*]}")) | |
| unset IFS | |
| # Note: This can break if any elements of dir_path contain glob characters. | |
| # However, all elements of dir_path are generated from the output of dirname | |
| # Which means all glob characters will already have been expanded. | |
| # shellcheck disable=SC2207 # Can't use mapfile in bash 3 | |
| local -a dir_paths_unique=($(IFS=$'\n' sort -u <<< "${dir_paths[*]}")) |
There was a problem hiding this comment.
No, that won't work.
IFS=$'\n' affects both the expansion of ${dir_paths[*]} and also how dir_paths_unique=( ... ) splits the content inside the brackets. It needs to apply to the whole command.
There was a problem hiding this comment.
This one-instead-of-three-lines seems to work on my end with both Bash v3 and v5 w/o a need to unset IFS (as it might have non-default value for some reason)
There was a problem hiding this comment.
It needs to apply to the whole command.
You're probably right.
Then we need to preserve IFS and set it back after this line.
ps: is the change from this PR worth of the effort? =)
There was a problem hiding this comment.
It needs to apply to the whole command.
You're probably right. Then we need to preserve
IFSand set it back after this line.
That's not trivial, but, as you identified earlier, it's also not necessary. When used like that, IFS is only set temporarily for the single command.
There was a problem hiding this comment.
When used like that,
IFSis only set temporarily for the single command.
Yeah, I know that. The unset IFS was what worried me and also my local tests didn't preserve IFS (despite it should've been preserved to the best of my knowledge):
> cat ~/tmp/zzz.sh
#!/home/giermulnik/tmp/bash/bash
#!/usr/bin/env bash
eee() {
local -a dir_paths_unique
IFS="|"
IFS=$'\n' dir_paths_unique=($(sort -u <<<"$*"))
echo "IFS: >$IFS<"
}
eee 11 44 11 "55 55"
> ~/tmp/zzz.sh
IFS: >
<Linefeed is there, though it should not 🤔
There was a problem hiding this comment.
Ah, this is because the line is just two variable assignments.
Try this:
$ foo=1 bar=2 ; echo "$foo"
1
$ foo=3 date ; echo "$foo"
Mon 19 Feb 2024 20:13:05 GMT
1
Notice how the first line sets foo=1 but the second one doesn't set foo=3.
This works without setting IFS permanently:
IFS=$'\n' eval 'dir_paths_unique=($(sort -u <<<"${dir_paths[*]}"))'
There was a problem hiding this comment.
And eval on top of it =)
While I support the direction, I see no point on trying to implement this bit of optimization to replace the existing one.
@robinbowes @MaxymVlasov If agreed, then this PR should be closed w/o merging. Thanks.
There was a problem hiding this comment.
No complaints from me.
Put an
xinto the box if that apply:Description of your changes
Follow-up to #627 that uses an alternate construct to sort an array of paths.
How can we test changes