linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHSET 0/3] fstests: various cleanups to ./new script
@ 2021-09-17  0:39 Darrick J. Wong
  2021-09-17  0:39 ` [PATCH 1/3] new: clean up the group name input code Darrick J. Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Darrick J. Wong @ 2021-09-17  0:39 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

Hi all,

In the process of reviewing the group name documentation, it became
obvious that there were quite a few cleanups that could be applied to
the ./new script.  Rather than lengthen the group name doc series, I've
moved them all to a separate patchset that cleans up the group name
validation code, tidies up some of the old bashisms with new ones, and
then disallows the creation of new tests in the 'other' group.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=new-script-cleanups
---
 new |   76 +++++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 42 insertions(+), 34 deletions(-)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] new: clean up the group name input code
  2021-09-17  0:39 [PATCHSET 0/3] fstests: various cleanups to ./new script Darrick J. Wong
@ 2021-09-17  0:39 ` Darrick J. Wong
  2021-09-17 10:28   ` Amir Goldstein
  2021-09-17  0:40 ` [PATCH 2/3] new: standardize group name checking Darrick J. Wong
  2021-09-17  0:40 ` [PATCH 3/3] new: don't allow new tests in group 'other' Darrick J. Wong
  2 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2021-09-17  0:39 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

Clean up the code that reads group names in from the command line to
take advantage of the read command's ability to display a prompt.  While
we're at it, we should abort the script if the group list encounters
EOF, and we can tighten up some of the other sh-isms too.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 new |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)


diff --git a/new b/new
index 6b7dc5d4..ea7cf25e 100755
--- a/new
+++ b/new
@@ -91,16 +91,13 @@ group_names() {
 	}' doc/group-names.txt
 }
 
-if [ $# -eq 0 ]
-then
-
+if [ $# -eq 0 ]; then
+	prompt="Add to group(s) [other] (separate by space, ? for list): "
     while true
     do
-	echo -n "Add to group(s) [other] (separate by space, ? for list): "
-	read ans
-	[ -z "$ans" ] && ans=other
-	if [ "X$ans" = "X?" ]
-	then
+	read -p "${prompt}" ans || exit 1
+	test -z "${ans}" && ans=other
+	if [ "${ans}" = "?" ]; then
 	    echo $(group_names)
 	else
 	    # only allow lower cases, spaces, digits and underscore in group


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/3] new: standardize group name checking
  2021-09-17  0:39 [PATCHSET 0/3] fstests: various cleanups to ./new script Darrick J. Wong
  2021-09-17  0:39 ` [PATCH 1/3] new: clean up the group name input code Darrick J. Wong
