Re: [PATCH 10/10] git-submodule.sh: don't use the -a or -b option with the test command

[Jonathan Nieder <jrnieder@gmail.com>]

Elia Pinto wrote:

> Even though POSIX.1 lists -a/-o as options to "test", they are
> marked "Obsolescent XSI". Scripts using these expressions
> should be converted  as follow:
[... many lines snipped ...]

This is a very long description, and it doesn't leave me excited by
the change.

Is there some potential bug this prevents, or is it just a style
fix?  If the latter, do we have a way of checking for new examples
of the same thing to avoid having to repeat the same patch again in
the future?

Are there any platforms that were broken which this fixes?  Even
posh seems to understand -a and -o.

Nowadays Documentation/CodingGuidelines says

 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.

   "Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up."
   Cf. http://article.gmane.org/gmane.linux.kernel/943020

which I think goes too far (some patterns really are error prone
or distracting and it can be worth fixing them tree-wide), but it
makes a reasonable case that an idiom not being preferred in the
style guide is not *on its own* enough reason to change it.

Perhaps something like the following would work?

	tree-wide: convert test -a/-o to && and ||

	The interaction with unary operators and operator precedence
	for && and || are better known than -a and -o, and for that
	reason we prefer them.  Replace all existing instances in git
	of -a and -o to save readers from the burden of thinking
	about such things.

	Add a check-non-portable-shell.pl to avoid more instances of
	test -a and -o arising in the future.

[...]
> -			test $status = D -o $status = T && echo "$sm_path" && continue
> +			 ( test $status = D || test $status = T ) && echo "$sm_path" && continue

There's no need for a subshell for this.  Better to use a block:

			{
				test "$status" = D ||
				test "$status" = T
			} &&
			echo "$sm_path" &&
			continue

or an if statement:

			if test "$status" = D || test "$status" = T
			then
				echo "$sm_path"
				continue
			fi

or case:

			case $status in
			D|T)
				echo "$sm_path"
				continue
				;;
			esac

Hope that helps,
Jonathan