[PATCHSET 0/3] fstests: various cleanups to ./new script

[PATCHSET 0/3] fstests: various cleanups to ./new script

[Darrick J. Wong]

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

[PATCH 1/3] new: clean up the group name input code

[Darrick J. Wong]

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

[PATCH 2/3] new: standardize group name checking

[Darrick J. Wong]

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

[PATCH 3/3] new: don't allow new tests in group 'other'

[Darrick J. Wong]

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
 			;;

Re: [PATCH 1/3] new: clean up the group name input code

[Amir Goldstein]

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
>

Re: [PATCH 2/3] new: standardize group name checking

[Amir Goldstein]

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

Re: [PATCH 3/3] new: don't allow new tests in group 'other'

[Amir Goldstein]

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.