Refactor six non-workflow large functions via targeted helper extraction

pkg/linters/registry.go

@@ -70,65 +70,67 @@ import (
 // analyzers. Use it to drive multichecker.Main, test assertions, and
 // doc-completeness checks so that every consumer stays in sync automatically.
 func All() []*analysis.Analyzer {
-	return []*analysis.Analyzer{
-		appendbytestring.Analyzer,
-		appendoneelement.Analyzer,
-		bytesbufferstring.Analyzer,
-		bytescomparestring.Analyzer,
-		contextcancelnotdeferred.Analyzer,
-		ctxbackground.Analyzer,
-		deferinloop.Analyzer,
-		errormessage.Analyzer,
-		errortypeassertion.Analyzer,
-		fprintlnsprintf.Analyzer,
-		errstringmatch.Analyzer,
-		errorfwrapv.Analyzer,
-		execcommandwithoutcontext.Analyzer,
-		excessivefuncparams.Analyzer,
-		fileclosenotdeferred.Analyzer,
-		fmterrorfnoverbs.Analyzer,
-		hardcodedfilepath.Analyzer,
-		httpnoctx.Analyzer,
-		httprespbodyclose.Analyzer,
-		ioutildeprecated.Analyzer,
-		httpstatuscode.Analyzer,
-		largefunc.Analyzer,
-		logfatallibrary.Analyzer,
-		manualmutexunlock.Analyzer,
-		mapclearloop.Analyzer,
-		mapdeletecheck.Analyzer,
-		nilctxpassed.Analyzer,
-		osexitinlibrary.Analyzer,
-		osgetenvlibrary.Analyzer,
-		ossetenvlibrary.Analyzer,
-		panicinlibrarycode.Analyzer,
-		rawloginlib.Analyzer,
-		regexpcompileinfunction.Analyzer,
-		ssljson.Analyzer,
-		seenmapbool.Analyzer,
-		sortslice.Analyzer,
-		sprintferrdot.Analyzer,
-		sprintferrorsnew.Analyzer,
-		sprintfbool.Analyzer,
-		sprintfint.Analyzer,
-		strconvparseignorederror.Analyzer,
-		stringbytesroundtrip.Analyzer,
-		stringreplaceminusone.Analyzer,
-		stringsconcatloop.Analyzer,
-		stringsindexcontains.Analyzer,
-		stringsindexhasprefix.Analyzer,
-		stringsjoinone.Analyzer,
-		stringscountcontains.Analyzer,
-		jsonmarshalignoredeerror.Analyzer,
-		lenstringzero.Analyzer,
-		lenstringsplit.Analyzer,
-		timeafterleak.Analyzer,
-		timesleepnocontext.Analyzer,
-		timenowsub.Analyzer,
-		tolowerequalfold.Analyzer,
-		trimleftright.Analyzer,
-		uncheckedtypeassertion.Analyzer,
-		wgdonenotdeferred.Analyzer,
-		writebytestring.Analyzer,
-	}
+	return append([]*analysis.Analyzer(nil), allAnalyzers...)
+}
+
+var allAnalyzers = []*analysis.Analyzer{
+	appendbytestring.Analyzer,
+	appendoneelement.Analyzer,
+	bytesbufferstring.Analyzer,
+	bytescomparestring.Analyzer,
+	contextcancelnotdeferred.Analyzer,
+	ctxbackground.Analyzer,
+	deferinloop.Analyzer,
+	errormessage.Analyzer,
+	errortypeassertion.Analyzer,
+	fprintlnsprintf.Analyzer,
+	errstringmatch.Analyzer,
+	errorfwrapv.Analyzer,
+	execcommandwithoutcontext.Analyzer,
+	excessivefuncparams.Analyzer,
+	fileclosenotdeferred.Analyzer,
+	fmterrorfnoverbs.Analyzer,
+	hardcodedfilepath.Analyzer,
+	httpnoctx.Analyzer,
+	httprespbodyclose.Analyzer,
+	ioutildeprecated.Analyzer,
+	httpstatuscode.Analyzer,
+	largefunc.Analyzer,
+	logfatallibrary.Analyzer,
+	manualmutexunlock.Analyzer,
+	mapclearloop.Analyzer,
+	mapdeletecheck.Analyzer,
+	nilctxpassed.Analyzer,
+	osexitinlibrary.Analyzer,
+	osgetenvlibrary.Analyzer,
+	ossetenvlibrary.Analyzer,
+	panicinlibrarycode.Analyzer,
+	rawloginlib.Analyzer,
+	regexpcompileinfunction.Analyzer,
+	ssljson.Analyzer,
+	seenmapbool.Analyzer,
+	sortslice.Analyzer,
+	sprintferrdot.Analyzer,
+	sprintferrorsnew.Analyzer,
+	sprintfbool.Analyzer,
+	sprintfint.Analyzer,
+	strconvparseignorederror.Analyzer,
+	stringbytesroundtrip.Analyzer,
+	stringreplaceminusone.Analyzer,
+	stringsconcatloop.Analyzer,
+	stringsindexcontains.Analyzer,
+	stringsindexhasprefix.Analyzer,
+	stringsjoinone.Analyzer,
+	stringscountcontains.Analyzer,
+	jsonmarshalignoredeerror.Analyzer,
+	lenstringzero.Analyzer,
+	lenstringsplit.Analyzer,
+	timeafterleak.Analyzer,
+	timesleepnocontext.Analyzer,
+	timenowsub.Analyzer,
+	tolowerequalfold.Analyzer,
+	trimleftright.Analyzer,
+	uncheckedtypeassertion.Analyzer,
+	wgdonenotdeferred.Analyzer,
+	writebytestring.Analyzer,
 }

pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go

@@ -53,82 +53,79 @@ func run(pass *analysis.Pass) (any, error) {
 // string/[]byte round-trip (string([]byte(s))) or a wasteful two-copy clone
 // ([]byte(string(b))) and reports a diagnostic if so.
 func analyzeRoundTrip(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
-	outer, ok := n.(*ast.CallExpr)
+	outer, inner, ok := roundTripCalls(n)
+	if !ok || shouldSkipRoundTrip(pass, outer, generatedFiles, noLintIndex) {
+		return
+	}
+	outerUnderlying, innerUnderlying, innerArgUnderlying, ok := roundTripUnderlyingTypes(pass, outer, inner)
 	if !ok {
 		return
 	}
-	// Must be a type conversion (single argument, no ellipsis).
-	if len(outer.Args) != 1 || outer.Ellipsis.IsValid() {
+	if reportRedundantRoundTrip(pass, outer, inner, outerUnderlying, innerUnderlying, innerArgUnderlying) {
 		return
 	}
+	reportWastefulCloneRoundTrip(pass, outer, inner, outerUnderlying, innerUnderlying, innerArgUnderlying)
+}
+
+func roundTripCalls(n ast.Node) (*ast.CallExpr, *ast.CallExpr, bool) {
+	outer, ok := n.(*ast.CallExpr)
+	if !ok || len(outer.Args) != 1 || outer.Ellipsis.IsValid() {
+		return nil, nil, false
+	}
+	inner, ok := outer.Args[0].(*ast.CallExpr)
+	if !ok || len(inner.Args) != 1 || inner.Ellipsis.IsValid() {
+		return nil, nil, false
+	}
+	return outer, inner, true
+}
 
+func shouldSkipRoundTrip(pass *analysis.Pass, outer *ast.CallExpr, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) bool {
 	pos := pass.Fset.PositionFor(outer.Pos(), false)
 	if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
-		return
-	}
-	if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringbytesroundtrip") {
-		return
+		return true
 	}
+	return nolint.HasDirectiveForLinter(pos, noLintIndex, "stringbytesroundtrip")
+}
 
-	// Must be a type conversion, not a function call.
+func roundTripUnderlyingTypes(pass *analysis.Pass, outer, inner *ast.CallExpr) (types.Type, types.Type, types.Type, bool) {
 	outerFunInfo, ok := pass.TypesInfo.Types[outer.Fun]
 	if !ok || !outerFunInfo.IsType() {
-		return
+		return nil, nil, nil, false
 	}
-
-	outerType := pass.TypesInfo.TypeOf(outer)
-	if outerType == nil {
-		return
-	}
-
-	inner, ok := outer.Args[0].(*ast.CallExpr)
-	if !ok {
-		return
-	}
-	if len(inner.Args) != 1 || inner.Ellipsis.IsValid() {
-		return
-	}
-
-	// The inner call must also be a type conversion, not a function call.
 	innerFunInfo, ok := pass.TypesInfo.Types[inner.Fun]
 	if !ok || !innerFunInfo.IsType() {
-		return
+		return nil, nil, nil, false
 	}
-
+	outerType := pass.TypesInfo.TypeOf(outer)
 	innerType := pass.TypesInfo.TypeOf(inner)
-	if innerType == nil {
-		return
-	}
 	innerArgType := pass.TypesInfo.TypeOf(inner.Args[0])
-	if innerArgType == nil {
-		return
+	if outerType == nil || innerType == nil || innerArgType == nil {
+		return nil, nil, nil, false
 	}
+	return outerType.Underlying(), innerType.Underlying(), innerArgType.Underlying(), true
+}
 
-	outerUnderlying := outerType.Underlying()
-	innerUnderlying := innerType.Underlying()
-	innerArgUnderlying := innerArgType.Underlying()
-
-	// Check string([]byte(s)) where s is already a string.
-	if isStringType(outerUnderlying) && isByteSliceType(innerUnderlying) && isStringType(innerArgUnderlying) {
-		argText := astutil.NodeText(pass.Fset, inner.Args[0])
-		pass.ReportRangef(outer,
-			"string([]byte(%s)) is a redundant round-trip; the inner []byte conversion copies the string unnecessarily",
-			argText,
-		)
-		return
+func reportRedundantRoundTrip(pass *analysis.Pass, outer, inner *ast.CallExpr, outerUnderlying, innerUnderlying, innerArgUnderlying types.Type) bool {
+	if !isStringType(outerUnderlying) || !isByteSliceType(innerUnderlying) || !isStringType(innerArgUnderlying) {
+		return false
 	}
+	argText := astutil.NodeText(pass.Fset, inner.Args[0])
+	pass.ReportRangef(outer,
+		"string([]byte(%s)) is a redundant round-trip; the inner []byte conversion copies the string unnecessarily",
+		argText,
+	)
+	return true
+}
 
-	// Check []byte(string(b)) where b is already a []byte.
-	// This is the defensive-copy idiom: the result is a non-aliasing copy, not
-	// a no-op.  The diagnostic is therefore not "redundant" but "wasteful":
-	// two memory copies are made when one would suffice.
-	if isByteSliceType(outerUnderlying) && isStringType(innerUnderlying) && isByteSliceType(innerArgUnderlying) {
-		argText := astutil.NodeText(pass.Fset, inner.Args[0])
-		pass.ReportRangef(outer,
-			"[]byte(string(%s)) makes two copies to clone %s; use slices.Clone(%s) or bytes.Clone(%s) for a single-copy independent slice",
-			argText, argText, argText, argText,
-		)
+func reportWastefulCloneRoundTrip(pass *analysis.Pass, outer, inner *ast.CallExpr, outerUnderlying, innerUnderlying, innerArgUnderlying types.Type) {
+	if !isByteSliceType(outerUnderlying) || !isStringType(innerUnderlying) || !isByteSliceType(innerArgUnderlying) {
+		return
 	}
+	argText := astutil.NodeText(pass.Fset, inner.Args[0])
+	pass.ReportRangef(outer,
+		"[]byte(string(%s)) makes two copies to clone %s; use slices.Clone(%s) or bytes.Clone(%s) for a single-copy independent slice",
+		argText, argText, argText, argText,
+	)
 }
 
 func isStringType(t types.Type) bool {

pkg/parser/remote_download_file.go

@@ -383,61 +383,83 @@ func downloadFileViaRawURL(ctx context.Context, owner, repo, filePath, ref strin
 func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host string) ([]byte, error) {
 	remoteLog.Printf("Attempting git clone fallback for %s/%s/%s@%s", owner, repo, path, ref)
 
+	if err := validateGitCloneFallbackInputs(ref, path); err != nil {
+		return nil, err
+	}
+	tmpDir, err := createGitCloneTempDir()
+	if err != nil {
+		return nil, err
+	}
+	defer os.RemoveAll(tmpDir)
+
+	repoURL := getRepoGitURL(owner, repo, host)
+	if len(ref) == 40 && gitutil.IsHexString(ref) {
+		if err := cloneAndCheckoutSHA(ctx, repoURL, tmpDir, ref); err != nil {
+			return nil, err
+		}
+	} else if err := cloneBranchOrTag(ctx, repoURL, tmpDir, ref); err != nil {
+		return nil, err
+	}
+
+	content, err := readFileFromClone(tmpDir, path)
+	if err != nil {
+		return nil, err
+	}
+
+	remoteLog.Printf("Successfully downloaded file via git clone: %s/%s/%s@%s", owner, repo, path, ref)
+	return content, nil
+}
+
+func validateGitCloneFallbackInputs(ref, path string) error {
 	if err := gitutil.ValidateGitRef(ref); err != nil {
-		return nil, fmt.Errorf("refusing git clone fallback: %w", err)
+		return fmt.Errorf("refusing git clone fallback: %w", err)
 	}
 	if err := gitutil.ValidateGitPath(path); err != nil {
-		return nil, fmt.Errorf("refusing git clone fallback: %w", err)
+		return fmt.Errorf("refusing git clone fallback: %w", err)
 	}
+	return nil
+}
 
-	// Create a temporary directory for the shallow clone
+func createGitCloneTempDir() (string, error) {
 	tmpDir, err := os.MkdirTemp("", "gh-aw-git-clone-*")
 	if err != nil {
-		return nil, fmt.Errorf("failed to create temp directory: %w", err)
+		return "", fmt.Errorf("failed to create temp directory: %w", err)
 	}
-	defer os.RemoveAll(tmpDir)
+	return tmpDir, nil
+}
 
-	var githubHost string
+func getRepoGitURL(owner, repo, host string) string {
+	githubHost := GetGitHubHostForRepo(owner, repo)
 	if host != "" {
 		githubHost = "https://" + host
-	} else {
-		githubHost = GetGitHubHostForRepo(owner, repo)
 	}
-	repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo)
+	return fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo)
+}
 
-	// Check if ref is a SHA (40 hex characters)
-	isSHA := len(ref) == 40 && gitutil.IsHexString(ref)
-
-	var cloneCmd *exec.Cmd
-	if isSHA {
-		// For SHA refs, we need to clone without --branch and then checkout the specific commit
-		// Clone with minimal depth and no branch specified
-		cloneCmd = exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--no-single-branch", repoURL, tmpDir)
-		if output, err := cloneCmd.CombinedOutput(); err != nil {
-			// Try without --no-single-branch if the first attempt fails
-			remoteLog.Printf("Clone with --no-single-branch failed, trying full clone: %s", string(output))
-			cloneCmd = exec.CommandContext(ctx, "git", "clone", repoURL, tmpDir)
-			if output, err := cloneCmd.CombinedOutput(); err != nil {
-				return nil, fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output))
-			}
+func cloneAndCheckoutSHA(ctx context.Context, repoURL, tmpDir, ref string) error {
+	if output, err := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--no-single-branch", repoURL, tmpDir).CombinedOutput(); err != nil {
+		remoteLog.Printf("Clone with --no-single-branch failed, trying full clone: %s", string(output))
+		if output, err := exec.CommandContext(ctx, "git", "clone", repoURL, tmpDir).CombinedOutput(); err != nil {
+			return fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output))
 		}
+	}
+	// ValidateGitRef above guarantees ref does not start with '-', so it remains a revision argument.
+	if output, err := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--detach", ref).CombinedOutput(); err != nil {
+		return fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(output))
+	}
+	return nil
+}
 
