pkgtools/pkglint: update to 5.5.15

Changes since 5.5.14:

* Check that the comments in .endif and .endfor lines match the
  corresponding conditions.

* Check for redundant variables (e.g. MASTER_SITES for R packages).

* Check for accidentally overwritten variables.

* Miscellaneous code cleanup and refactoring.

18 files changed, 583 insertions, 139 deletions

diff --git a/pkgtools/pkglint/Makefile b/pkgtools/pkglint/Makefile
index 97749254a68..560366cd0b3 100644
--- a/pkgtools/pkglint/Makefile
+++ b/pkgtools/pkglint/Makefile
@@ -1,6 +1,6 @@
-# $NetBSD: Makefile,v 1.542 2018/07/19 06:38:15 rillig Exp $
+# $NetBSD: Makefile,v 1.543 2018/07/28 18:31:23 rillig Exp $
 
-PKGNAME=	pkglint-5.5.14
+PKGNAME=	pkglint-5.5.15
 DISTFILES=	# none
 CATEGORIES=	pkgtools
 
diff --git a/pkgtools/pkglint/files/check_test.go b/pkgtools/pkglint/files/check_test.go
index ea496f13cef..51a692e8c7c 100644
--- a/pkgtools/pkglint/files/check_test.go
+++ b/pkgtools/pkglint/files/check_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"path"
@@ -296,11 +297,12 @@ func (t *Tester) CheckOutputLines(expectedLines ...string) {
 	t.c().Check(emptyToNil(actualLines), deepEquals, emptyToNil(expectedLines))
 }
 
-// EnableTracing redirects all logging output to stdout instead of
-// the buffer. This is useful when stepping through the code, especially
+// EnableTracing redirects all logging output (which is normally captured
+// in an in-memory buffer) additionally to stdout.
+// This is useful when stepping through the code, especially
 // in combination with SetupCommandLine("--debug").
 func (t *Tester) EnableTracing() {
-	G.logOut = NewSeparatorWriter(os.Stdout)
+	G.logOut = NewSeparatorWriter(io.MultiWriter(os.Stdout, &t.stdout))
 	trace.Out = os.Stdout
 	trace.Tracing = true
 }
diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go
index 12da7114ee1..0cce30af516 100644
--- a/pkgtools/pkglint/files/mkline.go
+++ b/pkgtools/pkglint/files/mkline.go
@@ -38,6 +38,7 @@ type mkLineConditional struct {
 	indent    string
 	directive string
 	args      string
+	comment   string
 }
 type mkLineInclude struct {
 	mustExist     bool
@@ -112,8 +113,8 @@ func NewMkLine(line Line) *MkLineImpl {
 		return &MkLineImpl{line, mkLineEmpty{}}
 	}
 
-	if m, indent, directive, args := matchMkCond(text); m {
-		return &MkLineImpl{line, mkLineConditional{indent, directive, args}}
+	if m, indent, directive, args, comment := matchMkCond(text); m {
+		return &MkLineImpl{line, mkLineConditional{indent, directive, args, comment}}
 	}
 
 	if m, indent, directive, includefile := MatchMkInclude(text); m {
@@ -217,7 +218,8 @@ func (mkline *MkLineImpl) Varparam() string { return mkline.data.(mkLineAssign).
 // Op applies to variable assignments and returns the assignment operator.
 func (mkline *MkLineImpl) Op() MkOperator { return mkline.data.(mkLineAssign).op }
 
-// For a variable assignment, the text up to and including the assignment operator, e.g. VARNAME+=\t
+// ValueAlign applies to variable assignments and returns all the text
+// before the variable value, e.g. "VARNAME+=\t".
 func (mkline *MkLineImpl) ValueAlign() string { return mkline.data.(mkLineAssign).valueAlign }
 func (mkline *MkLineImpl) Value() string      { return mkline.data.(mkLineAssign).value }
 
@@ -235,14 +237,25 @@ func (mkline *MkLineImpl) Indent() string {
 		return mkline.data.(mkLineInclude).indent
 	}
 }
-func (mkline *MkLineImpl) Directive() string   { return mkline.data.(mkLineConditional).directive }
-func (mkline *MkLineImpl) Args() string        { return mkline.data.(mkLineConditional).args }
+
+// Directive returns one of "if", "ifdef", "ifndef", "else", "elif", "endif", "for", "endfor", "undef".
+//
+// See matchMkCond.
+func (mkline *MkLineImpl) Directive() string { return mkline.data.(mkLineConditional).directive }
+func (mkline *MkLineImpl) Args() string      { return mkline.data.(mkLineConditional).args }
+
+// CondComment is the trailing end-of-line comment, typically at a deeply nested .endif or .endfor.
+func (mkline *MkLineImpl) CondComment() string { return mkline.data.(mkLineConditional).comment }
+
 func (mkline *MkLineImpl) MustExist() bool     { return mkline.data.(mkLineInclude).mustExist }
 func (mkline *MkLineImpl) IncludeFile() string { return mkline.data.(mkLineInclude).includeFile }
-func (mkline *MkLineImpl) Targets() string     { return mkline.data.(mkLineDependency).targets }
-func (mkline *MkLineImpl) Sources() string     { return mkline.data.(mkLineDependency).sources }
 
-// Initialized step by step, when parsing other lines
+func (mkline *MkLineImpl) Targets() string { return mkline.data.(mkLineDependency).targets }
+func (mkline *MkLineImpl) Sources() string { return mkline.data.(mkLineDependency).sources }
+
+// ConditionVars is a space-separated list of those variable names
+// on which the inclusion depends. It is initialized later,
+// step by step, when parsing other lines
 func (mkline *MkLineImpl) ConditionVars() string { return mkline.data.(mkLineInclude).conditionVars }
 func (mkline *MkLineImpl) SetConditionVars(varnames string) {
 	include := mkline.data.(mkLineInclude)
@@ -340,7 +353,7 @@ func (mkline *MkLineImpl) ExplainRelativeDirs() {
 		"main pkgsrc repository.")
 }
 
-func matchMkCond(text string) (m bool, indent, directive, args string) {
+func matchMkCond(text string) (m bool, indent, directive, args, comment string) {
 	i, n := 0, len(text)
 	if i < n && text[i] == '.' {
 		i++
@@ -375,6 +388,13 @@ func matchMkCond(text string) (m bool, indent, directive, args string) {
 	for i < n && (text[i] != '#' || text[i-1] == '\\') {
 		i++
 	}
+	commentStart := i
+	if commentStart < n {
+		commentStart++
+		for commentStart < n && (text[commentStart] == ' ' || text[commentStart] == '\t') {
+			commentStart++
+		}
+	}
 	for i > argsStart && (text[i-1] == ' ' || text[i-1] == '\t') {
 		i--
 	}
@@ -383,6 +403,7 @@ func matchMkCond(text string) (m bool, indent, directive, args string) {
 	m = true
 	indent = text[indentStart:indentEnd]
 	args = strings.Replace(text[argsStart:argsEnd], "\\#", "#", -1)
+	comment = text[commentStart:]
 	return
 }
 
@@ -728,57 +749,66 @@ func (vuc *VarUseContext) String() string {
 // An excepting are multiple-inclusion guards, they don't increase the
 // indentation.
 type Indentation struct {
-	depth         []int      // Number of space characters; always a multiple of 2
-	conditionVars [][]string // Variables on which the current path depends
+	levels []indentationLevel
+}
+
+func NewIndentation() *Indentation {
+	ind := &Indentation{}
+	ind.Push(0, "") // Dummy
+	return ind
+}
+
+type indentationLevel struct {
+	depth         int      // Number of space characters; always a multiple of 2
+	condition     string   // The corresponding condition from the .if or .elif
+	conditionVars []string // Variables on which the current path depends
 
 	// Files whose existence has been checked in a related path.
 	// The check counts for both the "if" and the "else" branch,
 	// but that sloppiness will be discovered by functional tests.
-	checkedFiles [][]string
+	checkedFiles []string
 }
 
 func (ind *Indentation) Len() int {
-	return len(ind.depth)
+	return len(ind.levels)
+}
+
+func (ind *Indentation) top() *indentationLevel {
+	return &ind.levels[ind.Len()-1]
 }
 
+// Depth returns the number of space characters by which the directive
+// should be indented.
 func (ind *Indentation) Depth(directive string) int {
 	switch directive {
-	case "elif", "else", "endfor", "endif":
-		return ind.depth[imax(0, len(ind.depth)-2)]
+	case "if", "elif", "else", "endfor", "endif":
+		return ind.levels[imax(0, ind.Len()-2)].depth
 	}
-	return ind.depth[len(ind.depth)-1]
+	return ind.top().depth
 }
 
 func (ind *Indentation) Pop() {
-	newlen := ind.Len() - 1
-	ind.depth = ind.depth[:newlen]
-	ind.conditionVars = ind.conditionVars[:newlen]
-	ind.checkedFiles = ind.checkedFiles[:newlen]
+	ind.levels = ind.levels[:ind.Len()-1]
 }
 
-func (ind *Indentation) Push(indent int) {
-	ind.depth = append(ind.depth, indent)
-	ind.conditionVars = append(ind.conditionVars, nil)
-	ind.checkedFiles = append(ind.checkedFiles, nil)
+func (ind *Indentation) Push(indent int, condition string) {
+	ind.levels = append(ind.levels, indentationLevel{indent, condition, nil, nil})
 }
 
 func (ind *Indentation) AddVar(varname string) {
-	level := ind.Len() - 1
-	if hasSuffix(varname, "_MK") {
-		return
-	}
-	for _, existingVarname := range ind.conditionVars[level] {
+	vars := &ind.top().conditionVars
+	for _, existingVarname := range *vars {
 		if varname == existingVarname {
 			return
 		}
 	}
 
-	ind.conditionVars[level] = append(ind.conditionVars[level], varname)
+	*vars = append(*vars, varname)
 }
 
 func (ind *Indentation) DependsOn(varname string) bool {
-	for _, levelVarnames := range ind.conditionVars {
-		for _, levelVarname := range levelVarnames {
+	for _, level := range ind.levels {
+		for _, levelVarname := range level.conditionVars {
 			if varname == levelVarname {
 				return true
 			}
@@ -788,34 +818,46 @@ func (ind *Indentation) DependsOn(varname string) bool {
 }
 
 func (ind *Indentation) IsConditional() bool {
-	for _, vars := range ind.conditionVars {
-		if len(vars) > 0 {
-			return true
+	for _, level := range ind.levels {
+		for _, varname := range level.conditionVars {
+			if !hasSuffix(varname, "_MK") {
+				return true
+			}
 		}
 	}
 	return false
 }
 
+// Varnames returns the list of all variables that are mentioned in any
+// condition or loop surrounding the current line.
+// Variables named *_MK are excluded since they are usually not interesting.
 func (ind *Indentation) Varnames() string {
 	sep := ""
 	varnames := ""
-	for _, levelVarnames := range ind.conditionVars {
-		for _, levelVarname := range levelVarnames {
-			varnames += sep + levelVarname
-			sep = ", "
+	for _, level := range ind.levels {
+		for _, levelVarname := range level.conditionVars {
+			if !hasSuffix(levelVarname, "_MK") {
+				varnames += sep + levelVarname
+				sep = ", "
+			}
 		}
 	}
 	return varnames
 }
 
+// Condition returns the condition for the innermost .if, .elif or .for.
+func (ind *Indentation) Condition() string {
+	return ind.top().condition
+}
+
 func (ind *Indentation) AddCheckedFile(filename string) {
-	level := ind.Len() - 1
-	ind.checkedFiles[level] = append(ind.checkedFiles[level], filename)
+	top := ind.top()
+	top.checkedFiles = append(top.checkedFiles, filename)
 }
 
 func (ind *Indentation) IsCheckedFile(filename string) bool {
-	for _, levelFilenames := range ind.checkedFiles {
-		for _, levelFilename := range levelFilenames {
+	for _, level := range ind.levels {
+		for _, levelFilename := range level.checkedFiles {
 			if filename == levelFilename {
 				return true
 			}
@@ -824,6 +866,68 @@ func (ind *Indentation) IsCheckedFile(filename string) bool {
 	return false
 }
 
+func (ind *Indentation) TrackBefore(mkline MkLine) {
+	if !mkline.IsCond() {
+		return
+	}
+	if trace.Tracing {
+		trace.Stepf("Indentation before line %s: %+v", mkline.Linenos(), ind.levels)
+	}
+
+	directive := mkline.Directive()
+	args := mkline.Args()
+
+	switch directive {
+	case "for", "if", "ifdef", "ifndef":
+		ind.Push(ind.top().depth, args)
+	}
+}
+
+func (ind *Indentation) TrackAfter(mkline MkLine) {
+	if !mkline.IsCond() {
+		return
+	}
+
+	directive := mkline.Directive()
+	args := mkline.Args()
+
+	switch directive {
+	case "if":
+		// For multiple-inclusion guards, the indentation stays at the same level.
+		if m, varname := match1(args, `^!defined\(([\w]+_MK)\)$`); m {
+			ind.AddVar(varname)
+		} else {
+			ind.top().depth += 2
+		}
+
+		// Note: adding the used variables for arbitrary conditions
+		// happens in MkLineChecker.CheckCond for performance reasons.
+
+		if contains(args, "exists") {
+			cond := NewMkParser(mkline.Line, args, false).MkCond()
+			cond.Visit("exists", func(node *Tree) {
+				ind.AddCheckedFile(node.args[0].(string))
+			})
+		}
+
+	case "for", "ifdef", "ifndef":
+		ind.top().depth += 2
+
+	case "elif":
+		// Handled here instead of TrackAfter to allow the action to access the previous condition.
+		ind.top().condition = args
+
+	case "endfor", "endif":
+		if ind.Len() > 1 { // Can only be false in unbalanced files.
+			ind.Pop()
+		}
+	}
+
+	if trace.Tracing {
+		trace.Stepf("Indentation after line %s: %+v", mkline.Linenos(), ind.levels)
+	}
+}
+
 func MatchVarassign(text string) (m, commented bool, varname, spaceAfterVarname, op, valueAlign, value, spaceAfterValue, comment string) {
 	i, n := 0, len(text)
 
diff --git a/pkgtools/pkglint/files/mkline_test.go b/pkgtools/pkglint/files/mkline_test.go
index 4931c6fe826..1c2d285bec3 100644
--- a/pkgtools/pkglint/files/mkline_test.go
+++ b/pkgtools/pkglint/files/mkline_test.go
@@ -150,6 +150,7 @@ func (s *Suite) Test_NewMkLine(c *check.C) {
 	c.Check(ln[4].Indent(), equals, "  ")
 	c.Check(ln[4].Directive(), equals, "if")
 	c.Check(ln[4].Args(), equals, "!empty(PKGNAME:M*-*) && ${RUBY_RAILS_SUPPORTED:[#]} == 1")
+	c.Check(ln[4].CondComment(), equals, "cond comment")
 
 	c.Check(ln[5].IsInclude(), equals, true)
 	c.Check(ln[5].Indent(), equals, "    ")
@@ -285,6 +286,7 @@ func (s *Suite) Test_MkLines_Check__extra(c *check.C) {
 		"COMMENT=\t# defined",
 		".endfor",
 		"GAMES_USER?=pkggames",
+		"GAMES_GROUP?=pkggames",
 		"PLIST_SUBST+= CONDITIONAL=${CONDITIONAL}",
 		"CONDITIONAL=\"@comment\"",
 		"BUILD_DIRS=\t${WRKSRC}/../build")
@@ -295,8 +297,8 @@ func (s *Suite) Test_MkLines_Check__extra(c *check.C) {
 		"WARN: options.mk:3: The values for PYTHON_VERSIONS_ACCEPTED should be in decreasing order.",
 		"NOTE: options.mk:5: Please use \"# empty\", \"# none\" or \"yes\" instead of \"# defined\".",
 		"WARN: options.mk:7: Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".",
-		"WARN: options.mk:10: Building the package should take place entirely inside ${WRKSRC}, not \"${WRKSRC}/..\".",
-		"NOTE: options.mk:10: You can use \"../build\" instead of \"${WRKSRC}/../build\".")
+		"WARN: options.mk:11: Building the package should take place entirely inside ${WRKSRC}, not \"${WRKSRC}/..\".",
+		"NOTE: options.mk:11: You can use \"../build\" instead of \"${WRKSRC}/../build\".")
 }
 
 func (s *Suite) Test_MkLine_variableNeedsQuoting__unknown_rhs(c *check.C) {
@@ -846,16 +848,14 @@ func (s *Suite) Test_MatchVarassign(c *check.C) {
 }
 
 func (s *Suite) Test_Indentation(c *check.C) {
-	ind := &Indentation{}
-
-	ind.Push(0)
+	ind := NewIndentation()
 
 	c.Check(ind.Depth("if"), equals, 0)
 	c.Check(ind.DependsOn("VARNAME"), equals, false)
 
-	ind.Push(2)
+	ind.Push(2, "")
 
-	c.Check(ind.Depth("if"), equals, 2)
+	c.Check(ind.Depth("if"), equals, 0) // Because "if" is handled in MkLines.TrackBefore.
 	c.Check(ind.Depth("endfor"), equals, 0)
 
 	ind.AddVar("LEVEL1.VAR1")
@@ -868,7 +868,7 @@ func (s *Suite) Test_Indentation(c *check.C) {
 	c.Check(ind.DependsOn("LEVEL1.VAR1"), equals, true)
 	c.Check(ind.DependsOn("OTHER_VAR"), equals, false)
 
-	ind.Push(2)
+	ind.Push(2, "")
 
 	ind.AddVar("LEVEL2.VAR")
 
diff --git a/pkgtools/pkglint/files/mklinechecker.go b/pkgtools/pkglint/files/mklinechecker.go
index 54e6970e8f2..8731a9cf1ac 100644
--- a/pkgtools/pkglint/files/mklinechecker.go
+++ b/pkgtools/pkglint/files/mklinechecker.go
@@ -110,31 +110,43 @@ func (ck MkLineChecker) checkCond(forVars map[string]bool, indentation *Indentat
 
 	directive := mkline.Directive()
 	args := mkline.Args()
+	comment := mkline.CondComment()
 
 	expectedDepth := indentation.Depth(directive)
 	ck.checkDirectiveIndentation(expectedDepth)
 
-	if directive == "if" && matches(args, `^!defined\([\w]+_MK\)$`) {
-		indentation.Push(indentation.Depth(directive))
-	} else if matches(directive, `^(?:if|ifdef|ifndef|for)$`) {
-		indentation.Push(indentation.Depth(directive) + 2)
-	} else if directive == "endfor" || directive == "endif" {
-		if indentation.Len() > 1 {
-			indentation.Pop()
-		} else {
+	if directive == "endfor" || directive == "endif" {
+		if directive == "endif" && comment != "" {
+			if condition := indentation.Condition(); !contains(condition, comment) {
+				mkline.Warnf("Comment %q does not match condition %q.", comment, condition)
+			}
+		}
+
+		if directive == "endfor" && comment != "" {
+			if condition := indentation.Condition(); !contains(condition, comment) {
+				mkline.Warnf("Comment %q does not match loop %q.", comment, condition)
+			}
+		}
+
+		if indentation.Len() <= 1 {
 			mkline.Errorf("Unmatched .%s.", directive)
 		}
 	}
 
-	needsArgument := matches(directive, `^(?:if|ifdef|ifndef|elif|for|undef)$`)
-	if needsArgument != (args != "") {
-		if needsArgument {
-			mkline.Errorf("\".%s\" requires arguments.", directive)
+	needsArgument := false
+	switch directive {
+	case "if", "ifdef", "ifndef", "elif", "for", "undef":
+		needsArgument = true
+	}
+
+	if needsArgument && args == "" {
+		mkline.Errorf("\".%s\" requires arguments.", directive)
+
+	} else if !needsArgument && args != "" {
+		if directive == "else" {
+			mkline.Errorf("\".%s\" does not take arguments. If you meant \"else if\", use \".elif\".", directive)
 		} else {
 			mkline.Errorf("\".%s\" does not take arguments.", directive)
-			if directive == "else" {
-				mkline.Notef("If you meant \"else if\", use \".elif\".")
-			}
 		}
 
 	} else if directive == "if" || directive == "elif" {
@@ -147,6 +159,7 @@ func (ck MkLineChecker) checkCond(forVars map[string]bool, indentation *Indentat
 	} else if directive == "for" {
 		if m, vars, values := match2(args, `^(\S+(?:\s*\S+)*?)\s+in\s+(.*)$`); m {
 			for _, forvar := range splitOnSpace(vars) {
+				indentation.AddVar(forvar)
 				if !G.Infrastructure && hasPrefix(forvar, "_") {
 					mkline.Warnf("Variable names starting with an underscore (%s) are reserved for internal pkgsrc use.", forvar)
 				}
@@ -850,6 +863,10 @@ func (ck MkLineChecker) checkVarassignBsdPrefs() {
 			return
 		}
 
+		if G.Mk != nil && !G.Mk.FirstTime("include-bsd.prefs.mk") {
+			return
+		}
+
 		mkline.Warnf("Please include \"../../mk/bsd.prefs.mk\" before using \"?=\".")
 		Explain(
 			"The ?= operator is used to provide a default value to a variable.",
diff --git a/pkgtools/pkglint/files/mklines.go b/pkgtools/pkglint/files/mklines.go
index e125fcf3d63..179698edf42 100644
--- a/pkgtools/pkglint/files/mklines.go
+++ b/pkgtools/pkglint/files/mklines.go
@@ -18,7 +18,7 @@ type MkLines struct {
 	tools          map[string]bool // Set of tools that are declared to be used.
 	toolRegistry   ToolRegistry    // Tools defined in file scope.
 	SeenBsdPrefsMk bool
-	indentation    Indentation // Indentation depth of preprocessing directives
+	indentation    *Indentation // Indentation depth of preprocessing directives; only available during MkLines.ForEach.
 	Once
 	// XXX: Why both tools and toolRegistry?
 }
@@ -46,7 +46,7 @@ func NewMkLines(lines []Line) *MkLines {
 		tools,
 		NewToolRegistry(),
 		false,
-		Indentation{},
+		nil,
 		Once{}}
 }
 
@@ -92,9 +92,9 @@ func (mklines *MkLines) Check() {
 
 	substcontext := NewSubstContext()
 	var varalign VaralignBlock
-	indentation := &mklines.indentation
-	indentation.Push(0)
-	for _, mkline := range mklines.mklines {
+	lastMkline := mklines.mklines[len(mklines.mklines)-1]
+
+	lineAction := func(mkline MkLine) bool {
 		ck := MkLineChecker{mkline}
 		ck.Check()
 		varalign.Check(mkline)
@@ -115,11 +115,11 @@ func (mklines *MkLines) Check() {
 				mklines.setSeenBsdPrefsMk()
 			}
 			if G.Pkg != nil {
-				G.Pkg.CheckInclude(mkline, indentation)
+				G.Pkg.CheckInclude(mkline, mklines.indentation)
 			}
 
 		case mkline.IsCond():
-			ck.checkCond(mklines.forVars, indentation)
+			ck.checkCond(mklines.forVars, mklines.indentation)
 			substcontext.Conditional(mkline)
 
 		case mkline.IsDependency():
@@ -129,18 +129,42 @@ func (mklines *MkLines) Check() {
 		case mkline.IsShellCommand():
 			mkline.Tokenize(mkline.ShellCommand())
 		}
+
+		return true
 	}
-	lastMkline := mklines.mklines[len(mklines.mklines)-1]
+
+	atEnd := func(mkline MkLine) {
+		ind := mklines.indentation
+		if ind.Len() != 1 && ind.Depth("") != 0 {
+			mkline.Errorf("Directive indentation is not 0, but %d.", ind.Depth(""))
+		}
+	}
+
+	mklines.ForEach(lineAction, atEnd)
+
 	substcontext.Finish(lastMkline)
 	varalign.Finish()
 
 	ChecklinesTrailingEmptyLines(mklines.lines)
 
-	if indentation.Len() != 1 && indentation.Depth("") != 0 {
-		lastMkline.Errorf("Directive indentation is not 0, but %d.", indentation.Depth(""))
+	SaveAutofixChanges(mklines.lines)
+}
+
+// ForEach calls the action for each line, until the action returns false.
+// It keeps track of the indentation and all conditionals.
+func (mklines *MkLines) ForEach(action func(mkline MkLine) bool, atEnd func(mkline MkLine)) {
+	mklines.indentation = NewIndentation()
+
+	for _, mkline := range mklines.mklines {
+		mklines.indentation.TrackBefore(mkline)
+		if !action(mkline) {
+			break
+		}
+		mklines.indentation.TrackAfter(mkline)
 	}
 
-	SaveAutofixChanges(mklines.lines)
+	atEnd(mklines.mklines[len(mklines.mklines)-1])
+	mklines.indentation = nil
 }
 
 func (mklines *MkLines) DetermineDefinedVariables() {
@@ -276,6 +300,41 @@ func (mklines *MkLines) setSeenBsdPrefsMk() {
 	}
 }
 
+func (mklines *MkLines) CheckRedundantVariables() {
+	scope := NewRedundantScope()
+	isRelevant := func(old, new MkLine) bool {
+		if path.Base(old.Filename) != "Makefile" && path.Base(new.Filename) == "Makefile" {
+			return false
+		}
+		if new.Op() == opAssignEval {
+			return false
+		}
+		return true
+	}
+	scope.OnIgnore = func(old, new MkLine) {
+		if isRelevant(old, new) {
+			old.Notef("Definition of %s is redundant because of %s.", new.Varname(), new.ReferenceFrom(old.Line))
+		}
+	}
+	scope.OnOverwrite = func(old, new MkLine) {
+		if isRelevant(old, new) {
+			old.Warnf("Variable %s is overwritten in %s.", new.Varname(), new.ReferenceFrom(old.Line))
+			Explain(
+				"The variable definition in this line does not have an effect since",
+				"it is overwritten elsewhere.  This typically happens because of a",
+				"typo (writing = instead of +=) or because the line that overwrites",
+				"is in another file that is used by several packages.")
+		}
+	}
+
+	mklines.ForEach(
+		func(mkline MkLine) bool {
+			scope.Handle(mkline)
+			return true
+		},
+		func(mkline MkLine) {})
+}
+
 // VaralignBlock checks that all variable assignments from a paragraph
 // use the same indentation depth for their values.
 // It also checks that the indentation uses tabs instead of spaces.
diff --git a/pkgtools/pkglint/files/mklines_test.go b/pkgtools/pkglint/files/mklines_test.go
index d0ed2cd51bd..6a5a2603ca1 100644
--- a/pkgtools/pkglint/files/mklines_test.go
+++ b/pkgtools/pkglint/files/mklines_test.go
@@ -409,8 +409,7 @@ func (s *Suite) Test_MkLines_Check_indentation(c *check.C) {
 		"NOTE: options.mk:9: This directive should be indented by 2 spaces.",
 		"NOTE: options.mk:10: This directive should be indented by 2 spaces.",
 		"NOTE: options.mk:11: This directive should be indented by 2 spaces.",
-		"ERROR: options.mk:11: \".else\" does not take arguments.",
-		"NOTE: options.mk:11: If you meant \"else if\", use \".elif\".",
+		"ERROR: options.mk:11: \".else\" does not take arguments. If you meant \"else if\", use \".elif\".",
 		"NOTE: options.mk:12: This directive should be indented by 2 spaces.",
 		"NOTE: options.mk:13: This directive should be indented by 0 spaces.",
 		"NOTE: options.mk:14: This directive should be indented by 0 spaces.",
@@ -418,6 +417,42 @@ func (s *Suite) Test_MkLines_Check_indentation(c *check.C) {
 		"ERROR: options.mk:15: Unmatched .endif.")
 }
 
+func (s *Suite) Test_MkLines_Check__endif_comment(c *check.C) {
+	t := s.Init(c)
+
+	t.SetupCommandLine("-Wall")
+	mklines := t.NewMkLines("opsys.mk",
+		MkRcsID,
+		"",
+		".for i in 1 2 3 4 5",
+		".  if ${OPSYS} == NetBSD",
+		".    if ${ARCH} == x86_64",
+		".      if ${OS_VERSION:M8.*}",
+		".      endif # ARCH",     // Wrong, should be OS_VERSION.
+		".    endif # OS_VERSION", // Wrong, should be ARCH.
+		".  endif # OPSYS",        // Correct.
+		".endfor # j",             // Wrong, should be i.
+		"",
+		".if ${PKG_OPTIONS:Moption}",
+		".endif # option",
+		"",
+		".if ${PKG_OPTIONS:Moption}",
+		".endif # opti", // This typo gets unnoticed since "opti" is a substring of the condition.
+		"",
+		".if ${OPSYS} == NetBSD",
+		".elif ${OPSYS} == FreeBSD",
+		".endif # NetBSD") // Wrong, should be FreeBSD from the .elif.
+
+	// See MkLineChecker.checkCond
+	mklines.Check()
+
+	t.CheckOutputLines(""+
+		"WARN: opsys.mk:7: Comment \"ARCH\" does not match condition \"${OS_VERSION:M8.*}\".",
+		"WARN: opsys.mk:8: Comment \"OS_VERSION\" does not match condition \"${ARCH} == x86_64\".",
+		"WARN: opsys.mk:10: Comment \"j\" does not match loop \"i in 1 2 3 4 5\".",
+		"WARN: opsys.mk:20: Comment \"NetBSD\" does not match condition \"${OPSYS} == FreeBSD\".")
+}
+
 // Demonstrates how to define your own make(1) targets.
 func (s *Suite) Test_MkLines_wip_category_Makefile(c *check.C) {
 	t := s.Init(c)
@@ -532,3 +567,35 @@ func (s *Suite) Test_MkLines__unknown_options(c *check.C) {
 	t.CheckOutputLines(
 		"WARN: options.mk:4: Unknown option \"unknown\".")
 }
+
+func (s *Suite) Test_MkLines_CheckRedundantVariables(c *check.C) {
+	t := s.Init(c)
+	included := t.NewMkLines("module.mk",
+		"VAR=\tvalue ${OTHER}",
+		"VAR?=\tvalue ${OTHER}",
+		"VAR=\tnew value")
+	makefile := t.NewMkLines("Makefile",
+		"VAR=\tthe package may overwrite variables from other files")
+	allLines := append(append([]Line(nil), included.lines...), makefile.lines...)
+	mklines := NewMkLines(allLines)
+
+	// XXX: The warnings from here are not in the same order as the other warnings.
+	// XXX: There may be some warnings for the same file separated by warnings for other files.
+	mklines.CheckRedundantVariables()
+
+	t.CheckOutputLines(
+		"NOTE: module.mk:1: Definition of VAR is redundant because of line 2.",
+		"WARN: module.mk:1: Variable VAR is overwritten in line 3.")
+}
+
+func (s *Suite) Test_MkLines_CheckRedundantVariables__procedure_call(c *check.C) {
+	t := s.Init(c)
+	mklines := t.NewMkLines("mk/pthread.buildlink3.mk",
+		"CHECK_BUILTIN.pthread:=\tyes",
+		".include \"../../mk/pthread.builtin.mk\"",
+		"CHECK_BUILTIN.pthread:=\tno")
+
+	mklines.CheckRedundantVariables()
+
+	t.CheckOutputEmpty()
+}
diff --git a/pkgtools/pkglint/files/mkparser.go b/pkgtools/pkglint/files/mkparser.go
index a0ae0200d62..dcfd2a77411 100644
--- a/pkgtools/pkglint/files/mkparser.go
+++ b/pkgtools/pkglint/files/mkparser.go
@@ -201,6 +201,8 @@ func (p *MkParser) VarUseModifiers(varname, closing string) []string {
 	return modifiers
 }
 
+// MkCond parses a condition like ${OPSYS} == "NetBSD".
+// See devel/bmake/files/cond.c.
 func (p *MkParser) MkCond() *Tree {
 	return p.mkCondOr()
 }
diff --git a/pkgtools/pkglint/files/mkparser_test.go b/pkgtools/pkglint/files/mkparser_test.go
index 2f7331827df..dd0f22a69a6 100644
--- a/pkgtools/pkglint/files/mkparser_test.go
+++ b/pkgtools/pkglint/files/mkparser_test.go
@@ -210,6 +210,8 @@ func (s *Suite) Test_MkParser_MkCond(c *check.C) {
 		NewTree("compareVarNum", varuse("OS_VERSION"), "==", "5.3"))
 	check("!empty(${OS_VARIANT:MIllumos})", // Probably not intended
 		NewTree("not", NewTree("empty", varuse("${OS_VARIANT:MIllumos}"))))
+	check("defined (VARNAME)", // There may be whitespace before the parenthesis; see devel/bmake/files/cond.c:^compare_function.
+		NewTree("defined", "VARNAME"))
 
 	// Errors
 	checkRest("!empty(PKG_OPTIONS:Msndfile) || defined(PKG_OPTIONS:Msamplerate)",
diff --git a/pkgtools/pkglint/files/package.go b/pkgtools/pkglint/files/package.go
index f073169f010..3ff5650f4fa 100644
--- a/pkgtools/pkglint/files/package.go
+++ b/pkgtools/pkglint/files/package.go
@@ -249,6 +249,7 @@ func (pkg *Package) loadPackageMakefile(fname string) *MkLines {
 	}
 
 	allLines.DetermineUsedVariables()
+	allLines.CheckRedundantVariables()
 
 	pkg.Pkgdir = pkg.expandVariableWithDefault("PKGDIR", ".")
 	pkg.DistinfoFile = pkg.expandVariableWithDefault("DISTINFO_FILE", "distinfo")
@@ -287,8 +288,8 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL
 
 	isMainMakefile := len(mainLines.mklines) == 0
 
-	for _, mkline := range fileMklines.mklines {
-
+	result := true
+	lineAction := func(mkline MkLine) bool {
 		if isMainMakefile {
 			mainLines.mklines = append(mainLines.mklines, mkline)
 			mainLines.lines = append(mainLines.lines, mkline.Line)
@@ -296,28 +297,6 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL
 		allLines.mklines = append(allLines.mklines, mkline)
 		allLines.lines = append(allLines.lines, mkline.Line)
 
-		if mkline.IsCond() {
-			ind := &fileMklines.indentation
-			switch mkline.Directive() {
-			case "if", "ifdef", "ifndef", "for":
-				ind.Push(0) // Dummy indentation, only the checkedFiles are interesting
-			case "endfor", "endif":
-				if ind.Len() > 1 {
-					ind.Pop()
-				}
-			}
-
-			if mkline.Directive() == "if" {
-				args := mkline.Args()
-				if contains(args, "exists") {
-					cond := NewMkParser(mkline.Line, args, false).MkCond()
-					cond.Visit("exists", func(node *Tree) {
-						ind.AddCheckedFile(node.args[0].(string))
-					})
-				}
-			}
-		}
-
 		var includeFile, incDir, incBase string
 		if mkline.IsInclude() {
 			inc := mkline.IncludeFile()
@@ -368,12 +347,13 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL
 				if !fileExists(dirname + "/" + includeFile) {
 
 					if fileMklines.indentation.IsCheckedFile(includeFile) {
-						continue // See https://github.com/rillig/pkglint/issues/1
+						return true // See https://github.com/rillig/pkglint/issues/1
 
 					} else if dirname != G.CurrentDir { // Prevent unnecessary syscalls
 						dirname = G.CurrentDir
 						if !fileExists(dirname + "/" + includeFile) {
 							mkline.Errorf("Cannot read %q.", dirname+"/"+includeFile)
+							result = false
 							return false
 						}
 					}
@@ -385,6 +365,7 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL
 				absIncluding := ifelseStr(incBase == "Makefile.common" && incDir != "", fname, "")
 				absIncluded := dirname + "/" + includeFile
 				if !pkg.readMakefile(absIncluded, mainLines, allLines, absIncluding) {
+					result = false
 					return false
 				}
 			}
@@ -400,13 +381,16 @@ func (pkg *Package) readMakefile(fname string, mainLines *MkLines, allLines *MkL
 				G.Pkg.vars.Define(varname, mkline)
 			}
 		}
+		return true
 	}
+	atEnd := func(mkline MkLine) {}
+	fileMklines.ForEach(lineAction, atEnd)
 
 	if includingFnameForUsedCheck != "" {
 		fileMklines.checkForUsedComment(G.Pkgsrc.ToRel(includingFnameForUsedCheck))
 	}
 
-	return true
+	return result
 }
 
 func (pkg *Package) checkfilePackageMakefile(fname string, mklines *MkLines) {
diff --git a/pkgtools/pkglint/files/package_test.go b/pkgtools/pkglint/files/package_test.go
index 96af064389a..2726a272f7f 100644
--- a/pkgtools/pkglint/files/package_test.go
+++ b/pkgtools/pkglint/files/package_test.go
@@ -419,7 +419,11 @@ func (s *Suite) Test_Package_loadPackageMakefile(c *check.C) {
 
 	// Including a package Makefile directly is an error (in the last line),
 	// but that is checked later.
-	t.CheckOutputEmpty()
+	// A file including itself does not lead to an endless loop while parsing
+	// but may still produce unexpected warnings, such as redundant definitions.
+	t.CheckOutputLines(
+		"WARN: ~/category/package/Makefile:3: Variable PKGNAME is overwritten in Makefile:3.",
+		"WARN: ~/category/package/Makefile:4: Variable DISTNAME is overwritten in Makefile:4.")
 }
 
 func (s *Suite) Test_Package_conditionalAndUnconditionalInclude(c *check.C) {
@@ -538,3 +542,42 @@ func (s *Suite) Test_Package_includeOtherAfterExists(c *check.C) {
 	t.CheckOutputLines(
 		"ERROR: ~/category/package/another.mk: Cannot be read.")
 }
+
+// See https://mail-index.netbsd.org/tech-pkg/2018/07/22/msg020092.html
+func (s *Suite) Test_Package__redundant_master_sites(c *check.C) {
+	t := s.Init(c)
+
+	t.SetupVartypes()
+	t.SetupMasterSite("MASTER_SITE_R_CRAN", "http://cran.r-project.org/src/")
+	t.CreateFileLines("mk/bsd.pkg.mk")
+	t.CreateFileLines("licenses/gnu-gpl-v2",
+		"The licenses for most software are designed to take away ...")
+	t.CreateFileLines("math/R/Makefile.extension",
+		MkRcsID,
+		"",
+		"PKGNAME?=\tR-${R_PKGNAME}-${R_PKGVER}",
+		"MASTER_SITES?=\t${MASTER_SITE_R_CRAN:=contrib/}",
+		"GENERATE_PLIST+=\techo \"bin/r-package\";",
+		"NO_CHECKSUM=\tyes",
+		"LICENSE?=\tgnu-gpl-v2")
+	t.CreateFileLines("math/R-date/Makefile",
+		MkRcsID,
+		"",
+		"R_PKGNAME=\tdate",
+		"R_PKGVER=\t1.2.3",
+		"COMMENT=\tR package for handling date arithmetic",
+		"MASTER_SITES=\t${MASTER_SITE_R_CRAN:=contrib/}", // Redundant; see math/R/Makefile.extension.
+		"",
+		".include \"../../math/R/Makefile.extension\"",
+		".include \"../../mk/bsd.pkg.mk\"")
+
+	G.CurrentDir = t.TempFilename("math/R-date")
+	G.CurPkgsrcdir = "../.."
+
+	// See Package.checkfilePackageMakefile
+	// See Scope.uncond
+	G.checkdirPackage(G.CurrentDir)
+
+	t.CheckOutputLines(
+		"NOTE: ~/math/R-date/Makefile:6: Definition of MASTER_SITES is redundant because of ../R/Makefile.extension:4.")
+}
diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go
index 3fcd0d4c4d4..c424cbc9029 100644
--- a/pkgtools/pkglint/files/patches.go
+++ b/pkgtools/pkglint/files/patches.go
@@ -143,7 +143,7 @@ func (ck *PatchChecker) checkUnifiedDiff(patchedFile string) {
 				linesToAdd--
 				ck.checklineAdded(text[1:], patchedFileType)
 			case hasPrefix(text, "\\"):
-				// \ No newline at end of file
+				// \ No newline at end of file (or a translation of that message)
 			default:
 				line.Errorf("Invalid line in unified patch hunk")
 				return
@@ -168,9 +168,10 @@ func (ck *PatchChecker) checkUnifiedDiff(patchedFile string) {
 		if !ck.isEmptyLine(line.Text) && !matches(line.Text, rePatchUniFileDel) {
 			line.Warnf("Empty line or end of file expected.")
 			Explain(
-				"This empty line makes the end of the patch clearly visible.",
-				"Otherwise the reader would have to count lines to see where",
-				"the patch ends.")
+				"This line is not part of the patch anymore, although it may",
+				"look so.  To make this situation clear, there should be an",
+				"empty line before this line.  If the line doesn't contain",
+				"useful information, it should be removed.")
 		}
 	}
 }
@@ -185,7 +186,13 @@ func (ck *PatchChecker) checkBeginDiff(line Line, patchedFiles int) {
 		Explain(
 			"Pkgsrc tries to have as few patches as possible.  Therefore, each",
 			"patch must document why it is necessary.  Typical reasons are",
-			"portability or security.",
+			"portability or security.  A typical documented patch looks like",
+			"this:",
+			"",
+			"\t$"+"NetBSD$",
+			"",
+			"\tPortability fixes for GCC 4.8 on Linux.",
+			"\tSee https://github.com/org/repo/issues/7",
 			"",
 			"Patches that are related to a security issue should mention the",
 			"corresponding CVE identifier.",
diff --git a/pkgtools/pkglint/files/patches_test.go b/pkgtools/pkglint/files/patches_test.go
index 774f7af9720..85705b871e9 100644
--- a/pkgtools/pkglint/files/patches_test.go
+++ b/pkgtools/pkglint/files/patches_test.go
@@ -2,6 +2,7 @@ package main
 
 import "gopkg.in/check.v1"
 
+// This is how each patch should look like.
 func (s *Suite) Test_ChecklinesPatch__with_comment(c *check.C) {
 	t := s.Init(c)
 
@@ -17,7 +18,7 @@ func (s *Suite) Test_ChecklinesPatch__with_comment(c *check.C) {
 		"@@ -5,3 +5,3 @@",
 		" context before",
 		"-old line",
-		"+old line",
+		"+new line",
 		" context after")
 
 	ChecklinesPatch(lines)
@@ -25,6 +26,8 @@ func (s *Suite) Test_ChecklinesPatch__with_comment(c *check.C) {
 	t.CheckOutputEmpty()
 }
 
+// To make the patch comment clearly visible, it should be surrounded by empty lines.
+// The missing empty lines are inserted by pkglint.
 func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) {
 	t := s.Init(c)
 
@@ -36,12 +39,13 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) {
 		"@@ -5,3 +5,3 @@",
 		" context before",
 		"-old line",
-		"+old line",
+		"+new line",
 		" context after")
 	t.SetupFileLines("distinfo",
 		RcsID,
 		"",
-		"SHA1 (some patch) = 87ffcaaa0b0677ec679fff612b44df1af05f04df") // Taken from breakpoint at AutofixDistinfo
+		// The hash is taken from a breakpoint at the beginning of AutofixDistinfo, oldSha1
+		"SHA1 (some patch) = 49abd735b7e706ea9ed6671628bb54e91f7f5ffb")
 
 	t.SetupCommandLine("-Wall", "--autofix")
 	G.CurrentDir = t.TmpDir()
@@ -52,8 +56,8 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) {
 	t.CheckOutputLines(
 		"AUTOFIX: ~/patch-WithoutEmptyLines:2: Inserting a line \"\" before this line.",
 		"AUTOFIX: ~/patch-WithoutEmptyLines:3: Inserting a line \"\" before this line.",
-		"AUTOFIX: ~/distinfo:3: Replacing \"87ffcaaa0b0677ec679fff612b44df1af05f04df\" "+
-			"with \"a7c35294b3853da0acedf8a972cb266baa9582a3\".")
+		"AUTOFIX: ~/distinfo:3: Replacing \"49abd735b7e706ea9ed6671628bb54e91f7f5ffb\" "+
+			"with \"4938fc8c0b483dc2e33e741b0da883d199e78164\".")
 
 	t.CheckFileLines("patch-WithoutEmptyLines",
 		RcsID,
@@ -65,12 +69,38 @@ func (s *Suite) Test_ChecklinesPatch__without_empty_line__autofix(c *check.C) {
 		"@@ -5,3 +5,3 @@",
 		" context before",
 		"-old line",
-		"+old line",
+		"+new line",
 		" context after")
 	t.CheckFileLines("distinfo",
 		RcsID,
 		"",
-		"SHA1 (some patch) = a7c35294b3853da0acedf8a972cb266baa9582a3")
+		"SHA1 (some patch) = 4938fc8c0b483dc2e33e741b0da883d199e78164")
+}
+
+func (s *Suite) Test_ChecklinesPatch__no_comment_and_no_empty_lines(c *check.C) {
+	t := s.Init(c)
+
+	patchLines := t.SetupFileLines("patch-WithoutEmptyLines",
+		RcsID,
+		"--- file.orig",
+		"+++ file",
+		"@@ -1,1 +1,1 @@",
+		"-old line",
+		"+new line")
+
+	t.SetupCommandLine("-Wall")
+
+	ChecklinesPatch(patchLines)
+
+	// These duplicate notes are actually correct. There should be an
+	// empty line above the documentation and one below it. Since there
+	// is no documentation yet, the line number for above and below is
+	// the same. Outside of the testing environment, this duplicate
+	// diagnostic is suppressed; see LogVerbose.
+	t.CheckOutputLines(
+		"NOTE: ~/patch-WithoutEmptyLines:2: Empty line expected.",
+		"ERROR: ~/patch-WithoutEmptyLines:2: Each patch must be documented.",
+		"NOTE: ~/patch-WithoutEmptyLines:2: Empty line expected.")
 }
 
 func (s *Suite) Test_ChecklinesPatch__without_comment(c *check.C) {
@@ -94,6 +124,8 @@ func (s *Suite) Test_ChecklinesPatch__without_comment(c *check.C) {
 		"ERROR: patch-WithoutComment:3: Each patch must be documented.")
 }
 
+// Autogenerated git "comments" don't count as real comments since they
+// don't convey any intention of a human developer.
 func (s *Suite) Test_ChecklinesPatch__git_without_comment(c *check.C) {
 	t := s.Init(c)
 
@@ -125,6 +157,9 @@ func (s *Suite) Test_checklineOtherAbsolutePathname(c *check.C) {
 	t.CheckOutputEmpty()
 }
 
+// The output of BSD Make typically contains "*** Error code".
+// In some really good patches, this output is included in the patch comment,
+// to document why the patch is necessary.
 func (s *Suite) Test_ChecklinesPatch__error_code(c *check.C) {
 	t := s.Init(c)
 
@@ -157,8 +192,8 @@ func (s *Suite) Test_ChecklinesPatch__wrong_header_order(c *check.C) {
 		"Text",
 		"Text",
 		"",
-		"+++ file",      // Wrong
-		"--- file.orig", // Wrong
+		"+++ file",      // Wrong order
+		"--- file.orig", // Wrong order
 		"@@ -5,3 +5,3 @@",
 		" context before",
 		"-old line",
@@ -171,6 +206,7 @@ func (s *Suite) Test_ChecklinesPatch__wrong_header_order(c *check.C) {
 		"WARN: patch-WrongOrder:7: Unified diff headers should be first ---, then +++.")
 }
 
+// Context diffs are old and deprecated. Therefore pkglint doesn't check them thoroughly.
 func (s *Suite) Test_ChecklinesPatch__context_diff(c *check.C) {
 	t := s.Init(c)
 
@@ -228,6 +264,7 @@ func (s *Suite) Test_ChecklinesPatch__two_patched_files(c *check.C) {
 		"WARN: patch-aa: Contains patches for 2 files, should be only one.")
 }
 
+// The patch headers are only recognized as such if they appear directly below each other.
 func (s *Suite) Test_ChecklinesPatch__documentation_that_looks_like_patch_lines(c *check.C) {
 	t := s.Init(c)
 
@@ -308,6 +345,11 @@ func (s *Suite) Test_ChecklinesPatch__Makefile_with_absolute_pathnames(c *check.
 		"WARN: patch-unified:10: Found absolute pathname: /bin/cp",
 		"WARN: patch-unified:13: Found absolute pathname: /bin/cp")
 
+	// With extra warnings turned on, absolute paths in the context lines
+	// are also checked, to detect absolute paths that might be overlooked.
+	// TODO: Maybe this should only be checked if the patch changes
+	// an absolute path to a relative one, because otherwise these
+	// absolute paths may be intentional.
 	G.opts.WarnExtra = true
 
 	ChecklinesPatch(lines)
@@ -363,6 +405,8 @@ func (s *Suite) Test_ChecklinesPatch__no_newline(c *check.C) {
 	t.CheckOutputEmpty()
 }
 
+// Some patch files may end before reaching the expected line count (in this case 7 lines).
+// This is ok if only context lines are missing. These context lines are assumed to be empty lines.
 func (s *Suite) Test_ChecklinesPatch__empty_lines_left_out_at_eof(c *check.C) {
 	t := s.Init(c)
 
@@ -386,7 +430,7 @@ func (s *Suite) Test_ChecklinesPatch__empty_lines_left_out_at_eof(c *check.C) {
 	t.CheckOutputEmpty()
 }
 
-// In some context lines, the leading space character is missing.
+// In some context lines, the leading space character may be missing.
 // Since this is no problem for patch(1), pkglint also doesn't complain.
 func (s *Suite) Test_ChecklinesPatch__context_lines_with_tab_instead_of_space(c *check.C) {
 	t := s.Init(c)
diff --git a/pkgtools/pkglint/files/pkgsrc.go b/pkgtools/pkglint/files/pkgsrc.go
index 6960e226646..fdd6e7ac7f2 100644
--- a/pkgtools/pkglint/files/pkgsrc.go
+++ b/pkgtools/pkglint/files/pkgsrc.go
@@ -208,7 +208,7 @@ func (src *Pkgsrc) loadTools() {
 					}
 				}
 
-			} else if m, _, cond, _ := matchMkCond(text); m {
+			} else if m, _, cond, _, _ := matchMkCond(text); m {
 				switch cond {
 				case "if", "ifdef", "ifndef", "for":
 					condDepth++
diff --git a/pkgtools/pkglint/files/shell_test.go b/pkgtools/pkglint/files/shell_test.go
index e89ab8eec65..de03db762f0 100644
--- a/pkgtools/pkglint/files/shell_test.go
+++ b/pkgtools/pkglint/files/shell_test.go
@@ -116,7 +116,12 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine(c *check.C) {
 			"\t"+shellCommand)
 		shline := NewShellLine(G.Mk.mklines[0])
 
-		shline.CheckShellCommandLine(shline.mkline.ShellCommand())
+		G.Mk.ForEach(
+			func(mkline MkLine) bool {
+				shline.CheckShellCommandLine(shline.mkline.ShellCommand())
+				return true
+			},
+			func(mkline MkLine) {})
 	}
 
 	checkShellCommandLine("@# Comment")
@@ -262,9 +267,14 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine_strip(c *check.C) {
 	checkShellCommandLine := func(shellCommand string) {
 		G.Mk = t.NewMkLines("fname",
 			"\t"+shellCommand)
-		shline := NewShellLine(G.Mk.mklines[0])
 
-		shline.CheckShellCommandLine(shline.mkline.ShellCommand())
+		G.Mk.ForEach(
+			func(mkline MkLine) bool {
+				shline := NewShellLine(mkline)
+				shline.CheckShellCommandLine(mkline.ShellCommand())
+				return true
+			},
+			func(mkline MkLine) {})
 	}
 
 	checkShellCommandLine("${STRIP} executable")
@@ -380,12 +390,22 @@ func (s *Suite) Test_ShellLine_CheckShellCommandLine__implementation(c *check.C)
 	c.Check(tokens, deepEquals, []string{text})
 	c.Check(rest, equals, "")
 
-	shline.CheckWord(text, false)
+	G.Mk.ForEach(
+		func(mkline MkLine) bool {
+			shline.CheckWord(text, false)
+			return true
+		},
+		func(mkline MkLine) {})
 
 	t.CheckOutputLines(
 		"WARN: fname:1: Unknown shell command \"echo\".")
 
-	shline.CheckShellCommandLine(text)
+	G.Mk.ForEach(
+		func(mkline MkLine) bool {
+			shline.CheckShellCommandLine(text)
+			return true
+		},
+		func(mkline MkLine) {})
 
 	// No parse errors
 	t.CheckOutputLines(
diff --git a/pkgtools/pkglint/files/util.go b/pkgtools/pkglint/files/util.go
index c1b1781fccd..478943ae2aa 100644
--- a/pkgtools/pkglint/files/util.go
+++ b/pkgtools/pkglint/files/util.go
@@ -561,3 +561,83 @@ func naturalLess(str1, str2 string) bool {
 	// it sorts last.
 	return len1 < len2
 }
+
+// RedundantScope checks for redundant variable definitions and accidentally
+// overwriting variables. It tries to be as correct as possible by not flagging
+// anything that is defined conditionally. There may be some edge cases though
+// like defining PKGNAME, then evaluating it using :=, then defining it again.
+// This pattern is so error-prone that it should not appear in pkgsrc at all,
+// thus pkglint doesn't even expect it. (Well, except for the PKGNAME case,
+// but that's deep in the infrastructure and only affects the "nb13" extension.
+type RedundantScope struct {
+	vars        map[string]*redundantScopeVarinfo
+	condLevel   int
+	OnIgnore    func(old, new MkLine)
+	OnOverwrite func(old, new MkLine)
+}
+type redundantScopeVarinfo struct {
+	mkline MkLine
+	value  string
+}
+
+func NewRedundantScope() *RedundantScope {
+	return &RedundantScope{vars: make(map[string]*redundantScopeVarinfo)}
+}
+
+func (s *RedundantScope) Handle(mkline MkLine) {
+	switch {
+	case mkline.IsVarassign():
+		varname := mkline.Varname()
+		if s.condLevel != 0 {
+			s.vars[varname] = nil
+			break
+		}
+
+		op := mkline.Op()
+		value := mkline.Value()
+		valueNovar := mkline.WithoutMakeVariables(value)
+		if op == opAssignEval && value == valueNovar {
+			op = opAssign // They are effectively the same in this case.
+		}
+		existing, found := s.vars[varname]
+		if !found {
+			if op == opAssignShell || op == opAssignEval {
+				s.vars[varname] = nil
+			} else {
+				if op == opAssignAppend {
+					value = " " + value
+				}
+				s.vars[varname] = &redundantScopeVarinfo{mkline, value}
+			}
+		} else if existing != nil {
+			switch op {
+			case opAssign:
+				if s.OnOverwrite != nil {
+					s.OnOverwrite(existing.mkline, mkline)
+				}
+				existing.value = value
+			case opAssignAppend:
+				existing.value += " " + value
+			case opAssignDefault:
+				if s.OnIgnore != nil {
+					s.OnIgnore(existing.mkline, mkline)
+				}
+			case opAssignShell:
+			case opAssignEval:
+				s.vars[varname] = nil
+			}
+		}
+
+	case mkline.IsCond():
+		switch mkline.Directive() {
+		case "for", "if", "ifdef", "ifndef":
+			s.condLevel++
+		case "endfor", "endif":
+			s.condLevel--
+		}
+	}
+}
+
+func (s *RedundantScope) IsConditional(varname string) bool {
+	return s.vars[varname] != nil
+}
diff --git a/pkgtools/pkglint/files/vartypecheck.go b/pkgtools/pkglint/files/vartypecheck.go
index 0b49bd9e99a..05b4f5daf57 100644
--- a/pkgtools/pkglint/files/vartypecheck.go
+++ b/pkgtools/pkglint/files/vartypecheck.go
@@ -274,6 +274,14 @@ func (cv *VartypeCheck) Dependency() {
 	if m, inside := match1(wildcard, `^\[(.*)\]\*$`); m {
 		if inside != "0-9" {
 			line.Warnf("Only [0-9]* is allowed in the numeric part of a dependency.")
+			Explain(
+				"The pattern -[0-9] means any version.  All other version patterns",
+				"should be expressed using the comparison operators like < or >= or",
+				"even >=2<3.",
+				"",
+				"Patterns like -[0-7] will only match the first digit of the version",
+				"number and will not do the correct thing when the package reaches",
+				"version 10.")
 		}
 
 	} else if m, ver, suffix := match2(wildcard, `^(\d\w*(?:\.\w+)*)(\.\*|\{,nb\*\}|\{,nb\[0-9\]\*\}|\*|)$`); m {
@@ -956,10 +964,12 @@ func (cv *VartypeCheck) Tool() {
 		switch tooldep {
 		case "", "bootstrap", "build", "pkgsrc", "run", "test":
 		default:
-			cv.Line.Errorf("Unknown tool dependency %q. Use one of \"bootstrap\", \"build\", \"pkgsrc\" or \"run\" or \"test\".", tooldep)
+			cv.Line.Errorf("Unknown tool dependency %q. Use one of \"bootstrap\", \"build\", \"pkgsrc\", \"run\" or \"test\".", tooldep)
 		}
-	} else if cv.Op != opUseMatch {
-		cv.Line.Errorf("Invalid tool syntax: %q.", cv.Value)
+	} else if cv.Op != opUseMatch && cv.Value == cv.ValueNoVar {
+		cv.Line.Errorf("Malformed tool dependency: %q.", cv.Value)
+		Explain(
+			"A tool dependency typically looks like \"sed\" or \"sed:run\".")
 	}
 }
 
diff --git a/pkgtools/pkglint/files/vartypecheck_test.go b/pkgtools/pkglint/files/vartypecheck_test.go
index ef07963bf47..cfa20db2daa 100644
--- a/pkgtools/pkglint/files/vartypecheck_test.go
+++ b/pkgtools/pkglint/files/vartypecheck_test.go
@@ -570,11 +570,14 @@ func (s *Suite) Test_VartypeCheck_Tool(c *check.C) {
 
 	runVartypeChecks(t, "USE_TOOLS", opAssignAppend, (*VartypeCheck).Tool,
 		"tool3:run",
-		"tool2:unknown")
+		"tool2:unknown",
+		"${t}",
+		"mal:formed:tool")
 
 	t.CheckOutputLines(
-		"ERROR: fname:2: Unknown tool dependency \"unknown\". " +
-			"Use one of \"bootstrap\", \"build\", \"pkgsrc\" or \"run\" or \"test\".")
+		"ERROR: fname:2: Unknown tool dependency \"unknown\". "+
+			"Use one of \"bootstrap\", \"build\", \"pkgsrc\", \"run\" or \"test\".",
+		"ERROR: fname:4: Malformed tool dependency: \"mal:formed:tool\".")
 
 	runVartypeChecks(t, "USE_TOOLS.NetBSD", opAssignAppend, (*VartypeCheck).Tool,
 		"tool3:run",
@@ -582,7 +585,7 @@ func (s *Suite) Test_VartypeCheck_Tool(c *check.C) {
 
 	t.CheckOutputLines(
 		"ERROR: fname:2: Unknown tool dependency \"unknown\". " +
-			"Use one of \"bootstrap\", \"build\", \"pkgsrc\" or \"run\" or \"test\".")
+			"Use one of \"bootstrap\", \"build\", \"pkgsrc\", \"run\" or \"test\".")
 }
 
 func (s *Suite) Test_VartypeCheck_VariableName(c *check.C) {