refactor: fix legacy problems in cmd/serv.go (#38505)

1. use var names "reqOwnerName, reqRepoName", because these values are
from request and should not be really used for path construction
2. simplify enable-pprof logic, don't "log.Fatal"
3. don't make runServe command to guess the repo storage path, instead,
let server return RepoStoragePath
4. don't process lfs verbs when the repo is a wiki
5. construct the request URI path correctly for the lfs transfer backend
(moved to the caller)
6. don't call "owner, err := user_model.GetUserByName", the "owner
rename redirection" has been done before
7. fix incorrect "repo.OwnerName = ownerName", the real owner might have
been "redirected"
8. fix incorrect "inactive owner" check, it should be checked even if
the repo is redirected


Use general error functions to handle responses, error handling code is
hugely simplified.

cmd/serv.go

@@ -201,35 +201,27 @@ func runServ(ctx context.Context, c *cli.Command) error {
 		return fail(ctx, "Too few arguments", "Too few arguments in cmd: %s", cmd)
 	}
 
-	repoPath := strings.TrimPrefix(sshCmdArgs[1], "/")
+	var reqOwnerName, reqRepoName string
-	repoPathFields := strings.SplitN(repoPath, "/", 2)
+	{
-	if len(repoPathFields) != 2 {
+		var ok bool
-		return fail(ctx, "Invalid repository path", "Invalid repository path: %v", repoPath)
+		reqRepoPath := strings.TrimPrefix(sshCmdArgs[1], "/")
+		reqOwnerName, reqRepoName, ok = strings.Cut(reqRepoPath, "/")
+		if !ok {
+			return fail(ctx, "Invalid repository path", "Invalid repository path: %v", reqRepoPath)
+		}
+		reqRepoName = strings.TrimSuffix(reqRepoName, ".git") // "the-repo-name" or "the-repo-name.wiki"
 	}
 
-	username := repoPathFields[0]
+	if !repo_model.IsValidSSHAccessRepoName(reqRepoName) {
-	reponame := strings.TrimSuffix(repoPathFields[1], ".git") // “the-repo-name" or "the-repo-name.wiki"
+		return fail(ctx, "Invalid repo name", "Invalid repo name: %s", reqRepoName)
-
-	if !repo_model.IsValidSSHAccessRepoName(reponame) {
-		return fail(ctx, "Invalid repo name", "Invalid repo name: %s", reponame)
 	}
 
 	if c.Bool("enable-pprof") {
-		if err := os.MkdirAll(setting.PprofDataPath, os.ModePerm); err != nil {
+		stopProfiler, err := pprof.DumpPprofForUsername(setting.PprofDataPath, reqOwnerName)
-			return fail(ctx, "Error while trying to create PPROF_DATA_PATH", "Error while trying to create PPROF_DATA_PATH: %v", err)
-		}
-
-		stopCPUProfiler, err := pprof.DumpCPUProfileForUsername(setting.PprofDataPath, username)
 		if err != nil {
-			return fail(ctx, "Unable to start CPU profiler", "Unable to start CPU profile: %v", err)
+			return fail(ctx, "Unable to start pprof profiler", "Unable to start pprof profile: %v", err)
 		}
-		defer func() {
+		defer stopProfiler()
-			stopCPUProfiler()
-			err := pprof.DumpMemProfileForUsername(setting.PprofDataPath, username)
-			if err != nil {
-				_ = fail(ctx, "Unable to dump Mem profile", "Unable to dump Mem Profile: %v", err)
-			}
-		}()
 	}
 
 	verb, lfsVerb := sshCmdArgs[0], ""
@@ -254,30 +246,29 @@ func runServ(ctx context.Context, c *cli.Command) error {
 		return fail(ctx, "Unknown git command", "Unknown git command %s %s", verb, lfsVerb)
 	}
 
-	results, extra := private.ServCommand(ctx, keyID, username, reponame, requestedMode, verb, lfsVerb)
+	results, extra := private.ServCommand(ctx, keyID, reqOwnerName, reqRepoName, requestedMode, verb, lfsVerb)
 	if extra.HasError() {
 		return fail(ctx, extra.UserMsg, "ServCommand failed: %s", extra.Error)
 	}
 
-	// because the original repoPath maybe redirected, we need to use the returned actual repository information
-	if results.IsWiki {
-		repoPath = repo_model.RelativeWikiPath(results.OwnerName, results.RepoName)
-	} else {
-		repoPath = repo_model.RelativePath(results.OwnerName, results.RepoName)
-	}
-
 	// LFS SSH protocol
 	if verb == git.CmdVerbLfsTransfer {
+		if results.IsWiki {
+			return fail(ctx, "LFS Transfer is not supported for wikis", "")
+		}
 		token, err := lfs.GetLFSAuthTokenWithBearer(lfs.AuthTokenOptions{Op: lfsVerb, UserID: results.UserID, RepoID: results.RepoID})
 		if err != nil {
 			return err
 		}
-		return lfstransfer.Main(ctx, repoPath, lfsVerb, token)
+		return lfstransfer.Main(ctx, results.OwnerName, results.RepoName, lfsVerb, token)
 	}
 
 	// LFS token authentication
 	if verb == git.CmdVerbLfsAuthenticate {
-		url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName))
+		if results.IsWiki {
+			return fail(ctx, "LFS Authenticate is not supported for wikis", "")
+		}
+		lfsTokenHref := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName))
 
 		token, err := lfs.GetLFSAuthTokenWithBearer(lfs.AuthTokenOptions{Op: lfsVerb, UserID: results.UserID, RepoID: results.RepoID})
 		if err != nil {
@@ -286,7 +277,7 @@ func runServ(ctx context.Context, c *cli.Command) error {
 
 		tokenAuthentication := &git_model.LFSTokenResponse{
 			Header: make(map[string]string),
-			Href:   url,
+			Href:   lfsTokenHref,
 		}
 		tokenAuthentication.Header["Authorization"] = token
 
@@ -307,12 +298,12 @@ func runServ(ctx context.Context, c *cli.Command) error {
 		verbFields := strings.SplitN(verb, "-", 2)
 		if len(verbFields) == 2 {
 			// use git binary with the sub-command part: "C:\...\bin\git.exe", "upload-pack", ...
-			command = exec.CommandContext(ctx, gitcmd.GitExecutable, verbFields[1], repoPath)
+			command = exec.CommandContext(ctx, gitcmd.GitExecutable, verbFields[1], results.RepoStoragePath)
 		}
 	}
 	if command == nil {
 		// by default, use the verb (it has been checked above by allowedCommands)
-		command = exec.CommandContext(ctx, gitBinVerb, repoPath)
+		command = exec.CommandContext(ctx, gitBinVerb, results.RepoStoragePath)
 	}
 
 	process.SetSysProcAttribute(command)

models/repo/repo.go

@@ -230,7 +230,7 @@ func RelativePath(ownerName, repoName string) string {
 	return strings.ToLower(ownerName) + "/" + strings.ToLower(repoName) + ".git"
 }
 
-// RelativePath should be an unix style path like username/reponame.git
+// RelativePath should be a unix style path like "owner-name/repo-name.git"
 func (repo *Repository) RelativePath() string {
 	return RelativePath(repo.OwnerName, repo.Name)
 }

models/repo/wiki.go

@@ -79,8 +79,8 @@ func RelativeWikiPath(ownerName, repoName string) string {
 	return strings.ToLower(ownerName) + "/" + strings.ToLower(repoName) + ".wiki.git"
 }
 
-// WikiStorageRepo returns the storage repo for the wiki
+// WikiStorageRepo returns the storage repo for the wiki like "owner-name/repo-name.wiki.git"
-// The wiki repository should have the same object format as the code repository
+// The wiki repository should have the same object format as the code repository. TODO: REALLY? Why?
 func (repo *Repository) WikiStorageRepo() StorageRepo {
 	return StorageRepo(RelativeWikiPath(repo.OwnerName, repo.Name))
 }

modules/lfstransfer/backend/backend.go

@@ -40,13 +40,12 @@ type GiteaBackend struct {
 	logger       transfer.Logger
 }
 
-func New(ctx context.Context, repo, op, token string, logger transfer.Logger) (transfer.Backend, error) {
+func New(ctx context.Context, reqPath, op, token string, logger transfer.Logger) (transfer.Backend, error) {
-	// runServ guarantees repo will be in form [owner]/[name].git
 	server, err := url.Parse(setting.LocalURL)
 	if err != nil {
 		return nil, err
 	}
-	server = server.JoinPath("api/internal/repo", repo, "info/lfs")
+	server = server.JoinPath(reqPath)
 	return &GiteaBackend{ctx: ctx, server: server, op: op, authToken: token, internalAuth: "Bearer " + setting.InternalToken, logger: logger}, nil
 }
 

modules/lfstransfer/main.go

@@ -6,6 +6,7 @@ package lfstransfer
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"os"
 
 	"gitea.dev/modules/lfstransfer/backend"
@@ -13,23 +14,24 @@ import (
 	"github.com/charmbracelet/git-lfs-transfer/transfer"
 )
 
-func Main(ctx context.Context, repo, verb, token string) error {
+func Main(ctx context.Context, ownerName, repoName, verb, token string) error {
 	logger := newLogger()
-	pktline := transfer.NewPktline(os.Stdin, os.Stdout, logger)
+	backendReqPath := fmt.Sprintf("api/internal/repo/%s/%s.git/info/lfs", url.PathEscape(ownerName), url.PathEscape(repoName))
-	giteaBackend, err := backend.New(ctx, repo, verb, token, logger)
+	giteaBackend, err := backend.New(ctx, backendReqPath, verb, token, logger)
 	if err != nil {
 		return err
 	}
 
+	pktLine := transfer.NewPktline(os.Stdin, os.Stdout, logger)
 	for _, cap := range backend.Capabilities {
-		if err := pktline.WritePacketText(cap); err != nil {
+		if err := pktLine.WritePacketText(cap); err != nil {
 			logger.Log("error sending capability due to error:", err)
 		}
 	}
-	if err := pktline.WriteFlush(); err != nil {
+	if err := pktLine.WriteFlush(); err != nil {
 		logger.Log("error flushing capabilities:", err)
 	}
-	p := transfer.NewProcessor(pktline, giteaBackend, logger)
+	p := transfer.NewProcessor(pktLine, giteaBackend, logger)
 	defer logger.Log("done processing commands")
 	switch verb {
 	case "upload":

modules/pprof/pprof.go

@@ -6,15 +6,15 @@ package pprof
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"runtime"
 	"runtime/pprof"
 
 	"gitea.dev/modules/log"
 )
 
-// DumpMemProfileForUsername dumps a memory profile at pprofDataPath as memprofile_<username>_<temporary id>
+func dumpMemProfileForUsername(pprofDataPath, subName string) error {
-func DumpMemProfileForUsername(pprofDataPath, username string) error {
+	f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("pprof_mem_%s_", filepath.Clean(subName)))
-	f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("memprofile_%s_", username))
 	if err != nil {
 		return err
 	}
@@ -23,23 +23,30 @@ func DumpMemProfileForUsername(pprofDataPath, username string) error {
 	return pprof.WriteHeapProfile(f)
 }
 
-// DumpCPUProfileForUsername dumps a CPU profile at pprofDataPath as cpuprofile_<username>_<temporary id>
+func DumpPprofForUsername(pprofDataPath, subName string) (func(), error) {
-// the stop function it returns stops, writes and closes the CPU profile file
+	if err := os.MkdirAll(pprofDataPath, os.ModePerm); err != nil {
-func DumpCPUProfileForUsername(pprofDataPath, username string) (func(), error) {
+		return nil, fmt.Errorf(`os.MkdirAll(pprofDataPath) failed: %v`, err)
-	f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("cpuprofile_%s_", username))
+	}
+
+	f, err := os.CreateTemp(pprofDataPath, fmt.Sprintf("pprof_cpu_%s_", filepath.Clean(subName)))
 	if err != nil {
 		return nil, err
 	}
 
 	err = pprof.StartCPUProfile(f)
 	if err != nil {
-		log.Fatal("StartCPUProfile: %v", err)
+		_ = f.Close()
+		return nil, fmt.Errorf("StartCPUProfile: %w", err)
 	}
 	return func() {
 		pprof.StopCPUProfile()
 		err = f.Close()
 		if err != nil {
-			log.Fatal("StopCPUProfile Close: %v", err)
+			log.Error("StopCPUProfile Close: %v", err)
+		}
+		err = dumpMemProfileForUsername(pprofDataPath, subName)
+		if err != nil {
+			log.Error("DumpMemProfile: %v", err)
 		}
 	}, nil
 }

modules/private/serv.go

@@ -43,6 +43,8 @@ type ServCommandResults struct {
 	OwnerName   string
 	RepoName    string
 	RepoID      int64
+
+	RepoStoragePath string
 }
 
 // ServCommand preps for a serv call

routers/private/hook_post_receive.go

@@ -63,7 +63,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts
 		}
 		if update.IsDelRef() {
 			if err := git_model.MarkBranchAsDeleted(ctx, repo.ID, update.RefFullName.BranchName(), update.PusherID); err != nil {
-				ctx.PrivateError(http.StatusInternalServerError, err, fmt.Sprintf("failed to mark branch %s as deleted", update.RefFullName))
+				ctx.PrivateInternalErrorf("failed to mark branch %s as deleted: %v", update.RefFullName, err)
 				return false
 			}
 		} else {
@@ -79,7 +79,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts
 
 	gitRepo, err := gitrepo.RepositoryFromRequestContextOrOpen(ctx, repo)
 	if err != nil {
-		ctx.PrivateError(http.StatusInternalServerError, err, "failed to open repository")
+		ctx.PrivateInternalErrorf("failed to open repository: %v", err)
 		return false
 	}
 
@@ -91,7 +91,7 @@ func hookPostReceiveSyncDatabaseBranches(ctx *gitea_context.PrivateContext, opts
 	}
 
 	if err = repo_service.SyncBranchesToDB(ctx, repo.ID, opts.UserID, gitRepo, branchNames, commitIDs); err != nil {
-		ctx.PrivateError(http.StatusInternalServerError, err, "failed to sync branch to DB")
+		ctx.PrivateInternalErrorf("failed to sync branch to DB: %v", err)
 		return false
 	}
 	return true
@@ -133,7 +133,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 
 	// push async updates
 	if err := repo_service.PushUpdates(updates...); err != nil {
-		ctx.PrivateError(http.StatusInternalServerError, err, "failed to push updates")
+		ctx.PrivateInternalErrorf("failed to push updates: %v", err)
 		return
 	}
 
@@ -147,16 +147,16 @@ func hookPostReceiveUpdateRepoByOptions(ctx *gitea_context.PrivateContext, opts
 	if isPrivate.Has() || isTemplate.Has() {
 		pusher, err := loadContextCacheUser(ctx, opts.UserID)
 		if err != nil {
-			ctx.PrivateError(http.StatusInternalServerError, err, "failed to load pusher user")
+			ctx.PrivateInternalErrorf("failed to load pusher user: %v", err)
 			return false
 		}
 		perm, err := access_model.GetDoerRepoPermission(ctx, repo, pusher)
 		if err != nil {
-			ctx.PrivateError(http.StatusInternalServerError, err, "failed to load doer repo permission")
+			ctx.PrivateInternalErrorf("failed to load doer repo permission: %v", err)
 			return false
 		}
 		if !perm.IsOwner() && !perm.IsAdmin() {
-			ctx.PrivateError(http.StatusNotFound, nil, "permission denied")
+			ctx.PrivateUserErrorf(http.StatusNotFound, "permission denied")
 			return false
 		}
 
@@ -191,7 +191,7 @@ func hookPostReceiveRespondWithTrailer(ctx *gitea_context.PrivateContext, opts *
 	baseRepo := repo
 	if repo.IsFork {
 		if err := repo.GetBaseRepo(ctx); err != nil {
-			ctx.PrivateError(http.StatusInternalServerError, err, "failed to load base repo")
+			ctx.PrivateInternalErrorf("failed to load base repo: %v", err)
 			return
 		}
 		if repo.BaseRepo.AllowsPulls(ctx) {
@@ -222,7 +222,7 @@ func hookPostReceiveRespondWithTrailer(ctx *gitea_context.PrivateContext, opts *
 
 			pr, err := issues_model.GetUnmergedPullRequest(ctx, repo.ID, baseRepo.ID, branch, baseRepo.DefaultBranch, issues_model.PullRequestFlowGithub)
 			if err != nil && !errors.Is(err, util.ErrNotExist) {
-				ctx.PrivateError(http.StatusInternalServerError, err, "failed to get active PR for branch "+branch)
+				ctx.PrivateInternalErrorf("failed to get active PR for branch %s: %v", branch, err)
 				return
 			}
 			if pr == nil {
@@ -252,20 +252,19 @@ func loadContextCacheUser(ctx context.Context, id int64) (*user_model.User, erro
 // hookPostReceiveHandlePullRequestMerging handle pull request merging, a pull request action should push at least 1 commit
 func hookPostReceiveHandlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, updates []*repo_module.PushUpdateOptions) bool {
 	if len(updates) == 0 {
-		err := fmt.Errorf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID)
+		ctx.PrivateInternalErrorf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID)
-		ctx.PrivateError(http.StatusInternalServerError, err, "no push update")
 		return false
 	}
 
 	pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID)
 	if err != nil {
-		ctx.PrivateError(http.StatusInternalServerError, err, "failed to load pull request")
+		ctx.PrivateInternalErrorf("failed to get pull request %d: %v", opts.PullRequestID, err)
 		return false
 	}
 
 	pusher, err := loadContextCacheUser(ctx, opts.UserID)
 	if err != nil {
-		ctx.PrivateError(http.StatusInternalServerError, err, "failed to load pusher user")
+		ctx.PrivateInternalErrorf("failed to load pusher user %d: %v", opts.UserID, err)
 		return false
 	}
 
@@ -273,7 +272,7 @@ func hookPostReceiveHandlePullRequestMerging(ctx *gitea_context.PrivateContext,
 	// here to keep it as before, that maybe PullRequestStatusMergeable
 	_, err = pull_service.SetMerged(ctx, pr, updates[len(updates)-1].NewCommitID, timeutil.TimeStampNow(), pusher, pr.Status)
 	if err != nil {
-		ctx.PrivateError(http.StatusInternalServerError, err, "failed to set pr to merged")
+		ctx.PrivateInternalErrorf("failed to set pr %d to merged: %v", pr.ID, err)
 		return false
 	}
 	return true

routers/private/serv.go

@@ -4,7 +4,6 @@
 package private
 
 import (
-	"fmt"
 	"net/http"
 	"strings"
 
@@ -18,6 +17,7 @@ import (
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/private"
 	"gitea.dev/modules/setting"
+	"gitea.dev/modules/util"
 	"gitea.dev/services/context"
 	repo_service "gitea.dev/services/repository"
 	wiki_service "gitea.dev/services/wiki"
@@ -27,24 +27,18 @@ import (
 func ServNoCommand(ctx *context.PrivateContext) {
 	keyID := ctx.PathParamInt64("keyid")
 	if keyID <= 0 {
-		ctx.JSON(http.StatusBadRequest, private.Response{
+		ctx.PrivateUserErrorf(http.StatusBadRequest, "Bad key id: %d", keyID)
-			UserMsg: fmt.Sprintf("Bad key id: %d", keyID),
+		return
-		})
 	}
 	results := private.KeyAndOwner{}
 
 	key, err := asymkey_model.GetPublicKeyByID(ctx, keyID)
 	if err != nil {
 		if asymkey_model.IsErrKeyNotExist(err) {
-			ctx.JSON(http.StatusUnauthorized, private.Response{
+			ctx.PrivateUserErrorf(http.StatusUnauthorized, "Cannot find key: %d", keyID)
-				UserMsg: fmt.Sprintf("Cannot find key: %d", keyID),
-			})
 			return
 		}
-		log.Error("Unable to get public key: %d Error: %v", keyID, err)
+		ctx.PrivateInternalErrorf("Unable to get public key: %d Error: %v", keyID, err)
-		ctx.JSON(http.StatusInternalServerError, private.Response{
-			Err: err.Error(),
-		})
 		return
 	}
 	results.Key = key
@@ -53,21 +47,14 @@ func ServNoCommand(ctx *context.PrivateContext) {
 		user, err := user_model.GetUserByID(ctx, key.OwnerID)
 		if err != nil {
 			if user_model.IsErrUserNotExist(err) {
-				ctx.JSON(http.StatusUnauthorized, private.Response{
+				ctx.PrivateUserErrorf(http.StatusUnauthorized, "Cannot find owner with id: %d for key: %d", key.OwnerID, keyID)
-					UserMsg: fmt.Sprintf("Cannot find owner with id: %d for key: %d", key.OwnerID, keyID),
-				})
 				return
 			}
-			log.Error("Unable to get owner with id: %d for public key: %d Error: %v", key.OwnerID, keyID, err)
+			ctx.PrivateInternalErrorf("Unable to get owner with id: %d for public key: %d Error: %v", key.OwnerID, keyID, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: err.Error(),
-			})
 			return
 		}
 		if !user.IsActive || user.ProhibitLogin {
-			ctx.JSON(http.StatusForbidden, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Your account is disabled.")
-				UserMsg: "Your account is disabled.",
-			})
 			return
 		}
 		results.Owner = user
@@ -78,87 +65,56 @@ func ServNoCommand(ctx *context.PrivateContext) {
 // ServCommand returns information about the provided keyid
 func ServCommand(ctx *context.PrivateContext) {
 	keyID := ctx.PathParamInt64("keyid")
-	ownerName := ctx.PathParam("owner")
+	reqOwnerName := ctx.PathParam("owner")
-	repoName := ctx.PathParam("repo")
+	reqRepoName := ctx.PathParam("repo")
 	mode := perm.AccessMode(ctx.FormInt("mode"))
 	verb := ctx.FormString("verb")
 
 	// Set the basic parts of the results to return
 	results := private.ServCommandResults{
-		RepoName:  repoName,
+		OwnerName: reqOwnerName, // it might be changed if there is "renamed user redirection"
-		OwnerName: ownerName,
+		RepoName:  reqRepoName,  // it might be changed if there is "renamed repo redirection", or the repo is a wiki
 		KeyID:     keyID,
 	}
+	repoLogName := reqOwnerName + "/" + reqRepoName
 
-	// Now because we're not translating things properly let's just default some English strings here
+	if reqWikiRepoName, ok := strings.CutSuffix(reqRepoName, ".wiki"); ok {
-	modeString := "read"
+		// in which case we need to look at the wiki, trim the ".wiki" suffix, only use the main repo name
-	if mode > perm.AccessModeRead {
-		modeString = "write to"
-	}
-
-	// The default unit we're trying to look at is code
-	unitType := unit.TypeCode
-
-	// Unless we're a wiki...
-	if strings.HasSuffix(repoName, ".wiki") {
-		// in which case we need to look at the wiki
-		unitType = unit.TypeWiki
-		// And we'd better munge the reponame and tell downstream we're looking at a wiki
 		results.IsWiki = true
-		results.RepoName = repoName[:len(repoName)-5]
+		results.RepoName = reqWikiRepoName
 	}
+	unitType := util.Iif(results.IsWiki, unit.TypeWiki, unit.TypeCode)
+	modeString := mode.ToString()
 
 	owner, err := user_model.GetUserByName(ctx, results.OwnerName)
 	if err != nil {
 		if !user_model.IsErrUserNotExist(err) {
-			log.Error("Unable to get repository owner: %s/%s Error: %v", results.OwnerName, results.RepoName, err)
+			ctx.PrivateInternalErrorf("Unable to get repository owner for %s, error: %v", repoLogName, err)
-			ctx.JSON(http.StatusForbidden, private.Response{
-				UserMsg: fmt.Sprintf("Unable to get repository owner: %s/%s %v", results.OwnerName, results.RepoName, err),
-			})
 			return
 		}
 
 		// Check if there is a user redirect for the requested owner
 		redirectedUserID, err := user_model.LookupUserRedirect(ctx, results.OwnerName)
 		if err != nil {
-			// User is fetching/cloning a non-existent repository
+			ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository %s", repoLogName)
-			log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr())
-			ctx.JSON(http.StatusNotFound, private.Response{
-				UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName),
-			})
 			return
 		}
-
 		redirectUser, err := user_model.GetUserByID(ctx, redirectedUserID)
 		if err != nil {
-			// User is fetching/cloning a non-existent repository
+			ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository: %s", repoLogName)
-			log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr())
-			ctx.JSON(http.StatusNotFound, private.Response{
-				UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName),
-			})
 			return
 		}
 
-		log.Info("User %s has been redirected to %s", results.OwnerName, redirectUser.Name)
+		log.Debug("User %s has been redirected to %s", results.OwnerName, redirectUser.Name)
 		results.OwnerName = redirectUser.Name
 		owner = redirectUser
 	}
-	if !owner.IsOrganization() && !owner.IsActive {
-		ctx.JSON(http.StatusForbidden, private.Response{
-			UserMsg: "Repository cannot be accessed, you could retry it later",
-		})
-		return
-	}
 
 	// Now get the Repository and set the results section
-	repoExist := true
 	repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, results.RepoName)
 	if err != nil {
 		if !repo_model.IsErrRepoNotExist(err) {
-			log.Error("Unable to get repository: %s/%s Error: %v", results.OwnerName, results.RepoName, err)
+			ctx.PrivateInternalErrorf("Unable to get repository %s, error: %v", repoLogName, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Unable to get repository: %s/%s %v", results.OwnerName, results.RepoName, err),
-			})
 			return
 		}
 
@@ -166,53 +122,53 @@ func ServCommand(ctx *context.PrivateContext) {
 		if err == nil {
 			redirectedRepo, err := repo_model.GetRepositoryByID(ctx, redirectedRepoID)
 			if err == nil {
-				log.Info("Repository %s/%s has been redirected to %s/%s", results.OwnerName, results.RepoName, redirectedRepo.OwnerName, redirectedRepo.Name)
+				log.Info("Repository %s has been redirected to %s/%s", repoLogName, redirectedRepo.OwnerName, redirectedRepo.Name)
+				repo = redirectedRepo
+				if err = repo.LoadOwner(ctx); err != nil {
+					ctx.PrivateInternalErrorf("Unable to repository owner %d", repo.OwnerID)
+					return
+				}
+				owner = repo.Owner
 				results.RepoName = redirectedRepo.Name
 				results.OwnerName = redirectedRepo.OwnerName
-				repo = redirectedRepo
+				repoLogName = results.OwnerName + "/" + results.RepoName + util.Iif(results.IsWiki, ".wiki", "")
-				owner.ID = redirectedRepo.OwnerID
 			} else {
-				log.Warn("Repo %s/%s has a redirect to repo with ID %d, but no repo with this ID could be found. Trying without redirect...", results.OwnerName, results.RepoName, redirectedRepoID)
+				log.Warn("Repo %s has a redirect to repo with ID %d, but no repo with this ID could be found. Trying without redirect...", repoLogName, redirectedRepoID)
 			}
 		}
 
 		if repo == nil {
-			repoExist = false
 			if mode == perm.AccessModeRead {
 				// User is fetching/cloning a non-existent repository
-				log.Warn("Failed authentication attempt (cannot find repository: %s/%s) from %s", results.OwnerName, results.RepoName, ctx.RemoteAddr())
+				ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository %s", repoLogName)
-				ctx.JSON(http.StatusNotFound, private.Response{
-					UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName),
-				})
 				return
 			}
 		}
 	}
 
-	if repoExist {
+	if !owner.IsOrganization() && !owner.IsActive {
+		ctx.PrivateUserErrorf(http.StatusForbidden, "Repository cannot be accessed, the owner is inactive.")
+		return
+	}
+
+	if repo != nil {
 		repo.Owner = owner
-		repo.OwnerName = ownerName
+		repo.OwnerName = owner.Name
 		results.RepoID = repo.ID
 
 		if repo.IsBeingCreated() {
-			ctx.JSON(http.StatusInternalServerError, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Repository is being created, you could retry after it finished")
-				Err: "Repository is being created, you could retry after it finished",
-			})
 			return
 		}
 
 		if repo.IsBroken() {
-			ctx.JSON(http.StatusInternalServerError, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Repository is in a broken state")
-				Err: "Repository is in a broken state",
-			})
 			return
 		}
 
 		// We can shortcut at this point if the repo is a mirror
 		if mode > perm.AccessModeRead && repo.IsMirror {
-			ctx.JSON(http.StatusForbidden, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Mirror Repository %s is read-only", repoLogName)
-				UserMsg: fmt.Sprintf("Mirror Repository %s/%s is read-only", results.OwnerName, results.RepoName),
-			})
 			return
 		}
 	}
@@ -221,48 +177,33 @@ func ServCommand(ctx *context.PrivateContext) {
 	key, err := asymkey_model.GetPublicKeyByID(ctx, keyID)
 	if err != nil {
 		if asymkey_model.IsErrKeyNotExist(err) {
-			ctx.JSON(http.StatusNotFound, private.Response{
+			ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find key: %d", keyID)
-				UserMsg: fmt.Sprintf("Cannot find key: %d", keyID),
-			})
 			return
 		}
-		log.Error("Unable to get public key: %d Error: %v", keyID, err)
+		ctx.PrivateInternalErrorf("Unable to get key: %d, error: %v", keyID, err)
-		ctx.JSON(http.StatusInternalServerError, private.Response{
-			Err: fmt.Sprintf("Unable to get key: %d  Error: %v", keyID, err),
-		})
 		return
 	}
 	results.KeyName = key.Name
 	results.KeyID = key.ID
 	results.UserID = key.OwnerID
 
-	// If repo doesn't exist, deploy key doesn't make sense
-	if !repoExist && key.Type == asymkey_model.KeyTypeDeploy {
-		ctx.JSON(http.StatusNotFound, private.Response{
-			UserMsg: fmt.Sprintf("Cannot find repository %s/%s", results.OwnerName, results.RepoName),
-		})
-		return
-	}
-
 	// Deploy Keys have ownerID set to 0 therefore we can't use the owner
 	// So now we need to check if the key is a deploy key
 	// We'll keep hold of the deploy key here for permissions checking
 	var deployKey *asymkey_model.DeployKey
 	var user *user_model.User
 	if key.Type == asymkey_model.KeyTypeDeploy {
-		var err error
+		if repo == nil {
+			ctx.PrivateUserErrorf(http.StatusNotFound, "Cannot find repository %s", repoLogName)
+			return
+		}
 		deployKey, err = asymkey_model.GetDeployKeyByRepo(ctx, key.ID, repo.ID)
 		if err != nil {
 			if asymkey_model.IsErrDeployKeyNotExist(err) {
-				ctx.JSON(http.StatusNotFound, private.Response{
+				ctx.PrivateUserErrorf(http.StatusNotFound, "Deploy key %d:%s has no %q permission for %s.", key.ID, key.Name, modeString, repoLogName)
-					UserMsg: fmt.Sprintf("Public (Deploy) Key: %d:%s is not authorized to %s %s/%s.", key.ID, key.Name, modeString, results.OwnerName, results.RepoName),
-				})
 				return
 			}
-			log.Error("Unable to get deploy for public (deploy) key: %d in %-v Error: %v", key.ID, repo, err)
+			ctx.PrivateInternalErrorf("Unable to get deploy for public (deploy) key %d for %s, error: %v", key.ID, repoLogName, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Unable to get Deploy Key for Public Key: %d:%s in %s/%s.", key.ID, key.Name, results.OwnerName, results.RepoName),
-			})
 			return
 		}
 		results.DeployKeyID = deployKey.ID
@@ -278,26 +219,18 @@ func ServCommand(ctx *context.PrivateContext) {
 		}
 	} else {
 		// Get the user represented by the Key
-		var err error
 		user, err = user_model.GetUserByID(ctx, key.OwnerID)
 		if err != nil {
 			if user_model.IsErrUserNotExist(err) {
-				ctx.JSON(http.StatusUnauthorized, private.Response{
+				ctx.PrivateUserErrorf(http.StatusUnauthorized, "Public key %d:%s owner %d does not exist.", key.ID, key.Name, key.OwnerID)
-					UserMsg: fmt.Sprintf("Public Key: %d:%s owner %d does not exist.", key.ID, key.Name, key.OwnerID),
-				})
 				return
 			}
-			log.Error("Unable to get owner: %d for public key: %d:%s Error: %v", key.OwnerID, key.ID, key.Name, err)
+			ctx.PrivateInternalErrorf("Unable to get key owner %d for public key %d:%s, error: %v", key.OwnerID, key.ID, key.Name, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Unable to get Owner: %d for Deploy Key: %d:%s in %s/%s.", key.OwnerID, key.ID, key.Name, ownerName, repoName),
-			})
 			return
 		}
 
 		if !user.IsActive || user.ProhibitLogin {
-			ctx.JSON(http.StatusForbidden, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Your account is disabled.")
-				UserMsg: "Your account is disabled.",
-			})
 			return
 		}
 
@@ -308,25 +241,21 @@ func ServCommand(ctx *context.PrivateContext) {
 	}
 
 	// Don't allow pushing if the repo is archived
-	if repoExist && mode > perm.AccessModeRead && repo.IsArchived {
+	if repo != nil && mode > perm.AccessModeRead && repo.IsArchived {
-		ctx.JSON(http.StatusUnauthorized, private.Response{
+		ctx.PrivateUserErrorf(http.StatusUnauthorized, "Repo %s is archived.", repoLogName)
-			UserMsg: fmt.Sprintf("Repo: %s/%s is archived.", results.OwnerName, results.RepoName),
-		})
 		return
 	}
 
 	// Permissions checking:
-	if repoExist &&
+	if repo != nil &&
 		(mode > perm.AccessModeRead ||
 			repo.IsPrivate ||
 			owner.Visibility.IsPrivate() ||
-			(user != nil && user.IsRestricted) || // user will be nil if the key is a deploykey
+			(user != nil && user.IsRestricted) || // user will be nil if the key is a deploy key
 			setting.Service.RequireSignInViewStrict) {
 		if key.Type == asymkey_model.KeyTypeDeploy {
-			if deployKey.Mode < mode {
+			if deployKey == nil || deployKey.Mode < mode {
-				ctx.JSON(http.StatusUnauthorized, private.Response{
+				ctx.PrivateUserErrorf(http.StatusUnauthorized, "Deploy key %d:%s has no %q permission for %s.", key.ID, key.Name, modeString, repoLogName)
-					UserMsg: fmt.Sprintf("Deploy Key: %d:%s is not authorized to %s %s/%s.", key.ID, key.Name, modeString, results.OwnerName, results.RepoName),
-				})
 				return
 			}
 		} else {
@@ -338,56 +267,34 @@ func ServCommand(ctx *context.PrivateContext) {
 				mode = perm.AccessModeRead
 			}
 
-			perm, err := access_model.GetDoerRepoPermission(ctx, repo, user)
+			userPerm, err := access_model.GetDoerRepoPermission(ctx, repo, user)
 			if err != nil {
-				log.Error("Unable to get permissions for %-v with key %d in %-v Error: %v", user, key.ID, repo, err)
+				ctx.PrivateInternalErrorf("Unable to get permissions for %-v with key %d in %-v, error: %v", user, key.ID, repo, err)
-				ctx.JSON(http.StatusInternalServerError, private.Response{
-					Err: fmt.Sprintf("Unable to get permissions for user %d:%s with key %d in %s/%s Error: %v", user.ID, user.Name, key.ID, results.OwnerName, results.RepoName, err),
-				})
 				return
 			}
 
-			userMode := perm.UnitAccessMode(unitType)
+			userMode := userPerm.UnitAccessMode(unitType)
-
 			if userMode < mode {
-				log.Warn("Failed authentication attempt for %s with key %s (not authorized to %s %s/%s) from %s", user.Name, key.Name, modeString, ownerName, repoName, ctx.RemoteAddr())
+				ctx.PrivateUserErrorf(http.StatusUnauthorized, "User %d with key %d:%s has no %q permission for %s", key.OwnerID, key.ID, key.Name, modeString, repoLogName)
-				ctx.JSON(http.StatusUnauthorized, private.Response{
-					UserMsg: fmt.Sprintf("User: %d:%s with Key: %d:%s is not authorized to %s %s/%s.", user.ID, user.Name, key.ID, key.Name, modeString, ownerName, repoName),
-				})
 				return
 			}
 		}
 	}
 
 	// We already know we aren't using a deploy key
-	if !repoExist {
+	if repo == nil {
-		owner, err := user_model.GetUserByName(ctx, ownerName)
-		if err != nil {
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Unable to get owner: %s %v", results.OwnerName, err),
-			})
-			return
-		}
-
 		if owner.IsOrganization() && !setting.Repository.EnablePushCreateOrg {
-			ctx.JSON(http.StatusForbidden, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Push to create is not enabled for organizations.")
-				UserMsg: "Push to create is not enabled for organizations.",
-			})
 			return
 		}
 		if !owner.IsOrganization() && !setting.Repository.EnablePushCreateUser {
-			ctx.JSON(http.StatusForbidden, private.Response{
+			ctx.PrivateUserErrorf(http.StatusForbidden, "Push to create is not enabled for users.")
-				UserMsg: "Push to create is not enabled for users.",
-			})
 			return
 		}
 
 		repo, err = repo_service.PushCreateRepo(ctx, user, owner, results.RepoName)
 		if err != nil {
-			log.Error("pushCreateRepo: %v", err)
+			ctx.PrivateInternalErrorf("pushCreateRepo: %v", err)
-			ctx.JSON(http.StatusNotFound, private.Response{
-				UserMsg: fmt.Sprintf("Cannot find repository: %s/%s", results.OwnerName, results.RepoName),
-			})
 			return
 		}
 		results.RepoID = repo.ID
@@ -397,38 +304,22 @@ func ServCommand(ctx *context.PrivateContext) {
 		// Ensure the wiki is enabled before we allow access to it
 		if _, err := repo.GetUnit(ctx, unit.TypeWiki); err != nil {
 			if repo_model.IsErrUnitTypeNotExist(err) {
-				ctx.JSON(http.StatusForbidden, private.Response{
+				ctx.PrivateUserErrorf(http.StatusForbidden, "repository wiki is disabled")
-					UserMsg: "repository wiki is disabled",
-				})
 				return
 			}
-			log.Error("Failed to get the wiki unit in %-v Error: %v", repo, err)
+			ctx.PrivateInternalErrorf("Failed to get the wiki unit in %-v, error: %v", repo, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Failed to get the wiki unit in %s/%s Error: %v", ownerName, repoName, err),
-			})
 			return
 		}
 
 		// Finally if we're trying to touch the wiki we should init it
 		if err = wiki_service.InitWiki(ctx, repo); err != nil {
-			log.Error("Failed to initialize the wiki in %-v Error: %v", repo, err)
+			ctx.PrivateInternalErrorf("Failed to initialize the wiki in %-v, error: %v", repo, err)
-			ctx.JSON(http.StatusInternalServerError, private.Response{
-				Err: fmt.Sprintf("Failed to initialize the wiki in %s/%s Error: %v", ownerName, repoName, err),
-			})
 			return
 		}
 	}
-	log.Debug("Serv Results:\nIsWiki: %t\nDeployKeyID: %d\nKeyID: %d\tKeyName: %s\nUserName: %s\nUserID: %d\nOwnerName: %s\nRepoName: %s\nRepoID: %d",
-		results.IsWiki,
-		results.DeployKeyID,
-		results.KeyID,
-		results.KeyName,
-		results.UserName,
-		results.UserID,
-		results.OwnerName,
-		results.RepoName,
-		results.RepoID)
 
+	results.RepoStoragePath = util.Iif(results.IsWiki, repo_model.RelativeWikiPath(repo.OwnerName, repo.Name), repo.RelativePath())
+	log.Debug("Serv Results: %+v", results)
 	ctx.JSON(http.StatusOK, results)
 	// We will update the keys in a different call.
 }

services/context/private.go

@@ -5,10 +5,12 @@ package context
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"time"
 
 	"gitea.dev/modules/graceful"
+	"gitea.dev/modules/log"
 	"gitea.dev/modules/private"
 	"gitea.dev/modules/process"
 	"gitea.dev/modules/web"
@@ -50,12 +52,14 @@ func (ctx *PrivateContext) Err() error {
 	return ctx.Base.Err()
 }
 
-func (ctx *PrivateContext) PrivateError(status int, err error, userMsg string) {
+func (ctx *PrivateContext) PrivateInternalErrorf(format string, args ...any) {
-	errMsg := ""
+	s := fmt.Sprintf(format, args...)
-	if err != nil {
+	log.ErrorWithSkip(1, "Internal error: %s", s)
-		errMsg = err.Error()
+	ctx.JSON(http.StatusInternalServerError, private.Response{Err: s})
-	}
+}
-	ctx.JSON(status, private.Response{Err: errMsg, UserMsg: userMsg})
+
+func (ctx *PrivateContext) PrivateUserErrorf(status int, format string, args ...any) {
+	ctx.JSON(status, private.Response{UserMsg: fmt.Sprintf(format, args...)})
 }
 
 type privateContextKeyType struct{}

services/repository/repository.go

@@ -72,6 +72,9 @@ func DeleteRepository(ctx context.Context, doer *user_model.User, repo *repo_mod
 
 // PushCreateRepo creates a repository when a new repository is pushed to an appropriate namespace
 func PushCreateRepo(ctx context.Context, authUser, owner *user_model.User, repoName string) (*repo_model.Repository, error) {
+	if authUser == nil {
+		return nil, errors.New("cannot push-create repository anonymously")
+	}
 	if !authUser.IsAdmin {
 		if owner.IsOrganization() {
 			if ok, err := organization.CanCreateOrgRepo(ctx, owner.ID, authUser.ID); err != nil {

tests/integration/git_general_test.go

@@ -171,7 +171,7 @@ func doSSHLFSAccessTest(_ APITestContext, keyID int64) func(*testing.T) {
 			_, err := cmd.Output()
 			var errExit *exec.ExitError
 			require.ErrorAs(t, err, &errExit) // inaccessible, error
-			assert.Contains(t, string(errExit.Stderr), fmt.Sprintf("User: 2:user2 with Key: %d:test-key is not authorized to write to user5/repo4.", keyID))
+			assert.Contains(t, string(errExit.Stderr), fmt.Sprintf(`User 2 with key %d:test-key has no "write" permission for user5/repo4`, keyID))
 		})
 	}
 }

tests/integration/git_helper_for_declarative_test.go

@@ -137,6 +137,7 @@ func onGiteaRun[T testing.TB](t T, callback func(T, *url.URL)) {
 
 func doGitClone(dstLocalPath string, u *url.URL) func(*testing.T) {
 	return func(t *testing.T) {
+		t.Helper()
 		assert.NoError(t, git.Clone(t.Context(), u.String(), dstLocalPath, git.CloneRepoOptions{}))
 		exist, err := util.IsExist(filepath.Join(dstLocalPath, "README.md"))
 		assert.NoError(t, err)