Use parameter substitution instead of dirname/basename, where safe to do so#1926
Use parameter substitution instead of dirname/basename, where safe to do so#1926NoahGorny merged 2 commits intoBash-it:masterfrom
dirname/basename, where safe to do so#1926Conversation
NoahGorny
left a comment
NoahGorny
left a comment
There was a problem hiding this comment.
LGTM, @davidpfarrell can you take a look as well?
If its only when its safe, I see no problem with merging this
|
I'd like to take a day to test this, but at first glance, this looks great. |
gaelicWizard
force-pushed
the
basenamed
branch
from
550b8e3 to
a0a7f7f
Compare
|
Hi @cornfeedhobo, did you have a time to test this? |
gaelicWizard
force-pushed
the
basenamed
branch
from
a0a7f7f to
6dbb216
Compare
|
@NoahGorny, the Mac OS X test runners are so slow that |
gaelicWizard
force-pushed
the
basenamed
branch
2 times, most recently
from
0d1a8e1 to
d76398d
Compare
Convert `var=${dirname $filename)` to `var="${filename%/*}` in cases where there is no ambiguity.
Make sure that the path in `$BASH_IT` is absolute because this path gets embedded in the template `.bash_profile` file if selected by the user.
Convert `var=$(basename $file)` to `var="${file##*/}"`
gaelicWizard
force-pushed
the
basenamed
branch
from
d76398d to
470341b
Compare
|
::: Not me checking this PR twice a day hoping @cornfeedhobo found some time to look through this, nope. ::: |
|
Sorry y'all. Rough week. Taking a test drive with this today. Will report back shortly. @gaelicWizard @NoahGorny if you have any other PRs that need review and/or testing, this is a good weekend for me to power through them. Ping me in the comments. Thanks!! |
cornfeedhobo
left a comment
cornfeedhobo
left a comment
There was a problem hiding this comment.
Everything I tested works as expected. Great work here, and my apologies again for the delay in review!
No worries, thanks for the review @cornfeedhobo ❤️ |
3 participants
Description
Reduce subshells and external commands by using Bash's string processing features instead of
dirnameandbasename. This basically uses$PWD,$BASH_SOURCE, and "${pathname%*/}" and similar constructs to avoid subshells for$(pwd),dirname, andbasename.Motivation and Context
This is part of a set of patches to reduce usage of subshells and external binaries in Bash It.
The two commits here were part of #1914, but that PR went further than this one and some edge cases were identified which could lead to unintended behavior so those uses were left as-is. Both commits here operate specifically on code which specifies the full path within the same function and thus won't run in to possible issues with user-supplied strings. Merging this PR resolves #1914.
The two edge-cases are:
/, and/.Since parameter substitution is string manipulation, it lacks logic to insert
./for an empty directory name, &c.How Has This Been Tested?
Tested locally, reviewed in part in #1914, and all tests pass.
Types of changes
Checklist:
clean_files.txtand formatted it usinglint_clean_files.sh.