refactor: code cleanup across codebase (#947)

## Summary

- Extract duplicate \`getReleaseByTag\` into shared \`cmd/releases/utils.go\`
- Replace \`log.Fatal\` calls with proper error returns in config and login commands; \`GetLoginByToken\`/\`GetLoginsByHost\`/\`GetLoginByHost\` now return errors
- Remove dead \`portChan\` channel in \`modules/auth/oauth.go\`
- Fix YAML integer detection to use \`strconv.ParseInt\` (correctly handles negatives and large ints)
- Fix \`path.go\` error handling to use \`errors.As\` + \`syscall.ENOTDIR\` instead of string comparison
- Extract repeated credential helper key into local variable in \`SetupHelper\`
- Use existing \`isRemoteDeleted()\` in \`pull_clean.go\` instead of duplicating the logic
- Fix ~30 error message casing violations to follow Go conventions
- Use \`fmt.Errorf\` consistently instead of string concatenation in \`generic.go\`

Reviewed-on: https://gitea.com/gitea/tea/pulls/947
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Reviewed-by: Bo-Yi Wu (吳柏毅) <appleboy.tw@gmail.com>
Co-authored-by: Nicolas <bircni@icloud.com>
Co-committed-by: Nicolas <bircni@icloud.com>

modules/auth/oauth.go

@@ -226,7 +226,6 @@ func startLocalServerAndOpenBrowser(authURL, expectedState string, opts OAuthOpt
 	codeChan := make(chan string, 1)
 	stateChan := make(chan string, 1)
 	errChan := make(chan error, 1)
-	portChan := make(chan int, 1)
 
 	// Parse the redirect URL to get the path
 	parsedURL, err := url.Parse(opts.RedirectURL)
@@ -311,7 +310,6 @@ func startLocalServerAndOpenBrowser(authURL, expectedState string, opts OAuthOpt
 	if port == 0 {
 		addr := listener.Addr().(*net.TCPAddr)
 		port = addr.Port
-		portChan <- port
 
 		// Update redirect URL with actual port
 		parsedURL.Host = fmt.Sprintf("%s:%d", hostname, port)

modules/config/config.go

@@ -5,7 +5,6 @@ package config
 
 import (
 	"fmt"
-	"log"
 	"os"
 	"path/filepath"
 	"sync"
@@ -74,7 +73,8 @@ func GetConfigPath() string {
 	}
 
 	if err != nil {
-		log.Fatal("unable to get or create config file")
+		fmt.Fprintln(os.Stderr, "unable to get or create config file")
+		os.Exit(1)
 	}
 
 	return configFilePath

modules/config/login.go

@@ -8,7 +8,6 @@ import (
 	"crypto/tls"
 	"errors"
 	"fmt"
-	"log"
 	"net/http"
 	"net/http/cookiejar"
 	"net/url"
@@ -132,7 +131,7 @@ func GetDefaultLogin() (*Login, error) {
 	}
 
 	if len(config.Logins) == 0 {
-		return nil, errors.New("No available login")
+		return nil, errors.New("no available login")
 	}
 	for _, l := range config.Logins {
 		if l.Default {
@@ -178,50 +177,51 @@ func GetLoginByName(name string) (*Login, error) {
 }
 
 // GetLoginByToken get login by token
-func GetLoginByToken(token string) *Login {
+func GetLoginByToken(token string) (*Login, error) {
 	if token == "" {
-		return nil
+		return nil, nil
 	}
-	err := loadConfig()
-	if err != nil {
-		log.Fatal(err)
+	if err := loadConfig(); err != nil {
+		return nil, err
 	}
 
 	for _, l := range config.Logins {
 		if l.Token == token {
-			return &l
+			return &l, nil
 		}
 	}
-	return nil
+	return nil, nil
 }
 
-// GetLoginByHost finds a login by it's server URL
-func GetLoginByHost(host string) *Login {
-	logins := GetLoginsByHost(host)
-	if len(logins) > 0 {
-		return logins[0]
+// GetLoginByHost finds a login by its server URL
+func GetLoginByHost(host string) (*Login, error) {
+	logins, err := GetLoginsByHost(host)
+	if err != nil {
+		return nil, err
 	}
-	return nil
+	if len(logins) > 0 {
+		return logins[0], nil
+	}
+	return nil, nil
 }
 
 // GetLoginsByHost returns all logins matching a host
-func GetLoginsByHost(host string) []*Login {
-	err := loadConfig()
-	if err != nil {
-		log.Fatal(err)
+func GetLoginsByHost(host string) ([]*Login, error) {
+	if err := loadConfig(); err != nil {
+		return nil, err
 	}
 
 	var matches []*Login
 	for i := range config.Logins {
 		loginURL, err := url.Parse(config.Logins[i].URL)
 		if err != nil {
-			log.Fatal(err)
+			return nil, err
 		}
 		if loginURL.Host == host {
 			matches = append(matches, &config.Logins[i])
 		}
 	}
-	return matches
+	return matches, nil
 }
 
 // DeleteLogin delete a login by name from config
@@ -417,12 +417,13 @@ func doOAuthRefresh(l *Login) (*oauth2.Token, error) {
 func (l *Login) Client(options ...gitea.ClientOption) *gitea.Client {
 	// Refresh OAuth token if expired or near expiry
 	if err := l.RefreshOAuthTokenIfNeeded(); err != nil {
-		log.Fatalf("Failed to refresh token: %s\nPlease use 'tea login oauth-refresh %s' to manually refresh the token.\n", err, l.Name)
+		fmt.Fprintf(os.Stderr, "Failed to refresh token: %s\nPlease use 'tea login oauth-refresh %s' to manually refresh the token.\n", err, l.Name)
+		os.Exit(1)
 	}
 
 	httpClient := &http.Client{}
 	if l.Insecure {
-		cookieJar, _ := cookiejar.New(nil)
+		cookieJar, _ := cookiejar.New(nil) // New with nil options never returns an error
 
 		httpClient = &http.Client{
 			Jar: cookieJar,
@@ -443,12 +444,18 @@ func (l *Login) Client(options ...gitea.ClientOption) *gitea.Client {
 	}
 
 	if l.SSHCertPrincipal != "" {
-		l.askForSSHPassphrase()
+		if err := l.askForSSHPassphrase(); err != nil {
+			fmt.Fprintf(os.Stderr, "Failed to read SSH passphrase: %s\n", err)
+			os.Exit(1)
+		}
 		options = append(options, gitea.UseSSHCert(l.SSHCertPrincipal, l.SSHKey, l.SSHPassphrase))
 	}
 
 	if l.SSHKeyFingerprint != "" {
-		l.askForSSHPassphrase()
+		if err := l.askForSSHPassphrase(); err != nil {
+			fmt.Fprintf(os.Stderr, "Failed to read SSH passphrase: %s\n", err)
+			os.Exit(1)
+		}
 		options = append(options, gitea.UseSSHPubkey(l.SSHKeyFingerprint, l.SSHKey, l.SSHPassphrase))
 	}
 
@@ -456,25 +463,25 @@ func (l *Login) Client(options ...gitea.ClientOption) *gitea.Client {
 	if err != nil {
 		var versionError *gitea.ErrUnknownVersion
 		if !errors.As(err, &versionError) {
-			log.Fatal(err)
+			fmt.Fprintf(os.Stderr, "Failed to create Gitea client: %s\n", err)
+			os.Exit(1)
 		}
 		fmt.Fprintf(os.Stderr, "WARNING: could not detect gitea version: %s\nINFO: set gitea version: to last supported one\n", versionError)
 	}
 	return client
 }
 
-func (l *Login) askForSSHPassphrase() {
+func (l *Login) askForSSHPassphrase() error {
 	if ok, err := utils.IsKeyEncrypted(l.SSHKey); ok && err == nil && l.SSHPassphrase == "" {
-		if err := huh.NewInput().
+		return huh.NewInput().
 			Title("ssh-key is encrypted please enter the passphrase: ").
 			Validate(huh.ValidateNotEmpty()).
 			EchoMode(huh.EchoModePassword).
 			Value(&l.SSHPassphrase).
 			WithTheme(theme.GetTheme()).
-			Run(); err != nil {
-			log.Fatal(err)
-		}
+			Run()
 	}
+	return nil
 }
 
 // GetSSHHost returns SSH host name

modules/context/context.go

@@ -20,7 +20,7 @@ import (
 	"golang.org/x/term"
 )
 
-var errNotAGiteaRepo = errors.New("No Gitea login found. You might want to specify --repo (and --login) to work outside of a repository")
+var errNotAGiteaRepo = errors.New("no Gitea login found; you might want to specify --repo (and --login) to work outside of a repository")
 
 // ErrCommandCanceled is returned when the user explicitly cancels an interactive prompt.
 var ErrCommandCanceled = errors.New("command canceled")

modules/git/branch.go

@@ -80,7 +80,7 @@ func (r TeaRepo) TeaFindBranchBySha(sha, repoURL string) (b *git_config.Branch,
 		return nil, err
 	}
 	if remote == nil {
-		return nil, fmt.Errorf("No remote found for '%s'", repoURL)
+		return nil, fmt.Errorf("no remote found for '%s'", repoURL)
 	}
 	remoteName := remote.Config().Name
 
@@ -133,7 +133,7 @@ func (r TeaRepo) TeaFindBranchByName(branchName, repoURL string) (b *git_config.
 		return nil, err
 	}
 	if remote == nil {
-		return nil, fmt.Errorf("No remote found for '%s'", repoURL)
+		return nil, fmt.Errorf("no remote found for '%s'", repoURL)
 	}
 	remoteName := remote.Config().Name
 

modules/interact/login.go

@@ -41,7 +41,7 @@ func CreateLogin() error {
 			}
 			_, err := url.Parse(s)
 			if err != nil {
-				return fmt.Errorf("Invalid URL: %v", err)
+				return fmt.Errorf("invalid URL: %v", err)
 			}
 			return nil
 		}).
@@ -69,7 +69,7 @@ func CreateLogin() error {
 		}
 		for _, login := range logins {
 			if login.Name == name {
-				return fmt.Errorf("Login with name '%s' already exists", name)
+				return fmt.Errorf("login with name '%s' already exists", name)
 			}
 		}
 		return nil
@@ -154,7 +154,7 @@ func CreateLogin() error {
 				Value(&tokenScopes).
 				Validate(func(s []string) error {
 					if len(s) == 0 {
-						return errors.New("At least one scope is required")
+						return errors.New("at least one scope is required")
 					}
 					return nil
 				}).

modules/interact/pull_merge.go

@@ -58,7 +58,7 @@ func getPullIndex(ctx *context.TeaContext, branch string) (int64, error) {
 			return 0, err
 		}
 		if len(prs) == 0 {
-			return 0, fmt.Errorf("No open PRs found")
+			return 0, fmt.Errorf("no open PRs found")
 		}
 		opts.ListOptions.Page++
 		prOptions := make([]string, 0)

modules/print/table.go

@@ -150,8 +150,7 @@ func outputYaml(f io.Writer, headers []string, values [][]string) error {
 			})
 
 			valueNode := &yaml.Node{Kind: yaml.ScalarNode, Value: val}
-			intVal, _ := strconv.Atoi(val)
-			if strconv.Itoa(intVal) == val {
+			if _, err := strconv.ParseInt(val, 10, 64); err == nil {
 				valueNode.Tag = "!!int"
 			} else {
 				valueNode.Tag = "!!str"

modules/task/issue_create.go

@@ -15,7 +15,7 @@ import (
 func CreateIssue(login *config.Login, repoOwner, repoName string, opts gitea.CreateIssueOption) error {
 	// title is required
 	if len(opts.Title) == 0 {
-		return fmt.Errorf("Title is required")
+		return fmt.Errorf("title is required")
 	}
 
 	issue, _, err := login.Client().CreateIssue(repoOwner, repoName, opts)

modules/task/login_create.go

@@ -20,12 +20,13 @@ import (
 func SetupHelper(login config.Login) (ok bool, err error) {
 	// Check that the URL is not blank
 	if login.URL == "" {
-		return false, fmt.Errorf("Invalid gitea url")
+		return false, fmt.Errorf("invalid Gitea URL")
 	}
 
 	// get all helper to URL in git config
+	helperKey := fmt.Sprintf("credential.%s.helper", login.URL)
 	var currentHelpers []byte
-	if currentHelpers, err = exec.Command("git", "config", "--global", "--get-all", fmt.Sprintf("credential.%s.helper", login.URL)).Output(); err != nil {
+	if currentHelpers, err = exec.Command("git", "config", "--global", "--get-all", helperKey).Output(); err != nil {
 		currentHelpers = []byte{}
 	}
 
@@ -37,10 +38,10 @@ func SetupHelper(login config.Login) (ok bool, err error) {
 	}
 
 	// Add tea helper
-	if _, err = exec.Command("git", "config", "--global", fmt.Sprintf("credential.%s.helper", login.URL), "").Output(); err != nil {
-		return false, fmt.Errorf("git config --global %s, error: %s", fmt.Sprintf("credential.%s.helper", login.URL), err)
-	} else if _, err = exec.Command("git", "config", "--global", "--add", fmt.Sprintf("credential.%s.helper", login.URL), "!tea login helper").Output(); err != nil {
-		return false, fmt.Errorf("git config --global --add %s %s, error: %s", fmt.Sprintf("credential.%s.helper", login.URL), "!tea login helper", err)
+	if _, err = exec.Command("git", "config", "--global", helperKey, "").Output(); err != nil {
+		return false, fmt.Errorf("git config --global %s, error: %s", helperKey, err)
+	} else if _, err = exec.Command("git", "config", "--global", "--add", helperKey, "!tea login helper").Output(); err != nil {
+		return false, fmt.Errorf("git config --global --add %s %s, error: %s", helperKey, "!tea login helper", err)
 	}
 
 	return true, nil
@@ -62,7 +63,11 @@ func CreateLogin(name, token, user, passwd, otp, scopes, sshKey, giteaURL, sshCe
 	}
 	// ... if we already use this token
 	if shouldCheckTokenUniqueness(token, sshAgent, sshKey, sshCertPrincipal, sshKeyFingerprint) {
-		if login := config.GetLoginByToken(token); login != nil {
+		login, err := config.GetLoginByToken(token)
+		if err != nil {
+			return err
+		}
+		if login != nil {
 			return fmt.Errorf("token already been used, delete login '%s' first", login.Name)
 		}
 	}

modules/task/milestone_create.go

@@ -17,7 +17,7 @@ import (
 func CreateMilestone(login *config.Login, repoOwner, repoName, title, description string, deadline *time.Time, state gitea.StateType) error {
 	// title is required
 	if len(title) == 0 {
-		return fmt.Errorf("Title is required")
+		return fmt.Errorf("title is required")
 	}
 
 	mile, _, err := login.Client().CreateMilestone(repoOwner, repoName, gitea.CreateMilestoneOption{

modules/task/pull_clean.go

@@ -39,7 +39,7 @@ func PullClean(login *config.Login, repoOwner, repoName string, index int64, ign
 
 	// if remote head branch is already deleted, pr.Head.Ref points to "pulls/<idx>/head"
 	remoteBranch := pr.Head.Ref
-	remoteDeleted := remoteBranch == fmt.Sprintf("refs/pull/%d/head", pr.Index)
+	remoteDeleted := isRemoteDeleted(pr)
 	if remoteDeleted {
 		remoteBranch = pr.Head.Name // this still holds the original branch name
 		fmt.Printf("Remote branch '%s' already deleted.\n", remoteBranch)
@@ -62,9 +62,9 @@ func PullClean(login *config.Login, repoOwner, repoName string, index int64, ign
 	}
 	if branch == nil {
 		if ignoreSHA {
-			return fmt.Errorf("Remote branch %s not found in local repo", remoteBranch)
+			return fmt.Errorf("remote branch %s not found in local repo", remoteBranch)
 		}
-		return fmt.Errorf(`Remote branch %s not found in local repo.
+		return fmt.Errorf(`remote branch %s not found in local repo.
 Either you don't track this PR, or the local branch has diverged from the remote.
 If you still want to continue & are sure you don't loose any important commits,
 call me again with the --ignore-sha flag`, remoteBranch)

modules/task/pull_merge.go

@@ -18,7 +18,7 @@ func PullMerge(login *config.Login, repoOwner, repoName string, index int64, opt
 		return err
 	}
 	if !success {
-		return fmt.Errorf("Failed to merge PR. Is it still open?")
+		return fmt.Errorf("failed to merge PR, is it still open?")
 	}
 	return nil
 }

