Skip to main content
added 405 characters in body
Source Link
Gao
  • 1.2k
  • 9
  • 21

One little nitpick: nowhere is it mentioned on the printf page that the utility shall conform to XBD Utility Syntax Guidelines, so it is unclear if -- works as intended on any POSIX-conforming printf implementation.

As the other answer has mentioned, eval should be seen as a last resort hack because it is extremely easy to shoot yourself in the foot with this potentially dangerous command, and you're even using it on unsanitized user input!

As the other answer has mentioned, eval should be seen as a last resort hack because it is extremely easy to shoot yourself in the foot with this potentially dangerous command, and you're even using it on unsanitized user input!

One little nitpick: nowhere is it mentioned on the printf page that the utility shall conform to XBD Utility Syntax Guidelines, so it is unclear if -- works as intended on any POSIX-conforming printf implementation.

As the other answer has mentioned, eval should be seen as a last resort hack because it is extremely easy to shoot yourself in the foot with this potentially dangerous command, and you're even using it on unsanitized user input!

Source Link
Gao
  • 1.2k
  • 9
  • 21

Whenever one sets out to reimplement some existing functions, it is wise to prepare a test suite to ensure the old behaviors have been faithfully preserved. I begin my review by presenting my test script that generates test cases using fuzzing and hand-picked edge cases:

#!/bin/sh

for dir in '' '/' '//' $(tr -dc abc/ </dev/urandom | head -c800 | split -b8 --filter='cat; echo' -)
do
    printf '%s' "Testing with input '$dir'..."
    my_output=$(./dirname.sh "$dir")
    reference_output=$(dirname "$dir")
    if [ "$my_output" != "$reference_output" ]
    then
        printf '%s\n' "my output ($my_output) differs from the reference output ($reference_output)!"
        exit 1
    else
        printf '%s\n' 'OK!'
    fi
done

It assumes the reimplemented function is named dirname.sh with the read and execute bits set, and is placed in and run from the same directory as the test script.

Congratulations! Your solution passes my tests! This gives us confidence in the correctness of your implementation. However, it turns out my test suite wasn't perfect, as it missed out on an extreme edge case that may or may not be important for you. If the user has set the nounset shell option, which may be inherited by scripts in the case of bash at least, or from the current environment if you decide to turn this into a function, your implementation will fail with an error message that may look like this:

./dirname.sh: line 4: $2: unbound variable
# or
./dirname.sh: line 9: o: unbound variable

while the original dirname utility will not be affected by the presence or absence of this shell option, unless an undefined variable is passed directly as an argument to it.

As the other answer has mentioned, eval should be seen as a last resort hack because it is extremely easy to shoot yourself in the foot with this potentially dangerous command, and you're even using it on unsanitized user input!

I gave the challenge a try and came up with my own solution that, in my opinion, is more readable and follows the outlined steps more closely. It does not spawn any subshells, staying true to your intentions. It also doesn't use any eval, and uses only one local variable that I promptly clean up to not clutter the environment so that it can readily be turned into a litter-free POSIX function. The environment variables SKIP_STEP_7 and SKIP_STEP_8 can be set to mimick implementation-dependent behaviors that are allowed by the spec. Here is my solution presented as an overall improvement (no function call cost and fewer lines of code, etc.):

#!/bin/sh

dir=$1
case $dir in
    '//') # Step 1
        ;;
    *[!/]* | '')
        dir=${dir%"${dir##*[!/]}"} # Step 3
        case $dir in
            */*)
                dir=${dir%"${dir##*/}"} # Step 5
                if [ "$dir" != '//' ]
                then
                    # The following steps are NOT implementation defined!
                    dir=${dir%"${dir##*[!/]}"} # Step 7
                    dir=${dir:-/} # Step 8
                fi
                ;;
            *)
                dir=. # Step 4
                ;;
        esac
        ;;
    *)
        dir=/ # Step 2
        ;;
esac
if [ "$dir" = '//' ] # Step 6
then
    # The following steps ARE implementation defined!
    [ "${SKIP_STEP_7+skip}" ] || dir=${dir%"${dir##*[!/]}"} # Step 7
    [ "${SKIP_STEP_8+skip}" ] || dir=${dir:-/} # Step 8
fi
printf '%s\n' "$dir"
unset -v dir