Hot answers tagged

10

I will offer some suggestions. Please note I have no experience with Mac OS, but I believe most of it will still apply. Using system is not particularly bad in your case since you use static arguments in those calls excluding the possibility of command injection. However, since your first C program is almost completely made up of system calls, you should ...


9

Some suggestions: #!/usr/bin/env bash is a more portable shebang line. I would use long options for set and xxd for readability. So set -o errexit -o nounset -o pipefail and xxd -revert -plain. If you quote the array members you can easily use full names. In case any remote workers are in another time zone offset you'll want to use a fixed one, such as TZ=...


7

Your code seems pretty much fine, and chicks has pointed out most of the issues I have. It's not a bad implementation of the approach you've chosen, but I have to wonder if that's the right approach. HTML is a complex format, and while using regex to parse it can work as long as you've structured it in a consistent manner known in advance (which definitely ...


7

I'm just reviewing the code, not the meaning behind it. Get out of the habit of using ALLCAPS variable names, leave those as reserved by the shell. One day you'll write PATH=something and then wonder why your script is broken. LOCK_FILE="/tmp/prepare_zfs.lock" if [[ -f ${LOCK_FILE} ]]; then printf "Lock file already exists. Exiting...\n&...


6

Good great variable names, but I quibble over a couple later on nice use of blank lines to organize code appropriate use of sed. (Regex search and replace is the only time to use it.) I wouldn't expect any XML tools to be an improvement. :) you're using a subset of bash strict mode using env for portability Questions Some things I wish the code had ...


6

One thing you might choose to do, given the neat correspondence of variable names and element ids, is to generate the sed script from the list of variables: # Produce a sed script that replaces TD elements' contents with value of # same-named variable. # Arguments: list of variable names make_subst_script() { for i do printf 's/\(<td id=&...


5

There's quite a bit of repetition: echo && print with roughly the same arguments - tee is probably better here error and non-error paths - perhaps use one function to prepend the ERROR: tag and call the other? It might be best to have the filename first, then any number of message words? That would make the interface more like echo, for example. ...


5

I'm not sure why we have #!/bin/bash - it looks like plain, portable #!/bin/sh would be fine here. Things I immediately like include the good error handling, using &2 for error messages and exiting non-zero. All the tar commands can be combined, as tar is able (with -a) to automatically identify any compression scheme it handles. As there's no need to ...


5

NOW=$(date +%m-%d-%Y) Prefer lower-case for variable names. This reduces the likelihood of conflict with environment variables used to change programs' behaviour (which are generally upper-case). That odd date format (with the day in the middle) is going to make it hard to find your data. It's best to stick with standard (ISO 8601) formats, which ...


5

It might be better to define BACK_UP_PATH and BACK_UP_LOG_PATH as "${BACK_UP_DIR}/ShareBackup${NOW}" and "${BACK_UP_LOG_DIR}/ShareBackupLog${NOW}" -- notice the slashes. It is safe to have multiple slashes in a row in a path, so explicitly adding them might be clearer and also helps make sure the script works as intended if someone ...


4

You're going to get false positives by comparing the output of ls -l. In many locales, this command gives date stamps in different formats for newer files (day+time) and older files (year+day). When any file transitions from "new" to "old" in this scheme, you'll get a spurious notification. If you have the GNU implementation of ls, you ...


4

Building on Toby's excellent answer, this offers 2 alternate ways to parse the options. #!/usr/bin/env bash readonly PROGRAM=${0##*/} readonly USAGE=$(cat <<END_USAGE Usage: $PROGRAM [FILE]... By default file(s) will be extracted to the working directory. OPTIONS -d, --directory, eg. nzip [file] [-d directory] END_USAGE ) die() { printf '%s\n' ...


4

#!/bin/bash I don't see why you're using Bash for this - there should be no problem using standard POSIX shell. if [ -z $1 ]; then I would recommend "$1" there, even though extra arguments will cause [ to return false for other reasons. Passing as a single argument won't emit any error messages. echo -e "Please choose an input file"...


4

Great 100% pass on shellcheck which means you're doing great with quoting everything that could be potentially problematic. scoping variables using [[ conditional for the if nice explanation of what it is doing and why Could be better a sh-bang line at the top is a good idea for scripts, even if this would usually just be sourced during your login scripts....


4

[ $? -ne 0 ] is a well-known over-complication - you can replace that with a simple || in the pipeline: DETAILS=`SUDO_ASKPASS="$(which ssh-askpass)" sudo --askpass mount "$DEVICE" "$PARTITION" 2>&1` \ || return $? And in fact we can have a whole chain here: launch() { findmnt "$PARTITION" \ || ...


4

This while true loop can be simplified: while true; do read -rp "Enter disk device path: " DRIVE if [[ ! -b "${DRIVE}" ]]; then printf "Error: not a device.\n" else break fi done Having true as the condition should be rare. Here, we want to loop until "$DRIVE" is a block device, so ...


4

Good consistent indentation good error checking nice comments great quoting of variable substitutions yay for putting it all in a function good use of case statement for processing options. (Typically this would be in a loop, but since you only allow for one argument above this it makes sense that there is no loop here.) ShellCheck suggests One of my ...


4

The only thing here that's not standard shell is [[. That's easily replaced with [, which would give a more portable function: usable with any POSIX shell, not just Bash. if [ -z "$1" ] We could have included this test as part of the case. mkdir -p "$1" Always using -p might not be desirable. If I mis-type one of the directory ...


3

We can use plain POSIX /bin/sh. The only Bashism I see is if [[ "$OSTYPE" == "linux-gnu"* ]]; then elif [[ "$OSTYPE" == "darwin"* ]]; then fi This is in any case more naturally expressed as case "$(uname -s)" in Linux) ;; Darwin) # fill in actual value here ;; esac It would be ...


3

I get that it's been a while since you posted this, and you might have fixed any number of these issues already. Still, I was bored, so here I am, pointing them out, hoping some of it will help. config_getPath Depending on what $1 contains, this might not be well-behaved - consider cases like config_getPath '.*|' - that turns into sed s/^.*| *= \([^ ]*.*\)/\...


3

Well, it took time to figure out what your program is doing. So I did shorten it in two phases. First phase: I removed all duplicate temporary files, and used a pipe when a temp file was used once. STEP1: you just make a checksum of all files in $path1 and $path2 ### Find all filename and convert to MD5 ### find $path1 -type f | xargs md5sum > checksums....


3

Shell Errorhandling There's one thing I would have done differently (though it's not relevant to the small piece of code here) and that is the error handling inside the function. You can use set -e to automatically exit the function, you could eliminate the [ $? -ne 0 ] && return 1 line with that. Optionally, I would only have set that flag inside ...


