3
\$\begingroup\$

I'm working with the Perforce command line client. I want to check for the case when a user tries to unshelve an unsubmitted changelist that has a valid changelist number, but has no shelved files. In this case the p4 unshelve command will still have an exit status of 0, but it will print an error message to stderr.

I'm fairly inexperienced with bash scripts, but I did run my code through shellcheck.net to find any obvious errors.

In a lot of places I think this script could be more concise or robust, but I don't know exactly what I should be changing.

Any help is greatly appreciated.

#!/bin/bash

p4 unshelve -f -s "$1"  &> tmp_file

# capture the exit status of the call to p4 unshelve
if [ $? -eq 0 ]
then
    flag0=0
else
    flag0=1
fi

std_err_output="$(cat tmp_file)"
error_message="No such file(s)."

# even if the exit status is 0, still check that the error message was not thrown.
if [ "$std_err_output" = "$error_message" ]
then
   flag1=1
else
   flag1=0
fi

rm tmp_file

exit_status="expr $flag0 | $flag1"

exit "$($exit_status)"
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I hoep you get some helpful answers. \$\endgroup\$ – SirPython Oct 12 '15 at 21:26
5
\$\begingroup\$

Pretty nice job for someone new to bash. I have a couple of tips though.


Instead of this:

# capture the exit status of the call to p4 unshelve
if [ $? -eq 0 ]
then
    flag0=0
else
    flag0=1
fi

You can write simply:

flag0=$?

It's not exactly the same, because the error code might not be 1, but we can make it work with this lazy way too, see below.


Similar to earlier, instead of this:

if [ "$std_err_output" = "$error_message" ]
then
   flag1=1
else
   flag1=0
fi

You can do simpler:

[ "$std_err_output" = "$error_message" ]
flag1=$?

Similarly, instead of this:

exit_status="expr $flag0 | $flag1"

exit "$($exit_status)"

You can benefit from the fact that the exit code of the script will be the exit code of the last command. So you can simply write:

[ $flag0 = 0 ] && [ $flag1 = 0 ]

Lastly, I suggest to move the rm tmp_file higher, to get it done as soon as you no longer need that file. No need to keep it around any longer than necessary.

\$\endgroup\$
3
\$\begingroup\$

Temporary files are mostly frowned upon. They cause all kinds of race conditions and security holes. In your case, you don't really need it since you may capture output directly into the variable:

    std_err_output=$(p4 unshelve -f -s "$1"  2>&1 )
\$\endgroup\$

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.