pkgtools/pkglint: update to 19.3.16

Changes since 19.3.15:

When a package-settable variable gets a default value using the ?=
operator, pkglint no longer suggests to include bsd.prefs.mk, since that
doesn't make sense. Including bsd.prefs.mk only defines user-settable
and system-provided variables.

User and group names may be a single character only. While not widely
used, it's syntactically valid and there's no reason to prevent this.

In variable assignments, when pkglint removes unnecessary whitespace
between the variable name and the operator, it keeps the indentation of
the variable value the same as before. Previously, the indentation had
been changed, which required another run of pkglint --autofix.

PREFIX can only be used as a replacement for LOCALBASE after the whole
package Makefile has been loaded. This is because PREFIX is defined
very late, by bsd.pkg.mk. Therefore, don't suggest to replace LOCALBASE
with PREFIX in .if conditions.

When pkglint suggests to replace INSTALL_DATA_DIR commands with setting
INSTALLATION_DIRS instead, paths with a trailing slash are correctly
looked up in the PLIST. This suggests to use AUTO_MKDIRS more often.

21 files changed, 685 insertions, 352 deletions

diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile
index e3ff5dd27fb..34e158dd2c4 100644
--- a/pkgtools/pkglint/Makefile
+++ b/pkgtools/pkglint/Makefile
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.615 2019/12/08 22:03:37 rillig Exp $
+# $NetBSD: Makefile,v 1.616 2019/12/09 20:38:15 rillig Exp $
 
-PKGNAME=	pkglint-19.3.15
+PKGNAME=	pkglint-19.3.16
 CATEGORIES=	pkgtools
 DISTNAME=	tools
 MASTER_SITES=	${MASTER_SITE_GITHUB:=golang/}
diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go
index 43fc43e64f1..698e4dcedb3 100644
--- a/pkgtools/pkglint/files/logging.go
+++ b/pkgtools/pkglint/files/logging.go
@@ -257,10 +257,14 @@ func (l *Logger) Logf(level *LogLevel, filename CurrPath, lineno, format, msg st
 
 	if G.Testing {
 		if level != Error {
-			assertf(!contains(format, "must"), "Must must only appear in errors: %s", format)
+			assertf(
+				!contains(format, "must"),
+				"The word \"must\" must only appear in errors: %s", format)
 		}
 		if level != Warn && level != Note {
-			assertf(!contains(format, "should"), "Should must only appear in warnings: %s", format)
+			assertf(
+				!contains(format, "should"),
+				"The word \"should\" must only appear in warnings: %s", format)
 		}
 	}
 
diff --git a/pkgtools/pkglint/files/logging_test.go b/pkgtools/pkglint/files/logging_test.go
index 72022cc5167..a6b5cb85b07 100644
--- a/pkgtools/pkglint/files/logging_test.go
+++ b/pkgtools/pkglint/files/logging_test.go
@@ -805,6 +805,23 @@ func (s *Suite) Test_Logger_Logf__panic(c *check.C) {
 		"Pkglint internal error: Diagnostic format \"No period\" must end in a period.")
 }
 
+func (s *Suite) Test_Logger_Logf__wording(c *check.C) {
+	t := s.Init(c)
+
+	t.ExpectPanic(
+		func() { G.Logger.Logf(Error, "filename", "13", "This should.", "This should.") },
+		"Pkglint internal error: The word \"should\" must only appear in warnings: This should.")
+
+	t.ExpectPanic(
+		func() { G.Logger.Logf(Warn, "filename", "13", "This must.", "This must.") },
+		"Pkglint internal error: The word \"must\" must only appear in errors: This must.")
+
+	G.Logger.Logf(Note, "filename", "13", "This should.", "This should.")
+
+	t.CheckOutputLines(
+		"NOTE: filename:13: This should.")
+}
+
 func (s *Suite) Test_Logger_TechErrorf__gcc_format(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go
index 8d476611d69..7490bd8dfbe 100644
--- a/pkgtools/pkglint/files/mkline.go
+++ b/pkgtools/pkglint/files/mkline.go
@@ -1277,29 +1277,46 @@ var (
 )
 
 func MatchMkInclude(text string) (m bool, indentation, directive string, filename RelPath) {
-	lexer := textproc.NewLexer(text)
-	if lexer.SkipByte('.') {
-		indentation = lexer.NextHspace()
-		directive = lexer.NextString("include")
-		if directive == "" {
-			directive = lexer.NextString("sinclude")
-		}
-		if directive != "" {
-			lexer.NextHspace()
-			if lexer.SkipByte('"') {
-				// Note: strictly speaking, the full MkVarUse would have to be parsed
-				// here. But since these usually don't contain double quotes, it has
-				// worked fine up to now.
-				filename = NewRelPathString(lexer.NextBytesFunc(func(c byte) bool { return c != '"' }))
-				if !filename.IsEmpty() && lexer.SkipByte('"') {
-					lexer.NextHspace()
-					if lexer.EOF() {
-						m = true
-						return
-					}
-				}
-			}
-		}
+	tokens, rest := NewMkLexer(text, nil).MkTokens()
+	if rest != "" {
+		return false, "", "", ""
+	}
+
+	lexer := NewMkTokensLexer(tokens)
+	if !lexer.SkipByte('.') {
+		return false, "", "", ""
+	}
+
+	indentation = lexer.NextHspace()
+
+	directive = lexer.NextString("include")
+	if directive == "" {
+		directive = lexer.NextString("sinclude")
+	}
+	if directive == "" {
+		return false, "", "", ""
 	}
-	return false, "", "", ""
+
+	lexer.SkipHspace()
+	if !lexer.SkipByte('"') {
+		return false, "", "", ""
+	}
+
+	mark := lexer.Mark()
+	for lexer.NextBytesFunc(func(c byte) bool { return c != '"' && c != '$' }) != "" ||
+		lexer.NextVarUse() != nil {
+	}
+	enclosed := NewPath(lexer.Since(mark))
+
+	if enclosed.IsEmpty() || enclosed.IsAbs() || !lexer.SkipByte('"') {
+		return false, "", "", ""
+	}
+	lexer.SkipHspace()
+	if !lexer.EOF() {
+		return false, "", "", ""
+	}
+
+	filename = NewRelPath(enclosed)
+	m = true
+	return
 }
diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go
index 470b65160c8..0c4cd50a919 100644
--- a/pkgtools/pkglint/files/mkline_test.go
+++ b/pkgtools/pkglint/files/mkline_test.go
@@ -1547,13 +1547,6 @@ func (s *Suite) Test_MatchMkInclude(c *check.C) {
 		}
 	}
 
-	testFail("")
-	testFail(".")
-	testFail(".include")
-	testFail(".include \"")
-	testFail(".include \"other.mk")
-	testFail(".include \"other.mk\" \"")
-
 	test(".include \"other.mk\"",
 		"", "include", "other.mk", "")
 
@@ -1566,5 +1559,25 @@ func (s *Suite) Test_MatchMkInclude(c *check.C) {
 	test(".include \"other.mk\"\t# comment",
 		"", "include", "other.mk", "# comment")
 
-	t.CheckOutputEmpty()
+	test(".  include \"${DIR}/file.mk\"",
+		"  ", "include", "${DIR}/file.mk", "")
+
+	test(".  include \"${DIR:S,\",',g}/file.mk\"",
+		"  ", "include", "${DIR:S,\",',g}/file.mk", "")
+
+	test(".include \"${}\"",
+		"", "include", "${}", "")
+
+	testFail("")
+	testFail(".")
+	testFail(".include")
+	testFail(".include \"")
+	testFail(".include \"\"")
+	testFail(".include \"$\"")
+	testFail(".include \"$$\"")
+	testFail(".include \"other.mk")
+	testFail(".include \"other.mk\" \"")
+	testFail(".include \"/absolute\"")
+	testFail(".include \"/absolute\"rest")
+	testFail(".include \"/absolute\" rest")
 }
diff --git a/pkgtools/pkglint/files/mklineparser.go b/pkgtools/pkglint/files/mklineparser.go
index 12c07c20c4a..3a449de9e11 100644
--- a/pkgtools/pkglint/files/mklineparser.go
+++ b/pkgtools/pkglint/files/mklineparser.go
@@ -72,33 +72,8 @@ func (p MkLineParser) parseVarassign(line *Line, text string, splitResult mkLine
 		return nil
 	}
 
-	if a.spaceAfterVarname != "" {
-		varname := a.varname
-		op := a.op
-		switch {
-		case hasSuffix(varname, "+") && (op == opAssign || op == opAssignAppend):
-			break
-		case matches(varname, `^[a-z]`) && op == opAssignEval:
-			break
-		default:
-			fix := line.Autofix()
-			fix.Notef("Unnecessary space after variable name %q.", varname)
-			fix.Replace(varname+a.spaceAfterVarname+op.String(), varname+op.String())
-			fix.Apply()
-			// FIXME: Preserve the alignment of the variable value.
-		}
-	}
-
-	if splitResult.hasComment && a.value != "" && splitResult.spaceBeforeComment == "" {
-		line.Warnf("The # character starts a Makefile comment.")
-		line.Explain(
-			"In a variable assignment, an unescaped # starts a comment that",
-			"continues until the end of the line.",
-			"To escape the #, write \\#.",
-			"",
-			"If this # character intentionally starts a comment,",
-			"it should be preceded by a space in order to make it more visible.")
-	}
+	p.fixSpaceAfterVarname(line, a)
+	p.checkUnintendedComment(&splitResult, a, line)
 
 	return &MkLine{line, splitResult, a}
 }
