From: Michael Santana <msantana@redhat.com>
To: Aaron Conole <aconole@redhat.com>
Cc: Dumitru Ceara <dceara@redhat.com>,
David Marchand <dmarchan@redhat.com>,
Ilya Maximets <imaximet@redhat.com>,
ci@dpdk.org
Subject: Re: [RFC 1/2] pw_mon: improve command line options
Date: Tue, 31 Oct 2023 10:45:09 -0400 [thread overview]
Message-ID: <CABVNPRpONHm4r3SzxDjRXT+4nOWRwOmBAoS34U3b5YuxSc8mBQ@mail.gmail.com> (raw)
In-Reply-To: <20231027130609.3035396-1-aconole@redhat.com>
On Fri, Oct 27, 2023 at 9:06 AM Aaron Conole <aconole@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@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
>
next prev parent reply other threads:[~2023-10-31 14:45 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-27 13:06 Aaron Conole
2023-10-27 13:06 ` [RFC 2/2] recheck: Add a recheck parser for patchwork comments Aaron Conole
2023-11-01 16:57 ` Michael Santana
2023-11-01 19:16 ` Aaron Conole
2023-11-02 10:44 ` Thomas Monjalon
2023-11-02 13:03 ` Aaron Conole
2023-11-02 13:32 ` Thomas Monjalon
2023-10-31 14:45 ` Michael Santana [this message]
2023-10-31 15:54 ` [RFC 1/2] pw_mon: improve command line options Aaron Conole
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABVNPRpONHm4r3SzxDjRXT+4nOWRwOmBAoS34U3b5YuxSc8mBQ@mail.gmail.com \
--to=msantana@redhat.com \
--cc=aconole@redhat.com \
--cc=ci@dpdk.org \
--cc=dceara@redhat.com \
--cc=dmarchan@redhat.com \
--cc=imaximet@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).