5
\$\begingroup\$

When working on a large codebase, I often need to switch between deeply nested directories quickly. To make my life easier, I came up with a bookmarking script.

#!/bin/bash

bm_list() {
    for i in "${!BOOKMARKS[@]}"; do
        echo -e $i'\t'"${BOOKMARKS[$i]}" ;
    done
}

bm_go() {
    cd "${BOOKMARKS[$1]}"
}

bm_add() {
    for i in "${!BOOKMARKS[@]}"; do
        [[ "$1" == ${BOOKMARKS[$i]} ]] && return
    done
    BOOKMARKS+=("$1")
}

bm_clear() {
    unset BOOKMARKS
}

bm_delete() {
    unset BOOKMARKS[$1]
}

bm_compact()
{
    local SIFS="$IFS"; IFS=
    BOOKMARKS=("${BOOKMARKS[@]}")
    IFS="$SIFS"
}

bm_load() {
    local bmfile=${1:-~/.bookmarks}
    local SIFS="$IFS"; IFS=$'\n'
    bm_clear
    while read -d ''; do BOOKMARKS+=("$REPLY"); done < "$bmfile"
    IFS="$SIFS"
}

bm_save() {
    local bmfile=${1:-~/.bookmarks}
    printf '%s\0' "${BOOKMARKS[@]}" > "$bmfile"
}

bm_help() {
    cat <<-EOF
bm: bookmark directories
Usage:
    bm                   bookmark current directory
    bm -l
    bm list              list bookmarks
    bm index
    bm -g index
    bm go index          cd to bookmark at index
    bm -d index
    bm delete index      delete bookmark at index
    bm -s [filename]
    bm save [filename]   save bookmarks in the file (default ~/.bookmarks)
    bm -L [filename]
    bm load [filename]   load bookmarks from the file (default !/.bookmarks)
    bm -c
    bm clear             remove all bookmarks
    bm -C
    bm compact           compact indices to a sequential list
EOF
}

bm() {
    local command=$1; shift
    case $command in
        -l | list ) bm_list ;;
        -g | go ) bm_go "$1" ;;
        -d | delete ) bm_delete "$1";;
        -s | save ) bm_save "$1" ;;
        -c | clear ) bm_clear ;;
        -C | compact ) bm_compact ;;
        -L | load ) bm_load "$1" ;;
        '') bm_add "$PWD" ;;
        *) [[ "$command" =~ [0-9]+ ]] && bm_go "$command" || bm_help ;;
    esac
}
\$\endgroup\$
4
\$\begingroup\$

Some suggestions:

  1. #!/usr/bin/env bash is a very common shebang line these days.
  2. Use More Quotes™:
    • echo -e $i'\t'"${BOOKMARKS[$i]}" should be echo -e $i'\t'"${BOOKMARKS[$i]}" (or even better, printf '%s\t%s' "$i" "${BOOKMARKS[$i]}")
    • command=$1 should be command="$1"
  3. In [[ "$command" =~ [0-9]+ ]] you'll want to anchor the regex.
  4. If this is for your own use I don't quite understand the need for both option-style and command-style invocation.
  5. == in Bash is a bit of a historical accident. It won't make any difference as long as you use Bash, but I usually prefer the POSIX = in this case.
  6. It seems bm_compact should run on every change, since it'll be super fast and you avoid changing indexes.
  7. bmfile would be better as either configuration or as a single variable.
  8. "default !/.bookmarks" should probably be "default ~/.bookmarks".
  9. You can set IFS for a single command, such as while IFS=… read …
  10. All the single semicolons are extraneous - you definitely don't need one for echo -e $i'\t'"${BOOKMARKS[$i]}" ;, and you can use

    for i in "${!BOOKMARKS[@]}"
    do
    

    for loops and conditionals.

\$\endgroup\$
  • \$\begingroup\$ I appreciate you elaborating on the bullet 2. \$\endgroup\$ – vnp Jul 15 at 21:51
1
\$\begingroup\$

Sanity check

Shellcheck.net reports a couple of issues. I will not call them out one by one, I suggest to review and fix.

Incorrectly saving empty bookmark

When there are no bookmarks, bm_save will write a single null byte to the file. Then, when you list bookmarks the empty entry shows up at index 0.

I suggest to add a check before actually writing to the file:

[[ ${#BOOKMARKS[@]} != 0 ]] || return 0

Do not raise error when file does not exist

bm_load raises an error if the file does not exist. Is that the intended behavior? I don't think so, because bm_save happily writes to the file. Perhaps bm_load should be more forgiving.

Overriding IFS

As @l0b0 already pointed out, it's good to prefix with IFS=... just the command where you want the modified value, when possible (usually). Another alternative is to use local IFS=...: the modified value will only be visible within the function, so no need to worry about backup and restore.

Unnecessary overriding of IFS

In bm_compact I don't see a reason to override IFS to recreate BOOKMARKS.

Btw, at first I didn't understand the purpose of this function. I think naming it bm_compact_indexes would have made it click.

What is REPLY?

I can find what is REPLY if I look at help read. I think it will be easier to understand the code if you specify a descriptive variable name with read.

Can you use Bash 4?

Where there is Bash, there's usually Bash 4. Using mapfile you could replace the loop reading BOOKMARKS in bm_load with:

mapfile -d '' -t BOOKMARKS < "$bmfile"

Avoid echo -e

The flags of echo don't work reliably in all systems, so I suggest to avoid it. Your alternatives are using printf (which is not POSIX compliant), or (in your specific case) to embed tab characters in the string.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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