[RFC 1/2] pw_mon: improve command line options

[Michael Santana]

On Fri, Oct 27, 2023 at 9:06 AM Aaron Conole <aconole at redhat.com> wrote:
>
> In the future, we'll use this to add support for passing opts into some parts
> of pw_mon.
>
> Signed-off-by: Aaron Conole <aconole at redhat.com>
> ---
>  pw_mon | 53 +++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/pw_mon b/pw_mon
> index 28feb8b..01bdd25 100755
> --- a/pw_mon
> +++ b/pw_mon
> @@ -21,34 +21,59 @@
>
>  [ -f "${HOME}/.pwmon-rc" ] && source "${HOME}/.pwmon-rc"
>
> -if [ "$1" != "" ]; then
> -    pw_project="$1"
> -    shift
> +if [ "$1" != ""  ]; then
nitpick: there is an extra white space between "" and ]
> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
I am trying to understand what you are trying to accomplish with these
if statements and the while loop. Couldnt you just move everything
into the while loop so you dont have to repeat the checks? I mean, you
will probably still have to use an OR || to check both conditions,
with and without the "--" at the begining. Maybe you could use some
sort of regex to make the "--" optional and then maybe you could
combine both checks into a single check and make it much easier

But just to double check, you are trying to parse both
"--pw-instance=myinstance" and "pw-instance=myinstance". Correct?
That's the whole goal of this patch?

The lack of strings around ^-- make me really unseasy, this also
applies to the sed line. Also, with grep you can tell it to not dump
any output/error. see -s and -q options. These apply to the other grep
commands in the script as well

> +        pw_project="$1"
> +        shift
> +    fi
>  fi
>
>  if [ "$1" != "" ]; then
> -    pw_instance="$1"
> -    shift
> -fi
> -
> -if [ "X$pw_instance" == "X" -o "X$pw_project" == "X" ]; then
> -   echo "ERROR: Patchwork instance and project are unset."
> -   echo "Please setup ${HOME}/.pwmon-rc and set pw_project "
> -   echo "(or pass it as an argument)."
> -   echo "Also either setup pw_instance or pass it as an argument."
> -   exit 1
> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
> +        pw_instance="$1"
> +        shift
> +    fi
>  fi
>
>  userpw=""
>
>  if [ "$1" != "" ]; then
> -    pw_credential="$1"
> +    if ! echo "$1" | grep -E ^-- >/dev/null 2>&1; then
why is this not inside the while loop? the use of -- forbidden for
credentials via the cli?
> +        pw_credential="$1"
> +        shift
> +    fi
>  fi
>
> +
> +while [ "$1" != "" ]; do
Im guessing the whole point of this patch was to add this while loop
> +    if echo "$1" | grep -E ^--pw-project= >/dev/null 2>&1; then
> +        pw_project=$(echo "$1" | sed s/--pw-project=//)
> +        shift
> +    elif echo "$1" | grep -E ^--pw-instance= >/dev/null 2>&1; then
> +        pw_instance=$(echo "$1" | sed s/--pw-instance=//)
> +        shift
> +    elif echo "$1" | grep -E ^--help >/dev/null 2>&1; then
> +        echo "pw_mon script"
> +        echo "$0 [proj|--pw-project=<proj>] [instance|--pw-instance=<inst url] opts.."
> +        echo "Options:"
> +        echo "    --add-filter-recheck=filter  Adds a filter to flag that a recheck needs to be done"
> +        echo ""
> +        exit 0
> +    fi
> +done
> +
>  if [ "X$pw_credential" != "X" ]; then
>     userpw="-u \"${pw_credential}\""
>  fi
>
> +if [ "X$pw_instance" == "X" -o "X$pw_project" == "X" ]; then
> +   echo "ERROR: Patchwork instance and project are unset."
> +   echo "Please setup ${HOME}/.pwmon-rc and set pw_project "
> +   echo "(or pass it as an argument)."
> +   echo "Also either setup pw_instance or pass it as an argument."
> +   exit 1
> +fi
> +
>  source $(dirname $0)/series_db_lib.sh
>
>  function emit_series() {
> --
> 2.41.0
>