1
\$\begingroup\$

How is the logic, efficiency and can it be improved?

This is a snippet from my .bashrc file, I just wrote it.

The code is supposed to do this:

Backup my .bash_history file if it is 1000 lines or less from the defined size of HISTSIZE=100000 and HISTFILESIZE=100000 lines, then backup the file.

  1. Check if .bash_history is 99000 lines or more.
  2. Check if ~/.bash_history.old exists if it doesn't use that filename.
  3. Increment i one digit larger than is already used by filenames.(filename-4.old)
  4. Check if there are more than 20 files already backed up and warn if it is.
  5. Set the new filename into a variable.
  6. Check if the last file with a digit in it's filename is older than the original.
  7. cp the file to new_name

Here is the code:

# Count the number of lines in `/.bash_history and check if it's equal or more than a thousand
if (( $(wc -l < ~/.bash_history) >= 99000 ))
then
    name=~/.bash_history
    # Here is the suffix of the new files and the ones I check for
    old=.old
    # -e FILE ==> True if FILE exists.
    if [[ ! -e ~/.bash_history.old ]]
    then
       printf "%s\n" "##################################################" ".bash_history will be cleared soon, backing up....!" "##################################################" 
       # Here I copy $name which is ~/.bash_history and create backup file
       cp $name ~/.bash_history.old
    else
        # i is the increment in the filenames to be checked and created
        i=0
        # $name$old is ~/.bash_history.old
        if [[ -e $name$old ]]
        then
            # Here I count how many copies there are with digits in the filename
            while [[ -e "$name-$i$old" ]]
            do
                let i++
            done
        fi
        # if there are 20 files already backed up then I need to archive them somewhere else
        if [[ "$i" -ge 20 ]]
        then
            printf "%s\n" "********************************************************" "You need to arhive your history files they are mounting up!!!" "**************************************************************"
        fi
        new_name=$name-$i$old
        minus=$(( i - 1 ))
        if [ $name -nt "$name-$minus$old" ]
        then
            printf "%s\n" "##################################################" ".bash_history will be cleared soon, backing up....!" "##################################################"
            cp ~/.bash_history "$new_name"
        fi
    fi
fi

This is the result from shellcheck.net:

Line 16:
                let i++
                ^-- SC2219: Instead of 'let expr', prefer (( expr )) .
\$\endgroup\$
  • \$\begingroup\$ This question is off-topic because you don't know whether it works. We are happy to review working code. \$\endgroup\$ – dfhwze Jun 5 '19 at 6:50
  • \$\begingroup\$ @dfhwze "how well it works" != "whether it works". "The code is supposed to do this" is debatable. \$\endgroup\$ – l0b0 Jun 5 '19 at 6:51
  • 1
    \$\begingroup\$ @l0b0 "Does my code work and can it be improved?" and "The code is supposed to do this:" tend to make me think "how well it works" is out of the question. \$\endgroup\$ – dfhwze Jun 5 '19 at 6:53
  • 2
    \$\begingroup\$ That's probably something OP can help us with…? \$\endgroup\$ – l0b0 Jun 5 '19 at 7:01
  • \$\begingroup\$ @Heslacher I cleaned the question and added comments to the code....... Is it any better like this then or what? \$\endgroup\$ – somethingSomething Jun 7 '19 at 23:04
3
\$\begingroup\$

First, logrotate is a tool built to do exactly this sort of thing. I would recommend using that to achieve the effectively the same extremely reliably and with lots of configuration options. That said, some suggestions on the code as written:

  • ShellCheck is great; I would recommend following its recommendation to use (( i++ )).
  • wc -l counts the number of lines, not bytes or kilobytes, which seems to be what you want.
  • old, minus and i are not helpful names; I have to read and understand all the code in the context in order to understand what they mean.
  • It looks like you only ever replace the last of the 20 files once you have 20 backups.
  • You have five instances of ~/.bash_history, even though one of them is the value of a variable. I would pull that variable out and reuse it everywhere.
  • Use More Quotes™ - it's good for you and the code.
  • Rather than a special unnumbered backup file (~/.bash_history.old), why not just start numbering the backups immediately? That way you can get rid of at least two checks for whether that file exists ([[ ! -e ~/.bash_history.old ]] and its inverse, [[ -e $name$old ]]).
| improve this answer | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.