On 21 April 2018 at 21:32, Hideki Yamane <henr...@iijmio-mail.jp> wrote: > > Thanks for the report, and here's a proposal fix. > > [ ! "$VERBOSE" ] && NVSWITCH="-nv" > local ret=0 > if [ "$USE_DEBIANINSTALLER_INTERACTION" ] && [ "$PROGRESS_NEXT" ]; > then > - wget "$@" 2>&1 >/dev/null | "$PKGDETAILS" "WGET%" > "$PROGRESS_NOW" "$PROGRESS_NEXT" "$PROGRESS_END" >&3 > + wget $@ 2>&1 >/dev/null | "$PKGDETAILS" "WGET%" > "$PROGRESS_NOW" "$PROGRESS_NEXT" "$PROGRESS_END" >&3 > ret=$? > else > - wget $NVSWITCH "$@" > + wget $NVSWITCH $@ > ret=$? > fi > return $ret
Hi, I have not looked into the actual problem, and I have not run the code, I have only looked at the patch above, but I offer some patch review comments in the hope you will find them useful. The "ret" variable in that function is entirely pointless. The exitstatus of an if statment is the exitstatus of the last command executed in the body of the if statement. And a bare return command will return the exitstatus of the previous command executed. So the below code will behave identically to the unpatched function: wgetprogress () { [ ! "$VERBOSE" ] && NVSWITCH="-nv" if [ "$USE_DEBIANINSTALLER_INTERACTION" ] && [ "$PROGRESS_NEXT" ]; then wget "$@" 2>&1 >/dev/null | "$PKGDETAILS" "WGET%" "$PROGRESS_NOW" "$PROGRESS_NEXT" "$PROGRESS_END" >&3 else wget $NVSWITCH "$@" fi } > "$@" is extracted as '' and wget tries to fetch it and fails, > then returns 1. Regarding the proposed fix, in general using $@ without quotes is fragile. And I don't understand how changing "$@" to $@ will change the behaviour of wget. Testing here, both wget and wget '' return exitstatus 1. To test if the function was given any positional parameters, instead use if [ $# -eq 0 ] ; then echo "There are no parameters" fi To test if $@ is empty, use if [ -z "$*" ] ; then echo "All parameters are empty" fi But wouldn't it be better to fix the actual problem? If somewhere this function is called without parameters, then that is where the problem occurs and so is likely the best place to fix it.