yadm-dev / yadm

Yet Another Dotfiles Manager
https://yadm.io/
GNU General Public License v3.0
5.15k stars 176 forks source link

Call bootstrap scripts with a tty. #449

Closed JeffFaer closed 5 days ago

JeffFaer commented 1 year ago

What does this PR do?

Changes the contributed bootstrap-in-dir script so that it passes the tty through to the individual bootstrap scripts.

What issues does this PR fix or reference?

344

Previous Behavior

stdin for the individual scripts was the pipe from the find command.

New Behavior

stdin for the individual scripts is the same as it is for the bootstrap-in-dir script (typically a tty)

Have tests been written for this change?

no

Have these commits been signed with GnuPG?

no


Please review yadm's Contributing Guide for best practices.

mweberjc commented 1 year ago

This triggers a shellcheck complaint:

In .config/yadm/bootstrap line 18:
bootstraps=( $(find -L "$BOOTSTRAP_D" -type f | sort) )
             ^-- SC2207 (warning): Prefer mapfile or read -a to split command output (or quote to avoid splitting).
JeffFaer commented 1 year ago

That shellcheck warning seems like a bit of a false positive to me. IFS is being explicitly set, so there shouldn't be any surprises about how the command is being split into the array.


https://www.shellcheck.net/wiki/SC2207

If you have already taken care (through setting IFS and set -f) to have word splitting work the way you intend, you can ignore this warning.

TIL that file name globbing would still happen in this situation:

$ print_elements() {
>   echo "foo"
>   echo "bar"
>   echo "D*"
> }
$ IFS=$'\n' arr=( $(print_elements) )
$ echo "${arr[@]}"
foo bar Desktop Documents Downloads

Wild. I'll switch to use mapfile

erijo commented 5 days ago

Thanks a lot for your PR! The (potential) problem with mapfile is that it doesn't work with bash 3 which is the default bash version om macOS. I've just now pushed ec100410 which fixes the tty problem but using read instead of mapfile.

JeffFaer commented 5 days ago

Cheers. Thanks for merging!