cdown / clipmenu

Clipboard management using dmenu
MIT License
1.11k stars 90 forks source link

clipmenud: fix killing background jobs by unquoting variable #171

Closed mehw closed 2 years ago

mehw commented 2 years ago

Hello,

I found out that a variable storing the multiline result of jobs -p should not be quoted when it's used as sequence of arguments.

Here I propose a trivial fix... to just unquote the variable.

This allows to properly kill all children jobs.

$ jobs -p
7345
7862

$ bg=$(jobs -p)

$ echo $bg
7345 7862

$ echo "$bg"
7345
7862

Correct:
$ kill -- $bg
$ kill -- 7345 7862

Wrong:
$ kill -- "$bg"
$ kill -- "7345"$'\n'"7862"
cdown commented 2 years ago

Thanks! This is indeed a bug, good catch.

In this case it makes no functional difference, but I'd prefer if this was done using an array instead of by field splitting, since it makes it clear what the intent is, so I guess read -ra to a local variable.

mehw commented 2 years ago

In this case it makes no functional difference, but I'd prefer if this was done using an array instead of by field splitting, since it makes it clear what the intent is, so I guess read -ra to a local variable.

Another way to use an array is by using round brackets and * as the expansion parameter.

$ bg=( $(jobs -p) )
$ echo ${bg[*]}
cdown commented 2 years ago

Sure, but it again is done by implicit IFS expansion, and I'd rather avoid that at all as a stylistic choice. It doesn't matter here, but it makes it clearer to readers and static analysis that the splitting is intentional.

mehw commented 2 years ago

Sure, but it again is done by implicit IFS expansion, and I'd rather avoid that at all as a stylistic choice. It doesn't matter here, but it makes it clearer to readers and static analysis that the splitting is intentional.

I experimented with readarray as an alternative solution. This implementation should be IFS agnostic.

Do you think it's better to explicitly use IFS?

PS: When you agree on a sulution I may squash the commits ;)

test_me() {
    local -a bg

    clipnotify &
    clipnotify &

    IFS=$'\t' # or anything else, just to try to mess things up...

    readarray -t bg < <(jobs -p)

    echo -n "\${bg[@]}: "
    echo ${bg[@]}

    echo -n "\${bg[*]}: "
    echo ${bg[*]}

    echo -n "\"\${bg[@]}\": "
    echo "${bg[@]}"

    echo -n "\"\${bg[*]}\": "
    echo "${bg[*]}"

    echo -n "\${!bg[@]}: "
    echo ${!bg[@]}

    echo -n "\${!bg[*]}: "
    echo ${!bg[*]}

    echo -n "\"\${!bg[@]}\": "
    echo "${!bg[@]}"

    echo -n "\"\${!bg[*]}\": "
    echo "${!bg[*]}"

    for i in ${!bg[@]}; do
    kill -- ${bg[i]}
    done
}

test_me
cdown commented 2 years ago

Just the question about "${bg[@]}" to go, I can squash if you like on merge.

mehw commented 2 years ago

Just the question about "${bg[@]}" to go, I can squash if you like on merge.

Possibly the last commit should fix it... Let me know.

Thanks.

cdown commented 2 years ago

squashed/merged to bc35b6f44ab0def13601c4ebd836437c5dc0e360, thanks!

mehw commented 2 years ago

Thank you for the patience fixing this PR ;)