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.

Sign up for GitHub

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

fix: Alternate approach to sorting paths #629

Closed
wants to merge 2 commits into from
Closed

fix: Alternate approach to sorting paths #629

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da96f03
style: alternate approach to sorting paths
robinbowes Feb 19, 2024
2855eda
refactor: silence shfmt's griping
robinbowes Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions hooks/_common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,13 @@ function common::per_dir_hook {

local pids=()

# shellcheck disable=SC2207 # More readable way
local -a dir_paths_unique=($(printf '%s\n' "${dir_paths[@]}" | sort -u))
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
Comment on lines +351 to +357
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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[*]}"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shfmt ok with that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? =)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When used like that, IFS is 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 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[*]}"))'

Copy link
Collaborator

@yermulnik yermulnik Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No complaints from me.


local length=${#dir_paths_unique[@]}
local last_index=$((${#dir_paths_unique[@]} - 1))
Expand Down
Loading