modules/utils/path.go

@@ -9,6 +9,7 @@ import (
 	"os/user"
 	"path/filepath"
 	"strings"
+	"syscall"
 )
 
 // PathExists returns whether the given file or directory exists or not
@@ -38,18 +39,19 @@ func exists(path string, expectDir bool) (bool, error) {
 	if err != nil {
 		if errors.Is(err, os.ErrNotExist) {
 			return false, nil
-		} else if err.(*os.PathError).Err.Error() == "not a directory" {
-			// some middle segment of path is a file, cannot traverse
-			// FIXME: catches error on linux; go does not provide a way to catch this properly..
+		}
+		var pathErr *os.PathError
+		if errors.As(err, &pathErr) && errors.Is(pathErr.Err, syscall.ENOTDIR) {
+			// a middle segment of path is a file, cannot traverse
 			return false, nil
 		}
 		return false, err
 	}
 	isDir := f.IsDir()
 	if isDir && !expectDir {
-		return false, errors.New("A directory with the same name exists")
+		return false, errors.New("a directory with the same name exists")
 	} else if !isDir && expectDir {
-		return false, errors.New("A file with the same name exists")
+		return false, errors.New("a file with the same name exists")
 	}
 	return true, nil
 }

modules/utils/validate.go

@@ -21,17 +21,17 @@ func ValidateAuthenticationMethod(
 	// Normalize URL
 	serverURL, err := NormalizeURL(giteaURL)
 	if err != nil {
-		return nil, fmt.Errorf("Unable to parse URL: %s", err)
+		return nil, fmt.Errorf("unable to parse URL: %s", err)
 	}
 
 	if !sshAgent && sshCertPrincipal == "" && sshKey == "" {
 		// .. if we have enough information to authenticate
 		if len(token) == 0 && (len(user)+len(passwd)) == 0 {
-			return nil, fmt.Errorf("No token set")
+			return nil, fmt.Errorf("no token set")
 		} else if len(user) != 0 && len(passwd) == 0 {
-			return nil, fmt.Errorf("No password set")
+			return nil, fmt.Errorf("no password set")
 		} else if len(user) == 0 && len(passwd) != 0 {
-			return nil, fmt.Errorf("No user set")
+			return nil, fmt.Errorf("no user set")
 		}
 	}
 	return serverURL, nil