@ 2021-09-17  0:40 ` Darrick J. Wong
  2021-09-17 10:32   ` Amir Goldstein
  2021-09-17  0:40 ` [PATCH 3/3] new: don't allow new tests in group 'other' Darrick J. Wong
  2 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2021-09-17  0:40 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

Use the same group name validation when reading group names from
standard input or from the command line.  Now that we require all group
names to be documented, there's no reason to leave these separate
requirements.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 new |   68 +++++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 38 insertions(+), 30 deletions(-)


diff --git a/new b/new
index ea7cf25e..3a657d20 100755
--- a/new
+++ b/new
@@ -91,38 +91,46 @@ group_names() {
 	}' doc/group-names.txt
 }
 
+# Make sure that the new test's groups fit the correct format and are listed
+# in the group documentation file.
+check_groups() {
+	for g in "$@"; do
+		local inval="$(echo "${g}" | tr -d '[:lower:][:space:][:digit:]_')"
+		if [ -n "${inval}" ]; then
+			echo "Invalid characters in group(s): ${inval}"
+			echo "Only lower cases, digits and underscore are allowed in groups, separated by space"
+			return 1
+		elif ! group_names | grep -q -w "${g}"; then
+			echo "Warning: group \"${g}\" not defined in documentation"
+			return 1
+		fi
+	done
+
+	return 0
+}
+
 if [ $# -eq 0 ]; then
+	# interactive mode
 	prompt="Add to group(s) [other] (separate by space, ? for list): "
-    while true
-    do
-	read -p "${prompt}" ans || exit 1
-	test -z "${ans}" && ans=other
-	if [ "${ans}" = "?" ]; then
-	    echo $(group_names)
-	else
-	    # only allow lower cases, spaces, digits and underscore in group
-	    inval=`echo $ans | tr -d '[:lower:][:space:][:digit:]_'`
-	    if [ "$inval" != "" ]; then
-		echo "Invalid characters in group(s): $inval"
-		echo "Only lower cases, digits and underscore are allowed in groups, separated by space"
-		continue
-	    else
-		# remove redundant spaces/tabs
-		ans=`echo "$ans" | sed 's/\s\+/ /g'`
-		break
-	    fi
-	fi
-    done
+	while true; do
+		read -p "${prompt}" -a new_groups || exit 1
+		case "${#new_groups[@]}" in
+		0)
+			new_groups=("other")
+			;;
+		1)
+			if [ "${new_groups[0]}" = "?" ]; then
+				echo $(group_names)
+				continue
+			fi
+			;;
+		esac
+		check_groups "${new_groups[@]}" && break
+	done
 else
-    # expert mode, groups are on the command line
-    #
-    for g in $*
-    do
-	if ! grep -q "^$g" doc/group-names.txt; then
-	    echo "Warning: group \"$g\" not defined in documentation"
-	fi
-    done
-    ans="$*"
+	# expert mode, groups are on the command line
+	new_groups=("$@")
+	check_groups "${new_groups[@]}" || exit 1
 fi
 
 echo -n "Creating skeletal script for you to edit ..."
@@ -139,7 +147,7 @@ cat <<End-of-File >$tdir/$id
 # what am I here for?
 #
 . ./common/preamble
-_begin_fstest $ans
+_begin_fstest ${new_groups[@]}
 
 # Override the default cleanup function.
 # _cleanup()


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 3/3] new: don't allow new tests in group 'other'
  2021-09-17  0:39 [PATCHSET 0/3] fstests: various cleanups to ./new script Darrick J. Wong
  2021-09-17  0:39 ` [PATCH 1/3] new: clean up the group name input code Darrick J. Wong
  2021-09-17  0:40 ` [PATCH 2/3] new: standardize group name checking Darrick J. Wong
@ 2021-09-17  0:40 ` Darrick J. Wong
  2021-09-17 10:42   ` Amir Goldstein
  2 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2021-09-17  0:40 UTC (permalink / raw)
  To: djwong, guaneryu; +Cc: linux-xfs, fstests, guan

From: Darrick J. Wong <djwong@kernel.org>

The 'other' group is vaguely defined at best -- other than what?  It's
not clear what tests belong in this group, and it has become a dumping
ground for random stuff that are classified in other groups.  Don't let
people create new other group tests.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 new |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


diff --git a/new b/new
index 3a657d20..9651e0e0 100755
--- a/new
+++ b/new
@@ -100,6 +100,9 @@ check_groups() {
 			echo "Invalid characters in group(s): ${inval}"
 			echo "Only lower cases, digits and underscore are allowed in groups, separated by space"
 			return 1
+		elif [ "${g}" = "other" ]; then
+			echo "Do not add more tests to group \"other\""
+			return 1
 		elif ! group_names | grep -q -w "${g}"; then
 			echo "Warning: group \"${g}\" not defined in documentation"
 			return 1
@@ -111,16 +114,16 @@ check_groups() {
 
 if [ $# -eq 0 ]; then
 	# interactive mode
-	prompt="Add to group(s) [other] (separate by space, ? for list): "
+	prompt="Add to group(s) [auto] (separate by space, ? for list): "
 	while true; do
 		read -p "${prompt}" -a new_groups || exit 1
 		case "${#new_groups[@]}" in
 		0)
-			new_groups=("other")
+			new_groups=("auto")
 			;;
 		1)
 			if [ "${new_groups[0]}" = "?" ]; then
-				echo $(group_names)
+				echo $(group_names | grep -v -w 'other')
 				continue
 			fi
 			;;


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] new: clean up the group name input code
  2021-09-17  0:39 ` [PATCH 1/3] new: clean up the group name input code Darrick J. Wong
