pkgtools/pkglint: update to 5.7.12

Changes since 5.7.11:

* Fixed an alignment bug when pkglint replaced SUBST_SED with
  SUBST_VARS.

* Added many test cases.

22 files changed, 747 insertions, 139 deletions

diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile
index 86e887fae03..830a9437403 100644
--- a/pkgtools/pkglint/Makefile
+++ b/pkgtools/pkglint/Makefile
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.582 2019/05/26 13:52:14 rillig Exp $
+# $NetBSD: Makefile,v 1.583 2019/05/26 14:06:42 rillig Exp $
 
-PKGNAME=	pkglint-5.7.11
+PKGNAME=	pkglint-5.7.12
 CATEGORIES=	pkgtools
 DISTNAME=	tools
 MASTER_SITES=	${MASTER_SITE_GITHUB:=golang/}
diff --git a/pkgtools/pkglint/files/autofix.go b/pkgtools/pkglint/files/autofix.go
index dd4349c8b6e..4ea2b5e6112 100644
--- a/pkgtools/pkglint/files/autofix.go
+++ b/pkgtools/pkglint/files/autofix.go
@@ -100,6 +100,15 @@ func (fix *Autofix) ReplaceAfter(prefix, from string, to string) {
 		if replaced != rawLine.textnl {
 			if G.Logger.IsAutofix() {
 				rawLine.textnl = replaced
+
+				// Fix the parsed text as well.
+				// This is only approximate and won't work in some edge cases
+				// that involve escaped comments or replacements across line breaks.
+				//
+				// TODO: Do this properly by parsing the whole line again,
+				//  and ideally everything that depends on the parsed line.
+				//  This probably requires a generic notification mechanism.
+				fix.line.Text = strings.Replace(fix.line.Text, prefix+from, prefix+to, 1)
 			}
 			fix.Describef(rawLine.Lineno, "Replacing %q with %q.", from, to)
 			return
@@ -141,6 +150,25 @@ func (fix *Autofix) ReplaceRegex(from regex.Pattern, toText string, howOften int
 			}
 		}
 	}
+
+	// Fix the parsed text as well.
+	// This is only approximate and won't work in some edge cases
+	// that involve escaped comments or replacements across line breaks.
+	//
+	// TODO: Do this properly by parsing the whole line again,
+	//  and ideally everything that depends on the parsed line.
+	//  This probably requires a generic notification mechanism.
+	done = 0
+	fix.line.Text = replaceAllFunc(
+		fix.line.Text,
+		from,
+		func(fromText string) string {
+			if howOften >= 0 && done >= howOften {
+				return fromText
+			}
+			done++
+			return toText
+		})
 }
 
 // Custom runs a custom fix action, unless the fix is skipped anyway
diff --git a/pkgtools/pkglint/files/autofix_test.go b/pkgtools/pkglint/files/autofix_test.go
index 5d3e3fefd7a..2f54fb1ca5e 100644
--- a/pkgtools/pkglint/files/autofix_test.go
+++ b/pkgtools/pkglint/files/autofix_test.go
@@ -965,6 +965,54 @@ func (s *Suite) Test_Autofix_Apply__source_without_explain(c *check.C) {
 		"+\ttext again")
 }
 
