bash script to bulk decode files

Question

I have written script that does the following. It uses cbl-log program to decode several log files in binary format. cbl-log only decodes 1 file at a time, so the script decodes files in bulk. If no arguments are given, it assumes log files (with extension .cbllog) are in the current directory, and it'll output files to decoded directory. Otherwise you need to give 2 arguments. The first is the directory with all cbllog files, and the second is the output directory.

#!/usr/bin/env bash
if (( $# == 1 || $# > 2 )); then
 echo "Usage: cbldecode" 
 echo " cbldecode <source directory> <target directory>"
 exit 1
fi
cmd="$(command -v {./,}cbl-log | head -n1)"
if [[ -z "$cmd" ]] ; then
 echo "cbl-log should be in PATH or current directory"
 exit 1
fi 
log_dir=${1:-$PWD}
out_dir=${2:-"decoded"}
count=$(find "$log_dir" -maxdepth 1 -name "*.cbllog" | wc -l) 
if (( "$count" == 0 )); then
 echo "no cbllog files in source directory"
 exit 1
fi
if ! [[ -d "$out_dir" ]]; then
 mkdir -p "$out_dir"
elif ! [[ -w "$out_dir" ]]; then
 echo "no write permission to $out_dir"
 exit 1
fi
for path in "$log_dir"/*.cbllog; do
 file=${path//$log_dir/}
 out_file="${file%.cbllog}"
 "$cmd" logcat "$path" "$out_dir"/"$out_file"
done

While this works, I'm looking for ways to make the code more compact and optimize for speed.

• user211881: Can you add the specific things you want to be reviewed for this question, such as if you followed general practices and/or if you want to know how to optimize it?
• user219820: @K00lman looking for ways to make code more compact, and any time optimizations.
• user211881: Could you add that to the question?

Answer 1

Use the -u and -e switches to bash, when possible, because they help to find bugs. Your log decoder may return errors that you want to ignore (making -e unhelpful) but -u is safe here.

Move the echo ... exit pattern into a function:

die() { printf "%s\n" "$@" >&2; exit 1; }

Quotes aren't needed on the left-hand side of [[ tests. For "die unless foo" assertions, consider writing them in the form foo || die "test failed":

[[ -z $cmd ]] && die "..."

(( $# == 1 || $# > 2 )) is more clearly written in the affirmative, as (( $# == 0 || $# == 2 ))

Collect the file list once instead of twice. Use globbing instead of find to get proper handling of filenames with spaces in them. Include contributing variables in error messages. Do a cd first so that you don't need to decompose filenames later. (( expressions return false when zero so there's usually no need to explicitly write x == 0:

shopt -s nullglob
pushd "$log_dir" || die "can't chdir to $log_dir"
declare -a in=( *.cbllog )
popd || die "can't return to original working directory"
(( ${#in} )) || die "no cbllog files in $log_dir"
...
for path in "${in[@]}"; do

Your command detector has a bug if cbl-log is an alias or function. Use type -P instead. If you truncate using read instead of head, the assignment doubles as an emptyness test:

read cmd < <( type -P {./,}cbl-log ) || die "cbl-log should be in PATH or current directory"

Your basename implementation has a bug if log_dir contains globbing characters. This is obsoleted by the pushd above but something like file=${path##*/} or file=$( basename "$path" ) would work.

mkdir -p is a no-op when the directory exists; no need to test for that yourself.

You'll need execute permission on $out_dir along with write.

Putting it all together:

 #!/bin/bash -u 
 die() { printf "%s\n" "$@" >&2; exit 1; }
 (( ${#} == 0 || ${#} == 2 )) || die "Usage: cbldecode" " cbldecode <source directory> <target directory>"
 log_dir=${1:-$PWD}
 out_dir=${2:-"decoded"}
 read cmd < <( type -P {./,}cbl-log ) || die "cbl-log should be in PATH or current directory"
 pushd "$log_dir" || die "can't chdir to $log_dir"
 shopt -s nullglob
 declare -a in=( *.cbllog )
 popd || die "can't return to original working directory"
 (( ${#in} )) || die "no cbllog files in $log_dir"
 mkdir -p "$out_dir"
 [[ -w $out_dir && -x $out_dir ]] || die "insufficient permissions on $out_dir"
 for basename in "${in[@]}"; do 
 "$cmd" logcat "$log_dir/$basename" "$out_dir/${basename%.cbllog}"
 done

For faster conversion, add & to the cbl-log invocation, or adapt the script to send basenames to xargs -P.

• user219820: Fantastic feedback. I noticed pushd displays stack in stdout so added 1>/dev/null to it. I think we can use -e if we add || true to the line that does log decoding, right? Finally I'm using macos so 'type -P` is invalid, unfortunately. Any alternatives?
• Oh My Goodness: Yes || true will satisfy -e. Prepend . to $PATH and invoke the command with a do-nothing switch like -v or -help. If that fails, throw error. Adding cwd to PATH is not a safe practice in general. It's fine here since the whole script is shell builtins. To make it safer declare a function like cmd(){ local PATH=.:$PATH; cbl-log "$@"; } so only that one command is affected by the unsafe PATH.