-		// Now checkout the specific commit in detached HEAD mode. ValidateGitRef above
-		// guarantees ref does not start with '-', so it remains a revision argument.
-		checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--detach", ref)
-		if output, err := checkoutCmd.CombinedOutput(); err != nil {
-			return nil, fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(output))
-		}
-	} else {
-		// For branch/tag refs, use --branch flag; the value is passed via a separate
-		// argument slot and cannot be confused with a flag because it follows --branch.
-		cloneCmd = exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", ref, repoURL, tmpDir)
-		if output, err := cloneCmd.CombinedOutput(); err != nil {
-			return nil, fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output))
-		}
+func cloneBranchOrTag(ctx context.Context, repoURL, tmpDir, ref string) error {
+	// For branch/tag refs, use --branch flag; the value is passed via a separate
+	// argument slot and cannot be confused with a flag because it follows --branch.
+	if output, err := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", ref, repoURL, tmpDir).CombinedOutput(); err != nil {
+		return fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output))
 	}
+	return nil
+}
 
-	// Read the file from the cloned repository
+func readFileFromClone(tmpDir, path string) ([]byte, error) {
 	filePath := filepath.Join(tmpDir, path)
 	if err := fileutil.ValidatePathWithinBase(tmpDir, filePath); err != nil {
 		return nil, fmt.Errorf("refusing to read file outside clone directory: %w", err)
@@ -446,7 +468,5 @@ func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host s
 	if err != nil {
 		return nil, fmt.Errorf("failed to read file from cloned repository: %w", err)
 	}
-
-	remoteLog.Printf("Successfully downloaded file via git clone: %s/%s/%s@%s", owner, repo, path, ref)
 	return content, nil
 }