docopt / docopts

Shell interpreter for docopt, the command-line interface description language.
Other
494 stars 53 forks source link

Bug in expansion of variable length parameters (when count of parameters == 0) #53

Closed DanaLacoste closed 2 years ago

DanaLacoste commented 2 years ago

Description

docopts.go is expanding empty arrays to be a 1-element array with a single null/empty string element in https://github.com/docopt/docopts/blob/c21151315bea2852ef3250c70a52352c9cd0d6eb/docopts.go#L185

Details

OK, this one is a bit edge-case-y, but it causes an issue with the way bash expansion works. Specifically, docopts is not creating an empty array when a ... element is defined, but no parameters are given. Instead, it creates an array containing a single null element.

This gets confusing, because bash considers it an empty array (if you query the length of the array) as the element is null, but if you pass it using standard bash expansion, it will be expanded to an empty string which will confuse any downstream commands.

Test Case 1

Here is the test script (I tried to use the use case defined in testcases.docopt, but I could not trace down exactly why that specific test case is not failing here)

NOTE: You need two scripts to see the problem clearly :)

test1.sh: this just runs the basic test case which has the bug:

#!/bin/bash

#docopts --debug -h - : "$@" <<EOF
#Test script for ... in docopts
#Usage:
  #$0 [NAME [NAME ...]]
  #$0 -h | --help
#EOF

eval "$(docopts -h - : "$@" <<EOF
Test script for ... in docopts
Usage:
  $0 [NAME [NAME ...]]
  $0 -h | --help
EOF
)"

echo "NAME: Contains ${#NAME} elements"

./test2.sh "${NAME[@]}"

and test2.sh:

#!/bin/bash

echo "$0: Called with $# parameters"

And the results:

% ./test1.sh 1
NAME: Contains 1 elements
./test2.sh: Called with 1 parameters
% ./test1.sh 1 2
NAME: Contains 1 elements
./test2.sh: Called with 2 parameters
% ./test1.sh
NAME: Contains 0 elements
./test2.sh: Called with 1 parameters

As you can see:

Test Case 2: Seeing this in pure bash, without docopts

So, let's find the root cause: what does docopts output that bash doesn't handle well?

test3.sh:

#!/bin/bash

declare -a empty_array
empty_array=()

declare -a one_null_array
one_null_array=('')

echo "Empty array:"
./test2.sh "${empty_array[@]}"

echo "One null array:"
./test2.sh "${one_null_array[@]}"

And the result:

% ./test3.sh
Empty array:
./test2.sh: Called with 0 parameters
One null array:
./test2.sh: Called with 1 parameters

Conclusion:

Somehow, this is expanding the empty array once: can we change it so that it just emits a name=() instead of name=('') for this use case? (I need to learn more go :) ) https://github.com/docopt/docopts/blob/c21151315bea2852ef3250c70a52352c9cd0d6eb/docopts.go#L185

Perhaps we could (maybe) move the ' into the Join rather than outside?

Sylvain303 commented 2 years ago

Hi @DanaLacoste

You're right:

# len of array in bash is ${#var_name[@]}
$ NAME=()
$ echo ${#NAME[@]}
0

$ NAME2=('')
$ echo ${#NAME2[@]}
1

lets try python's version 0.6.1

docker run -ti -v $PWD:/data debian-docpots  docopts -h 'Usage: prog [NAME...]' :
NAME=()

Which was in the code too:

https://github.com/docopt/docopts/blob/4c8b652998a5ffda5068197f002b5ab5735761d6/docopts#L66

So I fixed it. Tanks for reporting.

https://github.com/docopt/docopts/blob/e7d60ea17b98db2b1314d7eee7bdf3a3b80722d2/docopts.go#L185-L187

It wont fail with test in: testcases.docopt as test are run through testee.sh which in turn uses -A mode which uses a different code path.

So, I added a functional test case for that + unit test_case in go:

https://github.com/docopt/docopts/blob/e7d60ea17b98db2b1314d7eee7bdf3a3b80722d2/tests/functional_tests_docopts.bats#L70-L76

https://github.com/docopt/docopts/blob/e7d60ea17b98db2b1314d7eee7bdf3a3b80722d2/common_input_test.json#L21

Also test are now more documented:

https://github.com/docopt/docopts/blob/e7d60ea17b98db2b1314d7eee7bdf3a3b80722d2/docs/developer.md#add-more-test-to-docopts-use-case

Let me know it it works as expected for you.

DanaLacoste commented 2 years ago

Yep, that looks perfect, thanks!