@ 2021-09-17 10:28   ` Amir Goldstein
  0 siblings, 0 replies; 7+ messages in thread
From: Amir Goldstein @ 2021-09-17 10:28 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Eryu Guan, linux-xfs, fstests, Eryu Guan

On Fri, Sep 17, 2021 at 12:57 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> From: Darrick J. Wong <djwong@kernel.org>
>
> Clean up the code that reads group names in from the command line to
> take advantage of the read command's ability to display a prompt.  While
> we're at it, we should abort the script if the group list encounters
> EOF, and we can tighten up some of the other sh-isms too.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  new |   13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
>
> diff --git a/new b/new
> index 6b7dc5d4..ea7cf25e 100755
> --- a/new
> +++ b/new
> @@ -91,16 +91,13 @@ group_names() {
>         }' doc/group-names.txt
>  }
>
> -if [ $# -eq 0 ]
> -then
> -
> +if [ $# -eq 0 ]; then
> +       prompt="Add to group(s) [other] (separate by space, ? for list): "
>      while true
>      do
> -       echo -n "Add to group(s) [other] (separate by space, ? for list): "
> -       read ans
> -       [ -z "$ans" ] && ans=other
> -       if [ "X$ans" = "X?" ]
> -       then
> +       read -p "${prompt}" ans || exit 1
> +       test -z "${ans}" && ans=other
> +       if [ "${ans}" = "?" ]; then
>             echo $(group_names)
>         else
>             # only allow lower cases, spaces, digits and underscore in group
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] new: standardize group name checking
  2021-09-17  0:40 ` [PATCH 2/3] new: standardize group name checking Darrick J. Wong
@ 2021-09-17 10:32   ` Amir Goldstein
  0 siblings, 0 replies; 7+ messages in thread
From: Amir Goldstein @ 2021-09-17 10:32 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Eryu Guan, linux-xfs, fstests, Eryu Guan

On Fri, Sep 17, 2021 at 12:57 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> From: Darrick J. Wong <djwong@kernel.org>
>
> Use the same group name validation when reading group names from
> standard input or from the command line.  Now that we require all group
> names to be documented, there's no reason to leave these separate
> requirements.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  new |   68 +++++++++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 38 insertions(+), 30 deletions(-)
>
>
> diff --git a/new b/new
> index ea7cf25e..3a657d20 100755
> --- a/new
> +++ b/new
> @@ -91,38 +91,46 @@ group_names() {
>         }' doc/group-names.txt
>  }
>
> +# Make sure that the new test's groups fit the correct format and are listed
> +# in the group documentation file.
> +check_groups() {
> +       for g in "$@"; do
> +               local inval="$(echo "${g}" | tr -d '[:lower:][:space:][:digit:]_')"
> +               if [ -n "${inval}" ]; then
> +                       echo "Invalid characters in group(s): ${inval}"
> +                       echo "Only lower cases, digits and underscore are allowed in groups, separated by space"
> +                       return 1
> +               elif ! group_names | grep -q -w "${g}"; then
> +                       echo "Warning: group \"${g}\" not defined in documentation"
> +                       return 1
> +               fi
> +       done
> +
> +       return 0
> +}
> +
>  if [ $# -eq 0 ]; then
> +       # interactive mode
>         prompt="Add to group(s) [other] (separate by space, ? for list): "
> -    while true
> -    do
> -       read -p "${prompt}" ans || exit 1
> -       test -z "${ans}" && ans=other
> -       if [ "${ans}" = "?" ]; then
> -           echo $(group_names)
> -       else
> -           # only allow lower cases, spaces, digits and underscore in group
> -           inval=`echo $ans | tr -d '[:lower:][:space:][:digit:]_'`
> -           if [ "$inval" != "" ]; then
> -               echo "Invalid characters in group(s): $inval"
> -               echo "Only lower cases, digits and underscore are allowed in groups, separated by space"
> -               continue
> -           else
> -               # remove redundant spaces/tabs
> -               ans=`echo "$ans" | sed 's/\s\+/ /g'`
> -               break
> -           fi
> -       fi
> -    done
> +       while true; do
> +               read -p "${prompt}" -a new_groups || exit 1
> +               case "${#new_groups[@]}" in
> +               0)
> +                       new_groups=("other")
> +                       ;;
> +               1)
> +                       if [ "${new_groups[0]}" = "?" ]; then
> +                               echo $(group_names)
> +                               continue
> +                       fi
> +                       ;;
> +               esac
> +               check_groups "${new_groups[@]}" && break
> +       done
>  else
> -    # expert mode, groups are on the command line
> -    #
> -    for g in $*
> -    do
> -       if ! grep -q "^$g" doc/group-names.txt; then
> -           echo "Warning: group \"$g\" not defined in documentation"
> -       fi
> -    done
> -    ans="$*"
> +       # expert mode, groups are on the command line
> +       new_groups=("$@")
> +       check_groups "${new_groups[@]}" || exit 1
>  fi
>
>  echo -n "Creating skeletal script for you to edit ..."
> @@ -139,7 +147,7 @@ cat <<End-of-File >$tdir/$id
>  # what am I here for?
>  #
>  . ./common/preamble
> -_begin_fstest $ans
> +_begin_fstest ${new_groups[@]}
>
>  # Override the default cleanup function.
>  # _cleanup()
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] new: don't allow new tests in group 'other'
  2021-09-17  0:40 ` [PATCH 3/3] new: don't allow new tests in group 'other' Darrick J. Wong
