Problem
I am fairly new to bash (I have about 3 months of experience), and now I’ve written my first real application (sort of).
It’s a function inside of ~/.bash_profile
, but it has an installer. It is called loginfo and it keeps track of when the terminal was used by the user, and allows them to view this data.
It comes with an installer, and a built-in uninstaller.
install.sh
#!/bin/bash
width=$(tput cols)
printSolid(){
printf "%${width}sn" " " | tr " " "$1"
}
printCenter(){
textsize=${#1}
padding=$((($width+$textsize)/2))
printf "%${padding}sn" "$1"
}
install_loginfo(){
touch ~/.bash_profile
if grep -q "#loginfo-->" ~/.bash_profile
then
echo "[ER] Already installed 'loginfo'."
else
loginfofile=$(echo "$(dirname $0)/loginfo.txt")
test ! -e $loginfofile && (echo "[ER] Unable to locate '$loginfofile'")
cat $loginfofile >> ~/.bash_profile
test $? -eq 0 && (echo "[OK] Successfully appended '$loginfofile' to '~/.bash_profile'")
test $? -ne 0 && (echo "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'")
fi
}
printSolid "#"
printCenter "Installer for 'loginfo'"
printCenter "By 545H4"
printSolid "#"
echo "Would you like to install 'loginfo'? (y/n)"
while true
do
read -n 1 -r -s confirm
case $confirm in
y | Y)
install_loginfo
break
;;
n | N)
echo "'loginfo' was not installed."
break
;;
*) echo "Please enter a valid option (y/n)"
;;
esac
done
echo "You have got to restart your terminal for any change to take place."
exit
loginfo.txt
#loginfo-->
currLogin=$(date '+%d/%m/%Y %H:%M:%S')
echo "$currLogin" >> terminallogins.txt
deltLoginfo(){
while true
do
echo "Remove 'loginfo'? (y/n)"
read -n 1 -r -s confirm
case $confirm in
y | Y)
echo "$(sed '/^#loginfo-->/,/^#<--loginfo/d' ~/.bash_profile)" > ~/.bash_profile
test $? -eq 0 && (echo "[OK] Successfully removed 'loginfo' from '~/.bash_profile'")
test $? -ne 0 && (echo "[ER] Failed to remove 'loginfo' from '~/.bash_profile'")
touch ~/terminallogins.txt
rm ~/terminallogins.txt
test $? -eq 0 && (echo "[OK] Successfully removed '~/terminallogins.txt'")
test $? -ne 0 && (echo "[ER] Failed to remove '~/terminallogins.txt'")
echo "You have got to restart your terminal for any change to take place."
break
;;
n | N)
echo "'loginfo' wasn't removed."
break
;;
*)
echo
;;
esac
done
}
loginfo(){
case $1 in
last) echo "$(tail -2 ~/terminallogins.txt)" | head -n 1;;
curr) echo "$currLogin";;
list) cat ~/terminallogins.txt;;
delt) deltLoginfo;;
*) echo "Usage: loginfo <last/curr/list/delt>";;
esac
}
#<--loginfo
Inside of the repository install.sh and loginfo.txt are in the same folder, I am new to GitHub as well but using two files seemed the most easy to manage.
Solution
I like the way you center the text in your print statements 😉 What a neat solution.
The overall concept you have is pretty sound, but there are some implementation details you have which are concerning.
Defensive install
You shoudl probably start the loginfo.txt file with an empty line. This way, if the user happens to have a trailing line which is not terminated by a newline, then the loginfo.txt append will start on a new line anyway. Otherwise, you may end up with something like:
blah-blah
fi#loginfo-->
Error checking
This code is somewhat broken, though it may actually work….
cat $loginfofile >> ~/.bash_profile
test $? -eq 0 && (echo "[OK] Successfully appended '$loginfofile' to '~/.bash_profile'")
test $? -ne 0 && (echo "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'")
The $?
variable is the exit state of the previous command. In the first line, the $? refers to the exit-status of the append, which is what you want…
.. but, in the second test, the $? is the exit status of the previous test, not of the append. This is confusing, but, if the previous test fails, the exit status will reflect that….so, the second test tests the first test, not the actual operation…. you know that… right?
Additionally, is it OK if the tests fail? Shouldn’t you exit yourself with an error status so that someone else can check your success?
test $? -ne 0 && (echo "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'") && exit 1
Finally, I prefer to have a guard-clause style for error handling… you test for a fail, and exit, and print the success otherwise… I normally use if-statements, but, as a test, it would look like:
cat $loginfofile >> ~/.bash_profile
test $? -ne 0 && echo "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'" && exit 1
echo "[OK] Successfully appended '$loginfofile' to '~/.bash_profile'"
I normally have a function, though, that handles errors:
fail() {
echo "$*"
exit 1
}
Then have
cat $loginfofile >> ~/.bash_profile || fail "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'"
- indent your code!
-
don’t do this
printf "%${padding}sn" "$1"
do this
printf "%*sn" "$padding" "$1"
-
in this section:
cat $loginfofile >> ~/.bash_profile test $? -eq 0 && (echo "[OK] Successfully appended '$loginfofile' to '~/.bash_profile'") test $? -ne 0 && (echo "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'")
the 2
test
command is testing the exit status of the previous command, not thecat
command. It’s either testing the exit status of the previoustest
command or the firstecho
command. Either way, you get the result you want, but really by luck. You wantif cat $loginfofile >> ~/.bash_profile; then echo "[OK] Successfully appended '$loginfofile' to '~/.bash_profile'" else echo "[ER] Something went wrong appending '$loginfofile' to '~/.bash_profile'" fi
Be aware that the logic of
A && B || C
is different fromif A; then B; else C; fi
in one subtle way: what happens whenB
exits non-zero?- in
if A; then B; else C; fi
when A succeeds but B fails, C will NOT be executed. - in
A && B || C
when A succeeds but B fails, C WILL be executed — the||
operator detects the previous command in the pipeline (B) exits non-zero and will execute its right-hand side command.
- in
-
I’d recommend using
select
instead of a while loop to get your answerPS3="Would you like to install 'loginfo'? " select ans in Yes No; do case $ans in Yes) install_loginfo break ;; No) echo "'loginfo' was not installed." break ;; esac done