Implement string interpolation quote linter

This commit implement a linter checking for the usage of '%s' and "%s".
Using manual quoting and bare string interpolation is not safe. We have
a style guide for it here:
https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-q-when-interpolating-strings

7 files changed, 165 insertions, 0 deletions

diff --git a/.golangci.yml b/.golangci.yml
index 747e64a03..884a07a55 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -135,6 +135,11 @@ linters-settings:
       begin: false
     tb:
       begin: false
+  custom:
+    gitaly-styles:
+      path: ./_build/tools/gitaly-linters/styles.so
+      description: A collection of linters to enforce Gitaly style guide in STYLE.md
+      original-url: gitlab.com/gitlab-org/gitaly
 
 issues:
   exclude-use-default: false
diff --git a/tools/gitaly-linters/styles/go.mod b/tools/gitaly-linters/styles/go.mod
new file mode 100644
index 000000000..b46b66c1f
--- /dev/null
+++ b/tools/gitaly-linters/styles/go.mod
@@ -0,0 +1,10 @@
+module gitlab.com/gitlab-org/gitaly/tools/gitaly-linters/styles
+
+go 1.18
+
+require golang.org/x/tools v0.5.0
+
+require (
+	golang.org/x/mod v0.7.0 // indirect
+	golang.org/x/sys v0.4.0 // indirect
+)
diff --git a/tools/gitaly-linters/styles/go.sum b/tools/gitaly-linters/styles/go.sum
new file mode 100644
index 000000000..2baf09cbc
--- /dev/null
+++ b/tools/gitaly-linters/styles/go.sum
@@ -0,0 +1,7 @@
+golang.org/x/mod v0.7.0 h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA=
+golang.org/x/mod v0.7.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs=
+golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o=
+golang.org/x/sys v0.4.0 h1:Zr2JFtRQNX3BCZ8YtxRE9hNJYC8J6I1MVbMg6owUp18=
+golang.org/x/sys v0.4.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/tools v0.5.0 h1:+bSpV5HIeWkuvgaMfI3UmKRThoTA5ODJTUd8T17NO+4=
+golang.org/x/tools v0.5.0/go.mod h1:N+Kgy78s5I24c24dU8OfWNEotWjutIs8SnJvn5IDq+k=
diff --git a/tools/gitaly-linters/styles/interpolation/quote.go b/tools/gitaly-linters/styles/interpolation/quote.go
new file mode 100644
index 000000000..1adb88135
--- /dev/null
+++ b/tools/gitaly-linters/styles/interpolation/quote.go
@@ -0,0 +1,74 @@
+package interpolation
+
+import (
+	"go/ast"
+	"go/token"
+	"golang.org/x/tools/go/analysis"
+	"regexp"
+	"strings"
+)
+
+// QuoteInterpolationAnalyzer implements an analyzer to detect manually quoted string interpolation
+// with '%s' and "%s". Quoting this way doesn't escape special characters such as endline and makes
+// debugging harder later. We encourage to use %q verb instead.
+//
+//   - Bad
+//     return fmt.Errorf("gl_id='%s' is invalid", glID)
+//
+//   - Bad
+//     fmt.Sprintf("fatal: not a git repository: '%s'", repoPath)
+//
+//   - Good
+//     return fmt.Errorf("gl_id=%q is invalid", glID)
+//
+//   - Good
+//     fmt.Sprintf("fatal: not a git repository: %q", repoPath)
+//
+// For more information:
+// https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-q-when-interpolating-strings
+var QuoteInterpolationAnalyzer = &analysis.Analyzer{
+	Name: "string_interpolation_quote",
+	Doc: `Unless it would lead to incorrect results, always use %q when
+	interpolating strings. For more information:
+	https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md#use-q-when-interpolating-strings`,
+	Run: runStringInterpolationQuoteAnalyzer,
+}
+
+// offendedFormatPattern matches string interpolation having '%s' and "%s" format
+var offendedFormatPattern = regexp.MustCompile(`['"]%s['"]`)
+
+func analyzeInterpolation(str *ast.BasicLit, pass *analysis.Pass) {
+	value := str.Value
+	if strings.HasPrefix(value, `'`) || strings.HasPrefix(value, `"`) {
+		value = value[1:]
+	}
+	if strings.HasSuffix(value, `'`) || strings.HasSuffix(value, `"`) {
+		value = value[:len(value)-1]
+	}
+	for _, index := range offendedFormatPattern.FindAllIndex([]byte(value), -1) {
+		start := token.Pos(int(str.Pos()) + index[0] + 1)
+		end := token.Pos(int(str.Pos()) + index[1])
+		pass.Report(analysis.Diagnostic{
+			Pos:            start,
+			End:            end,
+			Message:        "wrapping %s verb with quotes is not encouraged, please use %q instead",
+			SuggestedFixes: nil,
+		})
+	}
+}
+
+func runStringInterpolationQuoteAnalyzer(pass *analysis.Pass) (interface{}, error) {
+	for _, file := range pass.Files {
+		ast.Inspect(file, func(n ast.Node) bool {
+			if call, ok := n.(*ast.CallExpr); ok {
+				if len(call.Args) >= 1 {
+					if str, ok := call.Args[0].(*ast.BasicLit); ok && str.Kind == token.STRING {
+						analyzeInterpolation(str, pass)
+					}
+				}
+			}
+			return true
+		})
+	}
+	return nil, nil
+}
diff --git a/tools/gitaly-linters/styles/interpolation/quote_test.go b/tools/gitaly-linters/styles/interpolation/quote_test.go
new file mode 100644
index 000000000..56e006285
--- /dev/null
+++ b/tools/gitaly-linters/styles/interpolation/quote_test.go
@@ -0,0 +1,18 @@
+package interpolation
+
+import (
+	"golang.org/x/tools/go/analysis/analysistest"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestQuoteInterpolationAnalyzer(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("Failed to get wd: %s", err)
+	}
+
+	testdata := filepath.Join(filepath.Dir(wd), "testdata")
+	analysistest.Run(t, testdata, QuoteInterpolationAnalyzer, "quote")
+}
diff --git a/tools/gitaly-linters/styles/lint.go b/tools/gitaly-linters/styles/lint.go
new file mode 100644
index 000000000..e2f61eefc
--- /dev/null
+++ b/tools/gitaly-linters/styles/lint.go
@@ -0,0 +1,24 @@
+package main
+
+import (
+	"gitlab.com/gitlab-org/gitaly/tools/gitaly-linters/styles/interpolation"
+	"golang.org/x/tools/go/analysis"
+)
+
+type analyzerPlugin struct{}
+
+func (*analyzerPlugin) GetAnalyzers() []*analysis.Analyzer {
+	return []*analysis.Analyzer{
+		interpolation.QuoteInterpolationAnalyzer,
+	}
+}
+
+// AnalyzerPlugin is a convention of golangci-lint to implement a custom linter. This variable
+// must implement `AnalyzerPlugin` interface:
+//
+//	type AnalyzerPlugin interface {
+//		GetAnalyzers() []*analysis.Analyzer
+//	}
+//
+// For more information, please visit https://golangci-lint.run/contributing/new-linters/
+var AnalyzerPlugin analyzerPlugin
diff --git a/tools/gitaly-linters/styles/testdata/src/quote/quote_test.go b/tools/gitaly-linters/styles/testdata/src/quote/quote_test.go
new file mode 100644
index 000000000..d0e832c59
--- /dev/null
+++ b/tools/gitaly-linters/styles/testdata/src/quote/quote_test.go
@@ -0,0 +1,27 @@
+package quote
+
+import "fmt"
+
+// This file is the test fixture for Gitaly linters
+
+func quoteOkay() {
+	fmt.Printf("hello world: %q", "today is a good day")
+	fmt.Printf("hello world: %d", 123)
+	fmt.Printf("%s something", "hello")
+	fmt.Printf("hello world: %s", "this is good")
+	fmt.Printf("hello world: %s something", "this is good")
+}
+
+func quoteNotOkay() {
+	fmt.Printf("hello world: '%s'", "today is a good day") //  want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, "today is a good day") // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, "today is a good day") // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	str := `so is
+tomorrow`
+	fmt.Printf("hello world: '%s'", str) // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, str) // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf(`hello world: "%s"`, str) // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+
+	fmt.Printf("hello world:%s %s '%s'", "today", "is a", "good day") // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+	fmt.Printf("hello world: %d '%s'", 123, "today is a good day")    // want "wrapping %s verb with quotes is not encouraged, please use %q instead"
+}