fix: classify git failures on stderr, restrict migration failure detail (#39010)

Signed-off-by: silverwind <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/git/error.go

@@ -124,20 +124,3 @@ func (err *ErrPushRejected) GenerateMessage() {
 	}
 	err.Message = strings.TrimSpace(messageBuilder.String())
 }
-
-// ErrMoreThanOne represents an error if pull request fails when there are more than one sources (branch, tag) with the same name
-type ErrMoreThanOne struct {
-	StdOut string
-	StdErr string
-	Err    error
-}
-
-// IsErrMoreThanOne checks if an error is a ErrMoreThanOne
-func IsErrMoreThanOne(err error) bool {
-	_, ok := err.(*ErrMoreThanOne)
-	return ok
-}
-
-func (err *ErrMoreThanOne) Error() string {
-	return fmt.Sprintf("ErrMoreThanOne Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
-}

modules/git/gitcmd/command.go

@@ -539,7 +539,7 @@ func (c *Command) WaitWithStderr() RunStdError {
 		// if no exec error but only stderr output, the stderr output is still saved in "c.cmdManagedStderr" and can be read later
 		return nil
 	}
-	return &runStdError{err: errWait, stderr: util.UnsafeBytesToString(c.cmdManagedStderr.Bytes())}
+	return NewRunStdError(errWait, util.UnsafeBytesToString(c.cmdManagedStderr.Bytes()))
 }
 
 func (c *Command) RunWithStderr(ctx context.Context) RunStdError {

modules/git/gitcmd/error.go

@@ -44,6 +44,10 @@ func (r *runStdError) Stderr() string {
 	return r.stderr
 }
 
+func NewRunStdError(err error, stderr string) RunStdError {
+	return &runStdError{err: err, stderr: util.NormalizeStringEOL(stderr)}
+}
+
 func ErrorAsStderr(err error) (string, bool) {
 	if runErr, ok := errors.AsType[RunStdError](err); ok {
 		return runErr.Stderr(), true
@@ -93,6 +97,9 @@ const (
 	StderrNoSuchRemote1 StderrPrefix = "fatal: no such remote" // git < 2.30, exit status 128
 	StderrNoSuchRemote2 StderrPrefix = "error: no such remote" // git >= 2.30. exit status 2
 
+	StderrAuthenticationFailed StderrPrefix = "fatal: Authentication failed for"
+	StderrCouldNotReadUsername StderrPrefix = "fatal: could not read Username"
+
 	StderrUnknownRevisionOrPath StderrRegexp = "^fatal: .*: unknown revision or path not in the working tree"
 	StderrNoMergeBase           StderrRegexp = "^fatal: .*: no merge base"
 	StderrFileNoEnoughLines     StderrRegexp = `^fatal: file .* has only \d+ lines?`
@@ -126,9 +133,11 @@ func IsStderr(err error, checks ...StderrCheck) bool {
 		return false
 	}
 
-	for _, checkIntf := range checks {
-		if matchStderrCheck(stderr, checkIntf) {
-			return true
+	for line := range strings.SplitSeq(stderr, "\n") { // git can emit multiple-line message in stderr
+		for _, checkIntf := range checks {
+			if matchStderrCheck(line, checkIntf) {
+				return true
+			}
 		}
 	}
 	return false

modules/git/gitcmd/error_test.go

@@ -17,8 +17,10 @@ func TestIsStderr(t *testing.T) {
 		{StderrUnknownRevisionOrPath, "fatal: ambiguous argument 'origin': unknown revision or path not in the working tree...."},
 		{StderrNoMergeBase, "fatal: origin/main..HEAD: no merge base...."},
 		{StderrFileNoEnoughLines, "fatal: file foo/bar has only 1 line"},
+		{StderrAuthenticationFailed, "Cloning into 'repo'...\r\nremote: Invalid username or token.\r\nfatal: Authentication failed for 'https://host/repo.git/'\r\n"},
 	}
 	for _, tc := range cases {
 		assert.True(t, IsStderr(&runStdError{stderr: tc.stderr}, tc.check), "stderr: %s", tc.stderr)
 	}
+	assert.False(t, IsStderr(&runStdError{stderr: "remote: fatal: Authentication failed for 'https://host/repo.git/'\n"}, StderrAuthenticationFailed))
 }

modules/git/remote.go

@@ -66,7 +66,7 @@ func (err *ErrInvalidCloneAddr) Unwrap() error {
 
 // IsRemoteNotExistError checks the prefix of the error message to see whether a remote does not exist.
 func IsRemoteNotExistError(err error) bool {
-	return gitcmd.IsStderr(err, gitcmd.StderrNoSuchRemote1) || gitcmd.IsStderr(err, gitcmd.StderrNoSuchRemote2)
+	return gitcmd.IsStderr(err, gitcmd.StderrNoSuchRemote1, gitcmd.StderrNoSuchRemote2)
 }
 
 // ParseRemoteAddr checks if given remote address is valid,

modules/git/remote_managed.go

@@ -47,11 +47,11 @@ func ManagedRemoteRemove(ctx context.Context, repo RepositoryFacade, remoteName
 
 func ParseRemoteAddressURL(ctx context.Context, repo RepositoryFacade, remoteName string) (*giturl.GitURL, error) {
 	addr, err := GetRemoteAddress(ctx, repo, remoteName)
+	if (addr == "" && err == nil) || IsRemoteNotExistError(err) {
+		return nil, util.NewNotExistErrorf("remote '%s' does not exist", remoteName)
+	}
 	if err != nil {
 		return nil, err
 	}
-	if addr == "" {
-		return nil, util.NewNotExistErrorf("remote '%s' does not exist", remoteName)
-	}
 	return giturl.ParseGitURL(addr)
 }

modules/git/repo.go

@@ -279,8 +279,6 @@ func Push(ctx context.Context, localRepoPath string, opts PushOptions) error {
 			err := &ErrPushRejected{StdOut: stdout, StdErr: stderr, Err: err}
 			err.GenerateMessage()
 			return err
-		} else if strings.Contains(stderr, "matches more than one") {
-			return &ErrMoreThanOne{StdOut: stdout, StdErr: stderr, Err: err}
 		}
 		return fmt.Errorf("push failed: %w - %s\n%s", err, stderr, stdout)
 	}

options/locale/locale_en-US.json

@@ -571,8 +571,6 @@
   "form.repository_files_already_exist.adopt": "Files already exist for this repository and can only be adopted.",
   "form.repository_files_already_exist.delete": "Files already exist for this repository. You must delete them.",
   "form.repository_files_already_exist.adopt_or_delete": "Files already exist for this repository. Either adopt them or delete them.",
-  "form.visit_rate_limit": "Remote visit addressed rate limitation.",
-  "form.2fa_auth_required": "Remote visit required two-factor authentication.",
   "form.org_name_been_taken": "The organization name is already taken.",
   "form.team_name_been_taken": "The team name is already taken.",
   "form.team_no_units_error": "Allow access to at least one repository section.",
@@ -603,7 +601,6 @@
   "form.invalid_ssh_principal": "Invalid principal: %s",
   "form.must_use_public_key": "The key you provided is a private key. Please do not upload your private key anywhere. Use your public key instead.",
   "form.unable_verify_ssh_key": "Cannot verify the SSH key. Double-check it for mistakes.",
-  "form.auth_failed": "Authentication failed: %v",
   "form.still_own_repo": "Your account owns one or more repositories. Delete or transfer them first.",
   "form.still_has_org": "Your account is a member of one or more organizations. Leave them first.",
   "form.still_own_packages": "Your account owns one or more packages. Delete them first.",

routers/api/v1/repo/migrate.go

@@ -8,7 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"strings"
 
 	"gitea.dev/models/db"
 	"gitea.dev/models/organization"
@@ -17,6 +16,7 @@ import (
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
+	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/graceful"
 	"gitea.dev/modules/lfs"
 	"gitea.dev/modules/log"
@@ -245,11 +245,9 @@ func handleMigrateError(ctx *context.APIContext, repoOwner *user_model.User, err
 		ctx.APIError(http.StatusUnprocessableEntity, err.Error())
 	default:
 		err = util.SanitizeErrorCredentialURLs(err)
-		if strings.Contains(err.Error(), "Authentication failed") ||
-			strings.Contains(err.Error(), "Bad credentials") ||
-			strings.Contains(err.Error(), "could not read Username") {
+		if migrations.IsAuthenticationError(err) {
 			ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Authentication failed: %v.", err))
-		} else if strings.Contains(err.Error(), "fatal:") {
+		} else if _, ok := gitcmd.ErrorAsStderr(err); ok {
 			ctx.APIError(http.StatusUnprocessableEntity, fmt.Sprintf("Migration failed: %v.", err))
 		} else {
 			ctx.APIErrorInternal(err)

routers/web/repo/issue_list.go

@@ -624,7 +624,7 @@ func prepareIssueFilterAndList(ctx *context.Context, milestoneID int64, projectI
 	showArchivedLabels := ctx.FormBool("archived_labels")
 	ctx.Data["ShowArchivedLabels"] = showArchivedLabels
 	ctx.Data["PinnedIssues"] = pinned
-	ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.Permission.IsAdmin() || ctx.Doer.IsAdmin)
+	ctx.Data["IsRepoAdmin"] = ctx.Repo.Permission.IsAdmin()
 	ctx.Data["IssueStats"] = issueStats
 	ctx.Data["OpenCount"] = issueStats.OpenCount
 	ctx.Data["ClosedCount"] = issueStats.ClosedCount

routers/web/repo/issue_view.go

@@ -399,7 +399,7 @@ func ViewIssue(ctx *context.Context) {
 	ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.Doer.ID)
 	ctx.Data["HasIssuesOrPullsWritePermission"] = ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull)
 	ctx.Data["HasProjectsWritePermission"] = ctx.Repo.Permission.CanWrite(unit.TypeProjects)
-	ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.Permission.IsAdmin() || ctx.Doer.IsAdmin)
+	ctx.Data["IsRepoAdmin"] = ctx.Repo.Permission.IsAdmin()
 	ctx.Data["LockReasons"] = setting.Repository.Issue.LockReasons
 	ctx.Data["RefEndName"] = git.RefName(issue.Ref).ShortName()
 
@@ -937,7 +937,7 @@ func (prInfo *pullRequestViewInfo) prepareMergeBox(ctx *context.Context, issue *
 	// Otherwise, there is nothing to do, because the PR view page already contains enough information.
 	data.ShowMergeBox = !pull.HasMerged || data.IsPullBranchDeletable
 
-	isRepoAdmin := ctx.IsSigned && (ctx.Repo.Permission.IsAdmin() || ctx.Doer.IsAdmin)
+	isRepoAdmin := ctx.Repo.Permission.IsAdmin()
 
 	// admin can merge without checks, writer can merge when checks succeed
 	// admin and writer both can make an auto merge schedule (not affected by overridable blockers)

routers/web/repo/migrate.go

@@ -8,13 +8,13 @@ import (
 	"errors"
 	"net/http"
 	"net/url"
-	"strings"
 
 	admin_model "gitea.dev/models/admin"
 	"gitea.dev/models/db"
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
+	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/json"
 	"gitea.dev/modules/lfs"
 	"gitea.dev/modules/log"
@@ -81,10 +81,6 @@ func handleMigrateError(ctx *context.Context, owner *user_model.User, err error,
 	var errNameReserved db.ErrNameReserved
 	var errNamePatternNotAllowed db.ErrNamePatternNotAllowed
 	switch {
-	case migrations.IsRateLimitError(err):
-		ctx.RenderWithErrDeprecated(ctx.Tr("form.visit_rate_limit"), tpl, form)
-	case migrations.IsTwoFactorAuthError(err):
-		ctx.RenderWithErrDeprecated(ctx.Tr("form.2fa_auth_required"), tpl, form)
 	case repo_model.IsErrReachLimitOfRepo(err):
 		maxCreationLimit := owner.MaxCreationLimit()
 		msg := ctx.TrN(maxCreationLimit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", maxCreationLimit)
@@ -112,12 +108,7 @@ func handleMigrateError(ctx *context.Context, owner *user_model.User, err error,
 		ctx.RenderWithErrDeprecated(ctx.Tr("repo.form.name_pattern_not_allowed", errNamePatternNotAllowed.Pattern), tpl, form)
 	default:
 		err = util.SanitizeErrorCredentialURLs(err)
-		if strings.Contains(err.Error(), "Authentication failed") ||
-			strings.Contains(err.Error(), "Bad credentials") ||
-			strings.Contains(err.Error(), "could not read Username") {
-			ctx.Data["Err_Auth"] = true
-			ctx.RenderWithErrDeprecated(ctx.Tr("form.auth_failed", err.Error()), tpl, form)
-		} else if strings.Contains(err.Error(), "fatal:") {
+		if _, fromGit := gitcmd.ErrorAsStderr(err); fromGit {
 			ctx.Data["Err_CloneAddr"] = true
 			ctx.RenderWithErrDeprecated(ctx.Tr("repo.migrate.failed", err.Error()), tpl, form)
 		} else {
@@ -312,7 +303,11 @@ func MigrateStatus(ctx *context.Context) {
 
 	message := task.Message
 
-	if task.Message != "" && task.Message[0] == '{' {
+	// a failure message can echo bytes the remote chose, so only whoever started the migration may read it
+	canSeeFailure := (ctx.Doer != nil && ctx.Doer.ID == task.DoerID) || ctx.Repo.Permission.IsAdmin()
+	if task.Status == structs.TaskStatusFailed && !canSeeFailure {
+		message = ctx.Locale.TrString("repo.migrate.migrating_failed_no_addr")
+	} else if message != "" && message[0] == '{' {
 		// assume message is actually a translatable string
 		var translatableMessage admin_model.TranslatableMessage
 		if err := json.Unmarshal([]byte(message), &translatableMessage); err != nil {

services/migrations/error.go

@@ -6,6 +6,9 @@ package migrations
 
 import (
 	"errors"
+	"net/http"
+
+	"gitea.dev/modules/git/gitcmd"
 
 	"github.com/google/go-github/v91/github"
 )
@@ -24,3 +27,12 @@ func IsTwoFactorAuthError(err error) bool {
 	_, ok := err.(*github.TwoFactorAuthError)
 	return ok
 }
+
+// IsAuthenticationError returns true if the remote rejected the credentials, over git or over its HTTP API
+func IsAuthenticationError(err error) bool {
+	if gitcmd.IsStderr(err, gitcmd.StderrAuthenticationFailed, gitcmd.StderrCouldNotReadUsername) {
+		return true
+	}
+	githubErr, ok := errors.AsType[*github.ErrorResponse](err)
+	return ok && githubErr.Response != nil && githubErr.Response.StatusCode == http.StatusUnauthorized
+}

services/migrations/migrate.go

@@ -130,7 +130,8 @@ func MigrateRepository(ctx context.Context, doer *user_model.User, ownerName str
 		if err1 := uploader.Rollback(); err1 != nil {
 			log.Error("rollback failed: %v", err1)
 		}
-		if err2 := system_model.CreateRepositoryNotice(fmt.Sprintf("Migrate repository (%s/%s) from %s failed: %v", ownerName, opts.RepoName, opts.OriginalURL, err)); err2 != nil {
+		noticeMsg := fmt.Sprintf("Migrate repository (%s/%s) from %s failed: %v", ownerName, opts.RepoName, util.SanitizeCredentialURLs(opts.OriginalURL), util.SanitizeErrorCredentialURLs(err))
+		if err2 := system_model.CreateRepositoryNotice(noticeMsg); err2 != nil {
 			log.Error("create repository notice failed: ", err2)
 		}
 		return nil, err

services/migrations/migrate_test.go

@@ -4,17 +4,44 @@
 package migrations
 
 import (
+	"errors"
+	"fmt"
 	"net"
+	"net/http"
 	"path/filepath"
 	"testing"
 
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/setting"
+	"gitea.dev/modules/util"
 
+	"github.com/google/go-github/v91/github"
 	"github.com/stretchr/testify/assert"
 )
 
+func TestIsAuthenticationError(t *testing.T) {
+	errDummy := errors.New("dummy")
+	cases := []struct {
+		name string
+		want bool
+		err  error
+	}{
+		{"git authentication failed", true, gitcmd.NewRunStdError(errDummy, "fatal: Authentication failed for 'https://host/repo.git/'")},
+		{"git could not read username", true, fmt.Errorf("%w", gitcmd.NewRunStdError(errDummy, "fatal: could not read Username for 'https://host'"))},
+		{"github unauthorized", true, util.SanitizeErrorCredentialURLs(&github.ErrorResponse{Response: &http.Response{StatusCode: http.StatusUnauthorized}})},
+		{"github other", false, &github.ErrorResponse{Response: &http.Response{StatusCode: http.StatusNotFound}}},
+		{"github nil response", false, &github.ErrorResponse{}},
+		{"unrelated error", false, errDummy},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			assert.Equal(t, c.want, IsAuthenticationError(c.err))
+		})
+	}
+}
+
 func TestMigrateWhiteBlocklist(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

services/mirror/mirror_push.go

@@ -14,6 +14,7 @@ import (
 	"gitea.dev/models/db"
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/modules/git"
+	"gitea.dev/modules/git/gitrepo"
 	"gitea.dev/modules/lfs"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/process"
@@ -102,6 +103,7 @@ func SyncPushMirror(ctx context.Context, mirrorID int64) bool {
 	log.Trace("SyncPushMirror [mirror: %d][repo: %-v]: Running Sync", m.ID, m.Repo)
 	err = runPushSync(ctx, m)
 	if err != nil {
+		err = util.SanitizeErrorCredentialURLs(err)
 		log.Error("SyncPushMirror [mirror: %d][repo: %-v]: %v", m.ID, m.Repo, err)
 		m.LastError = stripExitStatus.ReplaceAllLiteralString(err.Error(), "")
 	}
@@ -110,7 +112,6 @@ func SyncPushMirror(ctx context.Context, mirrorID int64) bool {
 
 	if err := repo_model.UpdatePushMirror(ctx, m); err != nil {
 		log.Error("UpdatePushMirror [%d]: %v", m.ID, err)
-
 		return false
 	}
 
@@ -122,16 +123,17 @@ func SyncPushMirror(ctx context.Context, mirrorID int64) bool {
 func runPushSync(ctx context.Context, m *repo_model.PushMirror) error {
 	timeout := time.Duration(setting.Git.Timeout.Mirror) * time.Second
 
-	performPush := func(repo *repo_model.Repository, isWiki bool) error {
-		storageRepo := repo.CodeStorageRepo()
-		if isWiki {
-			storageRepo = repo.WikiStorageRepo()
-		}
-		mirrorLogName := fmt.Sprintf("%s%s[mirror=%d]", m.Repo.FullName(), util.Iif(isWiki, ".wiki", ""), m.ID)
+	performPush := func(storageRepo gitrepo.RepositoryFacade) error {
 		remoteURL, err := git.ParseRemoteAddressURL(ctx, storageRepo, m.RemoteName)
 		if err != nil {
-			log.Error("GetRemoteURL %s failed, error %v", mirrorLogName, err)
-			return errors.New("GitRemoteGetURL failed")
+			return fmt.Errorf("ParseRemoteAddressURL failed: %w", err)
+		}
+		// re-validate every sync, the allow/block lists may have changed since the mirror was added
+		switch remoteURL.URL.Scheme {
+		case "http", "https", "git":
+			if err := migrations.IsMigrateURLAllowed(remoteURL.String(), nil); err != nil {
+				return fmt.Errorf("remote address is not allowed: %w", err)
+			}
 		}
 
 		if setting.LFS.StartServer {
@@ -139,21 +141,20 @@ func runPushSync(ctx context.Context, m *repo_model.PushMirror) error {
 
 			gitRepo, err := git.OpenRepository(ctx, storageRepo)
 			if err != nil {
-				log.Error("OpenRepository %s failed: %v", mirrorLogName, err)
-				return errors.New("OpenRepository failed")
+				return fmt.Errorf("OpenRepository failed: %w", err)
 			}
 			defer gitRepo.Close()
 
 			lfsClient, err := lfs.NewClientFromEndpoint(remoteURL.String(), "", migrations.NewMigrationHTTPTransport())
 			if err != nil {
-				return err
+				return fmt.Errorf("NewClientFromEndpoint failed: %w", err)
 			}
 			if err := pushAllLFSObjects(ctx, gitRepo, lfsClient); err != nil {
-				return util.SanitizeErrorCredentialURLs(err)
+				return fmt.Errorf("pushAllLFSObjects failed: %w", err)
 			}
 		}
 
-		log.Trace("Pushing %s remote %s", mirrorLogName, m.ID, m.RemoteName)
+		log.Trace("Pushing mirror %d repo %s to remote %s", m.ID, storageRepo.LogString(), m.RemoteName)
 
 		envs := proxy.EnvWithProxy(remoteURL.URL)
 		if err := git.PushToExternal(ctx, storageRepo, git.PushOptions{
@@ -163,26 +164,21 @@ func runPushSync(ctx context.Context, m *repo_model.PushMirror) error {
 			Timeout: timeout,
 			Env:     envs,
 		}); err != nil {
-			log.Error("Error pushing %s remote %s: %v", mirrorLogName, m.RemoteName, err)
-			return util.SanitizeErrorCredentialURLs(err)
+			return fmt.Errorf("PushToExternal failed: %w", err)
 		}
 
 		return nil
 	}
 
-	err := performPush(m.Repo, false)
+	err := performPush(m.Repo.CodeStorageRepo())
 	if err != nil {
-		return err
+		return fmt.Errorf("performPush(code) failed: %w", err)
 	}
 
 	if repo_service.HasWiki(ctx, m.Repo) {
-		if _, err := git.ParseRemoteAddressURL(ctx, m.Repo.WikiStorageRepo(), m.RemoteName); err == nil {
-			err := performPush(m.Repo, true)
-			if err != nil {
-				return err
-			}
-		} else if !errors.Is(err, util.ErrNotExist) {
-			log.Error("GetRemote of wiki failed: %v", err)
+		err := performPush(m.Repo.WikiStorageRepo())
+		if err != nil && !errors.Is(err, util.ErrNotExist) {
+			return fmt.Errorf("performPush(wiki) failed: %w", err)
 		}
 	}
 

services/pull/pull.go

@@ -559,58 +559,34 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
 // PushToBaseRepo pushes commits from branches of head repository to
 // corresponding branches of base repository.
 // FIXME: Only push branches that are actually updates?
-func PushToBaseRepo(ctx context.Context, pr *issues_model.PullRequest) (err error) {
-	return pushToBaseRepoHelper(ctx, pr, "")
-}
-
-func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, prefixHeadBranch string) (err error) {
+func PushToBaseRepo(ctx context.Context, pr *issues_model.PullRequest) error {
 	log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitHeadRefName())
 
 	if err := pr.LoadHeadRepo(ctx); err != nil {
-		log.Error("Unable to load head repository for PR[%d] Error: %v", pr.ID, err)
 		return err
 	}
-
 	if err := pr.LoadBaseRepo(ctx); err != nil {
-		log.Error("Unable to load base repository for PR[%d] Error: %v", pr.ID, err)
+		return err
+	}
+	if err := pr.LoadIssue(ctx); err != nil {
+		return err
+	}
+	if err := pr.Issue.LoadPoster(ctx); err != nil {
 		return err
 	}
 
-	if err = pr.LoadIssue(ctx); err != nil {
-		return fmt.Errorf("unable to load issue %d for pr %d: %w", pr.IssueID, pr.ID, err)
-	}
-	if err = pr.Issue.LoadPoster(ctx); err != nil {
-		return fmt.Errorf("unable to load poster %d for pr %d: %w", pr.Issue.PosterID, pr.ID, err)
-	}
-
-	gitRefName := pr.GetGitHeadRefName()
-
+	baseRepoHeadRefName := pr.GetGitHeadRefName()
 	if err := git.PushManaged(ctx, pr.HeadRepo, pr.BaseRepo, git.PushOptions{
-		Branch: prefixHeadBranch + pr.HeadBranch + ":" + gitRefName,
+		Branch: git.BranchPrefix + pr.HeadBranch + ":" + baseRepoHeadRefName,
 		Force:  true,
 		// Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/...
 		Env: repo_module.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo),
 	}); err != nil {
-		if git.IsErrPushOutOfDate(err) {
-			// This should not happen as we're using force!
-			log.Error("Unable to push PR head for %s#%d (%-v:%s) due to ErrPushOfDate: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err)
-			return err
-		} else if rejectErr, ok := err.(*git.ErrPushRejected); ok {
-			log.Info("Unable to push PR head for %s#%d (%-v:%s) due to rejection:\nStdout: %s\nStderr: %s\nError: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err)
-			return err
-		} else if git.IsErrMoreThanOne(err) {
-			if prefixHeadBranch != "" {
-				log.Info("Can't push with %s%s", prefixHeadBranch, pr.HeadBranch)
-				return err
-			}
-			log.Info("Retrying to push with %s%s", git.BranchPrefix, pr.HeadBranch)
-			err = pushToBaseRepoHelper(ctx, pr, git.BranchPrefix)
-			return err
-		}
-		log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err)
-		return fmt.Errorf("Push: %s:%s %s:%s %w", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), gitRefName, err)
+		// Since we use internal force-push, there should be no git error.
+		// If any error happens, it must be an internal error (e.g.: broken git hooks) but not user error.
+		return fmt.Errorf("unable to push from head branch %s:%s to base repo %s:%s, err: %w",
+			pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), baseRepoHeadRefName, err)
 	}
-
 	return nil
 }
 

services/repository/migrate.go

@@ -145,7 +145,7 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
 		}
 
 		if _, _, err := repo_module.SyncRepoBranchesWithRepo(ctx, repo, gitRepo, u.ID); err != nil {
-			return repo, fmt.Errorf("SyncRepoBranchesWithRepo: %v", err)
+			return repo, fmt.Errorf("SyncRepoBranchesWithRepo: %w", err)
 		}
 
 		// if releases migration are not requested, we will sync all tags here

services/task/migrate.go

@@ -7,13 +7,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	admin_model "gitea.dev/models/admin"
 	"gitea.dev/models/db"
 	repo_model "gitea.dev/models/repo"
 	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/graceful"
 	"gitea.dev/modules/json"
 	"gitea.dev/modules/log"
@@ -145,10 +145,10 @@ func runMigrateTask(ctx context.Context, t *admin_model.Task) (err error) {
 
 	// remoteAddr may contain credentials, so we sanitize it
 	err = util.SanitizeErrorCredentialURLs(err)
-	if strings.Contains(err.Error(), "Authentication failed") ||
-		strings.Contains(err.Error(), "could not read Username") {
+	if migrations.IsAuthenticationError(err) {
 		return fmt.Errorf("authentication failed: %w", err)
-	} else if strings.Contains(err.Error(), "fatal:") {
+	}
+	if _, fromGit := gitcmd.ErrorAsStderr(err); fromGit {
 		return fmt.Errorf("migration failed: %w", err)
 	}
 

templates/repo/migrate/codebase.tmpl

@@ -19,11 +19,11 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_username">{{ctx.Locale.Tr "username"}}</label>
 					<input id="auth_username" name="auth_username" value="{{.auth_username}}" {{if not .auth_username}}data-need-clear="true"{{end}}>
 				</div>
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_password">{{ctx.Locale.Tr "password"}}</label>
 					<input id="auth_password" name="auth_password" type="password" value="{{.auth_password}}">
 				</div>

templates/repo/migrate/codecommit.tmpl

@@ -19,19 +19,19 @@
 					</span>
 				</div>
 
-				<div class="inline required field {{if .Err_Auth}}error{{end}}">
+				<div class="inline required field">
 					<label for="aws_access_key_id">{{ctx.Locale.Tr "repo.migrate.codecommit.aws_access_key_id"}}</label>
 					<input id="aws_access_key_id" name="aws_access_key_id" value="{{.aws_access_key_id}}" required>
 				</div>
-				<div class="inline required field {{if .Err_Auth}}error{{end}}">
+				<div class="inline required field">
 					<label for="aws_secret_access_key">{{ctx.Locale.Tr "repo.migrate.codecommit.aws_secret_access_key"}}</label>
 					<input id="aws_secret_access_key" name="aws_secret_access_key" type="password" value="{{.aws_secret_access_key}}" required>
 				</div>
-				<div class="inline required field {{if .Err_Auth}}error{{end}}">
+				<div class="inline required field">
 					<label for="auth_username">{{ctx.Locale.Tr "repo.migrate.codecommit.https_git_credentials_username"}}</label>
 					<input id="auth_username" name="auth_username" value="{{.auth_username}}" required>
 				</div>
-				<div class="inline required field {{if .Err_Auth}}error{{end}}">
+				<div class="inline required field">
 					<label for="auth_password">{{ctx.Locale.Tr "repo.migrate.codecommit.https_git_credentials_password"}}</label>
 					<input id="auth_password" name="auth_password" type="password" value="{{.auth_password}}" required>
 				</div>

templates/repo/migrate/git.tmpl

@@ -18,11 +18,11 @@
 					{{ctx.Locale.Tr "repo.migrate.clone_address_desc"}}{{if .ContextUser.CanImportLocal}} {{ctx.Locale.Tr "repo.migrate.clone_local_path"}}{{end}}
 					</span>
 				</div>
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_username">{{ctx.Locale.Tr "username"}}</label>
 					<input id="auth_username" name="auth_username" value="{{.auth_username}}" {{if not .auth_username}}data-need-clear="true"{{end}}>
 				</div>
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_password">{{ctx.Locale.Tr "password"}}</label>
 					<input id="auth_password" name="auth_password" type="password" value="{{.auth_password}}">
 				</div>

templates/repo/migrate/gitbucket.tmpl

@@ -19,11 +19,11 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_username">{{ctx.Locale.Tr "username"}}</label>
 					<input id="auth_username" name="auth_username" value="{{.auth_username}}" {{if not .auth_username}}data-need-clear="true"{{end}}>
 				</div>
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_password">{{ctx.Locale.Tr "password"}}</label>
 					<input id="auth_password" name="auth_password" type="password" value="{{.auth_password}}">
 				</div>

templates/repo/migrate/gitea.tmpl

@@ -18,7 +18,7 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_token">{{ctx.Locale.Tr "access_token"}}</label>
 					<input id="auth_token" name="auth_token" type="password" autocomplete="new-password" value="{{.auth_token}}" {{if not .auth_token}} data-need-clear="true" {{end}}>
 					<a target="_blank" href="https://docs.gitea.com/development/api-usage">{{svg "octicon-question"}}</a>

templates/repo/migrate/github.tmpl

@@ -18,7 +18,7 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_token">{{ctx.Locale.Tr "access_token"}}</label>
 					<input id="auth_token" name="auth_token" type="password" autocomplete="new-password" value="{{.auth_token}}" {{if not .auth_token}}data-need-clear="true"{{end}}>
 					<a target="_blank" href="https://docs.github.com/en/github/authenticating-to-github/creating-a-personal-access-token">{{svg "octicon-question"}}</a>

templates/repo/migrate/gitlab.tmpl

@@ -18,7 +18,7 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_token">{{ctx.Locale.Tr "access_token"}}</label>
 					<input id="auth_token" name="auth_token" type="password" autocomplete="new-password" value="{{.auth_token}}" {{if not .auth_token}}data-need-clear="true"{{end}}>
 					<a target="_blank" href="https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html">{{svg "octicon-question"}}</a>

templates/repo/migrate/gogs.tmpl

@@ -18,7 +18,7 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_token">{{ctx.Locale.Tr "access_token"}}</label>
 					<input id="auth_token" name="auth_token" type="password" autocomplete="new-password" value="{{.auth_token}}" {{if not .auth_token}} data-need-clear="true" {{end}}>
 					<!-- <a target="_blank" href="https://docs.gitea.com/development/api-usage">{{svg "octicon-question"}}</a> -->

templates/repo/migrate/onedev.tmpl

@@ -19,11 +19,11 @@
 					</span>
 				</div>
 
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_username">{{ctx.Locale.Tr "username"}}</label>
 					<input id="auth_username" name="auth_username" value="{{.auth_username}}" {{if not .auth_username}}data-need-clear="true"{{end}}>
 				</div>
-				<div class="inline field {{if .Err_Auth}}error{{end}}">
+				<div class="inline field">
 					<label for="auth_password">{{ctx.Locale.Tr "password"}}</label>
 					<input id="auth_password" name="auth_password" type="password" value="{{.auth_password}}">
 				</div>

templates/repo/settings/options.tmpl

@@ -154,16 +154,16 @@
 											<input id="mirror_address" name="mirror_address" value="{{$address.Address}}" required>
 											<p class="help">{{ctx.Locale.Tr "repo.mirror_address_desc"}}</p>
 										</div>
-										<details class="ui optional field" {{if or .Err_Auth $address.Username}}open{{end}}>
+										<details class="ui optional field" {{if $address.Username}}open{{end}}>
 											<summary class="tw-p-1">
 												{{ctx.Locale.Tr "repo.need_auth"}}
 											</summary>
 											<div class="tw-p-1">
-												<div class="inline field {{if .Err_Auth}}error{{end}}">
+												<div class="inline field">
 													<label for="mirror_username">{{ctx.Locale.Tr "username"}}</label>
 													<input id="mirror_username" name="mirror_username" value="{{$address.Username}}" {{if not .mirror_username}}data-need-clear="true"{{end}}>
 												</div>
-												<div class="inline field {{if .Err_Auth}}error{{end}}">
+												<div class="inline field">
 													<label for="mirror_password">{{ctx.Locale.Tr "password"}}</label>
 													<input id="mirror_password" name="mirror_password" type="password" placeholder="{{if $address.Password}}{{ctx.Locale.Tr "repo.mirror_password_placeholder"}}{{else}}{{ctx.Locale.Tr "repo.mirror_password_blank_placeholder"}}{{end}}" value="" {{if not .mirror_password}}data-need-clear="true"{{end}} autocomplete="off">
 												</div>

tests/integration/mirror_push_test.go

@@ -76,6 +76,11 @@ func testMirrorPush(t *testing.T, u *url.URL) {
 
 	assert.Equal(t, srcCommit.ID, mirrorCommit.ID)
 
+	defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, false)() // the remote is re-checked every sync, not just when added
+	assert.NoError(t, migrations.Init())
+	t.Cleanup(func() { _ = migrations.Init() })
+	assert.False(t, mirror_service.SyncPushMirror(t.Context(), mirrors[0].ID))
+
 	// Cleanup
 	assert.True(t, doRemovePushMirror(t, session, user.Name, srcRepo.Name, mirrors[0].ID))
 	mirrors, _, err = repo_model.GetPushMirrorsByRepoID(t.Context(), srcRepo.ID, db.ListOptions{})