@@ -181,6 +156,45 @@ func (p MkLineParser) MatchVarassign(line *Line, text string, splitResult *mkLin
 	}
 }
 
+func (p MkLineParser) fixSpaceAfterVarname(line *Line, a *mkLineAssign) {
+	if !(a.spaceAfterVarname != "") {
+		return
+	}
+
+	varname := a.varname
+	op := a.op
+	switch {
+	case hasSuffix(varname, "+") && (op == opAssign || op == opAssignAppend):
+		break
+	case matches(varname, `^[a-z]`) && op == opAssignEval:
+		break
+
+	default:
+		before := a.valueAlign
+		after := alignWith(varname+op.String(), before)
+
+		fix := line.Autofix()
+		fix.Notef("Unnecessary space after variable name %q.", varname)
+		fix.Replace(before, after)
+		fix.Apply()
+	}
+}
+
+func (p MkLineParser) checkUnintendedComment(splitResult *mkLineSplitResult, a *mkLineAssign, line *Line) {
+	if !(splitResult.hasComment && a.value != "" && splitResult.spaceBeforeComment == "") {
+		return
+	}
+
+	line.Warnf("The # character starts a Makefile comment.")
+	line.Explain(
+		"In a variable assignment, an unescaped # starts a comment that",
+		"continues until the end of the line.",
+		"To escape the #, write \\#.",
+		"",
+		"If this # character intentionally starts a comment,",
+		"it should be preceded by a space in order to make it more visible.")
+}
+
 func (p MkLineParser) parseShellcmd(line *Line, splitResult mkLineSplitResult) *MkLine {
 	return &MkLine{line, splitResult, mkLineShell{line.Text[1:]}}
 }
diff --git a/pkgtools/pkglint/files/mklineparser_test.go b/pkgtools/pkglint/files/mklineparser_test.go
index cac89e8b511..4496bc19f35 100644
--- a/pkgtools/pkglint/files/mklineparser_test.go
+++ b/pkgtools/pkglint/files/mklineparser_test.go
@@ -146,58 +146,6 @@ func (s *Suite) Test_MkLineParser_parseVarassign__leading_space(c *check.C) {
 		"WARN: rubyversion.mk:427: Makefile lines should not start with space characters.")
 }
 
-func (s *Suite) Test_MkLineParser_parseVarassign__space_around_operator(c *check.C) {
-	t := s.Init(c)
-
-	t.SetUpCommandLine("--show-autofix", "--source")
-	t.NewMkLine("test.mk", 101,
-		"pkgbase = package")
-
-	t.CheckOutputLines(
-		"NOTE: test.mk:101: Unnecessary space after variable name \"pkgbase\".",
-		"AUTOFIX: test.mk:101: Replacing \"pkgbase =\" with \"pkgbase=\".",
-		"-\tpkgbase = package",
-		"+\tpkgbase= package")
-}
-
-func (s *Suite) Test_MkLineParser_parseVarassign__autofix_space_after_varname(c *check.C) {
-	t := s.Init(c)
-
-	filename := t.CreateFileLines("Makefile",
-		MkCvsID,
-		"VARNAME +=\t${VARNAME}",
-		"VARNAME+ =\t${VARNAME+}",
-		"VARNAME+ +=\t${VARNAME+}",
-		"VARNAME+ ?=\t${VARNAME}",
-		"pkgbase := pkglint")
-
-	CheckFileMk(filename)
-
-	t.CheckOutputLines(
-		"NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".",
-
-		// The assignment operators other than = and += cannot lead to ambiguities.
-		"NOTE: ~/Makefile:5: Unnecessary space after variable name \"VARNAME+\".",
-
-		"WARN: ~/Makefile:5: "+
-			"Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".")
-
-	t.SetUpCommandLine("-Wall", "--autofix")
-
-	CheckFileMk(filename)
-
-	t.CheckOutputLines(
-		"AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\" with \"VARNAME+=\".",
-		"AUTOFIX: ~/Makefile:5: Replacing \"VARNAME+ ?=\" with \"VARNAME+?=\".")
-	t.CheckFileLines("Makefile",
-		MkCvsID+"",
-		"VARNAME+=\t${VARNAME}",
-		"VARNAME+ =\t${VARNAME+}",
-		"VARNAME+ +=\t${VARNAME+}",
-		"VARNAME+?=\t${VARNAME}",
-		"pkgbase := pkglint")
-}
-
 func (s *Suite) Test_MkLineParser_parseVarassign__append(c *check.C) {
 	t := s.Init(c)
 
@@ -458,6 +406,88 @@ func (s *Suite) Test_MkLineParser_MatchVarassign(c *check.C) {
 		"")
 }
 
+func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__show_autofix(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("--show-autofix", "--source")
+	t.NewMkLine("test.mk", 101,
+		"pkgbase = package")
+
+	t.CheckOutputLines(
+		"NOTE: test.mk:101: Unnecessary space after variable name \"pkgbase\".",
+		"AUTOFIX: test.mk:101: Replacing \"pkgbase = \" with \"pkgbase=  \".",
+		"-\tpkgbase = package",
+		"+\tpkgbase=  package")
+}
+
+func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__autofix(c *check.C) {
+	t := s.Init(c)
+
+	filename := t.CreateFileLines("Makefile",
+		MkCvsID,
+		"VARNAME +=\t${VARNAME}",
+		"VARNAME+ =\t${VARNAME+}",
+		"VARNAME+ +=\t${VARNAME+}",
+		"VARNAME+ ?=\t${VARNAME}",
+		"pkgbase := pkglint")
+
+	CheckFileMk(filename)
+
+	t.CheckOutputLines(
+		"NOTE: ~/Makefile:2: Unnecessary space after variable name \"VARNAME\".",
+
+		// The assignment operators other than = and += cannot lead to ambiguities.
+		"NOTE: ~/Makefile:5: Unnecessary space after variable name \"VARNAME+\".",
+
+		"WARN: ~/Makefile:5: "+
+			"Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".")
+
+	t.SetUpCommandLine("-Wall", "--autofix")
+
+	CheckFileMk(filename)
+
+	t.CheckOutputLines(
+		"AUTOFIX: ~/Makefile:2: Replacing \"VARNAME +=\\t\" with \"VARNAME+=\\t\".",
+		"AUTOFIX: ~/Makefile:5: Replacing \"VARNAME+ ?=\\t\" with \"VARNAME+?=\\t\".")
+	t.CheckFileLines("Makefile",
+		MkCvsID+"",
+		"VARNAME+=\t${VARNAME}",
+		"VARNAME+ =\t${VARNAME+}",
+		"VARNAME+ +=\t${VARNAME+}",
+		"VARNAME+?=\t${VARNAME}",
+		"pkgbase := pkglint")
+}
+
+func (s *Suite) Test_MkLineParser_fixSpaceAfterVarname__preserve_alignment(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("--show-autofix")
+
+	test := func(before, after string, diagnostics ...string) {
+
+		doTest := func(autofix bool) {
+			mkline := t.NewMkLine("filename.mk", 123, before)
+			t.CheckEquals(mkline.Text, condStr(autofix, after, before))
+		}
+
+		t.ExpectDiagnosticsAutofix(doTest, diagnostics...)
+	}
+
+	test(
+		"V    +=         ${VARNAME}",
+		"V+=\t\t${VARNAME}",
+
+		"NOTE: filename.mk:123: Unnecessary space after variable name \"V\".",
+		"AUTOFIX: filename.mk:123: Replacing \"V    +=         \" with \"V+=\\t\\t\".")
+
+	test(
+		"V    +=     ${VARNAME}",
+		"V+=\t    ${VARNAME}",
+
+		"NOTE: filename.mk:123: Unnecessary space after variable name \"V\".",
+		"AUTOFIX: filename.mk:123: Replacing \"V    +=     \" with \"V+=\\t    \".")
+}
+
 func (s *Suite) Test_MkLineParser_parseShellcmd(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go
index cf5a1eb4aa4..cc4d6c47e9d 100644
--- a/pkgtools/pkglint/files/mklines_test.go
+++ b/pkgtools/pkglint/files/mklines_test.go
@@ -743,6 +743,28 @@ func (s *Suite) Test_MkLines_ForEach__conditional_variables(c *check.C) {
 	t.CheckEquals(seenUsesGettext, true)
 }
 
+func (s *Suite) Test_MkLines_ForEachEnd(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+
+	mklines := t.NewMkLines("filename.mk",
+		MkCvsID)
+
+	// Calls to MkLines.ForEach cannot nest since they modify fields
+	// in the MkLines type. As of December 2019 there is no separation
+	// between:
+	//  - The MkLines as a collection of data
+	//  - An iterator over the MkLines
+	//  - The MkLinesChecker
+	t.ExpectAssert(func() {
+		mklines.ForEach(func(mkline *MkLine) {
+			mklines.ForEach(func(mkline *MkLine) {
+			})
+		})
+	})
+}
+
 func (s *Suite) Test_MkLines_collectElse(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go
index 734a670b185..bb4f9700ed6 100644
--- a/pkgtools/pkglint/files/mkvarusechecker.go
+++ b/pkgtools/pkglint/files/mkvarusechecker.go
@@ -25,7 +25,7 @@ func (ck *MkVarUseChecker) Check(vuc *VarUseContext) {
 	ck.checkUndefined()
 	ck.checkPermissions(vuc)
 
-	ck.checkVarname()
+	ck.checkVarname(vuc.time)
 	ck.checkModifiers()
 	ck.checkQuoting(vuc)
 
@@ -130,7 +130,7 @@ func (ck *MkVarUseChecker) checkModifiersRange() {
 	fix.Apply()
 }
 
-func (ck *MkVarUseChecker) checkVarname() {
+func (ck *MkVarUseChecker) checkVarname(time VucTime) {
 	varname := ck.use.varname
 	if varname == "@" {
 		ck.MkLine.Warnf("Please use %q instead of %q.", "${.TARGET}", "$@")
@@ -139,7 +139,7 @@ func (ck *MkVarUseChecker) checkVarname() {
 			"of the same name.")
 	}
 
-	if varname == "LOCALBASE" && !G.Infrastructure {
+	if varname == "LOCALBASE" && !G.Infrastructure && time == VucRunTime {
 		fix := ck.MkLine.Autofix()
 		fix.Warnf("Please use PREFIX instead of LOCALBASE.")
 		fix.ReplaceRegex(`\$\{LOCALBASE\b`, "${PREFIX", 1)
@@ -380,12 +380,10 @@ func (ck *MkVarUseChecker) checkUseAtLoadTime(time VucTime) {
 		return
 	}
 
-	if ck.vartype.IsPackageSettable() &&
-		basename != "Makefile" && basename != "options.mk" {
-
-		// For package-settable variables, the explanation doesn't
-		// make sense since it talks about completely different
-		// types of variables.
+	if ck.vartype.IsPackageSettable() {
+		// For package-settable variables, the explanation below
+		// doesn't make sense since including bsd.prefs.mk won't
+		// define any package-settable variables.
 		return
 	}
 
diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go
index be9d5dfe83a..6fee1c066a8 100644
--- a/pkgtools/pkglint/files/mkvarusechecker_test.go
+++ b/pkgtools/pkglint/files/mkvarusechecker_test.go
@@ -355,7 +355,24 @@ func (s *Suite) Test_MkVarUseChecker_checkModifiersRange(c *check.C) {
 }
 
 func (s *Suite) Test_MkVarUseChecker_checkVarname(c *check.C) {
-	// FIXME
+	t := s.Init(c)
+
+	mklines := t.NewMkLines("filename.mk",
+		"\t: $@",
+		".if ${LOCALBASE} == /usr/pkg", // Use at load time
+		"\t: ${LOCALBASE}",             // Use at run time
+		".endif")
+
+	mklines.ForEach(func(mkline *MkLine) {
+		mkline.ForEachUsed(func(varUse *MkVarUse, time VucTime) {
+			ck := NewMkVarUseChecker(varUse, mklines, mkline)
+			ck.checkVarname(time)
+		})
+	})
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:1: Please use \"${.TARGET}\" instead of \"$@\".",
+		"WARN: filename.mk:3: Please use PREFIX instead of LOCALBASE.")
 }
 
 func (s *Suite) Test_MkVarUseChecker_checkPermissions(c *check.C) {
@@ -430,33 +447,6 @@ func (s *Suite) Test_MkVarUseChecker_checkPermissions__explain(c *check.C) {
 		"\ttech-pkg@NetBSD.org mailing list.", "")
 }
 
-func (s *Suite) Test_MkVarUseChecker_checkPermissions__load_time(c *check.C) {
-	t := s.Init(c)
-
-	t.SetUpPkgsrc()
-	t.Chdir("category/package")
-	t.FinishSetUp()
-	mklines := t.NewMkLines("options.mk",
-		MkCvsID,
-		"",
-		"# don't include bsd.prefs.mk here",
-		"",
-		"WRKSRC:=\t${.CURDIR}",
-		".if ${PKG_SYSCONFDIR.gdm} != \"etc\"",
-		".endif")
-
-	mklines.Check()
-
-	// The PKG_SYSCONFDIR.* depend on the directory layout that is
-	// specified in mk.conf, therefore bsd.prefs.mk must be included first.
-	//
-	// Evaluating .CURDIR at load time is definitely ok since it is defined
-	// internally by bmake to be AlwaysInScope.
-	t.CheckOutputLines(
-		"WARN: options.mk:6: To use PKG_SYSCONFDIR.gdm at load time, " +
-			".include \"../../mk/bsd.prefs.mk\" first.")
-}
-
 func (s *Suite) Test_MkVarUseChecker_checkPermissions__load_time_in_condition(c *check.C) {
 	t := s.Init(c)
 
@@ -860,6 +850,52 @@ func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__other_mk(c *check.C) {
 			".include \"../../mk/bsd.prefs.mk\" first.")
 }
 
+func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__PKG_SYSCONFDIR(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+	t.Chdir("category/package")
+	mklines := t.NewMkLines("options.mk",
+		MkCvsID,
+		".if ${PKG_SYSCONFDIR.gdm} != \"etc\"",
+		".endif")
+
+	mklines.Check()
+
+	// The PKG_SYSCONFDIR.* directories typically start with ${PREFIX}.
+	// Since PREFIX is not defined until bsd.pkg.mk, including bsd.prefs.mk
+	// wouldn't help, therefore pkglint doesn't suggest it.
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_MkVarUseChecker_checkUseAtLoadTime__package_settable(c *check.C) {
+	t := s.Init(c)
+
+	btAnything := &BasicType{"Anything", func(cv *VartypeCheck) {}}
+	t.SetUpType("PKG", btAnything, PackageSettable)
+	t.Chdir("category/package")
+
+	test := func(filename CurrPath, diagnostics ...string) {
+		mklines := t.NewMkLines(filename,
+			MkCvsID,
+			".if ${PKG} != \"etc\"",
+			".endif")
+
+		mklines.Check()
+
+		t.CheckOutput(diagnostics)
+	}
+
+	test("Makefile",
+		nil...)
+
+	test("options.mk",
+		nil...)
+
+	test("other.mk",
+		nil...)
+}
+
 func (s *Suite) Test_MkVarUseChecker_warnToolLoadTime(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go
index f7582cc6e6d..3c49278afb2 100644
--- a/pkgtools/pkglint/files/package_test.go
+++ b/pkgtools/pkglint/files/package_test.go
@@ -1471,13 +1471,25 @@ func (s *Suite) Test_Package_checkfilePackageMakefile__options_mk(c *check.C) {
 	t.CheckOutputEmpty()
 }
 
+// When the lines of the package Makefile are checked, it is necessary
+// to know whether bsd.prefs.mk has already been included.
+//
+// When the files are loaded recursively, Package.seenPrefs is set to
+// true as soon as some file includes bsd.prefs.mk. After that, when
+// loading reaches the main package Makefile again, Package.prefsLine
+// is set to the line that had just been included.
+//
+// In this test case, the preferences are loaded indirectly by line 22,
+// which includes common.mk, and that in turn includes bsd.prefs.mk.
 func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C) {
 	t := s.Init(c)
 
-	t.SetUpOption("option", "An example option")
+	// FIXME: remove t.SetUpOption("option", "An example option")
 	t.SetUpPackage("category/package",
-		".include \"common.mk\"",
-		".if ${OPSYS} == NetBSD",
+		".if ${OPSYS} == NetBSD", // 20: OPSYS is not yet defined here.
+		".endif",                 // 21
+		".include \"common.mk\"", // 22: OPSYS gets defined.
+		".if ${OPSYS} == NetBSD", // 23: Now OPSYS is defined.
 		".endif")
 	t.CreateFileLines("category/package/common.mk",
 		MkCvsID,
@@ -1491,16 +1503,19 @@ func (s *Suite) Test_Package_checkfilePackageMakefile__prefs_indirect(c *check.C
 	files, mklines, allLines := G.Pkg.load()
 
 	t.CheckEquals(G.Pkg.seenPrefs, false)
-	t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[19])
+	t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21])
 
 	G.Pkg.check(files, mklines, allLines)
 
 	t.CheckEquals(G.Pkg.seenPrefs, true)
-	t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[19])
+	t.CheckEquals(G.Pkg.prefsLine, mklines.mklines[21])
 
 	// Since bsd.prefs.mk is included indirectly by common.mk,
-	// OPSYS may be used at load time in line 21.
-	t.CheckOutputEmpty()
+	// OPSYS may be used at load time in line 23, but not in line 20.
+	t.CheckOutputLines(
+		"WARN: ~/category/package/Makefile:20: " +
+			"To use OPSYS at load time, " +
+			".include \"../../mk/bsd.prefs.mk\" first.")
 }
 
 func (s *Suite) Test_Package_checkfilePackageMakefile__redundancy_in_infra(c *check.C) {
diff --git a/pkgtools/pkglint/files/path_test.go b/pkgtools/pkglint/files/path_test.go
index c1efe6d3e4c..4cdc95e3ae2 100644
--- a/pkgtools/pkglint/files/path_test.go
+++ b/pkgtools/pkglint/files/path_test.go
@@ -525,8 +525,10 @@ func (s *Suite) Test_Path_Rel(c *check.C) {
 	test("a/../b", "c/../d", "../d")
 
 	test(".", "dir/file", "dir/file")
-	test(".", "dir/subdir/", "dir/subdir")  // FIXME: missing /. at the end
-	test(".", "dir/subdir/.", "dir/subdir") // FIXME: missing /. at the end
+	// XXX: maybe the /. is missing at the end
+	test(".", "dir/subdir/", "dir/subdir")
+	// XXX: maybe the /. is missing at the end
+	test(".", "dir/subdir/.", "dir/subdir")
 }
 
 func (s *Suite) Test_NewCurrPath(c *check.C) {
diff --git a/pkgtools/pkglint/files/pkglint.1 b/pkgtools/pkglint/files/pkglint.1
index 97b1ac283d5..918346c5449 100644
--- a/pkgtools/pkglint/files/pkglint.1
+++ b/pkgtools/pkglint/files/pkglint.1
@@ -1,4 +1,4 @@
-.\"	$NetBSD: pkglint.1,v 1.61 2019/12/08 22:03:38 rillig Exp $
+.\"	$NetBSD: pkglint.1,v 1.62 2019/12/09 20:38:16 rillig Exp $
 .\"	From FreeBSD: portlint.1,v 1.8 1997/11/25 14:53:14 itojun Exp
 .\"
 .\" Copyright (c) 1997 by Jun-ichiro Itoh <itojun@itojun.org>.
diff --git a/pkgtools/pkglint/files/pkglint_test.go b/pkgtools/pkglint/files/pkglint_test.go
index f60390ccede..8239505f8ed 100644
--- a/pkgtools/pkglint/files/pkglint_test.go
+++ b/pkgtools/pkglint/files/pkglint_test.go
@@ -275,12 +275,18 @@ func (s *Suite) Test_Pkglint_Main__autofix_exitcode(c *check.C) {
 // install https://github.com/rillig/gobco and adjust the following code:
 //
 //  env \
-//  PKGLINT_TESTDIR=C:/Users/rillig/git/pkgsrc \
-//  PKGLINT_TESTCMDLINE="-r -Wall -Call -e" \
-//  gobco -test.covermode=count \
-//      -test.coverprofile=pkglint-pkgsrc.pprof \
-//      -timeout=3600s -check.f '^Test_Pkglint__realistic' \
-//      > pkglint-pkgsrc.out
+//      PKGLINT_TESTDIR="C:/Users/rillig/git/pkgsrc" \
+//      PKGLINT_TESTCMDLINE="-r -Wall -Call -p -s -e" \
+//  gobco \
+//      -test=-test.covermode=count
+//      -test=-test.coverprofile="C:/Users/rillig/go/src/netbsd.org/pkglint/stats-go.txt"
+//      -test=-timeout=3600s \
+//      -test=-check.f="^Test_Pkglint_Main__realistic" \
+//      -stats="stats-gobco.json" \
+//      > out
+//
+// Note that the path to -test.coverprofile must be absolute, since gobco
+// runs "go test" in a temporary directory.
 //
 // See https://github.com/rillig/gobco for the tool to measure the branch coverage.
 func (s *Suite) Test_Pkglint_Main__realistic(c *check.C) {
diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go
index 04e4f4fdcf1..bdbd9d08625 100644
--- a/pkgtools/pkglint/files/plist.go
+++ b/pkgtools/pkglint/files/plist.go
@@ -578,8 +578,7 @@ func NewPlistLineSorter(plines []*PlistLine) *plistLineSorter {
 
 func (s *plistLineSorter) Sort() {
 	if line := s.unsortable; line != nil {
-		if G.Logger.IsAutofix() {
-			// FIXME: Missing trace.Enabled
+		if G.Logger.IsAutofix() && trace.Tracing {
 			trace.Stepf("%s: This line prevents pkglint from sorting the PLIST automatically.", line)
 		}
 		return
diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go
index c0c0338029d..203e6b0b102 100644
--- a/pkgtools/pkglint/files/shell.go
+++ b/pkgtools/pkglint/files/shell.go
@@ -9,15 +9,17 @@ import (
 // Parsing and checking shell commands embedded in Makefiles
 
 type SimpleCommandChecker struct {
-	*ShellLineChecker
 	cmd    *MkShSimpleCommand
 	strcmd *StrCommand
 	time   ToolTime
+
+	mkline  *MkLine
+	mklines *MkLines
 }
 
-func NewSimpleCommandChecker(ck *ShellLineChecker, cmd *MkShSimpleCommand, time ToolTime) *SimpleCommandChecker {
+func NewSimpleCommandChecker(cmd *MkShSimpleCommand, time ToolTime, mkline *MkLine, mklines *MkLines) *SimpleCommandChecker {
 	strcmd := NewStrCommand(cmd)
-	return &SimpleCommandChecker{ck, cmd, strcmd, time}
+	return &SimpleCommandChecker{cmd, strcmd, time, mkline, mklines}
 
 }
 
@@ -58,7 +60,7 @@ func (scc *SimpleCommandChecker) checkCommandStart() {
 	case matches(shellword, `\$\{(PKGSRCDIR|PREFIX)(:Q)?\}`):
 		break
 	default:
-		if G.Opts.WarnExtra && !scc.MkLines.indentation.DependsOn("OPSYS") {
+		if G.Opts.WarnExtra && !scc.mklines.indentation.DependsOn("OPSYS") {
 			scc.mkline.Warnf("Unknown shell command %q.", shellword)
 			scc.mkline.Explain(
 				"To make the package portable to all platforms that pkgsrc supports,",
@@ -69,6 +71,51 @@ func (scc *SimpleCommandChecker) checkCommandStart() {
 	}
 }
 
+// Some shell commands should not be used in the install phase.
+func (scc *SimpleCommandChecker) checkInstallCommand(shellcmd string) {
+	if trace.Tracing {
+		defer trace.Call0()()
+	}
+
+	if !matches(scc.mklines.target, `^(?:pre|do|post)-install$`) {
+		return
+	}
+
+	line := scc.mkline.Line
+	switch shellcmd {
+	case "${INSTALL}",
+		"${INSTALL_DATA}", "${INSTALL_DATA_DIR}",
+		"${INSTALL_LIB}", "${INSTALL_LIB_DIR}",
+		"${INSTALL_MAN}", "${INSTALL_MAN_DIR}",
+		"${INSTALL_PROGRAM}", "${INSTALL_PROGRAM_DIR}",
+		"${INSTALL_SCRIPT}",
+		"${LIBTOOL}",
+		"${LN}",
+		"${PAX}":
+		return
+
+	case "sed", "${SED}",
+		"tr", "${TR}":
+		// TODO: Pkglint should not complain when sed and tr are used to transform filenames.
+		line.Warnf("The shell command %q should not be used in the install phase.", shellcmd)
+		line.Explain(
+			"In the install phase, the only thing that should be done is to",
+			"install the prepared files to their final location.",
+			"The file's contents should not be changed anymore.")
+
+	case "cp", "${CP}":
+		line.Warnf("${CP} should not be used to install files.")
+		line.Explain(
+			"The ${CP} command is highly platform dependent and cannot overwrite read-only files.",
+			"Please use ${PAX} instead.",
+			"",
+			"For example, instead of:",
+			"\t${CP} -R ${WRKSRC}/* ${PREFIX}/foodir",
+			"use:",
+			"\tcd ${WRKSRC} && ${PAX} -wr * ${PREFIX}/foodir")
+	}
+}
+
 func (scc *SimpleCommandChecker) handleForbiddenCommand() bool {
 	if trace.Tracing {
 		defer trace.Call0()()
@@ -95,7 +142,7 @@ func (scc *SimpleCommandChecker) handleTool() bool {
 
 	command := scc.strcmd.Name
 
-	tool, usable := G.Tool(scc.MkLines, command, scc.time)
+	tool, usable := G.Tool(scc.mklines, command, scc.time)
 
 	if tool != nil && !usable {
 		scc.mkline.Warnf("The %q tool is used but not added to USE_TOOLS.", command)
@@ -121,14 +168,14 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool {
 
 	varname := varuse.varname
 
-	if vartype := G.Pkgsrc.VariableType(scc.MkLines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" {
+	if vartype := G.Pkgsrc.VariableType(scc.mklines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" {
 		scc.checkInstallCommand(shellword)
 		return true
 	}
 
 	// When the package author has explicitly defined a command
 	// variable, assume it to be valid.
-	if scc.MkLines.allVars.IsDefinedSimilar(varname) {
+	if scc.mklines.allVars.IsDefinedSimilar(varname) {
 		return true
 	}
 
@@ -205,45 +252,64 @@ func (scc *SimpleCommandChecker) checkAutoMkdirs() {
 		return
 	}
 
+	containsIgnoredVar := func(arg string) bool {
+		for _, token := range scc.mkline.Tokenize(arg, false) {
+			if token.Varuse != nil && matches(token.Varuse.varname, `^[_.]*[a-z]`) {
+				return true
+			}
+		}
+		return false
+	}
+
 	for _, arg := range scc.strcmd.Args {
-		// TODO: Replace regex with proper VarUse.
-		if !contains(arg, "$$") && !matches(arg, `\$\{[_.]*[a-z]`) {
-			if m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/(.*)`); m {
-				autoMkdirs := false
-				if G.Pkg != nil {
-					// FIXME: Add test for absolute path.
-					plistLine := G.Pkg.Plist.Dirs[NewRelPathString(dirname)]
-					if plistLine != nil && !containsVarRef(plistLine.Text) {
-						autoMkdirs = true
-					}
-				}
+		if contains(arg, "$$") || containsIgnoredVar(arg) {
+			continue
+		}
 
-				if autoMkdirs {
-					scc.Notef("You can use AUTO_MKDIRS=yes or \"INSTALLATION_DIRS+= %s\" instead of %q.", dirname, cmdname)
-					scc.Explain(
-						"Many packages include a list of all needed directories in their",
-						"PLIST file.",
-						"In such a case, you can just set AUTO_MKDIRS=yes and be done.",
-						"The pkgsrc infrastructure will then create all directories in advance.",
-						"",
-						"To create directories that are not mentioned in the PLIST file,",
-						"it is easier to just list them in INSTALLATION_DIRS than to execute the",
-						"commands explicitly.",
-						"That way, you don't have to think about which",
-						"of the many INSTALL_*_DIR variables is appropriate, since",
-						"INSTALLATION_DIRS takes care of that.")
-				} else {
-					scc.Notef("You can use \"INSTALLATION_DIRS+= %s\" instead of %q.", dirname, cmdname)
-					scc.Explain(
-						"To create directories during installation, it is easier to just",
-						"list them in INSTALLATION_DIRS than to execute the commands",
-						"explicitly.",
-						"That way, you don't have to think about which",
-						"of the many INSTALL_*_DIR variables is appropriate,",
-						"since INSTALLATION_DIRS takes care of that.")
-				}
+		m, dirname := match1(arg, `^(?:\$\{DESTDIR\})?\$\{PREFIX(?:|:Q)\}/+([^/]\S*)$`)
+		if !m {
+			continue
+		}
+
+		prefixRel := NewRelPathString(dirname).Clean()
+		if prefixRel == "." {
+			continue
+		}
+
+		autoMkdirs := false
+		if G.Pkg != nil && prefixRel != "." {
+			plistLine := G.Pkg.Plist.Dirs[prefixRel]
+			if plistLine != nil && !containsVarRef(plistLine.Text) {
+				autoMkdirs = true
 			}
 		}
+
+		if autoMkdirs {
+			scc.Notef("You can use AUTO_MKDIRS=yes or \"INSTALLATION_DIRS+= %s\" instead of %q.",
+				prefixRel.String(), cmdname)
+			scc.Explain(
+				"Many packages include a list of all needed directories in their",
+				"PLIST file.",
+				"In such a case, you can just set AUTO_MKDIRS=yes and be done.",
+				"The pkgsrc infrastructure will then create all directories in advance.",
+				"",
+				"To create directories that are not mentioned in the PLIST file,",
+				"it is easier to just list them in INSTALLATION_DIRS than to execute the",
+				"commands explicitly.",
+				"That way, you don't have to think about which",
+				"of the many INSTALL_*_DIR variables is appropriate, since",
+				"INSTALLATION_DIRS takes care of that.")
+		} else {
+			scc.Notef("You can use \"INSTALLATION_DIRS+= %s\" instead of %q.",
+				prefixRel.String(), cmdname)
+			scc.Explain(
+				"To create directories during installation, it is easier to just",
+				"list them in INSTALLATION_DIRS than to execute the commands",
+				"explicitly.",
+				"That way, you don't have to think about which",
+				"of the many INSTALL_*_DIR variables is appropriate,",
+				"since INSTALLATION_DIRS takes care of that.")
+		}
 	}
 }
 
@@ -283,7 +349,7 @@ func (scc *SimpleCommandChecker) checkPaxPe() {
 
 	if (scc.strcmd.Name == "${PAX}" || scc.strcmd.Name == "pax") && scc.strcmd.HasOption("-pe") {
 		scc.Warnf("Please use the -pp option to pax(1) instead of -pe.")
-		scc.mkline.Explain(
+		scc.Explain(
 			"The -pe option tells pax to preserve the ownership of the files.",
 			"",
 			"When extracting distfiles as root user, this means that whatever numeric uid was",
@@ -301,7 +367,7 @@ func (scc *SimpleCommandChecker) checkEchoN() {
 	}
 
 	if scc.strcmd.Name == "${ECHO}" && scc.strcmd.HasOption("-n") {
-		scc.mkline.Warnf("Please use ${ECHO_N} instead of \"echo -n\".")
+		scc.Warnf("Please use ${ECHO_N} instead of \"echo -n\".")
 	}
 }
 
@@ -671,7 +737,7 @@ func (ck *ShellLineChecker) CheckShellCommand(shellcmd string, pSetE *bool, time
 
 	walker := NewMkShWalker()
 	walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) {
-		scc := NewSimpleCommandChecker(ck, command, time)
+		scc := NewSimpleCommandChecker(command, time, ck.mkline, ck.MkLines)
 		scc.Check()
 		// TODO: Implement getopt parsing for StrCommand.
 		if scc.strcmd.Name == "set" && scc.strcmd.AnyArgMatches(`^-.*e`) {
@@ -953,51 +1019,6 @@ func (ck *ShellLineChecker) checkVaruseToken(atoms *[]*ShAtom, quoting ShQuoting
 	return true
 }
 
-// Some shell commands should not be used in the install phase.
-func (ck *ShellLineChecker) checkInstallCommand(shellcmd string) {
-	if trace.Tracing {
-		defer trace.Call0()()
-	}
-
-	if !matches(ck.MkLines.target, `^(?:pre|do|post)-install$`) {
-		return
-	}
-
-	line := ck.mkline.Line
-	switch shellcmd {
-	case "${INSTALL}",
-		"${INSTALL_DATA}", "${INSTALL_DATA_DIR}",
-		"${INSTALL_LIB}", "${INSTALL_LIB_DIR}",
-		"${INSTALL_MAN}", "${INSTALL_MAN_DIR}",
-		"${INSTALL_PROGRAM}", "${INSTALL_PROGRAM_DIR}",
-		"${INSTALL_SCRIPT}",
-		"${LIBTOOL}",
-		"${LN}",
-		"${PAX}":
-		return
-
-	case "sed", "${SED}",
-		"tr", "${TR}":
-		// TODO: Pkglint should not complain when sed and tr are used to transform filenames.
-		line.Warnf("The shell command %q should not be used in the install phase.", shellcmd)
-		line.Explain(
-			"In the install phase, the only thing that should be done is to",
-			"install the prepared files to their final location.",
-			"The file's contents should not be changed anymore.")
-
-	case "cp", "${CP}":
-		line.Warnf("${CP} should not be used to install files.")
-		line.Explain(
-			"The ${CP} command is highly platform dependent and cannot overwrite read-only files.",
-			"Please use ${PAX} instead.",
-			"",
-			"For example, instead of:",
-			"\t${CP} -R ${WRKSRC}/* ${PREFIX}/foodir",
-			"use:",
-			"\tcd ${WRKSRC} && ${PAX} -wr * ${PREFIX}/foodir")
-	}
-}
-
 func (ck *ShellLineChecker) checkMultiLineComment() {
 	mkline := ck.mkline
 	if !mkline.IsMultiline() || !contains(mkline.Text, "#") {
diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go
index 6fbbbb76e96..48ff0f4897b 100644
--- a/pkgtools/pkglint/files/shell_test.go
+++ b/pkgtools/pkglint/files/shell_test.go
@@ -44,6 +44,74 @@ func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__unknown_default(c *
 		"WARN: Makefile:8: UNKNOWN_TOOL is used but not defined.")
 }
 
+func (s *Suite) Test_SimpleCommandChecker_checkInstallCommand(c *check.C) {
+	t := s.Init(c)
+
+	lines := func(lines ...string) []string { return lines }
+
+	test := func(lines []string, diagnostics ...string) {
+		mklines := t.NewMkLines("filename.mk",
+			mapStr(lines, func(s string) string { return "\t" + s })...)
+		mklines.target = "do-install"
+
+		mklines.ForEach(func(mkline *MkLine) {
+			program, err := parseShellProgram(nil, mkline.ShellCommand())
+			assertNil(err, "")
+
+			walker := NewMkShWalker()
+			walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) {
+				scc := NewSimpleCommandChecker(command, RunTime, mkline, mklines)
+				scc.checkInstallCommand(command.Name.MkText)
+			}
+			walker.Walk(program)
+		})
+
+		t.CheckOutput(diagnostics)
+	}
+
+	test(
+		lines(
+			"sed",
+			"${SED}"),
+		"WARN: filename.mk:1: The shell command \"sed\" "+
+			"should not be used in the install phase.",
+		"WARN: filename.mk:2: The shell command \"${SED}\" "+
+			"should not be used in the install phase.")
+
+	test(
+		lines(
+			"tr",
+			"${TR}"),
+		"WARN: filename.mk:1: The shell command \"tr\" "+
+			"should not be used in the install phase.",
+		"WARN: filename.mk:2: The shell command \"${TR}\" "+
+			"should not be used in the install phase.")
+
+	test(
+		lines(
+			"cp",
+			"${CP}"),
+		"WARN: filename.mk:1: ${CP} should not be used to install files.",
+		"WARN: filename.mk:2: ${CP} should not be used to install files.")
+
+	test(
+		lines(
+			"${INSTALL}",
+			"${INSTALL_DATA}",
+			"${INSTALL_DATA_DIR}",
+			"${INSTALL_LIB}",
+			"${INSTALL_LIB_DIR}",
+			"${INSTALL_MAN}",
+			"${INSTALL_MAN_DIR}",
+			"${INSTALL_PROGRAM}",
+			"${INSTALL_PROGRAM_DIR}",
+			"${INSTALL_SCRIPT}",
+			"${LIBTOOL}",
+			"${LN}",
+			"${PAX}"),
+		nil...)
+}
+
 func (s *Suite) Test_SimpleCommandChecker_handleForbiddenCommand(c *check.C) {
 	t := s.Init(c)
 
@@ -155,10 +223,14 @@ func (s *Suite) Test_SimpleCommandChecker_handleCommandVariable__from_package(c
 func (s *Suite) Test_SimpleCommandChecker_handleShellBuiltin(c *check.C) {
 	t := s.Init(c)
 
+	mklines := t.NewMkLines("filename.mk",
+		"\t:")
+	mkline := mklines.mklines[0]
+
 	test := func(command string, isBuiltin bool) {
 		token := NewShToken(command, NewShAtom(shtText, command, shqPlain))
 		simpleCommand := &MkShSimpleCommand{Name: token}
-		scc := NewSimpleCommandChecker(nil, simpleCommand, RunTime)
+		scc := NewSimpleCommandChecker(simpleCommand, RunTime, mkline, mklines)
 		t.CheckEquals(scc.handleShellBuiltin(), isBuiltin)
 	}
 
@@ -352,6 +424,83 @@ func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs__conditional_PLIST(c *
 			"instead of \"${INSTALL_DATA_DIR}\".")
 }
 
+func (s *Suite) Test_SimpleCommandChecker_checkAutoMkdirs__strange_paths(c *check.C) {
+	t := s.Init(c)
+
+	test := func(path string, diagnostics ...string) {
+		mklines := t.NewMkLines("filename.mk",
+			"\t${INSTALL_DATA_DIR} "+path)
+		mklines.ForEach(func(mkline *MkLine) {
+			program, err := parseShellProgram(nil, mkline.ShellCommand())
+			assertNil(err, "")
+
+			walker := NewMkShWalker()
+			walker.Callback.SimpleCommand = func(command *MkShSimpleCommand) {
+				scc := NewSimpleCommandChecker(command, RunTime, mkline, mklines)
+				scc.checkAutoMkdirs()
+			}
+			walker.Walk(program)
+		})
+		t.CheckOutput(diagnostics)
+	}
+
+	t.Chdir("category/package")
+
+	test("${PREFIX}",
+		nil...)
+
+	test("${PREFIX}/",
+		nil...)
+
+	test("${PREFIX}//",
+		nil...)
+
+	test("${PREFIX}/.",
+		nil...)
+
+	test("${PREFIX}//non-canonical",
+		"NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= non-canonical\" "+
+			"instead of \"${INSTALL_DATA_DIR}\".")
+
+	test("${PREFIX}/non-canonical/////",
+		"NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= non-canonical\" "+
+			"instead of \"${INSTALL_DATA_DIR}\".")
+
+	test("${PREFIX}/${VAR}",
+		"NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${VAR}\" "+
+			"instead of \"${INSTALL_DATA_DIR}\".")
+
+	test("${PREFIX}/${VAR.param}",
+		"NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${VAR.param}\" "+
+			"instead of \"${INSTALL_DATA_DIR}\".")
+
+	test("${PREFIX}/${.CURDIR}",
+		"NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${.CURDIR}\" "+
+			"instead of \"${INSTALL_DATA_DIR}\".")
+
+	// Internal variables are ok.
+	test("${PREFIX}/${_INTERNAL}",
+		"NOTE: filename.mk:1: You can use \"INSTALLATION_DIRS+= ${_INTERNAL}\" "+
+			"instead of \"${INSTALL_DATA_DIR}\".")
+
+	// Ignore variables from a :@ modifier.
+	test("${PREFIX}/${.f.}",
+		nil...)
+
+	// Ignore variables from a .for loop.
+	test("${PREFIX}/${f}",
+		nil...)
+
+	// Ignore variables from a .for loop.
+	test("${PREFIX}/${_f_}",
+		nil...)
+
+	// Ignore paths containing shell variables as it is hard to
+	// predict their values using static analysis.
+	test("${PREFIX}/$$f",
+		nil...)
+}
+
 // This test ensures that the command line options to INSTALL_*_DIR are properly
 // parsed and do not lead to "can only handle one directory at a time" warnings.
 func (s *Suite) Test_SimpleCommandChecker_checkInstallMulti(c *check.C) {
@@ -1681,71 +1830,6 @@ func (s *Suite) Test_ShellLineChecker_variableNeedsQuoting__integration(c *check
 		"WARN: filename.mk:3: Unquoted shell variable \"target\".")
 }
 
-func (s *Suite) Test_ShellLineChecker_checkInstallCommand(c *check.C) {
-	t := s.Init(c)
-
-	test := func(lines ...string) {
-		var cmds []string
-		i := 0
-		for i < len(lines) && lines[i] != "" {
-			cmds = append(cmds, "\t"+lines[i])
-			i++
-		}
-		diagnostics := lines[i+1:]
-
-		mklines := t.NewMkLines("filename.mk", cmds...)
-		mklines.target = "do-install"
-
-		mklines.ForEach(func(mkline *MkLine) {
-			ck := NewShellLineChecker(mklines, mkline)
-			ck.checkInstallCommand(mkline.ShellCommand())
-		})
-
-		t.CheckOutput(diagnostics)
-	}
-
-	test(
-		"sed",
-		"${SED}",
-		"",
-		"WARN: filename.mk:1: The shell command \"sed\" "+
-			"should not be used in the install phase.",
-		"WARN: filename.mk:2: The shell command \"${SED}\" "+
-			"should not be used in the install phase.")
-
-	test(
-		"tr",
-		"${TR}",
-		"",
-		"WARN: filename.mk:1: The shell command \"tr\" "+
-			"should not be used in the install phase.",
-		"WARN: filename.mk:2: The shell command \"${TR}\" "+
-			"should not be used in the install phase.")
-
-	test(
-		"cp",
-		"${CP}",
-		"",
-		"WARN: filename.mk:1: ${CP} should not be used to install files.",
-		"WARN: filename.mk:2: ${CP} should not be used to install files.")
-
-	test(
-		"${INSTALL}",
-		"${INSTALL_DATA}",
-		"${INSTALL_DATA_DIR}",
-		"${INSTALL_LIB}",
-		"${INSTALL_LIB_DIR}",
-		"${INSTALL_MAN}",
-		"${INSTALL_MAN_DIR}",
-		"${INSTALL_PROGRAM}",
-		"${INSTALL_PROGRAM_DIR}",
-		"${INSTALL_SCRIPT}",
-		"${LIBTOOL}",
-		"${LN}",
-		"${PAX}",
-		"")
-}
-
 func (s *Suite) Test_ShellLineChecker_checkMultiLineComment(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go
index 39ae8dcc902..61b586bf202 100644
--- a/pkgtools/pkglint/files/util.go
+++ b/pkgtools/pkglint/files/util.go
@@ -88,6 +88,22 @@ func mapStr(slice []string, fn func(s string) string) []string {
 	return result
 }
 
+func invalidCharacters(s string, valid *textproc.ByteSet) string {
+	var unis strings.Builder
+
+	for _, r := range s {
+		if r == rune(byte(r)) && valid.Contains(byte(r)) {
+			continue
+		}
+		_, _ = fmt.Fprintf(&unis, " %U", r)
+	}
+
+	if unis.Len() == 0 {
+		return ""
+	}
+	return unis.String()[1:]
+}
+
 // intern returns an independent copy of the given string.
 //
 // It should be called when only a small substring of a large string
@@ -367,13 +383,19 @@ func detab(s string) string {
 	return detabbed.String()
 }
 
-// alignWith extends str with as many tabs as needed to reach
+// alignWith extends str with as many tabs and spaces as needed to reach
 // the same screen width as the other string.
 func alignWith(str, other string) string {
-	alignBefore := (tabWidth(other) + 7) & -8
-	alignAfter := tabWidth(str) & -8
-	tabsNeeded := imax((alignBefore-alignAfter)/8, 1)
-	return str + strings.Repeat("\t", tabsNeeded)
+	strWidth := tabWidth(str)
+	otherWidth := tabWidth(other)
+	if otherWidth <= strWidth {
+		return str + "\t"
+	}
+	if strWidth&-8 != otherWidth&-8 {
+		strWidth &= -8
+	}
+	alignment := indent(otherWidth - strWidth)
+	return str + alignment
 }
 
 func indent(width int) string {
@@ -462,8 +484,6 @@ func containsVarRef(s string) bool {
 	return false
 }
 
-func hasAlnumPrefix(s string) bool { return s != "" && textproc.AlnumU.Contains(s[0]) }
-
 // Once remembers with which arguments its FirstTime method has been called
 // and only returns true on each first call.
 type Once struct {
diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go
index eb2e1b4f460..4530a99aa88 100644
--- a/pkgtools/pkglint/files/util_test.go
+++ b/pkgtools/pkglint/files/util_test.go
@@ -227,9 +227,9 @@ func (s *Suite) Test_alignWith(c *check.C) {
 	// At least one tab is _always_ added.
 	test("", "", "\t")
 
-	test("VAR=", "1234567", "VAR=\t")
+	test("VAR=", "1234567", "VAR=   ")
 	test("VAR=", "12345678", "VAR=\t")
-	test("VAR=", "123456789", "VAR=\t\t")
+	test("VAR=", "123456789", "VAR=\t ")
 
 	// At least one tab is added in any case,
 	// even if the other string is shorter.
@@ -456,14 +456,6 @@ func (s *Suite) Test_containsVarRef(c *check.C) {
 	test("$$VAR", false)   // An escaped dollar character.
 }
 
-func (s *Suite) Test_hasAlnumPrefix(c *check.C) {
-	t := s.Init(c)
-
-	t.CheckEquals(hasAlnumPrefix(""), false)
-	t.CheckEquals(hasAlnumPrefix("A"), true)
-	t.CheckEquals(hasAlnumPrefix(","), false)
-}
-
 func (s *Suite) Test_Once(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go
index 16d92e57376..ef269e59b38 100644
--- a/pkgtools/pkglint/files/vartypecheck.go
+++ b/pkgtools/pkglint/files/vartypecheck.go
@@ -1327,8 +1327,22 @@ func (cv *VartypeCheck) URL() {
 }
 
 func (cv *VartypeCheck) UserGroupName() {
-	if cv.Value == cv.ValueNoVar && !matches(cv.Value, `^[0-9_a-z][0-9_a-z-]*[0-9_a-z]$`) {
-		cv.Warnf("Invalid user or group name %q.", cv.Value)
+	value := cv.Value
+	if value != cv.ValueNoVar {
+		return
+	}
+	invalid := invalidCharacters(value, textproc.NewByteSet("---0-9_a-z"))
+	if invalid != "" {
+		cv.Warnf("User or group name %q contains invalid characters: %s",
+			value, invalid)
+		return
+	}
+
+	if hasPrefix(value, "-") {
+		cv.Errorf("User or group name %q must not start with a hyphen.", value)
+	}
+	if hasSuffix(value, "-") {
+		cv.Errorf("User or group name %q must not end with a hyphen.", value)
 	}
 }
 
@@ -1473,7 +1487,7 @@ func (cv *VartypeCheck) Yes() {
 			"but using \".if defined(VARNAME)\" alone.")
 
 	default:
-		if !matches(cv.Value, `^(?:YES|yes)(?:[\t ]+#.*)?$`) {
+		if cv.Value != "YES" && cv.Value != "yes" {
 			cv.Warnf("%s should be set to YES or yes.", cv.Varname)
 			cv.Explain(
 				"This variable means \"yes\" if it is defined, and \"no\" if it is undefined.",
@@ -1511,7 +1525,7 @@ func (cv *VartypeCheck) YesNo() {
 			"both forms are actually used.",
 			"As long as this is the case, when checking the variable value,",
 			"both must be accepted.")
-	} else if !matches(cv.Value, `^(?:YES|yes|NO|no)(?:[\t ]+#.*)?$`) {
+	} else if !matches(cv.Value, `^(?:YES|yes|NO|no)$`) {
 		cv.Warnf("%s should be set to YES, yes, NO, or no.", cv.Varname)
 	}
 }
diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go
index 20d93b1649a..c34b7379f82 100644
--- a/pkgtools/pkglint/files/vartypecheck_test.go
+++ b/pkgtools/pkglint/files/vartypecheck_test.go
@@ -1875,11 +1875,20 @@ func (s *Suite) Test_VartypeCheck_UserGroupName(c *check.C) {
 		"typical_username",
 		"user123",
 		"domain\\user",
-		"${OTHER_VAR}")
+		"${OTHER_VAR}",
+		"r",
+		"-rf",
+		"rf-")
 
 	vt.Output(
-		"WARN: filename.mk:1: Invalid user or group name \"user with spaces\".",
-		"WARN: filename.mk:4: Invalid user or group name \"domain\\\\user\".")
+		"WARN: filename.mk:1: User or group name \"user with spaces\" "+
+			"contains invalid characters: U+0020 U+0020",
+		"WARN: filename.mk:4: User or group name \"domain\\\\user\" "+
+			"contains invalid characters: U+005C",
+		"ERROR: filename.mk:7: User or group name \"-rf\" "+
+			"must not start with a hyphen.",
+		"ERROR: filename.mk:8: User or group name \"rf-\" "+
+			"must not end with a hyphen.")
 }
 
 func (s *Suite) Test_VartypeCheck_VariableName(c *check.C) {
@@ -2053,6 +2062,14 @@ func (s *Suite) Test_VartypeCheck_Yes(c *check.C) {
 		"WARN: filename.mk:11: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.",
 		"WARN: filename.mk:12: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.",
 		"WARN: filename.mk:13: BUILD_USES_MSGFMT should only be used in a \".if defined(...)\" condition.")
+
+	vt.Op(opAssign)
+	vt.Values(
+		// This was accidentally accepted until 2019-12-09.
+		"yes \\# this is not a comment")
+
+	vt.Output(
+		"WARN: filename.mk:21: BUILD_USES_MSGFMT should be set to YES or yes.")
 }
 
 func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) {
@@ -2063,11 +2080,15 @@ func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) {
 		"yes",
 		"no",
 		"ja",
-		"${YESVAR}")
+		"${YESVAR}",
+		"yes # comment",
+		"no # comment",
+		"Yes indeed")
 
 	vt.Output(
 		"WARN: filename.mk:3: PKG_DEVELOPER should be set to YES, yes, NO, or no.",
-		"WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.")
+		"WARN: filename.mk:4: PKG_DEVELOPER should be set to YES, yes, NO, or no.",
+		"WARN: filename.mk:7: PKG_DEVELOPER should be set to YES, yes, NO, or no.")
 
 	vt.Op(opUseMatch)
 	vt.Values(
@@ -2086,6 +2107,14 @@ func (s *Suite) Test_VartypeCheck_YesNo(c *check.C) {
 			"\"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Yy]es\".",
 		"WARN: filename.mk:15: PKG_DEVELOPER should be matched against "+
 			"\"[yY][eE][sS]\" or \"[nN][oO]\", not \"[Nn]o\".")
+
+	vt.Op(opAssign)
+	vt.Values(
+		// This was accidentally accepted until 2019-12-09.
+		"yes \\# this is not a comment")
+
+	vt.Output(
+		"WARN: filename.mk:21: PKG_DEVELOPER should be set to YES, yes, NO, or no.")
 }
 
 func (s *Suite) Test_VartypeCheck_YesNoIndirectly(c *check.C) {