@ 2021-09-17 10:42   ` Amir Goldstein
  0 siblings, 0 replies; 7+ messages in thread
From: Amir Goldstein @ 2021-09-17 10:42 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Eryu Guan, linux-xfs, fstests, Eryu Guan

On Fri, Sep 17, 2021 at 12:57 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> From: Darrick J. Wong <djwong@kernel.org>
>
> The 'other' group is vaguely defined at best -- other than what?  It's
> not clear what tests belong in this group, and it has become a dumping
> ground for random stuff that are classified in other groups.  Don't let
> people create new other group tests.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  new |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
>
> diff --git a/new b/new
> index 3a657d20..9651e0e0 100755
> --- a/new
> +++ b/new
> @@ -100,6 +100,9 @@ check_groups() {
>                         echo "Invalid characters in group(s): ${inval}"
>                         echo "Only lower cases, digits and underscore are allowed in groups, separated by space"
>                         return 1
> +               elif [ "${g}" = "other" ]; then
> +                       echo "Do not add more tests to group \"other\""
> +                       return 1
>                 elif ! group_names | grep -q -w "${g}"; then
>                         echo "Warning: group \"${g}\" not defined in documentation"
>                         return 1
> @@ -111,16 +114,16 @@ check_groups() {
>
>  if [ $# -eq 0 ]; then
>         # interactive mode
> -       prompt="Add to group(s) [other] (separate by space, ? for list): "
> +       prompt="Add to group(s) [auto] (separate by space, ? for list): "
>         while true; do
>                 read -p "${prompt}" -a new_groups || exit 1
>                 case "${#new_groups[@]}" in
>                 0)
> -                       new_groups=("other")
> +                       new_groups=("auto")
>                         ;;
>                 1)
>                         if [ "${new_groups[0]}" = "?" ]; then
> -                               echo $(group_names)
> +                               echo $(group_names | grep -v -w 'other')
>                                 continue
>                         fi
>                         ;;
>

Darrick,

Thanks for the first cleanup and for this extra cleanup.
I wanted to point out that with this patch there is no implication
to filtering out "other" in group_names() SHOULD you think
this is better. I definitely have no problem with the patch as is!

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-17 10:42 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-17  0:39 [PATCHSET 0/3] fstests: various cleanups to ./new script Darrick J. Wong
2021-09-17  0:39 ` [PATCH 1/3] new: clean up the group name input code Darrick J. Wong
2021-09-17 10:28   ` Amir Goldstein
2021-09-17  0:40 ` [PATCH 2/3] new: standardize group name checking Darrick J. Wong
2021-09-17 10:32   ` Amir Goldstein
2021-09-17  0:40 ` [PATCH 3/3] new: don't allow new tests in group 'other' Darrick J. Wong
2021-09-17 10:42   ` Amir Goldstein

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).