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

Jump to bottom

Glob for *.bash properly when path contains spaces #1902

Merged
merged 3 commits into from
Feb 13, 2022
Merged

Glob for *.bash properly when path contains spaces #1902

merged 3 commits into from
Feb 13, 2022

Conversation

gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Jul 25, 2021 •
edited
Loading

Description

Instead of $LIB and $CUSTOM variables, use the glob pattern in the foreach statement quoting the path but not the glob. Quoting the glob will make it not a glob, but when expanding the not-quoted $LIB or $CUSTOM variable Bash will split all words, including spaces in the actual path. This helps towards #1696.

This is literally one of the examples under Code Style on the contribution page.

Closes #1435.

Motivation and Context

This resolves the failure to source the files under $BASH_IT/lib when there is any space in $BASH_IT, such as when the user name contains a space or the path to the repository contains a space.

You can reproduce this with a user short name with a space, which is possible with the "MacBuddy" default setup assistant on Mac OS X, or if $BASH_IT is set to a path containing an (additional) space, like if I cloned to ~/Library/Application Support/Bash It. Windows commonly has user names with spaces.

How Has This Been Tested?

This is my current working branch since July on my system where $BASH_IT is ~/Library/Application Support/Bash It (that's two spaces in the path).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@gaelicWizard gaelicWizard marked this pull request as ready for review July 25, 2021 22:50
davidpfarrell
davidpfarrell previously requested changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

Greetings - Thanks for taking the time to debug and create a PR, as well as the thorough description of the issue !

I've confirmed the issue (and fix) locally.

I have a small suggestion to keep the CUSTOM variable so we only have to do the complex definition once.

bash_it.sh Outdated Show resolved Hide resolved
bash_it.sh Outdated Show resolved Hide resolved
@gaelicWizard gaelicWizard force-pushed the glob branch 2 times, most recently from 72931a2 to 8a2d1a0 Compare July 26, 2021 03:19
@gaelicWizard
Copy link
Contributor Author

After just force-pushing twice, I remembered that the contribution document says not to do that... sorry, I'll be more careful.

gaelicWizard
gaelicWizard commented Jul 26, 2021
View reviewed changes
scripts/reloader.bash Outdated Show resolved Hide resolved
davidpfarrell
davidpfarrell reviewed Jul 27, 2021
View reviewed changes
bash_it.sh Outdated Show resolved Hide resolved
@gaelicWizard
Copy link
Contributor Author

Would someone do me the solid of enabling the tests to run for this PR? My main branch is so far ahead of this that I'd just like to see that BATS approves of my code. 😃

NoahGorny
NoahGorny approved these changes Aug 3, 2021
View reviewed changes
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Hi @gaelicWizard
I have been going through your PRs and wow- you did quite a lot of great quality changes!

Thank you for your efforts, and like always thanks @davidpfarrell for your thoughtful CR.
I am also sorry that I was not quite available in the last few months. I will try to have more time to look at things- considering the effort you put in 😄

@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Aug 9, 2021 •
edited
Loading

I've rebased on current master branch, and squashed some of my intermediate commits. Anything else needed for this PR? 😃

@NoahGorny
Copy link
Member

hi @gaelicWizard, seems like you need to shfmt your changes.
Also @davidpfarrell should take another look and change his review

@gaelicWizard gaelicWizard marked this pull request as draft August 12, 2021 19:25
@gaelicWizard
Copy link
Contributor Author

I've marked this PR as draft as I'm rebasing it on a different branch in my repo.

@gaelicWizard gaelicWizard force-pushed the glob branch 6 times, most recently from d88f4bb to 0f5aa4a Compare August 15, 2021 04:20
@gaelicWizard gaelicWizard force-pushed the glob branch 3 times, most recently from 93593da to a54fce8 Compare August 26, 2021 21:18
@gaelicWizard gaelicWizard force-pushed the glob branch 4 times, most recently from d10a278 to 25f14eb Compare September 6, 2021 00:41
NoahGorny
NoahGorny approved these changes Jan 18, 2022
View reviewed changes
Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

LGTM

@gaelicWizard gaelicWizard force-pushed the glob branch 2 times, most recently from 61d5bcf to 1d2e653 Compare January 18, 2022 21:57
@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Jan 18, 2022 •
edited
Loading

Rebased on master and removed some no-longer-relevant comments and unnecessary shellcheck directives.

@gaelicWizard gaelicWizard force-pushed the glob branch 4 times, most recently from bac5fb6 to 407c9a7 Compare January 25, 2022 05:34
@gaelicWizard
Copy link
Contributor Author

Rebased on current master and ready to go!

@gaelicWizard gaelicWizard force-pushed the glob branch 2 times, most recently from 1b998e1 to 77a032b Compare January 26, 2022 18:23
@gaelicWizard gaelicWizard mentioned this pull request Jan 26, 2022
Merged
6 tasks
@gaelicWizard gaelicWizard force-pushed the glob branch 5 times, most recently from 4ab63fa to ee12f5f Compare January 28, 2022 21:55
@NoahGorny
Copy link
Member

Hi @davidpfarrell, wanna take another look?

gaelicWizard and others added 2 commits February 6, 2022 16:59
@gaelicWizard
main: Glob for *.bash properly when path contains spaces
0d346b2 
- `shfmt`, `shellcheck`
- Clean up legacy/compatibility code to simpler control flow
- Move theme stuff down to where themes are handled
- Don't use `**` as _Bash It_ has never before set `globstar`; this eliminates varying behavior by environment; this alsö fixes users having any not-enabled themes under their custom dir.
- Lose weird Mac-specific alternate shell startup file (Bash loads startup files on Mac the same as it does on any other *nix system.)
- Place `composure.sh` init all in one place
- remove 10-years-deprecated backwards compatibility: Deprecated in `b59ee658f78ec6ff8c6c2754216e0322b7fe18e2` dated 2011-10-29.
@gaelicWizard
main: adopt _bash-it-log-prefix-by-path()
bc95ece
@gaelicWizard gaelicWizard force-pushed the glob branch 2 times, most recently from cd96fb9 to c41969f Compare February 10, 2022 18:42
@NoahGorny NoahGorny dismissed davidpfarrell’s stale review February 13, 2022 21:53

Long time has passed since the review

@NoahGorny NoahGorny merged commit 23f9b74 into Bash-it:master Feb 13, 2022
@gaelicWizard gaelicWizard deleted the glob branch February 13, 2022 22:25
@gaelicWizard gaelicWizard mentioned this pull request Feb 14, 2022
Merged
6 tasks
@akinomyoga akinomyoga mentioned this pull request Nov 13, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoahGorny NoahGorny NoahGorny approved these changes

@davidpfarrell davidpfarrell Awaiting requested review from davidpfarrell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BASH_IT_CUSTOM doesn't work without setting bash globstar option
3 participants
@gaelicWizard @NoahGorny @davidpfarrell