Split PlistLine.checkPathname into smaller functions

12 files changed, 123 insertions, 102 deletions

diff --git a/pkgtools/pkglint/files/expecter.go b/pkgtools/pkglint/files/expecter.go
index 81fa1fcd19c..d84273d333b 100644
--- a/pkgtools/pkglint/files/expecter.go
+++ b/pkgtools/pkglint/files/expecter.go
@@ -1,6 +1,6 @@
 package main
 
-// High-level iterating through lines and checking them.
+// Expecter records the state when checking a list of lines from top to bottom.
 type Expecter struct {
 	lines []*Line
 	index int
diff --git a/pkgtools/pkglint/files/globaldata.go b/pkgtools/pkglint/files/globaldata.go
index bc6299c50b6..38c8f2bbffc 100644
--- a/pkgtools/pkglint/files/globaldata.go
+++ b/pkgtools/pkglint/files/globaldata.go
@@ -6,7 +6,7 @@ import (
 	"sort"
 )
 
-// Constant data that is loaded once.
+// GlobalData contains data describing pkgsrc as a whole.
 type GlobalData struct {
 	pkgsrcdir           string              // Relative to the current working directory.
 	masterSiteUrls      map[string]string   // "https://github.com/" => "MASTER_SITE_GITHUB"
@@ -22,12 +22,12 @@ type GlobalData struct {
 	suggestedUpdates    []SuggestedUpdate   //
 	suggestedWipUpdates []SuggestedUpdate   //
 	lastChange          map[string]*Change  //
-	userDefinedVars     map[string]*Line    //
+	userDefinedVars     map[string]*Line    // varname => line (after calling parselineMk on it)
 	deprecated          map[string]string   //
 	vartypes            map[string]*Vartype // varcanon => type
 }
 
-// A change entry from doc/CHANGES-*
+// Change is a change entry from the `doc/CHANGES-*` files.
 type Change struct {
 	line    *Line
 	action  string
@@ -37,7 +37,7 @@ type Change struct {
 	date    string
 }
 
-// From the doc/TODO file.
+// SuggestedUpdate is from the `doc/TODO` file.
 type SuggestedUpdate struct {
 	line    *Line
 	pkgname string
@@ -357,7 +357,7 @@ func (gd *GlobalData) loadUserDefinedVars() {
 	gd.userDefinedVars = make(map[string]*Line)
 	for _, line := range lines {
 		parselineMk(line)
-		if m, varname, _, _, _ := matchVarassign(line.text); m {
+		if varname, ok := line.extra["varname"].(string); ok {
 			gd.userDefinedVars[varname] = line
 		}
 	}
diff --git a/pkgtools/pkglint/files/line.go b/pkgtools/pkglint/files/line.go
index 66eb1cbef45..d5d53398698 100644
--- a/pkgtools/pkglint/files/line.go
+++ b/pkgtools/pkglint/files/line.go
@@ -137,10 +137,10 @@ func (ln *Line) replaceRegex(from, to string) {
 	}
 }
 
-func (line *Line) noteAutofix(format string, args ...interface{}) {
-	line.changed = true
+func (ln *Line) noteAutofix(format string, args ...interface{}) {
+	ln.changed = true
 	if G.opts.Autofix || G.opts.PrintAutofix {
-		line.notef(format, args...)
+		ln.notef(format, args...)
 	}
 	G.autofixAvailable = true
 }
diff --git a/pkgtools/pkglint/files/makefiles.go b/pkgtools/pkglint/files/makefiles.go
index 681712e7105..faf95c41b71 100644
--- a/pkgtools/pkglint/files/makefiles.go
+++ b/pkgtools/pkglint/files/makefiles.go
@@ -394,11 +394,11 @@ func ChecklinesMk(lines []*Line) {
 					line.notef("For efficiency reasons, please include bsd.fast.prefs.mk instead of bsd.prefs.mk.")
 				}
 				if G.pkgContext != nil {
-					G.pkgContext.seen_bsd_prefs_mk = true
+					G.pkgContext.seenBsdPrefsMk = true
 				}
 			} else if includefile == "../../mk/bsd.fast.prefs.mk" {
 				if G.pkgContext != nil {
-					G.pkgContext.seen_bsd_prefs_mk = true
+					G.pkgContext.seenBsdPrefsMk = true
 				}
 			}
 
diff --git a/pkgtools/pkglint/files/mkcontext.go b/pkgtools/pkglint/files/mkcontext.go
index 6c8ac7a9337..5385e7f6151 100644
--- a/pkgtools/pkglint/files/mkcontext.go
+++ b/pkgtools/pkglint/files/mkcontext.go
@@ -1,6 +1,6 @@
 package main
 
-// Context of the Makefile that is currently checked.
+// MkContext contains data for the Makefile (or *.mk) that is currently checked.
 type MkContext struct {
 	forVars     map[string]bool  // The variables currently used in .for loops
 	indentation []int            // Indentation depth of preprocessing directives
diff --git a/pkgtools/pkglint/files/mkline.go b/pkgtools/pkglint/files/mkline.go
index e72005ec76d..b30ec6ff56d 100644
--- a/pkgtools/pkglint/files/mkline.go
+++ b/pkgtools/pkglint/files/mkline.go
@@ -282,7 +282,7 @@ func checklineMkVarassign(line *Line, varname, op, value, comment string) {
 
 	checklineMkVardef(line, varname, op)
 
-	if G.opts.WarnExtra && op == "?=" && G.pkgContext != nil && !G.pkgContext.seen_bsd_prefs_mk {
+	if G.opts.WarnExtra && op == "?=" && G.pkgContext != nil && !G.pkgContext.seenBsdPrefsMk {
 		switch varbase {
 		case "BUILDLINK_PKGSRCDIR", "BUILDLINK_DEPMETHOD", "BUILDLINK_ABI_DEPENDS":
 			// FIXME: What about these ones? They occur quite often.
diff --git a/pkgtools/pkglint/files/patches.go b/pkgtools/pkglint/files/patches.go
index b3c82439abd..b6c620e9b67 100644
--- a/pkgtools/pkglint/files/patches.go
+++ b/pkgtools/pkglint/files/patches.go
@@ -132,7 +132,6 @@ const (
 	rePatchUniLineNoNewline = `^\\ No newline at end of file$`
 )
 
-// See doc/statemachine.patch.dia
 type PatchState string
 
 const (
diff --git a/pkgtools/pkglint/files/pkgcontext.go b/pkgtools/pkglint/files/pkgcontext.go
index 23374666b9c..43b1d8b36e1 100644
--- a/pkgtools/pkglint/files/pkgcontext.go
+++ b/pkgtools/pkglint/files/pkgcontext.go
@@ -1,6 +1,6 @@
 package main
 
-// Context of the package that is currently checked.
+// PkgContext contains data for the package that is currently checked.
 type PkgContext struct {
 	pkgpath              string // e.g. "category/pkgdir"
 	pkgdir               string // PKGDIR from the package Makefile
@@ -11,7 +11,7 @@ type PkgContext struct {
 	effectivePkgbase     string // The effective PKGNAME without the version
 	effectivePkgversion  string // The version part of the effective PKGNAME
 	effectivePkgnameLine *Line  // The origin of the three effective_* values
-	seen_bsd_prefs_mk    bool   // Has bsd.prefs.mk already been included?
+	seenBsdPrefsMk       bool   // Has bsd.prefs.mk already been included?
 
 	vardef             map[string]*Line // varname => line
 	varuse             map[string]*Line // varname => line
diff --git a/pkgtools/pkglint/files/plist.go b/pkgtools/pkglint/files/plist.go
index 40d5fec0897..0d5ea3b86d6 100644
--- a/pkgtools/pkglint/files/plist.go
+++ b/pkgtools/pkglint/files/plist.go
@@ -165,19 +165,7 @@ func (pline *PlistLine) checkPathname(pctx *PlistContext, fullname string) {
 	sdirname, basename := path.Split(fullname)
 	dirname := strings.TrimSuffix(sdirname, "/")
 
-	if G.opts.WarnPlistSort && hasAlnumPrefix(text) && !containsVarRef(text) {
-		if pctx.lastFname != "" {
-			if pctx.lastFname > text {
-				line.warnf("%q should be sorted before %q.", text, pctx.lastFname)
-				line.explain(
-					"For aesthetic reasons, the files in the PLIST should be sorted",
-					"alphabetically.")
-			} else if pctx.lastFname == text {
-				line.errorf("Duplicate filename.")
-			}
-		}
-		pctx.lastFname = text
-	}
+	pline.checkSorted(pctx)
 
 	if contains(basename, "${IMAKE_MANNEWSUFFIX}") {
 		pline.warnAboutPlistImakeMannewsuffix()
@@ -188,20 +176,7 @@ func (pline *PlistLine) checkPathname(pctx *PlistContext, fullname string) {
 		line.warnf("The bin/ directory should not have subdirectories.")
 
 	case dirname == "bin":
-		switch {
-		case pctx.allFiles["man/man1/"+basename+".1"] != nil:
-		case pctx.allFiles["man/man6/"+basename+".6"] != nil:
-		case pctx.allFiles["${IMAKE_MAN_DIR}/"+basename+".${IMAKE_MANNEWSUFFIX}"] != nil:
-		default:
-			if G.opts.WarnExtra {
-				line.warnf("Manual page missing for bin/%s.", basename)
-				line.explain(
-					"All programs that can be run directly by the user should have a manual",
-					"page for quick reference. The programs in the bin/ directory should have",
-					"corresponding manual pages in section 1 (filename program.1), not in",
-					"section 8.")
-			}
-		}
+		pline.checkpathBin(pctx, basename)
 
 	case hasPrefix(text, "doc/"):
 		line.errorf("Documentation must be installed under share/doc, not doc.")
@@ -232,61 +207,13 @@ func (pline *PlistLine) checkPathname(pctx *PlistContext, fullname string) {
 		line.errorf("\"lib/locale\" must not be listed. Use ${PKGLOCALEDIR}/locale and set USE_PKGLOCALEDIR instead.")
 
 	case hasPrefix(text, "lib/"):
-		if m, dir, lib, ext := match3(text, `^(lib/(?:.*/)*)([^/]+)\.(so|a|la)$`); m {
-			if dir == "lib/" && !hasPrefix(lib, "lib") {
-				_ = G.opts.WarnExtra && line.warnf("Library filename does not start with \"lib\".")
-			}
-			if ext == "la" {
-				if G.pkgContext != nil && G.pkgContext.vardef["USE_LIBTOOL"] == nil {
-					line.warnf("Packages that install libtool libraries should define USE_LIBTOOL.")
-				}
-			}
-		}
+		pline.checkpathLib(pctx, basename)
 
 	case hasPrefix(text, "man/"):
-		if m, catOrMan, section, manpage, ext, gz := match5(text, `^man/(cat|man)(\w+)/(.*?)\.(\w+)(\.gz)?$`); m {
-
-			if !matches(section, `^[\dln]$`) {
-				line.warnf("Unknown section %q for manual page.", section)
-			}
-
-			if catOrMan == "cat" && pctx.allFiles["man/man"+section+"/"+manpage+"."+section] == nil {
-				line.warnf("Preformatted manual page without unformatted one.")
-			}
-
-			if catOrMan == "cat" {
-				if ext != "0" {
-					line.warnf("Preformatted manual pages should end in \".0\".")
-				}
-			} else {
-				if section != ext {
-					line.warnf("Mismatch between the section (%s) and extension (%s) of the manual page.", section, ext)
-				}
-			}
-
-			if gz != "" {
-				line.notef("The .gz extension is unnecessary for manual pages.")
-				line.explain(
-					"Whether the manual pages are installed in compressed form or not is",
-					"configured by the pkgsrc user. Compression and decompression takes place",
-					"automatically, no matter if the .gz extension is mentioned in the PLIST",
-					"or not.")
-			}
-		} else {
-			// maybe: line.warnf("Invalid filename %q for manual page.", text)
-		}
+		pline.checkpathMan(pctx)
 
 	case hasPrefix(text, "sbin/"):
-		binname := text[5:]
-
-		if pctx.allFiles["man/man8/"+binname+".8"] == nil && G.opts.WarnExtra {
-			line.warnf("Manual page missing for sbin/%s.", binname)
-			line.explain(
-				"All programs that can be run directly by the user should have a manual",
-				"page for quick reference. The programs in the sbin/ directory should have",
-				"corresponding manual pages in section 8 (filename program.8), not in",
-				"section 1.")
-		}
+		pline.checkpathSbin(pctx)
 
 	case hasPrefix(text, "share/applications/") && hasSuffix(text, ".desktop"):
 		f := "../../sysutils/desktop-file-utils/desktopdb.mk"
@@ -359,16 +286,111 @@ func (pline *PlistLine) checkPathname(pctx *PlistContext, fullname string) {
 			"This file is handled automatically by the INSTALL/DEINSTALL scripts,",
 			"since its contents changes frequently.")
 	}
+}
+
+func (pline *PlistLine) checkSorted(pctx *PlistContext) {
+	if text := pline.line.text; G.opts.WarnPlistSort && hasAlnumPrefix(text) && !containsVarRef(text) {
+		if pctx.lastFname != "" {
+			if pctx.lastFname > text {
+				pline.line.warnf("%q should be sorted before %q.", text, pctx.lastFname)
+				pline.line.explain(
+					"The files in the PLIST should be sorted alphabetically.")
+			} else if pctx.lastFname == text {
+				pline.line.errorf("Duplicate filename.")
+			}
+		}
+		pctx.lastFname = text
+	}
+}
+
+func (pline *PlistLine) checkpathBin(pctx *PlistContext, basename string) {
+	switch {
+	case pctx.allFiles["man/man1/"+basename+".1"] != nil:
+	case pctx.allFiles["man/man6/"+basename+".6"] != nil:
+	case pctx.allFiles["${IMAKE_MAN_DIR}/"+basename+".${IMAKE_MANNEWSUFFIX}"] != nil:
+	default:
+		if G.opts.WarnExtra {
+			pline.line.warnf("Manual page missing for bin/%s.", basename)
+			pline.line.explain(
+				"All programs that can be run directly by the user should have a manual",
+				"page for quick reference. The programs in the bin/ directory should have",
+				"corresponding manual pages in section 1 (filename program.1), not in",
+				"section 8.")
+		}
+	}
+}
+
+func (pline *PlistLine) checkpathLib(pctx *PlistContext, basename string) {
+	if m, dir, lib, ext := match3(pline.line.text, `^(lib/(?:.*/)*)([^/]+)\.(so|a|la)$`); m {
+		if dir == "lib/" && !hasPrefix(lib, "lib") {
+			_ = G.opts.WarnExtra && pline.line.warnf("Library filename does not start with \"lib\".")
+		}
+		if ext == "la" {
+			if G.pkgContext != nil && G.pkgContext.vardef["USE_LIBTOOL"] == nil {
+				pline.line.warnf("Packages that install libtool libraries should define USE_LIBTOOL.")
+			}
+		}
+	}
 
 	if contains(basename, ".a") || contains(basename, ".so") {
-		if m, basename := match1(text, `^(.*)(?:\.a|\.so[0-9.]*)$`); m {
-			if laLine := pctx.allFiles[basename+".la"]; laLine != nil {
-				line.warnf("Redundant library found. The libtool library is in line %s.", laLine)
+		if m, noext := match1(pline.line.text, `^(.*)(?:\.a|\.so[0-9.]*)$`); m {
+			if laLine := pctx.allFiles[noext+".la"]; laLine != nil {
+				pline.line.warnf("Redundant library found. The libtool library is in line %s.", laLine)
 			}
 		}
 	}
 }
 
+func (pline *PlistLine) checkpathMan(pctx *PlistContext) {
+	line := pline.line
+
+	m, catOrMan, section, manpage, ext, gz := match5(pline.line.text, `^man/(cat|man)(\w+)/(.*?)\.(\w+)(\.gz)?$`)
+	if !m {
+		// maybe: line.warnf("Invalid filename %q for manual page.", text)
+		return
+	}
+
+	if !matches(section, `^[\dln]$`) {
+		line.warnf("Unknown section %q for manual page.", section)
+	}
+
+	if catOrMan == "cat" && pctx.allFiles["man/man"+section+"/"+manpage+"."+section] == nil {
+		line.warnf("Preformatted manual page without unformatted one.")
+	}
+
+	if catOrMan == "cat" {
+		if ext != "0" {
+			line.warnf("Preformatted manual pages should end in \".0\".")
+		}
+	} else {
+		if section != ext {
+			line.warnf("Mismatch between the section (%s) and extension (%s) of the manual page.", section, ext)
+		}
+	}
+
+	if gz != "" {
+		line.notef("The .gz extension is unnecessary for manual pages.")
+		line.explain(
+			"Whether the manual pages are installed in compressed form or not is",
+			"configured by the pkgsrc user. Compression and decompression takes place",
+			"automatically, no matter if the .gz extension is mentioned in the PLIST",
+			"or not.")
+	}
+}
+
+func (pline *PlistLine) checkpathSbin(pctx *PlistContext) {
+	binname := pline.line.text[5:]
+
+	if pctx.allFiles["man/man8/"+binname+".8"] == nil && G.opts.WarnExtra {
+		pline.line.warnf("Manual page missing for sbin/%s.", binname)
+		pline.line.explain(
+			"All programs that can be run directly by the user should have a manual",
+			"page for quick reference. The programs in the sbin/ directory should have",
+			"corresponding manual pages in section 8 (filename program.8), not in",
+			"section 1.")
+	}
+}
+
 func (pline *PlistLine) warnAboutPlistImakeMannewsuffix() {
 	line := pline.line
 
diff --git a/pkgtools/pkglint/files/shell.go b/pkgtools/pkglint/files/shell.go
index 190bb7614d8..e2157b034f7 100644
--- a/pkgtools/pkglint/files/shell.go
+++ b/pkgtools/pkglint/files/shell.go
@@ -55,7 +55,7 @@ const (
 	reShVarassign = `^([A-Z_a-z]\w*)=`
 )
 
-// Shell command state; see doc/statemachine.shellcmd.dia.
+// ShellCommandState
 type scState string
 
 const (
diff --git a/pkgtools/pkglint/files/substcontext.go b/pkgtools/pkglint/files/substcontext.go
index d442ec5ca82..654eae85adc 100644
--- a/pkgtools/pkglint/files/substcontext.go
+++ b/pkgtools/pkglint/files/substcontext.go
@@ -1,7 +1,7 @@
 package main
 
-// Records the state of a block of variable assignments that make up a SUBST
-// class (see mk/subst.mk).
+// SubstContext records the state of a block of variable assignments
+// that make up a SUBST class (see `mk/subst.mk`).
 type SubstContext struct {
 	id        string
 	stage     string
diff --git a/pkgtools/pkglint/files/vartype.go b/pkgtools/pkglint/files/vartype.go
index 865268c944a..de1b723d39e 100644
--- a/pkgtools/pkglint/files/vartype.go
+++ b/pkgtools/pkglint/files/vartype.go
@@ -27,8 +27,8 @@ type AclEntry struct {
 	permissions string // Some of: "a"ppend, "d"efault, "s"et; "p"reprocessing, "u"se
 }
 
-// Whether the type definition is guessed (based on the variable name)
-// or explicitly defined (see vardefs.go).
+// Guessed says whether the type definition is guessed (based on the
+// variable name) or explicitly defined (see vardefs.go).
 type Guessed bool
 
 const (