add gocritic/ruleguard checks back again, cleanup code.

- remove some duplicated code
- reported a bug, separately fixed in #13664
- using strings.ReplaceAll() when needed
- using filepath.ToSlash() use when needed
- remove all non-Go style comments from the codebase

.golangci.yml

@@ -23,12 +23,19 @@ linters:
     - structcheck
     - unconvert
     - varcheck
+    - gocritic
 
 issues:
   exclude-use-default: false
   exclude:
       - should have a package comment
       - error strings should not be capitalized or end with punctuation or a newline
+      # todo fix these when we get enough time.
+      - "singleCaseSwitch: should rewrite switch statement to if statement"
+      - "unlambda: replace"
+      - "captLocal:"
+      - "ifElseChain:"
+      - "elseif:"
 
 service:
-  golangci-lint-version: 1.20.0 # use the fixed version to not introduce new linters unexpectedly
+  golangci-lint-version: 1.40.1 # use the fixed version to not introduce new linters unexpectedly

Makefile

@@ -19,7 +19,7 @@ help: ## print this help
 
 getdeps: ## fetch necessary dependencies
 	@mkdir -p ${GOPATH}/bin
-	@echo "Installing golangci-lint" && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH)/bin v1.40.1
+	@echo "Installing golangci-lint" && curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH)/bin v1.43.0
 	@echo "Installing msgp" && go install -v github.com/tinylib/msgp@latest
 	@echo "Installing stringer" && go install -v golang.org/x/tools/cmd/stringer@latest
 

cmd/admin-handlers-config-kv.go

@@ -215,11 +215,9 @@ func (a adminAPIHandlers) ClearConfigHistoryKVHandler(w http.ResponseWriter, r *
 				return
 			}
 		}
-	} else {
-		if err := delServerConfigHistory(ctx, objectAPI, restoreID); err != nil {
-			writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-			return
-		}
+	} else if err := delServerConfigHistory(ctx, objectAPI, restoreID); err != nil {
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		return
 	}
 }
 

cmd/admin-handlers-users.go

@@ -323,11 +323,12 @@ func (a adminAPIHandlers) SetGroupStatus(w http.ResponseWriter, r *http.Request)
 	status := vars["status"]
 
 	var err error