+// After fixing part of a line, the whole line needs to be parsed again.
+//
+// As of May 2019, this is not done yet.
+func (s *Suite) Test_Autofix_Apply__text_after_replacing_string(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wall", "--autofix")
+	mkline := t.NewMkLine("filename.mk", 123, "VAR=\tvalue")
+
+	fix := mkline.Autofix()
+	fix.Notef("Just a demo.")
+	fix.Replace("value", "new value")
+	fix.Apply()
+
+	t.CheckOutputLines(
+		"AUTOFIX: filename.mk:123: Replacing \"value\" with \"new value\".")
+
+	t.Check(mkline.raw[0].textnl, equals, "VAR=\tnew value\n")
+	t.Check(mkline.raw[0].orignl, equals, "VAR=\tvalue\n")
+	t.Check(mkline.Text, equals, "VAR=\tnew value")
+	// FIXME: should be updated as well.
+	t.Check(mkline.Value(), equals, "value")
+}
+
+// After fixing part of a line, the whole line needs to be parsed again.
+//
+// As of May 2019, this is not done yet.
+func (s *Suite) Test_Autofix_Apply__text_after_replacing_regex(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wall", "--autofix")
+	mkline := t.NewMkLine("filename.mk", 123, "VAR=\tvalue")
+
+	fix := mkline.Autofix()
+	fix.Notef("Just a demo.")
+	fix.ReplaceRegex(`va...`, "new value", -1)
+	fix.Apply()
+
+	t.CheckOutputLines(
+		"AUTOFIX: filename.mk:123: Replacing \"value\" with \"new value\".")
+
+	t.Check(mkline.raw[0].textnl, equals, "VAR=\tnew value\n")
+	t.Check(mkline.raw[0].orignl, equals, "VAR=\tvalue\n")
+	t.Check(mkline.Text, equals, "VAR=\tnew value")
+	// FIXME: should be updated as well.
+	t.Check(mkline.Value(), equals, "value")
+}
+
 func (s *Suite) Test_Autofix_Realign__wrong_line_type(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go
index a366ecddd00..a840eff89bc 100644
--- a/pkgtools/pkglint/files/mkline_test.go
+++ b/pkgtools/pkglint/files/mkline_test.go
@@ -564,11 +564,6 @@ func (s *Suite) Test_MkLine_VariableNeedsQuoting__LDFLAGS_in_single_quotes(c *ch
 }
 
 // No quoting is necessary when lists of options are appended to each other.
-// PKG_OPTIONS are declared as "lkShell" although they are processed
-// using make's .for loop, which splits them at whitespace and usually
-// requires the variable to be declared as "lkSpace".
-// In this case it doesn't matter though since each option is an identifier,
-// and these do not pose any quoting or escaping problems.
 func (s *Suite) Test_MkLine_VariableNeedsQuoting__package_options(c *check.C) {
 	t := s.Init(c)
 
@@ -1159,6 +1154,23 @@ func (s *Suite) Test_MkLine_ValueTokens(c *check.C) {
 		"WARN: Makefile:1: Missing closing \"}\" for \"UNFINISHED\".")
 }
 
+func (s *Suite) Test_MkLine_ValueTokens__parse_error(c *check.C) {
+	t := s.Init(c)
+
+	mkline := t.NewMkLine("filename.mk", 123, "VAR=\t$")
+
+	tokens, rest := mkline.ValueTokens()
+
+	t.Check(tokens, check.IsNil)
+	t.Check(rest, equals, "$")
+
+	// Returns the same values, this time from the cache.
+	tokens, rest = mkline.ValueTokens()
+
+	t.Check(tokens, check.IsNil)
+	t.Check(rest, equals, "$")
+}
+
 func (s *Suite) Test_MkLine_ValueTokens__caching(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go
index 2e8a258b6ff..926d3a03243 100644
--- a/pkgtools/pkglint/files/mklinechecker.go
+++ b/pkgtools/pkglint/files/mklinechecker.go
@@ -224,8 +224,8 @@ func (ck MkLineChecker) checkDirectiveFor(forVars map[string]bool, indentation *
 		// The guessed flag could also be determined more correctly. As of November 2018,
 		// running pkglint over the whole pkgsrc tree did not produce any different result
 		// whether guessed was true or false.
-		forLoopType := Vartype{btForLoop, List, []ACLEntry{{"*", aclpAllRead}}}
-		forLoopContext := VarUseContext{&forLoopType, vucTimeParse, VucQuotPlain, false}
+		forLoopType := NewVartype(btForLoop, List, NewACLEntry("*", aclpAllRead))
+		forLoopContext := VarUseContext{forLoopType, vucTimeParse, VucQuotPlain, false}
 		mkline.ForEachUsed(func(varUse *MkVarUse, time vucTime) {
 			ck.CheckVaruse(varUse, &forLoopContext)
 		})
@@ -1002,7 +1002,7 @@ func (ck MkLineChecker) checkVarassignLeft() {
 
 	ck.checkTextVarUse(
 		ck.MkLine.Varname(),
-		&Vartype{BtVariableName, NoVartypeOptions, []ACLEntry{{"*", aclpAll}}},
+		NewVartype(BtVariableName, NoVartypeOptions, NewACLEntry("*", aclpAll)),
 		vucTimeParse)
 }
 
diff --git a/pkgtools/pkglint/files/mklinechecker_test.go b/pkgtools/pkglint/files/mklinechecker_test.go
index e31ec5a4810..999793b856b 100644
--- a/pkgtools/pkglint/files/mklinechecker_test.go
+++ b/pkgtools/pkglint/files/mklinechecker_test.go
@@ -1218,7 +1218,7 @@ func (s *Suite) Test_MkLineChecker_checkVarusePermissions__assigned_to_infrastru
 	// This combination of BtUnknown and all permissions is typical for
 	// otherwise unknown variables from the pkgsrc infrastructure.
 	G.Pkgsrc.vartypes.Define("INFRA", BtUnknown, NoVartypeOptions,
-		ACLEntry{"*", aclpAll})
+		NewACLEntry("*", aclpAll))
 	G.Pkgsrc.vartypes.DefineParse("VAR", BtUnknown, NoVartypeOptions,
 		"buildlink3.mk: none",
 		"*: use")
diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go
index efaf6fb8dca..64361981ae2 100644
--- a/pkgtools/pkglint/files/mklines.go
+++ b/pkgtools/pkglint/files/mklines.go
@@ -130,15 +130,13 @@ func (mklines *MkLinesImpl) checkAll() {
 
 		varalign.Process(mkline)
 		mklines.Tools.ParseToolLine(mklines, mkline, false, false)
+		substContext.Process(mkline)
 
 		switch {
-		case mkline.IsEmpty():
-			substContext.Finish(mkline)
 
 		case mkline.IsVarassign():
 			mklines.target = ""
 			mkline.Tokenize(mkline.Value(), true) // Just for the side-effect of the warnings.
-			substContext.Varassign(mkline)
 
 			mklines.checkVarassignPlist(mkline)
 
@@ -150,7 +148,6 @@ func (mklines *MkLinesImpl) checkAll() {
 
 		case mkline.IsDirective():
 			ck.checkDirective(mklines.forVars, mklines.indentation)
-			substContext.Directive(mkline)
 
 		case mkline.IsDependency():
 			ck.checkDependencyRule(allowedTargets)
diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go
index c1d4d918731..ff26b88f27c 100644
--- a/pkgtools/pkglint/files/pkgsrc.go
+++ b/pkgtools/pkglint/files/pkgsrc.go
@@ -347,7 +347,7 @@ func (src *Pkgsrc) loadTools() {
 func (src *Pkgsrc) loadUntypedVars() {
 
 	// Setting guessed to false prevents the vartype.guessed case in MkLineChecker.CheckVaruse.
-	unknownType := Vartype{BtUnknown, NoVartypeOptions, []ACLEntry{{"*", aclpAll}}}
+	unknownType := NewVartype(BtUnknown, NoVartypeOptions, NewACLEntry("*", aclpAll))
 
 	define := func(varcanon string, mkline MkLine) {
 		switch {
@@ -371,7 +371,7 @@ func (src *Pkgsrc) loadUntypedVars() {
 			if trace.Tracing {
 				trace.Stepf("Untyped variable %q in %s", varcanon, mkline)
 			}
-			src.vartypes.DefineType(varcanon, &unknownType)
+			src.vartypes.DefineType(varcanon, unknownType)
 		}
 	}
 
@@ -922,75 +922,84 @@ func (src *Pkgsrc) VariableType(mklines MkLines, varname string) (vartype *Varty
 		if tool.Validity == AfterPrefsMk && mklines.Tools.SeenPrefs {
 			perms |= aclpUseLoadtime
 		}
-		return &Vartype{BtShellCommand, NoVartypeOptions, []ACLEntry{{"*", perms}}}
+		return NewVartype(BtShellCommand, NoVartypeOptions, NewACLEntry("*", perms))
 	}
 
 	if m, toolVarname := match1(varname, `^TOOLS_(.*)`); m {
 		if tool := G.ToolByVarname(mklines, toolVarname); tool != nil {
-			return &Vartype{BtPathname, NoVartypeOptions, []ACLEntry{{"*", aclpUse}}}
+			return NewVartype(BtPathname, NoVartypeOptions, NewACLEntry("*", aclpUse))
 		}
 	}
 
 	return src.guessVariableType(varname)
 }
 
+// guessVariableType guesses the data type of the variable based on naming conventions.
 func (src *Pkgsrc) guessVariableType(varname string) (vartype *Vartype) {
-	allowAll := []ACLEntry{{"*", aclpAll}}
-	allowRuntime := []ACLEntry{{"*", aclpAllRuntime}}
+	plainType := func(basicType *BasicType, permissions ACLPermissions) *Vartype {
+		gtype := NewVartype(basicType, Guessed, NewACLEntry("*", permissions))
+		trace.Step2("The guessed type of %q is %q.", varname, gtype.String())
+		return gtype
+	}
+	listType := func(basicType *BasicType, permissions ACLPermissions) *Vartype {
+		gtype := NewVartype(basicType, List|Guessed, NewACLEntry("*", permissions))
+		trace.Step2("The guessed type of %q is %q.", varname, gtype.String())
+		return gtype
+	}
 
-	// Guess the data type of the variable based on naming conventions.
 	varbase := varnameBase(varname)
-	var gtype *Vartype
 	switch {
 	case hasSuffix(varbase, "DIRS"):
-		gtype = &Vartype{BtPathmask, List | Guessed, allowRuntime}
+		return listType(BtPathmask, aclpAllRuntime)
 	case hasSuffix(varbase, "DIR") && !hasSuffix(varbase, "DESTDIR"), hasSuffix(varname, "_HOME"):
 		// TODO: hasSuffix(varbase, "BASE")
-		gtype = &Vartype{BtPathname, Guessed, allowRuntime}
+		return plainType(BtPathname, aclpAllRuntime)
 	case hasSuffix(varbase, "FILES"):
-		gtype = &Vartype{BtPathmask, List | Guessed, allowRuntime}
+		return listType(BtPathmask, aclpAllRuntime)
 	case hasSuffix(varbase, "FILE"):
-		gtype = &Vartype{BtPathname, Guessed, allowRuntime}
+		return plainType(BtPathname, aclpAllRuntime)
 	case hasSuffix(varbase, "PATH"):
-		gtype = &Vartype{BtPathlist, Guessed, allowRuntime}
+		return plainType(BtPathlist, aclpAllRuntime)
 	case hasSuffix(varbase, "PATHS"):
-		gtype = &Vartype{BtPathname, List | Guessed, allowRuntime}
+		return listType(BtPathname, aclpAllRuntime)
 	case hasSuffix(varbase, "_USER"):
-		gtype = &Vartype{BtUserGroupName, Guessed, allowAll}
+		return plainType(BtUserGroupName, aclpAll)
 	case hasSuffix(varbase, "_GROUP"):
-		gtype = &Vartype{BtUserGroupName, Guessed, allowAll}
+		return plainType(BtUserGroupName, aclpAll)
 	case hasSuffix(varbase, "_ENV"):
-		gtype = &Vartype{BtShellWord, List | Guessed, allowRuntime}
+		return listType(BtShellWord, aclpAllRuntime)
 	case hasSuffix(varbase, "_CMD"):
-		gtype = &Vartype{BtShellCommand, Guessed, allowRuntime}
+		return plainType(BtShellCommand, aclpAllRuntime)
 	case hasSuffix(varbase, "_ARGS"):
-		gtype = &Vartype{BtShellWord, List | Guessed, allowRuntime}
+		return listType(BtShellWord, aclpAllRuntime)
 	case hasSuffix(varbase, "_CFLAGS"), hasSuffix(varname, "_CPPFLAGS"), hasSuffix(varname, "_CXXFLAGS"):
-		gtype = &Vartype{BtCFlag, List | Guessed, allowRuntime}
+		return listType(BtCFlag, aclpAllRuntime)
 	case hasSuffix(varname, "_LDFLAGS"):
-		gtype = &Vartype{BtLdFlag, List | Guessed, allowRuntime}
+		return listType(BtLdFlag, aclpAllRuntime)
 	case hasSuffix(varbase, "_MK"):
 		// TODO: Add BtGuard for inclusion guards, since these variables may only be checked using defined().
-		gtype = &Vartype{BtUnknown, Guessed, allowAll}
+		return plainType(BtUnknown, aclpAll)
 	case hasSuffix(varbase, "_SKIP"):
-		gtype = &Vartype{BtPathmask, List | Guessed, allowRuntime}
+		return listType(BtPathmask, aclpAllRuntime)
 	}
 
-	if gtype == nil {
-		vartype = src.vartypes.Canon(varname)
-		if vartype != nil {
-			return vartype
-		}
+	// Variables whose name doesn't match the above patterns may be
+	// looked up from the pkgsrc infrastructure.
+	//
+	// As of May 2019, pkglint only distinguishes plain variables and
+	// list variables, but not "unknown". Therefore the above patterns
+	// must take precedence over this rule, because otherwise, list
+	// variables from the infrastructure would be guessed to be plain
+	// variables.
+	vartype = src.vartypes.Canon(varname)
+	if vartype != nil {
+		return vartype
 	}
 
 	if trace.Tracing {
-		if gtype != nil {
-			trace.Step2("The guessed type of %q is %q.", varname, gtype.String())
-		} else {
-			trace.Step1("No type definition found for %q.", varname)
-		}
+		trace.Step1("No type definition found for %q.", varname)
 	}
-	return gtype
+	return nil
 }
 
 // Change describes a modification to a single package, from the doc/CHANGES-* files.
diff --git a/pkgtools/pkglint/files/redundantscope.go b/pkgtools/pkglint/files/redundantscope.go
index 2583ae01b98..f40d923dd63 100644
--- a/pkgtools/pkglint/files/redundantscope.go
+++ b/pkgtools/pkglint/files/redundantscope.go
@@ -29,15 +29,19 @@ func NewRedundantScope() *RedundantScope {
 
 func (s *RedundantScope) Check(mklines MkLines) {
 	mklines.ForEach(func(mkline MkLine) {
-		s.updateIncludePath(mkline)
+		s.checkLine(mklines, mkline)
+	})
+}
 
-		switch {
-		case mkline.IsVarassign():
-			s.handleVarassign(mkline, mklines.indentation)
-		}
+func (s *RedundantScope) checkLine(mklines MkLines, mkline MkLine) {
+	s.updateIncludePath(mkline)
 
-		s.handleVarUse(mkline)
-	})
+	switch {
+	case mkline.IsVarassign():
+		s.handleVarassign(mkline, mklines.indentation)
+	}
+
+	s.handleVarUse(mkline)
 }
 
 func (s *RedundantScope) updateIncludePath(mkline MkLine) {
diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go
index 35e23aea7fd..5a9ffa0c6b0 100644
--- a/pkgtools/pkglint/files/shell.go
+++ b/pkgtools/pkglint/files/shell.go
@@ -35,7 +35,7 @@ func (ck *ShellLineChecker) Explain(explanation ...string) {
 	ck.mkline.Explain(explanation...)
 }
 
-var shellCommandsType = &Vartype{BtShellCommands, NoVartypeOptions, []ACLEntry{{"*", aclpAllRuntime}}}
+var shellCommandsType = NewVartype(BtShellCommands, NoVartypeOptions, NewACLEntry("*", aclpAllRuntime))
 var shellWordVuc = &VarUseContext{shellCommandsType, vucTimeUnknown, VucQuotPlain, false}
 
 func (ck *ShellLineChecker) CheckWord(token string, checkQuoting bool, time ToolTime) {
@@ -648,13 +648,13 @@ func (scc *SimpleCommandChecker) checkRegexReplace() {
 	}
 
 	checkArg := func(arg string) {
-		if matches(arg, `"^[\"\'].*[\"\']$`) {
+		if matches(arg, `^["'].*["']$`) {
 			return
 		}
 
 		// Substitution commands that consist only of safe characters cannot
 		// have any side effects, therefore they don't need to be quoted.
-		if matches(arg, `^([\w,.]|\\[.])+$`) {
+		if matches(arg, `^([\w,.]|\\.)+$`) {
 			return
 		}
 
diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go
index fc567737565..164631a8a9c 100644
--- a/pkgtools/pkglint/files/shell_test.go
+++ b/pkgtools/pkglint/files/shell_test.go
@@ -1214,6 +1214,14 @@ func (s *Suite) Test_SimpleCommandChecker_checkRegexReplace(c *check.C) {
 	test("sed -e s,.*,, src dst",
 		"WARN: Makefile:3: Substitution commands like \"s,.*,,\" should always be quoted.")
 
+	// The * is properly enclosed in quotes.
+	test("sed -e 's,.*,,' -e \"s,-*,,\"",
+		nil...)
+
+	// The * is properly escaped.
+	test("sed -e s,.\\*,,",
+		nil...)
+
 	test("pax -s s,\\.orig,, src dst",
 		nil...)
 
@@ -1221,7 +1229,13 @@ func (s *Suite) Test_SimpleCommandChecker_checkRegexReplace(c *check.C) {
 		nil...)
 
 	// TODO: Merge the code with BtSedCommands.
+
 	// TODO: Finally, remove the G.Testing from the main code.
+	//  Then, remove this test case.
+	G.Testing = false
+	test("sed -e s,.*,match,",
+		nil...)
+	G.Testing = true
 }
 
 func (s *Suite) Test_ShellProgramChecker_checkSetE__simple_commands(c *check.C) {
diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go
index 30e8b262113..d4f8765f25e 100644
--- a/pkgtools/pkglint/files/substcontext.go
+++ b/pkgtools/pkglint/files/substcontext.go
@@ -1,9 +1,6 @@
 package pkglint
 
-import (
-	"netbsd.org/pkglint/textproc"
-	"strings"
-)
+import "netbsd.org/pkglint/textproc"
 
 // SubstContext records the state of a block of variable assignments
 // that make up a SUBST class (see `mk/subst.mk`).
@@ -47,6 +44,17 @@ func (st *SubstContextStats) Or(other SubstContextStats) {
 	st.seenTransform = st.seenTransform || other.seenTransform
 }
 
+func (ctx *SubstContext) Process(mkline MkLine) {
+	switch {
+	case mkline.IsEmpty():
+		ctx.Finish(mkline)
+	case mkline.IsVarassign():
+		ctx.Varassign(mkline)
+	case mkline.IsDirective():
+		ctx.Directive(mkline)
+	}
+}
+
 func (ctx *SubstContext) Varassign(mkline MkLine) {
 	if trace.Tracing {
 		trace.Stepf("SubstContext.Varassign curr=%v all=%v", ctx.curr, ctx.inAllBranches)
@@ -203,10 +211,7 @@ func (ctx *SubstContext) Directive(mkline MkLine) {
 }
 
 func (ctx *SubstContext) IsComplete() bool {
-	return ctx.id != "" &&
-		ctx.stage != "" &&
-		ctx.curr.seenFiles &&
-		ctx.curr.seenTransform
+	return ctx.stage != "" && ctx.curr.seenFiles && ctx.curr.seenTransform
 }
 
 func (ctx *SubstContext) Finish(mkline MkLine) {
@@ -295,11 +300,7 @@ func (ctx *SubstContext) suggestSubstVars(mkline MkLine) {
 			"Replacing @VAR@ with ${VAR} is such a typical pattern that pkgsrc has built-in support for it,",
 			"requiring only the variable name instead of the full sed command.")
 		if mkline.VarassignComment() == "" && len(tokens) == 2 && tokens[0] == "-e" {
-			// TODO: Extract the alignment computation somewhere else, so that it is generally available.
-			alignBefore := tabWidth(mkline.ValueAlign())
-			alignAfter := tabWidth(varop + "\t")
-			tabs := strings.Repeat("\t", imax((alignAfter-alignBefore)/8, 0))
-			fix.Replace(mkline.Text, varop+"\t"+tabs+varname)
+			fix.Replace(mkline.Text, alignWith(varop, mkline.ValueAlign())+varname)
 		}
 		fix.Anyway()
 		fix.Apply()
diff --git a/pkgtools/pkglint/files/substcontext_test.go b/pkgtools/pkglint/files/substcontext_test.go
index 736835f4e6e..cc35930765d 100644
--- a/pkgtools/pkglint/files/substcontext_test.go
+++ b/pkgtools/pkglint/files/substcontext_test.go
@@ -11,23 +11,23 @@ func (s *Suite) Test_SubstContext__incomplete(c *check.C) {
 	t.SetUpCommandLine("-Wextra")
 	ctx := NewSubstContext()
 
-	ctx.Varassign(newSubstLine(t, 10, "PKGNAME=pkgname-1.0"))
+	ctx.Varassign(t.NewMkLine("Makefile", 10, "PKGNAME=pkgname-1.0"))
 
 	c.Check(ctx.id, equals, "")
 
-	ctx.Varassign(newSubstLine(t, 11, "SUBST_CLASSES+=interp"))
+	ctx.Varassign(t.NewMkLine("Makefile", 11, "SUBST_CLASSES+=interp"))
 
 	c.Check(ctx.id, equals, "interp")
 
-	ctx.Varassign(newSubstLine(t, 12, "SUBST_FILES.interp=Makefile"))
+	ctx.Varassign(t.NewMkLine("Makefile", 12, "SUBST_FILES.interp=Makefile"))
 
 	c.Check(ctx.IsComplete(), equals, false)
 
-	ctx.Varassign(newSubstLine(t, 13, "SUBST_SED.interp=s,@PREFIX@,${PREFIX},g"))
+	ctx.Varassign(t.NewMkLine("Makefile", 13, "SUBST_SED.interp=s,@PREFIX@,${PREFIX},g"))
 
 	c.Check(ctx.IsComplete(), equals, false)
 
-	ctx.Finish(newSubstLine(t, 14, ""))
+	ctx.Finish(t.NewMkLine("Makefile", 14, ""))
 
 	t.CheckOutputLines(
 		"NOTE: Makefile:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" "+
@@ -41,18 +41,18 @@ func (s *Suite) Test_SubstContext__complete(c *check.C) {
 	t.SetUpCommandLine("-Wextra")
 	ctx := NewSubstContext()
 
-	ctx.Varassign(newSubstLine(t, 10, "PKGNAME=pkgname-1.0"))
-	ctx.Varassign(newSubstLine(t, 11, "SUBST_CLASSES+=p"))
-	ctx.Varassign(newSubstLine(t, 12, "SUBST_FILES.p=Makefile"))
-	ctx.Varassign(newSubstLine(t, 13, "SUBST_SED.p=s,@PREFIX@,${PREFIX},g"))
+	ctx.Varassign(t.NewMkLine("Makefile", 10, "PKGNAME=pkgname-1.0"))
+	ctx.Varassign(t.NewMkLine("Makefile", 11, "SUBST_CLASSES+=p"))
+	ctx.Varassign(t.NewMkLine("Makefile", 12, "SUBST_FILES.p=Makefile"))
+	ctx.Varassign(t.NewMkLine("Makefile", 13, "SUBST_SED.p=s,@PREFIX@,${PREFIX},g"))
 
 	c.Check(ctx.IsComplete(), equals, false)
 
-	ctx.Varassign(newSubstLine(t, 14, "SUBST_STAGE.p=post-configure"))
+	ctx.Varassign(t.NewMkLine("Makefile", 14, "SUBST_STAGE.p=post-configure"))
 
 	c.Check(ctx.IsComplete(), equals, true)
 
-	ctx.Finish(newSubstLine(t, 15, ""))
+	ctx.Finish(t.NewMkLine("Makefile", 15, ""))
 
 	t.CheckOutputLines(
 		"NOTE: Makefile:13: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
@@ -66,15 +66,15 @@ func (s *Suite) Test_SubstContext__OPSYSVARS(c *check.C) {
 	ctx := NewSubstContext()
 
 	// SUBST_CLASSES is added to OPSYSVARS in mk/bsd.pkg.mk.
-	ctx.Varassign(newSubstLine(t, 11, "SUBST_CLASSES.SunOS+=prefix"))
-	ctx.Varassign(newSubstLine(t, 12, "SUBST_CLASSES.NetBSD+=prefix"))
-	ctx.Varassign(newSubstLine(t, 13, "SUBST_FILES.prefix=Makefile"))
-	ctx.Varassign(newSubstLine(t, 14, "SUBST_SED.prefix=s,@PREFIX@,${PREFIX},g"))
-	ctx.Varassign(newSubstLine(t, 15, "SUBST_STAGE.prefix=post-configure"))
+	ctx.Varassign(t.NewMkLine("Makefile", 11, "SUBST_CLASSES.SunOS+=prefix"))
+	ctx.Varassign(t.NewMkLine("Makefile", 12, "SUBST_CLASSES.NetBSD+=prefix"))
+	ctx.Varassign(t.NewMkLine("Makefile", 13, "SUBST_FILES.prefix=Makefile"))
+	ctx.Varassign(t.NewMkLine("Makefile", 14, "SUBST_SED.prefix=s,@PREFIX@,${PREFIX},g"))
+	ctx.Varassign(t.NewMkLine("Makefile", 15, "SUBST_STAGE.prefix=post-configure"))
 
 	c.Check(ctx.IsComplete(), equals, true)
 
-	ctx.Finish(newSubstLine(t, 15, ""))
+	ctx.Finish(t.NewMkLine("Makefile", 15, ""))
 
 	t.CheckOutputLines(
 		"NOTE: Makefile:14: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
@@ -87,10 +87,10 @@ func (s *Suite) Test_SubstContext__no_class(c *check.C) {
 	t.SetUpCommandLine("-Wextra")
 	ctx := NewSubstContext()
 
-	ctx.Varassign(newSubstLine(t, 10, "UNRELATED=anything"))
-	ctx.Varassign(newSubstLine(t, 11, "SUBST_FILES.repl+=Makefile.in"))
-	ctx.Varassign(newSubstLine(t, 12, "SUBST_SED.repl+=-e s,from,to,g"))
-	ctx.Finish(newSubstLine(t, 13, ""))
+	ctx.Varassign(t.NewMkLine("Makefile", 10, "UNRELATED=anything"))
+	ctx.Varassign(t.NewMkLine("Makefile", 11, "SUBST_FILES.repl+=Makefile.in"))
+	ctx.Varassign(t.NewMkLine("Makefile", 12, "SUBST_SED.repl+=-e s,from,to,g"))
+	ctx.Finish(t.NewMkLine("Makefile", 13, ""))
 
 	t.CheckOutputLines(
 		"WARN: Makefile:11: SUBST_CLASSES should come before the definition of \"SUBST_FILES.repl\".",
@@ -108,12 +108,11 @@ func (s *Suite) Test_SubstContext__multiple_classes_in_one_line(c *check.C) {
 		"12: SUBST_FILES.one=        one.txt",
 		"13: SUBST_SED.one=          s,one,1,g",
 		"14: SUBST_STAGE.two=        post-configure",
-		"15: SUBST_FILES.two=        two.txt",
-		"17: ")
+		"15: SUBST_FILES.two=        two.txt")
 
 	t.CheckOutputLines(
 		"WARN: Makefile:10: Please add only one class at a time to SUBST_CLASSES.",
-		"WARN: Makefile:17: Incomplete SUBST block: SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.")
+		"WARN: Makefile:16: Incomplete SUBST block: SUBST_SED.two, SUBST_VARS.two or SUBST_FILTER_CMD.two missing.")
 }
 
 func (s *Suite) Test_SubstContext__multiple_classes_in_one_block(c *check.C) {
@@ -130,8 +129,7 @@ func (s *Suite) Test_SubstContext__multiple_classes_in_one_block(c *check.C) {
 		"15: SUBST_SED.one=          s,one,1,g",
 		"16: SUBST_STAGE.two=        post-configure",
 		"17: SUBST_FILES.two=        two.txt",
-		"18: SUBST_SED.two=          s,two,2,g",
-		"19: ")
+		"18: SUBST_SED.two=          s,two,2,g")
 
 	t.CheckOutputLines(
 		"WARN: Makefile:12: Duplicate definition of \"SUBST_STAGE.one\".",
@@ -140,6 +138,25 @@ func (s *Suite) Test_SubstContext__multiple_classes_in_one_block(c *check.C) {
 		"WARN: Makefile:15: Variable \"SUBST_SED.one\" does not match SUBST class \"two\".")
 }
 
+func (s *Suite) Test_SubstContext__files_missing(c *check.C) {
+	t := s.Init(c)
+
+	simulateSubstLines(t,
+		"10: SUBST_CLASSES+=         one",
+		"11: SUBST_STAGE.one=        pre-configure",
+		"12: SUBST_CLASSES+=         two",
+		"13: SUBST_STAGE.two=        pre-configure",
+		"14: SUBST_FILES.two=        two.txt",
+		"15: SUBST_SED.two=          s,two,2,g")
+
+	t.CheckOutputLines(
+		"WARN: Makefile:12: Incomplete SUBST block: SUBST_FILES.one missing.",
+		"WARN: Makefile:12: Incomplete SUBST block: "+
+			"SUBST_SED.one, SUBST_VARS.one or SUBST_FILTER_CMD.one missing.",
+		"WARN: Makefile:12: Subst block \"one\" should be finished "+
+			"before adding the next class to SUBST_CLASSES.")
+}
+
 func (s *Suite) Test_SubstContext__directives(c *check.C) {
 	t := s.Init(c)
 
@@ -156,11 +173,10 @@ func (s *Suite) Test_SubstContext__directives(c *check.C) {
 		"17: SUBST_SED.os=           -e s,@OPSYS@,Darwin1,",
 		"18: SUBST_SED.os=           -e s,@OPSYS@,Darwin2,",
 		"19: .elif ${OPSYS} == Linux",
-		"18: SUBST_SED.os=           -e s,@OPSYS@,Linux,",
-		"19: .else",
-		"20: SUBST_VARS.os=           OPSYS",
-		"21: .endif",
-		"22: ")
+		"20: SUBST_SED.os=           -e s,@OPSYS@,Linux,",
+		"21: .else",
+		"22: SUBST_VARS.os=           OPSYS",
+		"23: .endif")
 
 	// All the other lines are correctly determined as being alternatives
 	// to each other. And since every branch contains some transformation
@@ -169,6 +185,67 @@ func (s *Suite) Test_SubstContext__directives(c *check.C) {
 		"WARN: Makefile:18: All but the first \"SUBST_SED.os\" lines should use the \"+=\" operator.")
 }
 
+func (s *Suite) Test_SubstContext__directives_around_everything_then(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wextra")
+
+	simulateSubstLines(t,
+		"10: SUBST_CLASSES+=         os",
+		"11: .if ${OPSYS} == NetBSD",
+		"12: SUBST_VARS.os=          OPSYS",
+		"13: SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
+		"14: SUBST_STAGE.os=         post-configure",
+		"15: SUBST_MESSAGE.os=       Guessing operating system",
+		"16: SUBST_FILES.os=         guess-os.h",
+		"17: .endif")
+
+	// TODO: The SUBST variables are not guaranteed to be defined in all cases.
+	t.CheckOutputEmpty()
+}
+
+func (s *Suite) Test_SubstContext__directives_around_everything_else(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wextra")
+
+	simulateSubstLines(t,
+		"10: SUBST_CLASSES+=         os",
+		"11: .if ${OPSYS} == NetBSD",
+		"12: .else",
+		"13: SUBST_VARS.os=          OPSYS",
+		"14: SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
+		"15: SUBST_STAGE.os=         post-configure",
+		"16: SUBST_MESSAGE.os=       Guessing operating system",
+		"17: SUBST_FILES.os=         guess-os.h",
+		"18: .endif")
+
+	// FIXME: The warnings must be the same as in the "then" test case.
+	t.CheckOutputLines(
+		"WARN: Makefile:19: Incomplete SUBST block: SUBST_FILES.os missing.",
+		"WARN: Makefile:19: Incomplete SUBST block: SUBST_SED.os, SUBST_VARS.os or "+
+			"SUBST_FILTER_CMD.os missing.")
+}
+
+func (s *Suite) Test_SubstContext__empty_directive(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wextra")
+
+	simulateSubstLines(t,
+		"10: SUBST_CLASSES+=         os",
+		"11: SUBST_VARS.os=          OPSYS",
+		"12: SUBST_SED.os=           -e s,@OPSYS@,NetBSD,",
+		"13: SUBST_STAGE.os=         post-configure",
+		"14: SUBST_MESSAGE.os=       Guessing operating system",
+		"15: SUBST_FILES.os=         guess-os.h",
+		"16: .if ${OPSYS} == NetBSD",
+		"17: .else",
+		"18: .endif")
+
+	t.CheckOutputEmpty()
+}
+
 func (s *Suite) Test_SubstContext__missing_transformation_in_one_branch(c *check.C) {
 	t := s.Init(c)
 
@@ -186,8 +263,7 @@ func (s *Suite) Test_SubstContext__missing_transformation_in_one_branch(c *check
 		"18: SUBST_SED.os=           -e s,@OPSYS@,Darwin2,",
 		"19: .else",
 		"20: SUBST_VARS.os=           OPSYS",
-		"21: .endif",
-		"22: ")
+		"21: .endif")
 
 	t.CheckOutputLines(
 		"WARN: Makefile:15: All but the first \"SUBST_FILES.os\" lines should use the \"+=\" operator.",
@@ -204,8 +280,8 @@ func (s *Suite) Test_SubstContext__nested_conditionals(c *check.C) {
 		"10: SUBST_CLASSES+=         os",
 		"11: SUBST_STAGE.os=         post-configure",
 		"12: SUBST_MESSAGE.os=       Guessing operating system",
-		"14: .if ${OPSYS} == NetBSD",
-		"13: SUBST_FILES.os=         guess-netbsd.h",
+		"13: .if ${OPSYS} == NetBSD",
+		"14: SUBST_FILES.os=         guess-netbsd.h",
 		"15: .  if ${ARCH} == i386",
 		"16: SUBST_FILTER_CMD.os=    ${SED} -e s,@OPSYS,NetBSD-i386,",
 		"17: .  elif ${ARCH} == x86_64",
@@ -215,14 +291,34 @@ func (s *Suite) Test_SubstContext__nested_conditionals(c *check.C) {
 		"21: .  endif",
 		"22: .else",
 		"23: SUBST_SED.os=           -e s,@OPSYS@,unknown,",
-		"24: .endif",
-		"25: ")
+		"24: .endif")
 
 	// The branch in line 23 omits SUBST_FILES.
 	t.CheckOutputLines(
 		"WARN: Makefile:25: Incomplete SUBST block: SUBST_FILES.os missing.")
 }
 
+func (s *Suite) Test_SubstContext__pre_patch(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wextra,no-space", "--show-autofix")
+	t.SetUpVartypes()
+
+	mklines := t.NewMkLines("os.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=         os",
+		"SUBST_STAGE.os=         pre-patch",
+		"SUBST_FILES.os=         guess-os.h",
+		"SUBST_SED.os=           -e s,@OPSYS@,Darwin,")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"WARN: os.mk:4: Substitutions should not happen in the patch phase.",
+		"AUTOFIX: os.mk:4: Replacing \"pre-patch\" with \"post-extract\".")
+}
+
 func (s *Suite) Test_SubstContext__post_patch(c *check.C) {
 	t := s.Init(c)
 
@@ -244,15 +340,25 @@ func (s *Suite) Test_SubstContext__post_patch(c *check.C) {
 		"AUTOFIX: os.mk:4: Replacing \"post-patch\" with \"pre-configure\".")
 }
 
-func (s *Suite) Test_SubstContext__pre_configure_with_NO_CONFIGURE(c *check.C) {
+func (s *Suite) Test_SubstContext__with_NO_CONFIGURE(c *check.C) {
 	t := s.Init(c)
 
 	t.SetUpCommandLine("-Wall,no-space")
 	pkg := t.SetUpPackage("category/package",
-		"SUBST_CLASSES+=         os",
-		"SUBST_STAGE.os=         pre-configure",
-		"SUBST_FILES.os=         guess-os.h",
-		"SUBST_SED.os=           -e s,@OPSYS@,Darwin,",
+		"SUBST_CLASSES+=         pre",
+		"SUBST_STAGE.pre=        pre-configure",
+		"SUBST_FILES.pre=        guess-os.h",
+		"SUBST_SED.pre=          -e s,@OPSYS@,Darwin,",
+		"",
+		"SUBST_CLASSES+=         post",
+		"SUBST_STAGE.post=       post-configure",
+		"SUBST_FILES.post=       guess-os.h",
+		"SUBST_SED.post=         -e s,@OPSYS@,Darwin,",
+		"",
+		"SUBST_CLASSES+=         e",
+		"SUBST_STAGE.e=          post-extract",
+		"SUBST_FILES.e=          guess-os.h",
+		"SUBST_SED.e=            -e s,@OPSYS@,Darwin,",
 		"",
 		"NO_CONFIGURE=           yes")
 	t.FinishSetUp()
@@ -260,7 +366,26 @@ func (s *Suite) Test_SubstContext__pre_configure_with_NO_CONFIGURE(c *check.C) {
 	G.Check(pkg)
 
 	t.CheckOutputLines(
-		"WARN: ~/category/package/Makefile:21: SUBST_STAGE pre-configure has no effect when NO_CONFIGURE is set (in line 25).")
+		"WARN: ~/category/package/Makefile:21: SUBST_STAGE pre-configure has no effect "+
+			"when NO_CONFIGURE is set (in line 35).",
+		"WARN: ~/category/package/Makefile:26: SUBST_STAGE post-configure has no effect "+
+			"when NO_CONFIGURE is set (in line 35).")
+}
+
+func (s *Suite) Test_SubstContext__without_NO_CONFIGURE(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wall,no-space")
+	pkg := t.SetUpPackage("category/package",
+		"SUBST_CLASSES+=         pre",
+		"SUBST_STAGE.pre=        pre-configure",
+		"SUBST_FILES.pre=        guess-os.h",
+		"SUBST_SED.pre=          -e s,@OPSYS@,Darwin,")
+	t.FinishSetUp()
+
+	G.Check(pkg)
+
+	t.CheckOutputEmpty()
 }
 
 func (s *Suite) Test_SubstContext__adjacent(c *check.C) {
@@ -359,6 +484,51 @@ func (s *Suite) Test_SubstContext__SUBST_VARS_in_next_paragraph(c *check.C) {
 		"WARN: os.mk:9: TODAY2 is defined but not used.")
 }
 
+func (s *Suite) Test_SubstContext__multiple_SUBST_VARS(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wextra,no-space")
+	t.SetUpVartypes()
+
+	mklines := t.NewMkLines("os.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=         os",
+		"SUBST_STAGE.os=         pre-configure",
+		"SUBST_FILES.os=         guess-os.h",
+		"SUBST_VARS.os=          PREFIX VARBASE")
+
+	mklines.Check()
+
+	t.CheckOutputEmpty()
+}
+
+// Since the SUBST_CLASSES definition starts the SUBST block, all
+// directives above it are ignored by the SUBST context.
+func (s *Suite) Test_SubstContext_Directive__before_SUBST_CLASSES(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wextra,no-space")
+	t.SetUpVartypes()
+	t.DisableTracing() // Just for branch coverage.
+
+	mklines := t.NewMkLines("os.mk",
+		MkRcsID,
+		"",
+		".if 0",
+		".endif",
+		"SUBST_CLASSES+=         os",
+		".elif 0") // Just for branch coverage.
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"WARN: os.mk:EOF: Incomplete SUBST block: SUBST_STAGE.os missing.",
+		"WARN: os.mk:EOF: Incomplete SUBST block: SUBST_FILES.os missing.",
+		"WARN: os.mk:EOF: Incomplete SUBST block: "+
+			"SUBST_SED.os, SUBST_VARS.os or SUBST_FILTER_CMD.os missing.")
+}
+
 func (s *Suite) Test_SubstContext_suggestSubstVars(c *check.C) {
 	t := s.Init(c)
 
@@ -482,16 +652,197 @@ func (s *Suite) Test_SubstContext_suggestSubstVars__plus(c *check.C) {
 			"with \"SUBST_VARS.gtk+ +=\\tSH\".")
 }
 
+// The last of the SUBST_SED variables is 15 characters wide. When SUBST_SED
+// is replaced with SUBST_VARS, this becomes 16 characters and therefore
+// requires the whole paragraph to be indented by one more tab.
+func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_realign_paragraph(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+	t.Chdir(".")
+
+	mklines := t.SetUpFileMkLines("subst.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=\t\tpfx",
+		"SUBST_STAGE.pfx=\tpre-configure",
+		"SUBST_FILES.pfx=\tfilename",
+		"SUBST_SED.pfx=\t\t-e s,@PREFIX@,${PREFIX},g",
+		"SUBST_SED.pfx+=\t\t-e s,@PREFIX@,${PREFIX},g")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"NOTE: subst.mk:6: The substitution command \"s,@PREFIX@,${PREFIX},g\" "+
+			"can be replaced with \"SUBST_VARS.pfx= PREFIX\".",
+		"NOTE: subst.mk:7: The substitution command \"s,@PREFIX@,${PREFIX},g\" "+
+			"can be replaced with \"SUBST_VARS.pfx+= PREFIX\".")
+
+	t.SetUpCommandLine("--autofix")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"AUTOFIX: subst.mk:6: Replacing \"SUBST_SED.pfx=\\t\\t-e s,@PREFIX@,${PREFIX},g\" "+
+			"with \"SUBST_VARS.pfx=\\t\\tPREFIX\".",
+		"AUTOFIX: subst.mk:7: Replacing \"SUBST_SED.pfx+=\\t\\t-e s,@PREFIX@,${PREFIX},g\" "+
+			"with \"SUBST_VARS.pfx+=\\tPREFIX\".")
+
+	t.CheckFileLinesDetab("subst.mk",
+		"# $NetBSD: substcontext_test.go,v 1.25 2019/05/26 14:05:57 rillig Exp $",
+		"",
+		"SUBST_CLASSES+=         pfx",
+		"SUBST_STAGE.pfx=        pre-configure",
+		"SUBST_FILES.pfx=        filename",
+		"SUBST_VARS.pfx=         PREFIX",
+		"SUBST_VARS.pfx+=        PREFIX")
+}
+
+func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_sed(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+	t.Chdir(".")
+
+	mklines := t.SetUpFileMkLines("subst.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=\t\tpfx",
+		"SUBST_STAGE.pfx=\tpre-configure",
+		"SUBST_FILES.pfx=\tfilename",
+		"SUBST_SED.pfx=\t\t-e s,@PREFIX@,${PREFIX},g",
+		"SUBST_SED.pfx+=\t\t-e s,@PREFIX@,other,g")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"NOTE: subst.mk:6: The substitution command \"s,@PREFIX@,${PREFIX},g\" " +
+			"can be replaced with \"SUBST_VARS.pfx= PREFIX\".")
+
+	t.SetUpCommandLine("-Wall", "--autofix")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"AUTOFIX: subst.mk:6: Replacing \"SUBST_SED.pfx=\\t\\t-e s,@PREFIX@,${PREFIX},g\" " +
+			"with \"SUBST_VARS.pfx=\\t\\tPREFIX\".")
+
+	t.CheckFileLinesDetab("subst.mk",
+		"# $NetBSD: substcontext_test.go,v 1.25 2019/05/26 14:05:57 rillig Exp $",
+		"",
+		"SUBST_CLASSES+=         pfx",
+		"SUBST_STAGE.pfx=        pre-configure",
+		"SUBST_FILES.pfx=        filename",
+		"SUBST_VARS.pfx=         PREFIX",
+		// TODO: If this subst class is used nowhere else, pkglint could
+		//  replace this += with a simple =.
+		"SUBST_SED.pfx+=         -e s,@PREFIX@,other,g")
+}
+
+func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_plus_vars(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wall", "--autofix")
+	t.SetUpVartypes()
+	t.Chdir(".")
+
+	mklines := t.SetUpFileMkLines("subst.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=\tid",
+		"SUBST_STAGE.id=\tpre-configure",
+		"SUBST_FILES.id=\tfilename",
+		"SUBST_SED.id=\t-e s,@PREFIX@,${PREFIX},g",
+		"SUBST_VARS.id=\tPKGMANDIR")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"AUTOFIX: subst.mk:6: Replacing \"SUBST_SED.id=\\t-e s,@PREFIX@,${PREFIX},g\" " +
+			"with \"SUBST_VARS.id=\\tPREFIX\".")
+
+	t.CheckFileLinesDetab("subst.mk",
+		"# $NetBSD: substcontext_test.go,v 1.25 2019/05/26 14:05:57 rillig Exp $",
+		"",
+		"SUBST_CLASSES+= id",
+		"SUBST_STAGE.id= pre-configure",
+		"SUBST_FILES.id= filename",
+		"SUBST_VARS.id=  PREFIX",
+		// FIXME: This must be += instead of = since the previous line already uses =.
+		//  Luckily the check for redundant assignments catches this already.
+		"SUBST_VARS.id=  PKGMANDIR")
+}
+
+func (s *Suite) Test_SubstContext_suggestSubstVars__autofix_indentation(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpCommandLine("-Wall", "--autofix")
+	t.SetUpVartypes()
+	t.Chdir(".")
+
+	mklines := t.SetUpFileMkLines("subst.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=\t\t\tfix-paths",
+		"SUBST_STAGE.fix-paths=\t\tpre-configure",
+		"SUBST_MESSAGE.fix-paths=\tMessage",
+		"SUBST_FILES.fix-paths=\t\tfilename",
+		"SUBST_SED.fix-paths=\t\t-e s,@PREFIX@,${PREFIX},g")
+
+	mklines.Check()
+
+	t.CheckOutputLines(
+		"AUTOFIX: subst.mk:7: Replacing \"SUBST_SED.fix-paths=\\t\\t-e s,@PREFIX@,${PREFIX},g\" " +
+			"with \"SUBST_VARS.fix-paths=\\t\\tPREFIX\".")
+
+	t.CheckFileLinesDetab("subst.mk",
+		"# $NetBSD: substcontext_test.go,v 1.25 2019/05/26 14:05:57 rillig Exp $",
+		"",
+		"SUBST_CLASSES+=                 fix-paths",
+		"SUBST_STAGE.fix-paths=          pre-configure",
+		"SUBST_MESSAGE.fix-paths=        Message",
+		"SUBST_FILES.fix-paths=          filename",
+		"SUBST_VARS.fix-paths=           PREFIX")
+}
+
+// As of May 2019, pkglint does not check the order of the variables in
+// a SUBST block. Enforcing this order, or at least suggesting it, would
+// make pkgsrc packages more uniform, which is a good idea, but not urgent.
+func (s *Suite) Test_SubstContext__unusual_variable_order(c *check.C) {
+	t := s.Init(c)
+
+	t.SetUpVartypes()
+
+	mklines := t.NewMkLines("subst.mk",
+		MkRcsID,
+		"",
+		"SUBST_CLASSES+=\t\tid",
+		"SUBST_SED.id=\t\t-e /deleteme/d",
+		"SUBST_FILES.id=\t\tfile",
+		"SUBST_MESSAGE.id=\tMessage",
+		"SUBST_STAGE.id=\t\tpre-configure")
+
+	mklines.Check()
+
+	t.CheckOutputEmpty()
+}
+
 // simulateSubstLines only tests some of the inner workings of SubstContext.
 // It is not realistic for all cases. If in doubt, use MkLines.Check.
 func simulateSubstLines(t *Tester, texts ...string) {
 	ctx := NewSubstContext()
+	lineno := 0
 	for _, lineText := range texts {
-		var lineno int
-		_, err := fmt.Sscanf(lineText[0:4], "%d: ", &lineno)
+		var curr int
+		_, err := fmt.Sscanf(lineText[0:4], "%d: ", &curr)
 		G.AssertNil(err, "")
+
+		if lineno != 0 {
+			t.Check(curr, equals, lineno)
+		}
+
 		text := lineText[4:]
-		line := newSubstLine(t, lineno, text)
+		line := t.NewMkLine("Makefile", curr, text)
 
 		switch {
 		case text == "":
@@ -501,9 +852,9 @@ func simulateSubstLines(t *Tester, texts ...string) {
 		default:
 			ctx.Varassign(line)
 		}
+
+		lineno = curr + 1
 	}
-}
 
-func newSubstLine(t *Tester, lineno int, text string) MkLine {
-	return t.NewMkLine("Makefile", lineno, text)
+	ctx.Finish(t.NewMkLine("Makefile", lineno, ""))
 }
diff --git a/pkgtools/pkglint/files/tools.go b/pkgtools/pkglint/files/tools.go
index a0d52295485..ef4799af923 100644
--- a/pkgtools/pkglint/files/tools.go
+++ b/pkgtools/pkglint/files/tools.go
@@ -292,7 +292,7 @@ func (tr *Tools) addAlias(tool *Tool, alias string) {
 // parseUseTools interprets a "USE_TOOLS+=" line from a Makefile fragment.
 // It determines the validity of the tool, i.e. in which places it may be used.
 //
-// If createIfAbsent is true and the tools is unknown, it is registered.
+// If createIfAbsent is true and the tool is unknown, it is registered.
 // This can be done only in the pkgsrc infrastructure files, where the
 // actual definition is assumed to be in some other file. In packages
 // though, this assumption cannot be made and pkglint needs to be strict.
diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go
index 63fc756eca2..13a48a3ac9f 100644
--- a/pkgtools/pkglint/files/util.go
+++ b/pkgtools/pkglint/files/util.go
@@ -265,12 +265,21 @@ func detab(s string) string {
 		if r == '\t' {
 			detabbed.WriteString("        "[:8-detabbed.Len()%8])
 		} else {
-			detabbed.WriteString(string(r))
+			detabbed.WriteRune(r)
 		}
 	}
 	return detabbed.String()
 }
 
+// alignWith extends str with as many tabs 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)
+}
+
 func shorten(s string, maxChars int) string {
 	codePoints := 0
 	for i := range s {
@@ -368,7 +377,7 @@ func relpath(from, to string) (result string) {
 	}
 
 	// Take a shortcut for the common case from "dir" to "dir/subdir/...".
-	if hasPrefix(cto, cfrom) && len(cto) > len(cfrom)+1 && cto[len(cfrom)] == '/' {
+	if hasPrefix(cto, cfrom) && hasPrefix(cto[len(cfrom):], "/") {
 		return cleanpath(cto[len(cfrom)+1:])
 	}
 
@@ -683,13 +692,13 @@ func (s *Scope) Commented(varname string) MkLine {
 	}
 
 	for _, mkline := range mklines {
-		if mkline != nil && mkline.IsVarassign() {
+		if mkline.IsVarassign() {
 			return nil
 		}
 	}
 
 	for _, mkline := range mklines {
-		if mkline != nil && mkline.IsCommentedVarassign() {
+		if mkline.IsCommentedVarassign() {
 			return mkline
 		}
 	}
@@ -878,7 +887,9 @@ func (c *FileCache) Put(filename string, options LoadOptions, lines Lines) {
 }
 
 func (c *FileCache) removeOldEntries() {
-	sort.Slice(c.table, func(i, j int) bool { return c.table[j].count < c.table[i].count })
+	sort.Slice(c.table, func(i, j int) bool {
+		return c.table[j].count < c.table[i].count
+	})
 
 	if G.Testing {
 		for _, e := range c.table {
diff --git a/pkgtools/pkglint/files/util_test.go b/pkgtools/pkglint/files/util_test.go
index 6411f6fa88c..b8d631a119d 100644
--- a/pkgtools/pkglint/files/util_test.go
+++ b/pkgtools/pkglint/files/util_test.go
@@ -122,6 +122,7 @@ func (s *Suite) Test_relpath(c *check.C) {
 	}
 
 	test("some/dir", "some/directory", "../../some/directory")
+	test("some/directory", "some/dir", "../../some/dir")
 
 	test("category/package/.", ".", "../..")
 
@@ -131,6 +132,9 @@ func (s *Suite) Test_relpath(c *check.C) {
 		"x11/frameworkintegration/../../meta-pkgs/kde/kf5.mk",
 		"meta-pkgs/kde/kf5.mk")
 
+	test(".hidden/dir", ".", "../..")
+	test("dir/.hidden", ".", "../..")
+
 	// This happens when "pkglint -r x11" is run.
 	G.Pkgsrc.topdir = "x11/.."
 
@@ -238,6 +242,25 @@ func (s *Suite) Test_detab(c *check.C) {
 	c.Check(detab("12345678\t"), equals, "12345678        ")
 }
 
+func (s *Suite) Test_alignWith(c *check.C) {
+	t := s.Init(c)
+
+	test := func(str, other, expected string) {
+		t.Check(alignWith(str, other), equals, expected)
+	}
+
+	// At least one tab is _always_ added.
+	test("", "", "\t")
+
+	test("VAR=", "1234567", "VAR=\t")
+	test("VAR=", "12345678", "VAR=\t")
+	test("VAR=", "123456789", "VAR=\t\t")
+
+	// At least one tab is added in any case,
+	// even if the other string is shorter.
+	test("1234567890=", "V=", "1234567890=\t")
+}
+
 const reMkIncludeBenchmark = `^\.([\t ]*)(s?include)[\t ]+\"([^\"]+)\"[\t ]*(?:#.*)?$`
 const reMkIncludeBenchmarkPositive = `^\.([\t ]*)(s?include)[\t ]+\"(.+)\"[\t ]*(?:#.*)?$`
 
@@ -311,6 +334,37 @@ func (s *Suite) Test_trimHspace(c *check.C) {
 	t.Check(trimHspace(" \t a b\t \t"), equals, "a b")
 }
 
+func (s *Suite) Test_trimCommon(c *check.C) {
+	t := s.Init(c)
+
+	test := func(a, b, trimmedA, trimmedB string) {
+		ta, tb := trimCommon(a, b)
+		t.Check(ta, equals, trimmedA)
+		t.Check(tb, equals, trimmedB)
+	}
+
+	test("", "",
+		"", "")
+
+	test("equal", "equal",
+		"", "")
+
+	test("prefixA", "prefixB",
+		"A", "B")
+
+	test("ASuffix", "BSuffix",
+		"A", "B")
+
+	test("PreMiddlePost", "PreCenterPost",
+		"Middle", "Center")
+
+	test("", "b",
+		"", "b")
+
+	test("a", "",
+		"a", "")
+}
+
 func (s *Suite) Test_isLocallyModified(c *check.C) {
 	t := s.Init(c)
 
@@ -628,6 +682,28 @@ func (s *Suite) Test_FileCache(c *check.C) {
 		"TRACE:   FileCache.Halve \"Makefile\" with count 4.")
 }
 
+func (s *Suite) Test_FileCache_removeOldEntries__branch_coverage(c *check.C) {
+	t := s.Init(c)
+
+	t.EnableTracingToLog()
+	G.Testing = false
+
+	lines := t.NewLines("filename.mk",
+		MkRcsID)
+	cache := NewFileCache(3)
+	cache.Put("filename1.mk", 0, lines)
+	cache.Put("filename2.mk", 0, lines)
+	cache.Get("filename2.mk", 0)
+	cache.Get("filename2.mk", 0)
+	cache.Put("filename3.mk", 0, lines)
+	cache.Put("filename4.mk", 0, lines)
+
+	t.CheckOutputLines(
+		"TRACE:   FileCache.Evict \"filename3.mk\" with count 1.",
+		"TRACE:   FileCache.Evict \"filename1.mk\" with count 1.",
+		"TRACE:   FileCache.Halve \"filename2.mk\" with count 3.")
+}
+
 func (s *Suite) Test_makeHelp(c *check.C) {
 	c.Check(makeHelp("subst"), equals, confMake+" help topic=subst")
 }
diff --git a/pkgtools/pkglint/files/var_test.go b/pkgtools/pkglint/files/var_test.go
index a11fa4aa145..0242098af21 100644
--- a/pkgtools/pkglint/files/var_test.go
+++ b/pkgtools/pkglint/files/var_test.go
@@ -212,6 +212,29 @@ func (s *Suite) Test_Var_Value__initial_conditional_write(c *check.C) {
 	t.Check(v.Value(), equals, "overwritten conditionally")
 }
 
+func (s *Suite) Test_Var_Write__conditional_without_variables(c *check.C) {
+	t := s.Init(c)
+
+	mklines := t.NewMkLines("filename.mk",
+		MkRcsID,
+		".if exists(/usr/bin)",
+		"VAR=\tvalue",
+		".endif")
+
+	scope := NewRedundantScope()
+	mklines.ForEach(func(mkline MkLine) {
+		if mkline.IsVarassign() {
+			t.Check(scope.get("VAR").vari.Conditional(), equals, false)
+		}
+
+		scope.checkLine(mklines, mkline)
+
+		if mkline.IsVarassign() {
+			t.Check(scope.get("VAR").vari.Conditional(), equals, true)
+		}
+	})
+}
+
 func (s *Suite) Test_Var_Value__conditional_write_after_unconditional(c *check.C) {
 	t := s.Init(c)
 
diff --git a/pkgtools/pkglint/files/vardefs.go b/pkgtools/pkglint/files/vardefs.go
index 5d35ddeffa8..f7da2f6993b 100644
--- a/pkgtools/pkglint/files/vardefs.go
+++ b/pkgtools/pkglint/files/vardefs.go
@@ -60,13 +60,13 @@ func (reg *VarTypeRegistry) Define(varname string, basicType *BasicType, options
 	m, varbase, varparam := match2(varname, `^([A-Z_.][A-Z0-9_]*|@)(|\*|\.\*)$`)
 	G.Assertf(m, "invalid variable name")
 
-	vartype := Vartype{basicType, options, aclEntries}
+	vartype := NewVartype(basicType, options, aclEntries...)
 
 	if varparam == "" || varparam == "*" {
-		reg.types[varbase] = &vartype
+		reg.types[varbase] = vartype
 	}
 	if varparam == "*" || varparam == ".*" {
-		reg.types[varbase+".*"] = &vartype
+		reg.types[varbase+".*"] = vartype
 	}
 }
 
@@ -1729,7 +1729,7 @@ func (reg *VarTypeRegistry) parseACLEntries(varname string, aclEntries ...string
 				G.AssertNil(err, "Invalid ACL pattern %q for %q", glob, varname)
 				G.Assertf(!matched, "Unreachable ACL pattern %q for %q.", glob, varname)
 			}
-			result = append(result, ACLEntry{glob, permissions})
+			result = append(result, NewACLEntry(glob, permissions))
 		}
 	}
 
diff --git a/pkgtools/pkglint/files/vardefs_test.go b/pkgtools/pkglint/files/vardefs_test.go
index eea4e45ed21..ec786ad2f3d 100644
--- a/pkgtools/pkglint/files/vardefs_test.go
+++ b/pkgtools/pkglint/files/vardefs_test.go
@@ -143,7 +143,6 @@ func (s *Suite) Test_VarTypeRegistry_Init__LP64PLATFORMS(c *check.C) {
 	G.Check(pkg)
 
 	// No warning about a missing :Q operator.
-	// All PLATFORM variables must be either lkNone or lkSpace.
 	t.CheckOutputEmpty()
 }
 
@@ -161,3 +160,19 @@ func (s *Suite) Test_VarTypeRegistry_Init__no_tracing(c *check.C) {
 
 	t.CheckOutputEmpty()
 }
+
+func (s *Suite) Test_VarTypeRegistry_Init__MASTER_SITES(c *check.C) {
+	t := s.Init(c)
+
+	t.CreateFileLines("mk/fetch/sites.mk",
+		MkRcsID,
+		"",
+		"MASTER_SITE_GITHUB=\thttps://github.com/",
+		"",
+		"OTHER=\tvalue") // For branch coverage of hasPrefix.*MASTER_SITE_
+
+	t.SetUpVartypes()
+
+	vartype := G.Pkgsrc.VariableType(nil, "MASTER_SITE_GITHUB")
+	t.Check(vartype.String(), equals, "FetchURL (list, system-provided)")
+}
diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go
index e2eb7727ec9..628115f1bd0 100644
--- a/pkgtools/pkglint/files/vartype.go
+++ b/pkgtools/pkglint/files/vartype.go
@@ -13,6 +13,15 @@ type Vartype struct {
 	aclEntries []ACLEntry
 }
 
+func NewVartype(basicType *BasicType, options vartypeOptions, aclEntries ...ACLEntry) *Vartype {
+	for _, aclEntry := range aclEntries {
+		_, err := path.Match(aclEntry.glob, "")
+		G.AssertNil(err, "path.Match")
+	}
+
+	return &Vartype{basicType, options, aclEntries}
+}
+
 type vartypeOptions uint8
 
 const (
@@ -42,6 +51,10 @@ type ACLEntry struct {
 	permissions ACLPermissions
 }
 
+func NewACLEntry(glob string, permissions ACLPermissions) ACLEntry {
+	return ACLEntry{glob, permissions}
+}
+
 type ACLPermissions uint8
 
 const (
@@ -119,8 +132,8 @@ func (vt *Vartype) Union() ACLPermissions {
 //
 // If the permission is allowed nowhere, an empty string is returned.
 func (vt *Vartype) AlternativeFiles(perms ACLPermissions) string {
-	pos := make([]string, 0, len(vt.aclEntries))
-	neg := make([]string, 0, len(vt.aclEntries))
+	var pos []string
+	var neg []string
 
 	merge := func(slice []string) []string {
 		di := 0
@@ -128,7 +141,8 @@ func (vt *Vartype) AlternativeFiles(perms ACLPermissions) string {
 			redundant := false
 			for _, late := range slice[si+1:] {
 				matched, err := path.Match(late, early)
-				if err == nil && matched {
+				G.AssertNil(err, "path.Match")
+				if matched {
 					redundant = true
 					break
 				}
diff --git a/pkgtools/pkglint/files/vartype_test.go b/pkgtools/pkglint/files/vartype_test.go
index cf7ff24a00d..8b23a2b397d 100644
--- a/pkgtools/pkglint/files/vartype_test.go
+++ b/pkgtools/pkglint/files/vartype_test.go
@@ -11,7 +11,7 @@ func (s *Suite) Test_Vartype_EffectivePermissions(c *check.C) {
 
 	if typ := G.Pkgsrc.vartypes.Canon("PREFIX"); c.Check(typ, check.NotNil) {
 		c.Check(typ.basicType.name, equals, "Pathname")
-		c.Check(typ.aclEntries, check.DeepEquals, []ACLEntry{{"*", aclpUse}})
+		c.Check(typ.aclEntries, deepEquals, []ACLEntry{NewACLEntry("*", aclpUse)})
 		c.Check(typ.EffectivePermissions("Makefile"), equals, aclpUse)
 		c.Check(typ.EffectivePermissions("buildlink3.mk"), equals, aclpUse)
 	}
@@ -28,7 +28,7 @@ func (s *Suite) Test_Vartype_AlternativeFiles(c *check.C) {
 	// test generates the files description for the "set" permission.
 	test := func(rules []string, alternatives string) {
 		aclEntries := (*VarTypeRegistry).parseACLEntries(nil, "", rules...)
-		vartype := Vartype{BtYesNo, NoVartypeOptions, aclEntries}
+		vartype := NewVartype(BtYesNo, NoVartypeOptions, aclEntries...)
 
 		alternativeFiles := vartype.AlternativeFiles(aclpSet)
 
diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go
index 47e7e94a6f3..cfa9d5b4edc 100644
--- a/pkgtools/pkglint/files/vartypecheck_test.go
+++ b/pkgtools/pkglint/files/vartypecheck_test.go
@@ -534,13 +534,18 @@ func (s *Suite) Test_VartypeCheck_FileMask(c *check.C) {
 
 	vt.Varname("FNAME")
 	vt.Values(
+		"filename.txt",
+		"*.txt",
+		"[12345].txt",
+		"[0-9].txt",
+		"???.txt",
 		"FileMask with spaces.docx",
 		"OS/2-manual.txt")
 
 	vt.Output(
-		"WARN: filename.mk:1: The filename pattern \"FileMask with spaces.docx\" "+
+		"WARN: filename.mk:6: The filename pattern \"FileMask with spaces.docx\" "+
 			"contains the invalid characters \"  \".",
-		"WARN: filename.mk:2: The filename pattern \"OS/2-manual.txt\" "+
+		"WARN: filename.mk:7: The filename pattern \"OS/2-manual.txt\" "+
 			"contains the invalid character \"/\".")
 
 	vt.Op(opUseMatch)