This is the script :


set -euo pipefail

SOFT="$(basename $0)"
random=$(( ( RANDOM % 100000 ) ))

mkdir -p "$tempfolder"
pushd "$tempfolder" > /dev/null
cleanup (){
    cat wget.txt | sed 's/:$//'  | url_matcher
    popd > /dev/null
    rm -rf -- ${tempfolder}

trap cleanup EXIT

wget -nd --spider --recursive --no-verbose --output-file=wget.txt "$site"

the script essentially creates a subfolder in /tmp/crawl and puts into it the result of wget, then feeds it to sed and url_matcher.

Initially I though of putting the cat instruction just after the wget one, but for long wget it didn't work.

The sed instruction removes colons at the end of lines, which is added by wget when said url isn't valid. (something along the lines of "http://...: Nothing there")

url_matcher is a c++ scanner made with flex, which recognises urls in a text fed with standard input and prints them to standard output, separated with new lines characters.

the scripts allows me to fetch all urls in a website and use that as a stream directly.

Example :

$ crawl www.example.com | sed 's/some_treatments//' > super_file

There's no need to use cat here:

cat wget.txt | sed 's/:$//'  | url_matcher

Instead, simply have sed take its input from the file:

sed 's/:$//' wget.txt | url_matcher

Instead of using $RANDOM and mkdir, we can use the mktemp command (GNU coreutils) to create us a new directory (without risk of conflict with an existing one):

tempfolder=$(mktemp -d)

I think that now removes the only Bashism in the script.

Avoid pushd and popd in scripts (having to redirect the unwanted output should be a sign that these are intended for interactive shells). Instead, we can use cd "$tempfolder" and something like cd .. to leave it (it doesn't matter where we end up, as we only use absolute paths after that). Alternatively, don't change directory at all, but specify paths in full (I'll use a shorter name for the temporary directory):

dir=$(mktemp -d)

cleanup() {
    sed -e 's/:$//' "$dir/wget.txt" | url_matcher
    rm -rf -- "$dir"

trap cleanup EXIT

wget -nd --spider --recursive --no-verbose --output-file="$dir/wget.txt" "$1"

Other alternatives to pushd/popd include saving $PWD into a variable before changing directory, or using cd - (which internally does exactly that, as cd saves $PWD into $OLDPWD for that purpose).

We can avoid the temporary file altogether, to simplify the code and to allow us to start producing output without waiting for the crawl to finish. We can do this in a couple of ways, by being creative with --output-file. If we're on Linux, we can write to /dev/stdout instead of a file:

wget -nd --spider --recursive --no-verbose --output-file=/dev/stdout "$1" \
  | sed -e 's/:$//' | url_matcher

If we're happy to accept a Bash dependency again, we can use process substitution:

wget -nd --spider --recursive --no-verbose \
     --output-file=>(sed -e 's/:$//' | url_matcher) "$1"

However, neither of those dependencies are necessary, as wget follows the common convention of treating the filename - specially, to mean "standard output stream":

wget -nd --spider --recursive --no-verbose --output-file=- "$1" \
  | sed -e 's/:$//' | url_matcher

Modified code


set -uo pipefail

exec wget -nd --spider --recursive --no-verbose --output-file=- "$1" \
    | sed -e 's/:$//' | url_matcher
| improve this answer | |
  • \$\begingroup\$ Neat ! Why can't I put the sed ... after the wget though ? \$\endgroup\$ – Pierre-Antoine Guillaume Jul 12 '18 at 9:42
  • \$\begingroup\$ I'm not sure what you mean by "put the sed after the wget" - do you mean that you'd like to pipe the wget output into sed? That's certainly possible, and I'll edit to show how. \$\endgroup\$ – Toby Speight Jul 12 '18 at 9:44
  • \$\begingroup\$ that'd be good, sure. At first, I used to put the sed outside of the cleanup, after the wget. And for "long" crawls (with wget running for a few minutes), the cat - now sed - would not take place at all. \$\endgroup\$ – Pierre-Antoine Guillaume Jul 12 '18 at 9:46

A few points on top of Toby's excellent review.

This is not correctly protected from word splitting, and will fail when the script's path contains spaces:

SOFT="$(basename $0)"

The only thing you need to double quote there is the $0:

SOFT=$(basename "$0")

Since RANDOM generates a random integer in the range 0 - 32767, the % 100000 is unnecessary.

The popd is unlikely to ever be useful in a cleanup script. Directory changes within a script have no effect outside an executed (as opposed to sourced script). In any case, it's best to not change directories within scripts at all.

| improve this answer | |
  • \$\begingroup\$ I think the popd was probably there to make the temporary directory not be the working directory when it's removed (possible confusion with attempting to umount a filesystem when a process is using it for working directory, perhaps?) \$\endgroup\$ – Toby Speight Jul 13 '18 at 8:15
  • \$\begingroup\$ I really appreciate. I am going to change my boilerplate script asap ! The popd was there for the bad reasons ;) thank you ! The cd (or pushd) was there for a "good" reason : when I started the script, I didn't know of the wget -nd option. If you don't use it, you create a real mess of subfolders. hence, create a new tempfolder, cd into it and delete it afterward. \$\endgroup\$ – Pierre-Antoine Guillaume Jul 13 '18 at 12:39
  • \$\begingroup\$ I have re-read your answer, and I have still not understood word splitting. why cant I quote the whole substitution, and why is quoting the whole substitution unnecessary ? \$\endgroup\$ – Pierre-Antoine Guillaume Apr 7 '19 at 18:13
  • \$\begingroup\$ @PierreAntoineGuillaume if the path to your script contains spaces, then in basename $0 the $0 will be split to multiple arguments passed to basename. But you need it in one. So you must write basename "$0". As for name="$(...)", the double-quotes are unnecessary, because that's just how assignments work in Bash: they are safe without the double-quotes. \$\endgroup\$ – janos Apr 7 '19 at 20:39
  • 1
    \$\begingroup\$ @PierreAntoineGuillaume No, I'm not saying any of those things. What I'm saying is that name=$value and name=$(...) are safe without double-quotes, and in cmd "$a" "$b" and cmd "$(...)" "$(...)" the double-quotes are necessary. I'm saying nothing more, nothing less. Nothing is "treated separately". Everything is "parsed". \$\endgroup\$ – janos Apr 8 '19 at 5:24

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.