-	if status == statusEnabled {
+	switch status {
+	case statusEnabled:
 		err = globalIAMSys.SetGroupStatus(ctx, group, true)
-	} else if status == statusDisabled {
+	case statusDisabled:
 		err = globalIAMSys.SetGroupStatus(ctx, group, false)
-	} else {
+	default:
 		err = errInvalidArgument
 	}
 	if err != nil {

cmd/admin-handlers.go

@@ -1356,6 +1356,7 @@ func getServerInfo(ctx context.Context, r *http.Request) madmin.InfoMessage {
 	ldap := madmin.LDAP{}
 	if globalLDAPConfig.Enabled {
 		ldapConn, err := globalLDAPConfig.Connect()
+		//nolint:gocritic
 		if err != nil {
 			ldap.Status = string(madmin.ItemOffline)
 		} else if ldapConn == nil {
@@ -1636,8 +1637,8 @@ func (a adminAPIHandlers) HealthInfoHandler(w http.ResponseWriter, r *http.Reque
 	anonymizeCmdLine := func(cmdLine string) string {
 		if !globalIsDistErasure {
 			// FS mode - single server - hard code to `server1`
-			anonCmdLine := strings.Replace(cmdLine, globalLocalNodeName, "server1", -1)
-			return strings.Replace(anonCmdLine, globalMinioConsoleHost, "server1", -1)
+			anonCmdLine := strings.ReplaceAll(cmdLine, globalLocalNodeName, "server1")
+			return strings.ReplaceAll(anonCmdLine, globalMinioConsoleHost, "server1")
 		}
 
 		// Server start command regex groups:

cmd/admin-heal-ops.go

@@ -491,7 +491,7 @@ func (h *healSequence) getScannedItemsCount() int64 {
 	defer h.mutex.RUnlock()
 
 	for _, v := range h.scannedItemsMap {
-		count = count + v
+		count += v
 	}
 	return count
 }

cmd/admin-router.go

@@ -43,8 +43,6 @@ func registerAdminRouter(router *mux.Router, enableConfigOps bool) {
 	// Admin router
 	adminRouter := router.PathPrefix(adminPathPrefix).Subrouter()
 
-	/// Service operations
-
 	adminVersions := []string{
 		adminAPIVersionPrefix,
 	}
@@ -71,17 +69,14 @@ func registerAdminRouter(router *mux.Router, enableConfigOps bool) {
 		adminRouter.Methods(http.MethodGet).Path(adminVersion + "/datausageinfo").HandlerFunc(gz(httpTraceAll(adminAPI.DataUsageInfoHandler)))
 
 		if globalIsDistErasure || globalIsErasure {
-			/// Heal operations
+			// Heal operations
 
 			// Heal processing endpoint.
 			adminRouter.Methods(http.MethodPost).Path(adminVersion + "/heal/").HandlerFunc(gz(httpTraceAll(adminAPI.HealHandler)))
 			adminRouter.Methods(http.MethodPost).Path(adminVersion + "/heal/{bucket}").HandlerFunc(gz(httpTraceAll(adminAPI.HealHandler)))
 			adminRouter.Methods(http.MethodPost).Path(adminVersion + "/heal/{bucket}/{prefix:.*}").HandlerFunc(gz(httpTraceAll(adminAPI.HealHandler)))
 
 			adminRouter.Methods(http.MethodPost).Path(adminVersion + "/background-heal/status").HandlerFunc(gz(httpTraceAll(adminAPI.BackgroundHealStatusHandler)))
-
-			/// Health operations
-
 		}
 
 		// Profiling operations
@@ -106,7 +101,7 @@ func registerAdminRouter(router *mux.Router, enableConfigOps bool) {
 			adminRouter.Methods(http.MethodPut).Path(adminVersion+"/restore-config-history-kv").HandlerFunc(gz(httpTraceHdrs(adminAPI.RestoreConfigHistoryKVHandler))).Queries("restoreId", "{restoreId:.*}")
 		}
 
-		/// Config import/export bulk operations
+		// Config import/export bulk operations
 		if enableConfigOps {
 			// Get config
 			adminRouter.Methods(http.MethodGet).Path(adminVersion + "/config").HandlerFunc(gz(httpTraceHdrs(adminAPI.GetConfigHandler)))

cmd/api-errors.go

@@ -973,7 +973,7 @@ var errorCodes = errorCodeMap{
 		HTTPStatusCode: http.StatusNotFound,
 	},
 
-	/// Bucket notification related errors.
+	// Bucket notification related errors.
 	ErrEventNotification: {
 		Code:           "InvalidArgument",
 		Description:    "A specified event is not supported for notifications.",
@@ -1120,14 +1120,14 @@ var errorCodes = errorCodeMap{
 		HTTPStatusCode: http.StatusForbidden,
 	},
 
-	/// S3 extensions.
+	// S3 extensions.
 	ErrContentSHA256Mismatch: {
 		Code:           "XAmzContentSHA256Mismatch",
 		Description:    "The provided 'x-amz-content-sha256' header does not match what was computed.",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
 
-	/// MinIO extensions.
+	// MinIO extensions.
 	ErrStorageFull: {
 		Code:           "XMinioStorageFull",
 		Description:    "Storage backend has reached its minimum free disk threshold. Please delete a few objects to proceed.",
@@ -1370,7 +1370,7 @@ var errorCodes = errorCodeMap{
 		Description:    "The continuation token provided is incorrect",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
-	//S3 Select API Errors
+	// S3 Select API Errors
 	ErrEmptyRequestBody: {
 		Code:           "EmptyRequestBody",
 		Description:    "Request body cannot be empty.",
@@ -2074,6 +2074,7 @@ func toAPIErrorCode(ctx context.Context, err error) (apiErr APIErrorCode) {
 	default:
 		var ie, iw int
 		// This work-around is to handle the issue golang/go#30648
+		//nolint:gocritic
 		if _, ferr := fmt.Fscanf(strings.NewReader(err.Error()),
 			"request declared a Content-Length of %d but only wrote %d bytes",
 			&ie, &iw); ferr != nil {
@@ -2229,6 +2230,7 @@ func toAPIError(ctx context.Context, err error) APIError {
 			}
 			// Add more Gateway SDKs here if any in future.
 		default:
+			//nolint:gocritic
 			if errors.Is(err, errMalformedEncoding) {
 				apiErr = APIError{
 					Code:           "BadRequest",

cmd/api-router.go

@@ -301,7 +301,8 @@ func registerAPIRouter(router *mux.Router) {
 		router.Methods(http.MethodPost).Path("/{object:.+}").HandlerFunc(
 			collectAPIStats("restoreobject", maxClients(gz(httpTraceAll(api.PostRestoreObjectHandler))))).Queries("restore", "")
 
-		/// Bucket operations
+		// Bucket operations
+
 		// GetBucketLocation
 		router.Methods(http.MethodGet).HandlerFunc(
 			collectAPIStats("getbucketlocation", maxClients(gz(httpTraceAll(api.GetBucketLocationHandler))))).Queries("location", "")
@@ -355,7 +356,7 @@ func registerAPIRouter(router *mux.Router) {
 		// GetBucketTaggingHandler
 		router.Methods(http.MethodGet).HandlerFunc(
 			collectAPIStats("getbuckettagging", maxClients(gz(httpTraceAll(api.GetBucketTaggingHandler))))).Queries("tagging", "")
-		//DeleteBucketWebsiteHandler
+		// DeleteBucketWebsiteHandler
 		router.Methods(http.MethodDelete).HandlerFunc(
 			collectAPIStats("deletebucketwebsite", maxClients(gz(httpTraceAll(api.DeleteBucketWebsiteHandler))))).Queries("website", "")
 		// DeleteBucketTaggingHandler
@@ -452,7 +453,7 @@ func registerAPIRouter(router *mux.Router) {
 			collectAPIStats("listobjectsv1", maxClients(gz(httpTraceAll(api.ListObjectsV1Handler)))))
 	}
 
-	/// Root operation
+	// Root operation
 
 	// ListenNotification
 	apiRouter.Methods(http.MethodGet).Path(SlashSeparator).HandlerFunc(

cmd/bucket-handlers.go

@@ -903,7 +903,7 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h
 	if fileName != "" && strings.Contains(formValues.Get("Key"), "${filename}") {
 		// S3 feature to replace ${filename} found in Key form field
 		// by the filename attribute passed in multipart
-		formValues.Set("Key", strings.Replace(formValues.Get("Key"), "${filename}", fileName, -1))
+		formValues.Set("Key", strings.ReplaceAll(formValues.Get("Key"), "${filename}", fileName))
 	}
 	object := trimLeadingSlash(formValues.Get("Key"))
 

cmd/bucket-listobjects-handlers.go

@@ -59,8 +59,8 @@ func validateListObjectsArgs(marker, delimiter, encodingType string, maxKeys int
 	}
 
 	if encodingType != "" {
-		// Only url encoding type is supported
-		if strings.ToLower(encodingType) != "url" {
+		// AWS S3 spec only supports 'url' encoding type
+		if !strings.EqualFold(encodingType, "url") {
 			return ErrInvalidEncodingMethod
 		}
 	}

cmd/bucket-policy.go

@@ -172,11 +172,12 @@ func getConditionValues(r *http.Request, lc string, username string, claims map[
 		vStr, ok := v.(string)
 		if ok {
 			// Special case for AD/LDAP STS users
-			if k == ldapUser {
+			switch k {
+			case ldapUser:
 				args["user"] = []string{vStr}
-			} else if k == ldapUserN {
+			case ldapUserN:
 				args["username"] = []string{vStr}
-			} else {
+			default:
 				args[k] = []string{vStr}
 			}
 		}

cmd/bucket-replication-utils.go

@@ -172,7 +172,7 @@ func (o *ObjectInfo) TargetReplicationStatus(arn string) (status replication.Sta
 type replicateTargetDecision struct {
 	Replicate   bool   // Replicate to this target
 	Synchronous bool   // Synchronous replication configured.
-	Arn         string //ARN of replication target
+	Arn         string // ARN of replication target
 	ID          string
 }
 

cmd/bucket-replication-utils_test.go

@@ -32,7 +32,7 @@ var replicatedInfosTests = []struct {
 	expectedOpType                    replication.Type
 	expectedAction                    replicationAction
 }{
-	{ //1. empty tgtInfos slice
+	{ // 1. empty tgtInfos slice
 		name:                              "no replicated targets",
 		tgtInfos:                          []replicatedTargetInfo{},
 		expectedCompletedSize:             0,
@@ -41,7 +41,7 @@ var replicatedInfosTests = []struct {
 		expectedOpType:                    replication.UnsetReplicationType,
 		expectedAction:                    replicateNone,
 	},
-	{ //2. replication completed to single target
+	{ // 2. replication completed to single target
 		name: "replication completed to single target",
 		tgtInfos: []replicatedTargetInfo{
 			{
@@ -59,7 +59,7 @@ var replicatedInfosTests = []struct {
 		expectedOpType:                    replication.ObjectReplicationType,
 		expectedAction:                    replicateAll,
 	},
-	{ //3. replication completed to single target; failed to another
+	{ // 3. replication completed to single target; failed to another
 		name: "replication completed to single target",
 		tgtInfos: []replicatedTargetInfo{
 			{
@@ -84,7 +84,7 @@ var replicatedInfosTests = []struct {
 		expectedOpType:                    replication.ObjectReplicationType,
 		expectedAction:                    replicateAll,
 	},
-	{ //4. replication pending on one target; failed to another
+	{ // 4. replication pending on one target; failed to another
 		name: "replication completed to single target",
 		tgtInfos: []replicatedTargetInfo{
 			{
@@ -137,7 +137,7 @@ var parseReplicationDecisionTest = []struct {
 	expDsc ReplicateDecision
 	expErr error
 }{
-	{ //1.
+	{ // 1.
 		name: "empty string",
 		dsc:  "",
 		expDsc: ReplicateDecision{
@@ -146,7 +146,7 @@ var parseReplicationDecisionTest = []struct {
 		expErr: nil,
 	},
 
-	{ //2.
+	{ // 2.
 		name:   "replicate decision for one target",
 		dsc:    "arn:minio:replication::id:bucket=true;false;arn:minio:replication::id:bucket;id",
 		expErr: nil,
@@ -156,7 +156,7 @@ var parseReplicationDecisionTest = []struct {
 			},
 		},
 	},
-	{ //3.
+	{ // 3.
 		name:   "replicate decision for multiple targets",
 		dsc:    "arn:minio:replication::id:bucket=true;false;arn:minio:replication::id:bucket;id,arn:minio:replication::id2:bucket=false;true;arn:minio:replication::id2:bucket;id2",
 		expErr: nil,
@@ -167,7 +167,7 @@ var parseReplicationDecisionTest = []struct {
 			},
 		},
 	},
-	{ //4.
+	{ // 4.
 		name:   "invalid format replicate decision for one target",
 		dsc:    "arn:minio:replication::id:bucket:true;false;arn:minio:replication::id:bucket;id",
 		expErr: errInvalidReplicateDecisionFormat,
@@ -181,7 +181,6 @@ var parseReplicationDecisionTest = []struct {
 
 func TestParseReplicateDecision(t *testing.T) {
 	for i, test := range parseReplicationDecisionTest {
-		//dsc, err := parseReplicateDecision(test.dsc)
 		dsc, err := parseReplicateDecision(test.expDsc.String())
 
 		if err != nil {
@@ -208,30 +207,30 @@ var replicationStateTest = []struct {
 	arn       string
 	expStatus replication.StatusType
 }{
-	{ //1. no replication status header
+	{ // 1. no replication status header
 		name:      "no replicated targets",
 		rs:        ReplicationState{},
 		expStatus: replication.StatusType(""),
 	},
-	{ //2. replication status for one target
+	{ // 2. replication status for one target
 		name:      "replication status for one target",
 		rs:        ReplicationState{ReplicationStatusInternal: "arn1=PENDING;", Targets: map[string]replication.StatusType{"arn1": "PENDING"}},
 		expStatus: replication.Pending,
 	},
-	{ //3. replication status for one target - incorrect format
+	{ // 3. replication status for one target - incorrect format
 		name:      "replication status for one target",
 		rs:        ReplicationState{ReplicationStatusInternal: "arn1=PENDING"},
 		expStatus: replication.StatusType(""),
 	},
-	{ //4. replication status for 3 targets, one of them failed
+	{ // 4. replication status for 3 targets, one of them failed
 		name: "replication status for 3 targets - one failed",
 		rs: ReplicationState{
 			ReplicationStatusInternal: "arn1=COMPLETED;arn2=COMPLETED;arn3=FAILED;",
 			Targets:                   map[string]replication.StatusType{"arn1": "COMPLETED", "arn2": "COMPLETED", "arn3": "FAILED"},
 		},
 		expStatus: replication.Failed,
 	},
-	{ //5. replication status for replica version
+	{ // 5. replication status for replica version
 		name:      "replication status for replica version",
 		rs:        ReplicationState{ReplicationStatusInternal: string(replication.Replica)},
 		expStatus: replication.Replica,

cmd/bucket-replication.go

@@ -1740,7 +1740,7 @@ func resyncTarget(oi ObjectInfo, arn string, resetID string, resetBeforeDate tim
 	}
 	rs, ok := oi.UserDefined[targetResetHeader(arn)]
 	if !ok {
-		rs, ok = oi.UserDefined[xhttp.MinIOReplicationResetStatus] //for backward compatibility
+		rs, ok = oi.UserDefined[xhttp.MinIOReplicationResetStatus] // for backward compatibility
 	}
 	if !ok { // existing object replication is enabled and object version is unreplicated so far.
 		if resetID != "" && oi.ModTime.Before(resetBeforeDate) { // trigger replication if `mc replicate reset` requested