mk/subst.mk: fix combination of SUBST_FILTER_CMD with SUBST_NOOP_OK=no

Since SUBST_FILTER_CMD is a shell command, it may contain arbitrary
characters.  The condition in mk/subst.mk that tested whether
SUBST_FILTER_CMD was the default filter command was evaluated at run
time.  In such an evaluation, the variables (lhs and rhs) are fully
expanded before parsing the condition.  This means that these variables
must not contain quotes or unquoted condition operators.

Exactly this situation came up in one of the regression tests.  The
quoted "0-9" was copied verbatimly into the condition, including the
quotes.  The resulting condition was:

	"tr -d "0-9"" == "LC_ALL=C /usr/bin/sed "

This produced a syntax error because of the left-hand side. Adding a :Q
modifier would have helped for the left-hand side, but this would have
been necessary for the right-hand side as well.  Since an empty SUBST_SED
is defined not to "contain only identity substitutions", the first
condition can simply be removed.

The whole condition in the shell program had not worked anyway since it
expanded to either "[ true ]" or to "[ false ]", and both of these
commands exited successfully.

1 files changed, 97 insertions, 1 deletions

diff --git a/regress/infra-unittests/subst.sh b/regress/infra-unittests/subst.sh
index 882d7726dc4..8e7de065189 100644
--- a/regress/infra-unittests/subst.sh
+++ b/regress/infra-unittests/subst.sh
@@ -1,5 +1,5 @@
 #! /bin/sh
-# $NetBSD: subst.sh,v 1.26 2020/04/29 18:33:56 rillig Exp $
+# $NetBSD: subst.sh,v 1.27 2020/04/29 22:46:42 rillig Exp $
 #
 # Tests for mk/subst.mk.
 #
@@ -1329,3 +1329,99 @@ if test_case_begin "identity + no-op substitution"; then
 
 	test_case_end
 fi
+
+
+if test_case_begin "SUBST_FILTER_CMD + SUBST_SED in NOOP_OK=no mode"; then
+
+	# If SUBST_FILTER_CMD is defined for a SUBST class, the
+	# corresponding SUBST_SED and SUBST_VARS are ignored. To avoid
+	# redundant variable definitions, this case fails fast.
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_FILTER_CMD.id=	tr -d "0-9"' \
+		'SUBST_SED.id=		-e s,x,x,' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'letters 123 letters'
+	create_file_lines "main.mk" \
+		"PKGSRCDIR=	$pkgsrcdir" \
+		".PATH:		$mocked_pkgsrcdir" \
+		".PATH:		$pkgsrcdir" \
+		".include \"testcase.mk\"" \
+		'' \
+		'all: subst-id' \
+		'	@printf '\''fail reason: %s\n'\'' ${PKG_FAIL_REASON} 1>&2'
+
+	"$make" -f "$tmpdir/main.mk" "all" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file' \
+		'fail reason: [subst.mk:id] SUBST_FILTER_CMD and SUBST_SED/SUBST_VARS cannot be combined.'
+	assert_that "file" --file-is-lines \
+		'letters  letters'
+
+	test_case_end
+fi
+
+
+if test_case_begin "effective SUBST_FILTER_CMD in NOOP_OK=no mode"; then
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_FILTER_CMD.id=	tr -d "0-9"' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'letters 123 letters'
+
+	run_bmake "testcase.mk" "subst-id" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file'
+	assert_that "file" --file-is-lines \
+		'letters  letters'
+
+	test_case_end
+fi
+
+
+if test_case_begin "no-op SUBST_FILTER_CMD in NOOP_OK=no mode"; then
+
+	create_file_lines "testcase.mk" \
+		'SUBST_CLASSES+=	id' \
+		'SUBST_FILES.id=	file' \
+		'SUBST_FILTER_CMD.id=	tr -d "0-9"' \
+		'SUBST_NOOP_OK.id=	no' \
+		'' \
+		'.include "prepare-subst.mk"' \
+		'.include "mk/subst.mk"'
+	create_file_lines "file" \
+		'only letters'
+
+	run_bmake "testcase.mk" "subst-id" 1> "$tmpdir/out" 2>&1 \
+	&& exitcode=0 || exitcode=$?
+
+	assert_that "out" --file-is-lines \
+		'=> Substituting "id" in file' \
+		'warning: [subst.mk:id] Nothing changed in "file".' \
+		'fail: [subst.mk:id] The filename pattern "file" has no effect.' \
+		'*** Error code 1' \
+		'' \
+		'Stop.' \
+		"$make: stopped in $PWD"
+
+	assert_that "file" --file-is-lines \
+		'only letters'
+
+	test_case_end
+fi