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

Call bootstrap scripts with a tty. #449

Closed
wants to merge 2 commits into from

Conversation

JeffFaer
Copy link

@JeffFaer JeffFaer commented Mar 8, 2023

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.

The exising bootstrip-in-dir script is changing stdin to be the result
of the find command.

fix yadm-dev#344
@TheLocehiliosan TheLocehiliosan self-assigned this Mar 16, 2023
@mweberjc
Copy link

mweberjc commented Jun 7, 2023

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
Copy link
Author

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 added a commit that referenced this pull request Nov 24, 2024
Inspired by #449 but using read instead of mapfile to make it work with bash 3.
erijo added a commit that referenced this pull request Nov 24, 2024
Inspired by #449 but using read instead of mapfile to make it work with bash
3. Fixes #344.
erijo added a commit that referenced this pull request Nov 24, 2024
Inspired by #449 but using read instead of mapfile to make it work with bash
3. Fixes #344.
@erijo
Copy link
Collaborator

erijo commented Nov 24, 2024

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 ec10041 which fixes the tty problem but using read instead of mapfile.

@erijo erijo closed this Nov 24, 2024
@JeffFaer JeffFaer deleted the bootstrap branch November 24, 2024 22:29
@JeffFaer
Copy link
Author

Cheers. Thanks for merging!

erijo added a commit that referenced this pull request Nov 25, 2024
Inspired by #449 but using read instead of mapfile to make it work with bash
3. Fixes #344.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants