pkgtools/pkglint: update to 19.4.10

Changes since 19.4.9:

In continuation lines with long values, pkglint no longer suggests to
move the continuation backslash in the middle of the variable value, as
that would be impossible.

Warn when a shell command is assigned to a variable that only takes
pathnames. Shell commands can contain command line options, and these
are not pathnames.

The TOOLS_PLATFORM.tool variables are not defined on every platform.
When these variables are used outside an OPSYS check, a warning lists
the platforms where the tool is undefined or only defined conditionally.

22 files changed, 480 insertions, 142 deletions

diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile
index b09a3c9283e..965a4cf21a8 100644
--- a/pkgtools/pkglint/Makefile
+++ b/pkgtools/pkglint/Makefile
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.632 2020/02/17 20:22:21 rillig Exp $
+# $NetBSD: Makefile,v 1.633 2020/03/07 23:35:35 rillig Exp $
 
-PKGNAME=	pkglint-19.4.9
+PKGNAME=	pkglint-19.4.10
 CATEGORIES=	pkgtools
 DISTNAME=	tools
 MASTER_SITES=	${MASTER_SITE_GITHUB:=golang/}
diff --git a/pkgtools/pkglint/files/homepage_test.go b/pkgtools/pkglint/files/homepage_test.go
index bdb9e3cd063..49a6d90e491 100644
--- a/pkgtools/pkglint/files/homepage_test.go
+++ b/pkgtools/pkglint/files/homepage_test.go
@@ -64,8 +64,8 @@ func (s *Suite) Test_HomepageChecker_checkBasedOnMasterSites(c *check.C) {
 	vt.Output(
 		"WARN: filename.mk:11: HOMEPAGE should not be defined in terms of MASTER_SITEs.")
 
-	delete(pkg.vars.firstDef, "MASTER_SITES")
-	delete(pkg.vars.lastDef, "MASTER_SITES")
+	pkg.vars.v("MASTER_SITES").firstDef = nil
+	pkg.vars.v("MASTER_SITES").lastDef = nil
 	pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5,
 		"MASTER_SITES=\thttps://cdn.NetBSD.org/pub/pkgsrc/distfiles/"))
 
@@ -76,8 +76,8 @@ func (s *Suite) Test_HomepageChecker_checkBasedOnMasterSites(c *check.C) {
 		"WARN: filename.mk:21: HOMEPAGE should not be defined in terms of MASTER_SITEs. " +
 			"Use https://cdn.NetBSD.org/pub/pkgsrc/distfiles/ directly.")
 
-	delete(pkg.vars.firstDef, "MASTER_SITES")
-	delete(pkg.vars.lastDef, "MASTER_SITES")
+	pkg.vars.v("MASTER_SITES").firstDef = nil
+	pkg.vars.v("MASTER_SITES").lastDef = nil
 	pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5,
 		"MASTER_SITES=\t${MASTER_SITE_GITHUB}"))
 
@@ -89,8 +89,8 @@ func (s *Suite) Test_HomepageChecker_checkBasedOnMasterSites(c *check.C) {
 	vt.Output(
 		"WARN: filename.mk:31: HOMEPAGE should not be defined in terms of MASTER_SITEs.")
 
-	delete(pkg.vars.firstDef, "MASTER_SITES")
-	delete(pkg.vars.lastDef, "MASTER_SITES")
+	pkg.vars.v("MASTER_SITES").firstDef = nil
+	pkg.vars.v("MASTER_SITES").lastDef = nil
 	pkg.vars.Define("MASTER_SITES", t.NewMkLine(pkg.File("Makefile"), 5,
 		"MASTER_SITES=\t# none"))
 
@@ -393,7 +393,7 @@ func (s *Suite) Test_HomepageChecker_checkReachable(c *check.C) {
 		"https://no-such-name.example.org/",
 		// The "unknown network error" is for compatibility with Go < 1.13.
 		`^WARN: filename\.mk:1: Homepage "https://no-such-name.example.org/" `+
-			`cannot be checked: (name not found|unknown network error:.*)$`)
+			`cannot be checked: (name not found|timeout|unknown network error:.*)$`)
 
 	// Syntactically invalid URLs are silently skipped since VartypeCheck.URL
 	// already warns about them.
diff --git a/pkgtools/pkglint/files/logging.go b/pkgtools/pkglint/files/logging.go
index eeb194263a0..e0f3db1b792 100644
--- a/pkgtools/pkglint/files/logging.go
+++ b/pkgtools/pkglint/files/logging.go
@@ -368,7 +368,7 @@ func (l *Logger) ShowSummary(args []string) {
 		}
 
 		l.out.Write(sprintf("%s found.\n",
-			joinSkipEmptyCambridge("and",
+			joinCambridge("and",
 				num(l.errors, "error", "errors"),
 				num(l.warnings, "warning", "warnings"),
 				num(l.notes, "note", "notes"))))
diff --git a/pkgtools/pkglint/files/mkcondchecker.go b/pkgtools/pkglint/files/mkcondchecker.go
index 3bba635f716..ebe0915d5dd 100644
--- a/pkgtools/pkglint/files/mkcondchecker.go
+++ b/pkgtools/pkglint/files/mkcondchecker.go
@@ -29,6 +29,7 @@ func (ck *MkCondChecker) Check() {
 
 	checkVarUse := func(varuse *MkVarUse) {
 		var vartype *Vartype // TODO: Insert a better type guess here.
+		// See Test_MkVarUseChecker_checkAssignable__shell_command_in_exists.
 		vuc := VarUseContext{vartype, VucLoadTime, VucQuotPlain, false}
 		NewMkVarUseChecker(varuse, ck.MkLines, mkline).Check(&vuc)
 	}
diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go
index 312b24f71d6..2114dcfcdca 100644
--- a/pkgtools/pkglint/files/mkline_test.go
+++ b/pkgtools/pkglint/files/mkline_test.go
@@ -1034,6 +1034,10 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__shellword_part(c *check.C) {
 }
 
 // Tools, when used in a shell command, must not be quoted.
+// Shell commands may have command line arguments, pathnames must not.
+// The original intention of having both CONFIG_SHELL and CONFIG_SHELL_FLAGS
+// was to separate the command from its arguments.
+// It doesn't hurt though if the command includes some of the arguments as well.
 func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_shell_command(c *check.C) {
 	t := s.Init(c)
 
@@ -1043,11 +1047,14 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__tool_in_shell_command(c *check
 	mklines := t.SetUpFileMkLines("Makefile",
 		MkCvsID,
 		"",
-		"CONFIG_SHELL=\t${BASH}")
+		"CONFIG_SHELL=\t${BASH}",
+		"DIST_SUBDIR=\t${BASH}")
 
 	mklines.Check()
 
-	t.CheckOutputEmpty()
+	t.CheckOutputLines(
+		"WARN: ~/Makefile:4: Incompatible types: " +
+			"BASH (type \"ShellCommand\") cannot be assigned to type \"Pathname\".")
 }
 
 // This test provides code coverage for the "switch vuc.quoting" in the case
diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go
index 378b98b23e7..80e7679f950 100644
--- a/pkgtools/pkglint/files/mklines.go
+++ b/pkgtools/pkglint/files/mklines.go
@@ -194,9 +194,9 @@ func (mklines *MkLines) collectDocumentedVariables() {
 		// The commentLines include the the line containing the variable name,
 		// leaving 2 of these 3 lines for the actual documentation.
 		if commentLines >= 3 && relevant {
-			forEachStringMkLine(scope.used, func(varname string, mkline *MkLine) {
-				mklines.allVars.Define(varname, mkline)
-				mklines.allVars.Use(varname, mkline, VucRunTime)
+			scope.forEach(func(varname string, data *scopeVar) {
+				mklines.allVars.Define(varname, data.used)
+				mklines.allVars.Use(varname, data.used, VucRunTime)
 			})
 		}
 
diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go
index acdf1ec616a..cfc0d4040e1 100644
--- a/pkgtools/pkglint/files/mklines_test.go
+++ b/pkgtools/pkglint/files/mklines_test.go
@@ -494,7 +494,8 @@ func (s *Suite) Test_MkLines_collectUsedVariables__simple(c *check.C) {
 
 	mklines.collectUsedVariables()
 
-	t.CheckDeepEquals(mklines.allVars.used, map[string]*MkLine{"VAR": mkline})
+	t.Check(mklines.allVars.vs, check.HasLen, 1)
+	t.CheckEquals(mklines.allVars.v("VAR").used, mkline)
 	t.CheckEquals(mklines.allVars.FirstUse("VAR"), mkline)
 }
 
@@ -513,7 +514,7 @@ func (s *Suite) Test_MkLines_collectUsedVariables__nested(c *check.C) {
 
 	mklines.collectUsedVariables()
 
-	t.CheckEquals(len(mklines.allVars.used), 5)
+	t.CheckEquals(len(mklines.allVars.vs), 5)
 	t.CheckEquals(mklines.allVars.FirstUse("lparam"), assignMkline)
 	t.CheckEquals(mklines.allVars.FirstUse("rparam"), assignMkline)
 	t.CheckEquals(mklines.allVars.FirstUse("inner"), shellMkline)
@@ -570,9 +571,11 @@ func (s *Suite) Test_MkLines_collectDocumentedVariables(c *check.C) {
 	mklines.collectDocumentedVariables()
 
 	var varnames []string
-	for varname, mkline := range mklines.allVars.used {
-		varnames = append(varnames, sprintf("%s (line %s)", varname, mkline.Linenos()))
-	}
+	mklines.allVars.forEach(func(varname string, data *scopeVar) {
+		if data.used != nil {
+			varnames = append(varnames, sprintf("%s (line %s)", varname, data.used.Linenos()))
+		}
+	})
 	sort.Strings(varnames)
 
 	expected := []string{
@@ -694,7 +697,7 @@ func (s *Suite) Test_MkLines_collectVariables__find_files_and_headers(c *check.C
 	mklines.Check()
 
 	t.CheckDeepEquals(
-		keys(mklines.allVars.firstDef),
+		keys(mklines.allVars.vs),
 		[]string{
 			"BUILTIN_FIND_FILES_VAR",
 			"BUILTIN_FIND_HEADERS_VAR",
diff --git a/pkgtools/pkglint/files/mkvarusechecker.go b/pkgtools/pkglint/files/mkvarusechecker.go
index 8538c6003a8..01964291922 100644
--- a/pkgtools/pkglint/files/mkvarusechecker.go
+++ b/pkgtools/pkglint/files/mkvarusechecker.go
@@ -27,8 +27,10 @@ func (ck *MkVarUseChecker) Check(vuc *VarUseContext) {
 
 	ck.checkVarname(vuc.time)
 	ck.checkModifiers()
+	ck.checkAssignable(vuc)
 	ck.checkQuoting(vuc)
 
+	ck.checkToolsPlatform()
 	ck.checkBuildDefs()
 	ck.checkDeprecated()
 
@@ -445,6 +447,35 @@ func (ck *MkVarUseChecker) warnToolLoadTime(varname string, tool *Tool) {
 		"except in the package Makefile itself.")
 }
 
+func (ck *MkVarUseChecker) checkAssignable(vuc *VarUseContext) {
+	leftType := vuc.vartype
+	if leftType == nil || leftType.basicType != BtPathname {
+		return
+	}
+	rightType := G.Pkgsrc.VariableType(ck.MkLines, ck.use.varname)
+	if rightType == nil || rightType.basicType != BtShellCommand {
+		return
+	}
+
+	mkline := ck.MkLine
+	if mkline.Varcanon() == "PKG_SHELL.*" {
+		switch ck.use.varname {
+		case "SH", "BASH", "TOOLS_PLATFORM.sh":
+			return
+		}
+	}
+
+	mkline.Warnf(
+		"Incompatible types: %s (type %q) cannot be assigned to type %q.",
+		ck.use.varname, rightType.basicType.name, leftType.basicType.name)
+	mkline.Explain(
+		"Shell commands often start with a pathname.",
+		"They could also start with a list of environment variable",
+		"definitions, since that is accepted by the shell.",
+		"They can also contain addition command line arguments",
+		"that are not filenames at all.")
+}
+
 // checkVarUseWords checks whether a variable use of the form ${VAR}
 // or ${VAR:modifiers} is allowed in a certain context.
 func (ck *MkVarUseChecker) checkQuoting(vuc *VarUseContext) {
@@ -638,6 +669,40 @@ func (ck *MkVarUseChecker) warnRedundantModifierQ(mod string) {
 	fix.Apply()
 }
 
+func (ck *MkVarUseChecker) checkToolsPlatform() {
+	if ck.MkLine.IsDirective() {
+		return
+	}
+
+	varname := ck.use.varname
+	if varnameCanon(varname) != "TOOLS_PLATFORM.*" {
+		return
+	}
+
+	indentation := ck.MkLines.indentation
+	switch {
+	case indentation.DependsOn("OPSYS"),
+		indentation.DependsOn("MACHINE_PLATFORM"),
+		indentation.DependsOn(varname):
+		// TODO: Only return if the conditional is on the correct OPSYS.
+		return
+	}
+
+	toolName := varnameParam(varname)
+	tool := G.Pkgsrc.Tools.ByName(toolName)
+	if tool == nil {
+		return
+	}
+
+	if len(tool.undefinedOn) > 0 {
+		ck.MkLine.Warnf("%s is undefined on %s.",
+			varname, joinCambridge("and", tool.undefinedOn...))
+	} else if len(tool.conditionalOn) > 0 {
+		ck.MkLine.Warnf("%s may be undefined on %s.",
+			varname, joinCambridge("and", tool.conditionalOn...))
+	}
+}
+
 func (ck *MkVarUseChecker) checkBuildDefs() {
 	varname := ck.use.varname
 
diff --git a/pkgtools/pkglint/files/mkvarusechecker_test.go b/pkgtools/pkglint/files/mkvarusechecker_test.go
index 85483bd5b12..c2638fd11f0 100644
--- a/pkgtools/pkglint/files/mkvarusechecker_test.go
+++ b/pkgtools/pkglint/files/mkvarusechecker_test.go
@@ -957,6 +957,77 @@ func (s *Suite) Test_MkVarUseChecker_warnToolLoadTime__local_tool(c *check.C) {
 		"WARN: ~/category/package/Makefile:7: The tool ${MK_TOOL} cannot be used at load time.")
 }
 
+func (s *Suite) Test_MkVarUseChecker_checkAssignable(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+	mklines := t.NewMkLines("filename.mk",
+		"BUILTIN_FIND_FILES_VAR:=\tBIN_FILE",
+		"BUILTIN_FIND_FILES.BIN_FILE=\t${TOOLS_PLATFORM.file} /bin/file /usr/bin/file")
+
+	mklines.ForEach(func(mkline *MkLine) {
+		ck := NewMkAssignChecker(mkline, mklines)
+		ck.checkVarassignRight()
+	})
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:2: Incompatible types: " +
+			"TOOLS_PLATFORM.file (type \"ShellCommand\") " +
+			"cannot be assigned to type \"Pathname\".")
+}
+
+// NetBSD's chsh program only allows a simple pathname for the shell, without
+// any command line arguments. This makes sense since the shell is started
+// using execve, not system (which would require shell-like argument parsing).
+//
+// Under the assumption that TOOLS_PLATFORM.sh does not contain any command
+// line arguments, it's ok in that special case. This covers most of the
+// real-life situations where this type mismatch (Pathname := ShellCommand)
+// occurs.
+func (s *Suite) Test_MkVarUseChecker_checkAssignable__shell_command_to_pathname(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+	t.SetUpTool("sh", "SH", AtRunTime)
+	t.SetUpTool("bash", "BASH", AtRunTime)
+	mklines := t.NewMkLines("filename.mk",
+		"PKG_SHELL.user=\t${TOOLS_PLATFORM.sh}",
+		"PKG_SHELL.user=\t${SH}",
+		"PKG_SHELL.user=\t${BASH}")
+
+	mklines.ForEach(func(mkline *MkLine) {
+		ck := NewMkAssignChecker(mkline, mklines)
+		ck.checkVarassignRight()
+	})
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:1: Please use ${TOOLS_PLATFORM.sh:Q} " +
+			"instead of ${TOOLS_PLATFORM.sh}.")
+}
+
+func (s *Suite) Test_MkVarUseChecker_checkAssignable__shell_command_in_exists(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpTool("sh", "SH", AfterPrefsMk)
+	t.SetUpTool("bash", "BASH", AfterPrefsMk)
+	t.SetUpPkgsrc()
+	t.Chdir(".")
+	t.FinishSetUp()
+	mklines := t.NewMkLines("filename.mk",
+		MkCvsID,
+		".include \"mk/bsd.prefs.mk\"",
+		".if exists(${TOOLS_PLATFORM.sh})",
+		".elif exists(${SH})",
+		".elif exists(${BASH})",
+		".endif")
+
+	mklines.Check()
+
+	// TODO: Call MkVarUseChecker.checkAssignable with a VarUseContext of type
+	//  BtPathname here.
+	t.CheckOutputEmpty()
+}
+
 func (s *Suite) Test_MkVarUseChecker_checkQuoting(c *check.C) {
 	t := s.Init(c)
 
@@ -1153,6 +1224,65 @@ func (s *Suite) Test_MkVarUseChecker_fixQuotingModifiers(c *check.C) {
 		"AUTOFIX: ~/filename.mk:5: Replacing \"${CFLAGS:N*:Q}\" with \"${CFLAGS:N*:M*:Q}\".")
 }
 
+func (s *Suite) Test_MkVarUseChecker_checkToolsPlatform(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpPkgsrc()
+	t.SetUpTool("available", "", AfterPrefsMk)
+	t.SetUpTool("cond1", "", AfterPrefsMk)
+	t.SetUpTool("cond2", "", AfterPrefsMk)
+	t.SetUpTool("undefined", "", AfterPrefsMk)
+	t.CreateFileLines("mk/tools/tools.NetBSD.mk",
+		"TOOLS_PLATFORM.available?=\t/bin/available",
+		"TOOLS_PLATFORM.cond1?=\t/usr/cond1",
+		"TOOLS_PLATFORM.cond2?=\t/usr/cond2",
+		"TOOLS_PLATFORM.undefined?=\t/usr/undefined")
+	t.CreateFileLines("mk/tools/tools.SunOS.mk",
+		"TOOLS_PLATFORM.available?=\t/bin/available",
+		"",
+		".if exists(/usr/gnu/bin/cond1)",
+		"TOOLS_PLATFORM.cond1?=\t/usr/gnu/bin/cond1",
+		".endif",
+		"",
+		".if exists(/usr/gnu/bin/cond2)",
+		"TOOLS_PLATFORM.cond2?=\t/usr/gnu/bin/cond2",
+		".else",
+		"TOOLS_PLATFORM.cond2?=\t/usr/sfw/bin/cond2",
+		".endif",
+		"",
+		"# No definition for undefined.")
+	t.Chdir(".")
+	t.FinishSetUp()
+	mklines := t.NewMkLines("filename.mk",
+		MkCvsID,
+		"",
+		".include \"mk/bsd.prefs.mk\"",
+		"",
+		".if ${OPSYS} == SunOS",
+		"post-build:",
+		"\t${TOOLS_PLATFORM.available}",
+		"\t${TOOLS_PLATFORM.cond1}",
+		"\t${TOOLS_PLATFORM.cond2}",
+		"\t${TOOLS_PLATFORM.undefined}",
+		".endif",
+		"",
+		"do-build:",
+		"\t${TOOLS_PLATFORM.available}",
+		"\t${TOOLS_PLATFORM.cond1}",
+		"\t${TOOLS_PLATFORM.cond2}",
+		"\t${TOOLS_PLATFORM.undefined}",
+		"",
+		".if defined(TOOLS_PLATFORM.undefined)",
+		"\t${TOOLS_PLATFORM.undefined}",
+		".endif")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"WARN: filename.mk:15: TOOLS_PLATFORM.cond1 may be undefined on SunOS.",
+		"WARN: filename.mk:17: TOOLS_PLATFORM.undefined is undefined on SunOS.")
+}
+
 func (s *Suite) Test_MkVarUseChecker_checkBuildDefs(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go
index 370adf6345c..5014cffd7e8 100644
--- a/pkgtools/pkglint/files/package.go
+++ b/pkgtools/pkglint/files/package.go
@@ -135,7 +135,7 @@ func (pkg *Package) load() ([]CurrPath, *MkLines, *MkLines) {
 		files = append(files, pkg.File(pkg.Pkgdir).ReadPaths()...)
 	}
 	files = append(files, pkg.File(pkg.Patchdir).ReadPaths()...)
-	if pkg.DistinfoFile != NewPackagePathString(pkg.vars.fallback["DISTINFO_FILE"]) {
+	if pkg.DistinfoFile != NewPackagePathString(pkg.vars.v("DISTINFO_FILE").fallback) {
 		files = append(files, pkg.File(pkg.DistinfoFile))
 	}
 
diff --git a/pkgtools/pkglint/files/pkglint.go b/pkgtools/pkglint/files/pkglint.go
index a205945c84f..579e50498f2 100644
--- a/pkgtools/pkglint/files/pkglint.go
+++ b/pkgtools/pkglint/files/pkglint.go
@@ -384,13 +384,13 @@ func resolveVariableRefs(text string, mklines *MkLines, pkg *Package) string {
 
 			if mklines != nil {
 				// TODO: At load time, use mklines.loadVars instead.
-				if value, ok := mklines.allVars.LastValueFound(varname); ok {
+				if value, found, indeterminate := mklines.allVars.LastValueFound(varname); found && !indeterminate {
 					return value
 				}
 			}
 
 			if pkg != nil {
-				if value, ok := pkg.vars.LastValueFound(varname); ok {
+				if value, found, indeterminate := pkg.vars.LastValueFound(varname); found && !indeterminate {
 					return value
 				}
 			}
diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go
index 300d7d755ba..e176fe7f190 100644
--- a/pkgtools/pkglint/files/pkgsrc.go
+++ b/pkgtools/pkglint/files/pkgsrc.go
@@ -465,11 +465,84 @@ func (src *Pkgsrc) loadTools() {
 		})
 	}
 
+	src.loadToolsPlatform()
+
 	if trace.Tracing {
 		tools.Trace()
 	}
 }
 
+func (src *Pkgsrc) loadToolsPlatform() {
+	var systems []string
+	scopes := make(map[string]*RedundantScope)
+	for _, mkFile := range src.File("mk/tools").ReadPaths() {
+		m, opsys := match1(mkFile.Base(), `^tools\.(.+)\.mk$`)
+		if !m {
+			continue
+		}
+		systems = append(systems, opsys)
+
+		mklines := LoadMk(mkFile, nil, MustSucceed)
+		scope := NewRedundantScope()
+		// Suppress any warnings, just compute the variable state.
+		scope.IsRelevant = func(*MkLine) bool { return false }
+		scope.Check(mklines)
+		scopes[opsys] = scope
+	}
+
+	// 0 = undefined, 1 = conditional, 2 = definitely assigned
+	type status int
+	statusByNameAndOpsys := make(map[string]map[string]status)
+
+	for opsys, scope := range scopes {
+		for varname, varinfo := range scope.vars {
+			if varnameCanon(varname) == "TOOLS_PLATFORM.*" {
+				var s status
+				if varinfo.vari.IsConditional() {
+					if len(varinfo.vari.WriteLocations()) == 1 {
+						s = 1
+					} else {
+						// TODO: Don't just count the number of assignments,
+						//  check whether they definitely assign the variable.
+						//  See substScope.
+						s = 2
+					}
+				} else if varinfo.vari.IsConstant() {
+					s = 2
+				} else {
+					continue
+				}
+
+				name := varnameParam(varname)
+				if statusByNameAndOpsys[name] == nil {
+					statusByNameAndOpsys[name] = make(map[string]status)
+				}
+				statusByNameAndOpsys[name][opsys] = s
+			}
+		}
+	}
+
+	for name, tool := range src.Tools.byName {
+		undefined := make(map[string]bool)
+		conditional := make(map[string]bool)
+		for _, opsys := range systems {
+			undefined[opsys] = true
+			conditional[opsys] = true
+		}
+		for opsys, status := range statusByNameAndOpsys[name] {
+			switch status {
+			case 1:
+				delete(undefined, opsys)
+			case 2:
+				delete(undefined, opsys)
+				delete(conditional, opsys)
+			}
+		}
+		tool.undefinedOn = keysSorted(undefined)
+		tool.conditionalOn = keysSorted(conditional)
+	}
+}
+
 func (src *Pkgsrc) addBuildDefs(varnames ...string) {
 	for _, varname := range varnames {
 		src.buildDefs[varname] = true
@@ -680,11 +753,16 @@ func (src *Pkgsrc) loadUntypedVars() {
 		mklines := LoadMk(path, nil, MustSucceed)
 		mklines.collectVariables()
 		mklines.collectUsedVariables()
-		def := func(varname string, mkline *MkLine) {
-			define(varnameCanon(varname), mkline)
-		}
-		forEachStringMkLine(mklines.allVars.firstDef, def)
-		forEachStringMkLine(mklines.allVars.used, def)
+		mklines.allVars.forEach(func(varname string, data *scopeVar) {
+			if data.firstDef != nil {
+				define(varnameCanon(varname), data.firstDef)
+			}
+		})
+		mklines.allVars.forEach(func(varname string, data *scopeVar) {
+			if data.used != nil {
+				define(varnameCanon(varname), data.used)
+			}
+		})
 	}
 
 	handleFile := func(pathName string, info os.FileInfo, err error) error {
diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go
index 41b305f2d8e..99c9f4449a0 100644
--- a/pkgtools/pkglint/files/shell.go
+++ b/pkgtools/pkglint/files/shell.go
@@ -168,7 +168,8 @@ func (scc *SimpleCommandChecker) handleCommandVariable() bool {
 
 	varname := varuse.varname
 
-	if vartype := G.Pkgsrc.VariableType(scc.mklines, varname); vartype != nil && vartype.basicType.name == "ShellCommand" {
+	vartype := G.Pkgsrc.VariableType(scc.mklines, varname)
+	if vartype != nil && (vartype.basicType == BtShellCommand || vartype.basicType == BtPathname) {
 		scc.checkInstallCommand(shellword)
 		return true
 	}
diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go
index 720505b1fbd..1664ed71b9c 100644
--- a/pkgtools/pkglint/files/shell_test.go
+++ b/pkgtools/pkglint/files/shell_test.go
@@ -45,6 +45,20 @@ func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__unknown_default(c *
 		"WARN: Makefile:8: UNKNOWN_TOOL is used but not defined.")
 }
 
+// Despite its name, the TOOLS_PATH.* name the whole shell command,
+// not just the path of its executable.
+func (s *Suite) Test_SimpleCommandChecker_checkCommandStart__TOOLS_PATH(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpPackage("category/package",
+		"CONFIG_SHELL=\t${TOOLS_PATH.bash}")
+	t.Chdir("category/package")
+	t.FinishSetUp()
+	G.checkdirPackage(".")
+
+	t.CheckOutputEmpty()
+}
+
 func (s *Suite) Test_SimpleCommandChecker_checkInstallCommand(c *check.C) {
 	t := s.Init(c)
 
@@ -841,6 +855,7 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) {
 	t := s.Init(c)
 
 	t.SetUpVartypes()
+	t.SetUpTool("[", "TEST", AtRunTime)
 	t.SetUpTool("awk", "AWK", AtRunTime)
 	t.SetUpTool("cp", "CP", AtRunTime)
 	t.SetUpTool("echo", "", AtRunTime)
@@ -927,16 +942,13 @@ func (s *Suite) Test_ShellLineChecker_CheckShellCommandLine(c *check.C) {
 		"WARN: filename.mk:1: The exitcode of \"${UNZIP_CMD}\" at the left of the | operator is ignored.")
 
 	// From x11/wxGTK28/Makefile
-	// TODO: Why is TOOLS_PATH.msgfmt not recognized?
-	//  At least, the warning should be more specific, mentioning USE_TOOLS.
 	test(""+
 		"set -e; cd ${WRKSRC}/locale; "+
 		"for lang in *.po; do "+
 		"  [ \"$${lang}\" = \"wxstd.po\" ] && continue; "+
 		"  ${TOOLS_PATH.msgfmt} -c -o \"$${lang%.po}.mo\" \"$${lang}\"; "+
 		"done",
-		"WARN: filename.mk:1: Unknown shell command \"[\".",
-		"WARN: filename.mk:1: Unknown shell command \"${TOOLS_PATH.msgfmt}\".")
+		nil...)
 
 	test("@cp from to",
 		"WARN: filename.mk:1: The shell command \"cp\" should not be hidden.")
diff --git a/pkgtools/pkglint/files/tools.go b/pkgtools/pkglint/files/tools.go
index 7036716e47a..5ec09453f09 100644
--- a/pkgtools/pkglint/files/tools.go
+++ b/pkgtools/pkglint/files/tools.go
@@ -33,6 +33,15 @@ type Tool struct {
 	MustUseVarForm bool
 	Validity       Validity
 	Aliases        []string
+
+	// The operating systems on which the tool is defined conditionally,
+	// usually by enclosing the tool definition in an ".if exists".
+	// See mk/tools/tools.*.mk.
+	conditionalOn []string
+
+	// The operating systems on which the tool is not defined at all.
+	// See mk/tools/tools.*.mk.
+	undefinedOn []string
 }
 
 func (tool *Tool) String() string {
@@ -152,7 +161,7 @@ func (tr *Tools) Define(name, varname string, mkline *MkLine) *Tool {
 func (tr *Tools) def(name, varname string, mustUseVarForm bool, validity Validity, aliases []string) *Tool {
 	assert(tr.IsValidToolName(name))
 
-	fresh := Tool{name, varname, mustUseVarForm, validity, aliases}
+	fresh := Tool{name, varname, mustUseVarForm, validity, aliases, nil, nil}
 
 	tool := tr.byName[name]
 	if tool == nil {
diff --git a/pkgtools/pkglint/files/tools_test.go b/pkgtools/pkglint/files/tools_test.go
index 8383c0d5d28..6c454462596 100644
--- a/pkgtools/pkglint/files/tools_test.go
+++ b/pkgtools/pkglint/files/tools_test.go
@@ -5,15 +5,15 @@ import "gopkg.in/check.v1"
 func (s *Suite) Test_Tool_UsableAtLoadTime(c *check.C) {
 	t := s.Init(c)
 
-	nowhere := Tool{"nowhere", "", false, Nowhere, nil}
+	nowhere := Tool{"nowhere", "", false, Nowhere, nil, nil, nil}
 	t.CheckEquals(nowhere.UsableAtLoadTime(false), false)
 	t.CheckEquals(nowhere.UsableAtLoadTime(true), false)
 
-	load := Tool{"load", "", false, AfterPrefsMk, nil}
+	load := Tool{"load", "", false, AfterPrefsMk, nil, nil, nil}
 	t.CheckEquals(load.UsableAtLoadTime(false), false)
 	t.CheckEquals(load.UsableAtLoadTime(true), true)
 
-	run := Tool{"run", "", false, AtRunTime, nil}
+	run := Tool{"run", "", false, AtRunTime, nil, nil, nil}
 	t.CheckEquals(run.UsableAtLoadTime(false), false)
 	t.CheckEquals(run.UsableAtLoadTime(true), false)
 }
@@ -52,13 +52,13 @@ func (s *Suite) Test_Tool_UsableAtLoadTime__pkgconfig_builtin_mk(c *check.C) {
 func (s *Suite) Test_Tool_UsableAtRunTime(c *check.C) {
 	t := s.Init(c)
 
-	nowhere := Tool{"nowhere", "", false, Nowhere, nil}
+	nowhere := Tool{"nowhere", "", false, Nowhere, nil, nil, nil}
 	t.CheckEquals(nowhere.UsableAtRunTime(), false)
 
-	load := Tool{"load", "", false, AfterPrefsMk, nil}
+	load := Tool{"load", "", false, AfterPrefsMk, nil, nil, nil}
 	t.CheckEquals(load.UsableAtRunTime(), true)
 
-	run := Tool{"run", "", false, AtRunTime, nil}
+	run := Tool{"run", "", false, AtRunTime, nil, nil, nil}
 	t.CheckEquals(run.UsableAtRunTime(), true)
 }
 
diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go
index f76f358f69c..e12ad7964e9 100644
--- a/pkgtools/pkglint/files/util.go
+++ b/pkgtools/pkglint/files/util.go
@@ -210,12 +210,16 @@ func condInt(cond bool, trueValue, falseValue int) int {
 }
 
 func keysJoined(m map[string]bool) string {
+	return strings.Join(keysSorted(m), " ")
+}
+
+func keysSorted(m map[string]bool) []string {
 	var keys []string
 	for key := range m {
 		keys = append(keys, key)
 	}
 	sort.Strings(keys)
-	return strings.Join(keys, " ")
+	return keys
 }
 
 func copyStringMkLine(m map[string]*MkLine) map[string]*MkLine {
@@ -617,22 +621,30 @@ func (o *Once) check(key uint64) bool {
 //
 // See also RedundantScope.
 type Scope struct {
-	firstDef       map[string]*MkLine // TODO: Can this be removed?
-	lastDef        map[string]*MkLine
-	value          map[string]string
-	used           map[string]*MkLine
-	usedAtLoadTime map[string]bool
-	fallback       map[string]string
+	vs map[string]*scopeVar
+}
+
+type scopeVar struct {
+	firstDef       *MkLine
+	lastDef        *MkLine
+	value          string
+	used           *MkLine
+	fallback       string
+	usedAtLoadTime bool
+	indeterminate  bool
 }
 
 func NewScope() Scope {
-	return Scope{
-		make(map[string]*MkLine),
-		make(map[string]*MkLine),
-		make(map[string]string),
-		make(map[string]*MkLine),
-		make(map[string]bool),
-		make(map[string]string)}
+	return Scope{make(map[string]*scopeVar)}
+}
+
+func (s *Scope) v(varname string) *scopeVar {
+	if v, found := s.vs[varname]; found {
+		return v
+	}
+	var sv scopeVar
+	s.vs[varname] = &sv
+	return &sv
 }
 
 // Define marks the variable and its canonicalized form as defined.
@@ -645,8 +657,9 @@ func (s *Scope) Define(varname string, mkline *MkLine) {
 }
 
 func (s *Scope) def(name string, mkline *MkLine) {
-	if s.firstDef[name] == nil {
-		s.firstDef[name] = mkline
+	v := s.v(name)
+	if v.firstDef == nil {
+		v.firstDef = mkline
 		if trace.Tracing {
 			trace.Step2("Defining %q for the first time in %s", name, mkline.String())
 		}
@@ -654,7 +667,7 @@ func (s *Scope) def(name string, mkline *MkLine) {
 		trace.Step2("Defining %q in %s", name, mkline.String())
 	}
 
-	s.lastDef[name] = mkline
+	v.lastDef = mkline
 
 	// In most cases the defining lines are indeed variable assignments.
 	// Exceptions are comments from documentation sections, which still mark
@@ -669,35 +682,37 @@ func (s *Scope) def(name string, mkline *MkLine) {
 		value := mkline.Value()
 		if trace.Tracing {
 			trace.Stepf("Scope.Define.append %s: %s = %q + %q",
-				mkline.String(), name, s.value[name], value)
+				mkline.String(), name, v.value, value)
 		}
-		s.value[name] += " " + value
+		v.value += " " + value
 	case opAssignDefault:
-		if _, set := s.value[name]; !set {
-			s.value[name] = mkline.Value()
+		if v.value == "" && !v.indeterminate {
+			v.value = mkline.Value()
 		}
 	case opAssignShell:
-		delete(s.value, name)
+		v.value = ""
+		v.indeterminate = true
 	default:
-		s.value[name] = mkline.Value()
+		v.value = mkline.Value()
 	}
 }
 
 func (s *Scope) Fallback(varname string, value string) {
-	s.fallback[varname] = value
+	s.v(varname).fallback = value
 }
 
 // Use marks the variable and its canonicalized form as used.
 func (s *Scope) Use(varname string, line *MkLine, time VucTime) {
 	use := func(name string) {
-		if s.used[name] == nil {
-			s.used[name] = line
+		v := s.v(name)
+		if v.used == nil {
+			v.used = line
 			if trace.Tracing {
 				trace.Step2("Using %q in %s", name, line.String())
 			}
 		}
 		if time == VucLoadTime {
-			s.usedAtLoadTime[name] = true
+			v.usedAtLoadTime = true
 		}
 	}
 
@@ -710,7 +725,7 @@ func (s *Scope) Use(varname string, line *MkLine, time VucTime) {
 //  - mentioned in a commented variable assignment,
 //  - mentioned in a documentation comment.
 func (s *Scope) Mentioned(varname string) *MkLine {
-	return s.firstDef[varname]
+	return s.v(varname).firstDef
 }
 
 // IsDefined tests whether the variable is defined.
@@ -719,7 +734,7 @@ func (s *Scope) Mentioned(varname string) *MkLine {
 // Even if IsDefined returns true, FirstDefinition doesn't necessarily return true
 // since the latter ignores the default definitions from vardefs.go, keyword dummyVardefMkline.
 func (s *Scope) IsDefined(varname string) bool {
-	mkline := s.firstDef[varname]
+	mkline := s.v(varname).firstDef
 	return mkline != nil && mkline.IsVarassign()
 }
 
@@ -745,21 +760,21 @@ func (s *Scope) IsDefinedSimilar(varname string) bool {
 // IsUsed tests whether the variable is used.
 // It does NOT test the canonicalized variable name.
 func (s *Scope) IsUsed(varname string) bool {
-	return s.used[varname] != nil
+	return s.v(varname).used != nil
 }
 
 // IsUsedSimilar tests whether the variable or its canonicalized form is used.
 func (s *Scope) IsUsedSimilar(varname string) bool {
-	if s.used[varname] != nil {
+	if s.v(varname).used != nil {
 		return true
 	}
-	return s.used[varnameCanon(varname)] != nil
+	return s.v(varnameCanon(varname)).used != nil
 }
 
 // IsUsedAtLoadTime returns true if the variable is used at load time
 // somewhere.
 func (s *Scope) IsUsedAtLoadTime(varname string) bool {
-	return s.usedAtLoadTime[varname]
+	return s.v(varname).usedAtLoadTime
 }
 
 // FirstDefinition returns the line in which the variable has been defined first.
@@ -771,7 +786,7 @@ func (s *Scope) IsUsedAtLoadTime(varname string) bool {
 // round: the including file sets a value first, and the included file then
 // assigns a default value using ?=.
 func (s *Scope) FirstDefinition(varname string) *MkLine {
-	mkline := s.firstDef[varname]
+	mkline := s.v(varname).firstDef
 	if mkline != nil && mkline.IsVarassign() {
 		lastLine := s.LastDefinition(varname)
 		if trace.Tracing && lastLine != mkline {
@@ -792,7 +807,7 @@ func (s *Scope) FirstDefinition(varname string) *MkLine {
 // round: the including file sets a value first, and the included file then
 // assigns a default value using ?=.
 func (s *Scope) LastDefinition(varname string) *MkLine {
-	mkline := s.lastDef[varname]
+	mkline := s.v(varname).lastDef
 	if mkline != nil && mkline.IsVarassign() {
 		return mkline
 	}
@@ -804,10 +819,10 @@ func (s *Scope) LastDefinition(varname string) *MkLine {
 // mk/defaults/mk.conf for documentation.
 func (s *Scope) Commented(varname string) *MkLine {
 	var mklines []*MkLine
-	if first := s.firstDef[varname]; first != nil {
+	if first := s.v(varname).firstDef; first != nil {
 		mklines = append(mklines, first)
 	}
-	if last := s.lastDef[varname]; last != nil {
+	if last := s.v(varname).lastDef; last != nil {
 		mklines = append(mklines, last)
 	}
 
@@ -827,7 +842,7 @@ func (s *Scope) Commented(varname string) *MkLine {
 }
 
 func (s *Scope) FirstUse(varname string) *MkLine {
-	return s.used[varname]
+	return s.v(varname).used
 }
 
 // LastValue returns the value from the last variable definition.
@@ -837,28 +852,50 @@ func (s *Scope) FirstUse(varname string) *MkLine {
 // was not found, or that the variable value cannot be determined
 // reliably. To distinguish these cases, call LastValueFound instead.
 func (s *Scope) LastValue(varname string) string {
-	value, _ := s.LastValueFound(varname)
+	value, _, _ := s.LastValueFound(varname)
 	return value
 }
 
-func (s *Scope) LastValueFound(varname string) (value string, found bool) {
-	value, found = s.value[varname]
-	if !found {
-		value, found = s.fallback[varname]
+func (s *Scope) LastValueFound(varname string) (value string, found bool, indeterminate bool) {
+	v := s.vs[varname]
+	if v == nil {
+		return
 	}
-	return
+
+	value = v.value
+	found = v.firstDef != nil && v.firstDef.IsVarassign()
+	indeterminate = v.indeterminate
+	if found {
+		return
+	}
+
+	return v.fallback, v.fallback != "", v.indeterminate
 }
 
 func (s *Scope) DefineAll(other Scope) {
 	var varnames []string
-	for varname := range other.firstDef {
+	for varname := range other.vs {
 		varnames = append(varnames, varname)
 	}
 	sort.Strings(varnames)
 
 	for _, varname := range varnames {
-		s.Define(varname, other.firstDef[varname])
-		s.Define(varname, other.lastDef[varname])
+		v := other.vs[varname]
+		if v.firstDef != nil {
+			s.Define(varname, v.firstDef)
+			s.Define(varname, v.lastDef)
+		}
+	}
+}
+
+func (s *Scope) forEach(action func(varname string, data *scopeVar)) {
+	var keys []string
+	for key := range s.vs {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	for _, key := range keys {
+		action(key, s.vs[key])
 	}
 }
 
@@ -1183,32 +1220,32 @@ func joinSkipEmpty(sep string, elements ...string) string {
 	return strings.Join(nonempty, sep)
 }
 
-func joinSkipEmptyCambridge(conn string, elements ...string) string {
-	var nonempty []string
+// joinCambridge returns "first, second conn third".
+// It is used when each element is a single word.
+// Empty elements are ignored completely.
+func joinCambridge(conn string, elements ...string) string {
+	parts := make([]string, 0, 2+2*len(elements))
 	for _, element := range elements {
 		if element != "" {
-			nonempty = append(nonempty, element)
+			parts = append(parts, ", ", element)
 		}
 	}
 
-	var sb strings.Builder
-	for i, element := range nonempty {
-		if i > 0 {
-			if i == len(nonempty)-1 {
-				sb.WriteRune(' ')
-				sb.WriteString(conn)
-				sb.WriteRune(' ')
-			} else {
-				sb.WriteString(", ")
-			}
-		}
-		sb.WriteString(element)
+	if len(parts) == 0 {
+		return ""
+	}
+	if len(parts) < 4 {
+		return parts[1]
 	}
 
-	return sb.String()
+	parts = append(parts[1:len(parts)-2], " ", conn, " ", parts[len(parts)-1])
+	return strings.Join(parts, "")
 }
 
-func joinSkipEmptyOxford(conn string, elements ...string) string {
+// joinOxford returns "first, second, conn third".
+// It is used when each element may consist of multiple words.
+// Empty elements are ignored completely.
+func joinOxford(conn string, elements ...string) string {
 	var nonempty []string
 	for _, element := range elements {
 		if element != "" {
diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go
index e25a2685fde..b8f9358c027 100644
--- a/pkgtools/pkglint/files/util_test.go
+++ b/pkgtools/pkglint/files/util_test.go
@@ -543,9 +543,10 @@ func (s *Suite) Test_Scope__commented_varassign(c *check.C) {
 	t.CheckEquals(scope.Mentioned("VAR"), mkline)
 	t.CheckEquals(scope.Commented("VAR"), mkline)
 
-	value, found := scope.LastValueFound("VAR")
+	value, found, indeterminate := scope.LastValueFound("VAR")
 	t.CheckEquals(value, "")
 	t.CheckEquals(found, false)
+	t.CheckEquals(indeterminate, false)
 }
 
 func (s *Suite) Test_Scope_Define(c *check.C) {
@@ -553,39 +554,40 @@ func (s *Suite) Test_Scope_Define(c *check.C) {
 
 	scope := NewScope()
 
-	test := func(line string, ok bool, value string) {
+	test := func(line string, found, indeterminate bool, value string) {
 		mkline := t.NewMkLine("file.mk", 123, line)
 		scope.Define("BUILD_DIRS", mkline)
 
-		actualValue, actualFound := scope.LastValueFound("BUILD_DIRS")
+		actualValue, actualFound, actualIndeterminate := scope.LastValueFound("BUILD_DIRS")
 
-		t.CheckEquals(actualValue, value)
-		t.CheckEquals(actualFound, ok)
-		t.CheckEquals(scope.value["BUILD_DIRS"], value)
+		t.CheckDeepEquals(
+			[]interface{}{actualFound, actualIndeterminate, actualValue},
+			[]interface{}{found, indeterminate, value})
+		t.CheckEquals(scope.vs["BUILD_DIRS"].value, value)
 	}
 
 	test("BUILD_DIRS?=\tdefault",
-		true, "default")
+		true, false, "default")
 
 	test(
 		"BUILD_DIRS=\tone two three",
-		true, "one two three")
+		true, false, "one two three")
 
 	test(
 		"BUILD_DIRS+=\tfour",
-		true, "one two three four")
+		true, false, "one two three four")
 
 	// Later default assignments do not have an effect.
 	test("BUILD_DIRS?=\tdefault",
-		true, "one two three four")
+		true, false, "one two three four")
 
 	test("BUILD_DIRS!=\techo dynamic",
-		false, "")
+		true, true, "")
 
-	// FIXME: This is not correct. The shell assignment sets the variable,
-	//  after which all further default assignments are ignored.
+	// The shell assignment above sets the variable to an indeterminate
+	// value, after which all further default assignments are ignored.
 	test("BUILD_DIRS?=\tdefault after shell assignment",
-		true, "default after shell assignment")
+		true, true, "")
 }
 
 func (s *Suite) Test_Scope_Mentioned(c *check.C) {
@@ -727,9 +729,7 @@ func (s *Suite) Test_Scope_DefineAll(c *check.C) {
 	dst := NewScope()
 	dst.DefineAll(src)
 
-	c.Check(dst.firstDef, check.HasLen, 0)
-	c.Check(dst.lastDef, check.HasLen, 0)
-	c.Check(dst.used, check.HasLen, 0)
+	c.Check(dst.vs, check.HasLen, 0)
 
 	src.Define("VAR", t.NewMkLine("file.mk", 1, "VAR=value"))
 	dst.DefineAll(src)
@@ -1020,23 +1020,23 @@ func (s *Suite) Test_joinSkipEmpty(c *check.C) {
 		"one, two, three")
 }
 
-func (s *Suite) Test_joinSkipEmptyCambridge(c *check.C) {
+func (s *Suite) Test_joinCambridge(c *check.C) {
 	t := s.Init(c)
 
 	t.CheckDeepEquals(
-		joinSkipEmptyCambridge("and", "", "one", "", "", "two", "", "three"),
+		joinCambridge("and", "", "one", "", "", "two", "", "three"),
 		"one, two and three")
 
 	t.CheckDeepEquals(
-		joinSkipEmptyCambridge("and", "", "one", "", ""),
+		joinCambridge("and", "", "one", "", ""),
 		"one")
 }
 
-func (s *Suite) Test_joinSkipEmptyOxford(c *check.C) {
+func (s *Suite) Test_joinOxford(c *check.C) {
 	t := s.Init(c)
 
 	t.CheckDeepEquals(
-		joinSkipEmptyOxford("and", "", "one", "", "", "two", "", "three"),
+		joinOxford("and", "", "one", "", "", "two", "", "three"),
 		"one, two, and three")
 }
 
diff --git a/pkgtools/pkglint/files/varalignblock.go b/pkgtools/pkglint/files/varalignblock.go
index 34f443e14fc..c891072be28 100644
--- a/pkgtools/pkglint/files/varalignblock.go
+++ b/pkgtools/pkglint/files/varalignblock.go
@@ -562,6 +562,7 @@ func (info *varalignLine) alignValueSingle(newWidth int) {
 	}
 	fix.ReplaceAt(info.rawIndex, info.spaceBeforeValueIndex(), oldSpace, newSpace)
 	fix.Apply()
+	info.spaceBeforeValue = newSpace
 }
 
 func (info *varalignLine) alignValueInitial(newWidth int) {
@@ -855,10 +856,7 @@ func (p *varalignParts) isCanonicalFollow() bool {
 }
 
 func (p *varalignParts) isTooLongFor(valueColumn int) bool {
-	// FIXME: As of 2020-01-27, this method is called from
-	//  Test_VaralignBlock__right_margin with valueColumn == 0,
-	//  which doesn't make sense. It should at least be 8.
-	column := tabWidthAppend(valueColumn, p.value)
+	column := tabWidthAppend(imax(valueColumn, 8), p.value)
 	if p.isContinuation() {
 		column += 2
 	}
diff --git a/pkgtools/pkglint/files/varalignblock_test.go b/pkgtools/pkglint/files/varalignblock_test.go
index adaff884274..d4d15e6d6cf 100644
--- a/pkgtools/pkglint/files/varalignblock_test.go
+++ b/pkgtools/pkglint/files/varalignblock_test.go
@@ -2073,7 +2073,7 @@ func (s *Suite) Test_VaralignBlock__right_margin(c *check.C) {
 		"\t............................................................70 \t\t\\",
 		"\t........................................................66")
 	vt.Diagnostics(
-		"NOTE: Makefile:2: The continuation backslash should be in column 73, not 81.",
+		"NOTE: Makefile:2: The continuation backslash should be preceded by a single space.",
 		"NOTE: Makefile:3: The continuation backslash should be in column 73, not 81.")
 	vt.Autofixes(
 		"AUTOFIX: Makefile:2: Replacing \"\\t\" with \" \".",
@@ -3585,11 +3585,7 @@ func (s *Suite) Test_varalignLine_alignValueSingle(c *check.C) {
 			t.CheckEqualsf(
 				mkline.RawText(0), after,
 				"Line.raw.text, autofix=%v", autofix)
-
-			// As of 2019-12-11, the info fields are not updated
-			// accordingly, but they should.
-			// FIXME: update info accordingly
-			t.CheckEqualsf(info.String(), before,
+			t.CheckEqualsf(info.String(), after,
 				"info.String, autofix=%v", autofix)
 		}
 
diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go
index f0a696227b9..445fde5039f 100644
--- a/pkgtools/pkglint/files/vardefs.go
+++ b/pkgtools/pkglint/files/vardefs.go
@@ -421,7 +421,7 @@ func (reg *VarTypeRegistry) enumFrom(
 		G.Logger.TechFatalf(
 			mklines.lines.Filename,
 			"Must contain at least 1 variable definition for %s.",
-			joinSkipEmptyCambridge("or", varcanons...))
+			joinCambridge("or", varcanons...))
 	}
 
 	if trace.Tracing {
@@ -1067,7 +1067,8 @@ func (reg *VarTypeRegistry) Init(src *Pkgsrc) {
 	reg.pkg("CONFIGURE_SCRIPT", BtPathname)
 	reg.pkglist("CONFIG_GUESS_OVERRIDE", BtPathPattern)
 	reg.pkglist("CONFIG_STATUS_OVERRIDE", BtPathPattern)
-	reg.pkg("CONFIG_SHELL", BtPathname)
+	reg.pkg("CONFIG_SHELL", BtShellCommand)
+	reg.cmdline("CONFIG_SHELL_FLAGS", BtShellWord, List)
 	reg.pkglist("CONFIG_SUB_OVERRIDE", BtPathPattern)
 	reg.pkglist("CONFLICTS", BtDependencyPattern)
 	reg.pkgappend("CONF_FILES", BtConfFiles)
diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go
index 51fda5eef32..deab6ea8bb3 100644
--- a/pkgtools/pkglint/files/vartype.go
+++ b/pkgtools/pkglint/files/vartype.go
@@ -182,7 +182,7 @@ func (perms ACLPermissions) String() string {
 }
 
 func (perms ACLPermissions) HumanString() string {
-	return joinSkipEmptyOxford("or",
+	return joinOxford("or",
 		condStr(perms.Contains(aclpSet), "set", ""),
 		condStr(perms.Contains(aclpSetDefault), "given a default value", ""),
 		condStr(perms.Contains(aclpAppend), "appended to", ""),
@@ -265,12 +265,12 @@ func (vt *Vartype) AlternativeFiles(perms ACLPermissions) string {
 		neg = merge(neg)
 	}
 
-	positive := joinSkipEmptyCambridge("or", pos...)
+	positive := joinCambridge("or", pos...)
 	if positive == "" {
 		return ""
 	}
 
-	negative := joinSkipEmptyCambridge("or", neg...)
+	negative := joinCambridge("or", neg...)
 	if negative == "" {
 		return positive
 	}