3

As @TobySpeight pointed out in a comment, the cat is redundant. Any time you have a line like cat "$file" | grep "$pattern" you can replace it with grep "$pattern" "$file" If your log entries are already ordered by time (at least for each user's login event), you can likely use tac instead of sort -r Can a user have ...


3

Minor nitpicks While assigning variables from unquoted command substitutions apparently is safe, I personally prefer double quoting my command substitutions regardless of context - I find rules easier to remember if I follow them consistently. So I would have double quoted the mktemp near the start and the find when handling formats, but you should be just ...


3

Your chmod needs to go away; d.sh should already have the correct permissions, including when extracted from a source archive You should probably delete all of your output diagnostic fluff. f.c and d.sh are very bad filenames. The former should probably be renamed to fractal.c or somesuch. You've mis-used make. The whole point of make is to express a ...


3

First, as someone who writes a lot of bash, I would recommend rewriting this in something other than bash. You're using bash as a programming language (and trying quite hard to make it readable), but it's simply not the best programming language. The basic idea of gluing together a bunch of curl and jq commands seems solid. I would contact the maintainer of ...


3

You should probably address these Shellcheck issues: 265799.sh:32:9: warning: Use array+=("item") to append items to an array. [SC2179] 265799.sh:37:14: note: $/${} is unnecessary on arithmetic variables. [SC2004] 265799.sh:37:17: note: $/${} is unnecessary on arithmetic variables. [SC2004] 265799.sh:44:5: warning: Variable was used as an array but ...


2

EDITOR="${EDITOR:-vi}" Good! Respect the user's choice of editor, and use the well-known environment variable to convey that. bold='\e[1m' red='\033[1;31m' green='\033[1;32m' reset='\033[0m' Eww, please don't do that. Even when output and error streams are connected to a terminal, not all terminals use the same escape sequences. It's much ...


2

if [[ $NAME = "Fedora" ]]; then elif [[ $NAME = "void" ]]; then elif [[ $NAME = "Debian" ]] || [[ $NAME = "Ubuntu" ]] || [[ $NAME = "Linux Mint" ]]; then That looks like a candidate for case "$NAME" in. However, instead of changing that, let's look at what we're doing: if [[ $NAME = "Fedora&...


2

The other review makes most of the points I would have, so I'll just add three more points. Don't ignore the return value of mktemp For security reasons, mktemp will refuse to create a file if the name it has chosen already exists. This is to avoid security flaws. You will probably be saved because /tmp/ should always have the sticky bit set, preventing ...


Only top voted, non community-wiki answers of a minimum length are eligible