Merge branch 'main' into limit-repo-size

modules/git/diff.go

@@ -5,6 +5,7 @@ package git
 
 import (
 	"bufio"
+	"context"
 	"fmt"
 	"io"
 	"regexp"
@@ -14,34 +15,64 @@ import (
 	"code.gitea.io/gitea/modules/git/gitcmd"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
-// RawDiffType type of a raw diff.
+// RawDiffType output format: diff or patch
 type RawDiffType string
 
-// RawDiffType possible values.
 const (
 	RawDiffNormal RawDiffType = "diff"
 	RawDiffPatch  RawDiffType = "patch"
 )
 
 // GetRawDiff dumps diff results of repository in given commit ID to io.Writer.
-func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) error {
-	return GetRepoRawDiffForFile(repo, "", commitID, diffType, "", writer)
-}
-
-// GetRepoRawDiffForFile dumps diff results of file in given commit ID to io.Writer according given repository
-func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diffType RawDiffType, file string, writer io.Writer) error {
-	commit, err := repo.GetCommit(endCommit)
+func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) (retErr error) {
+	diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, "", commitID, diffType, "")
 	if err != nil {
 		return err
 	}
+	defer func() {
+		err := diffFinish()
+		if retErr == nil {
+			retErr = err // only return command's error if no previous error
+		}
+	}()
+	_, err = io.Copy(writer, diffOutput)
+	return err
+}
+
+// GetFileDiffCutAroundLine cuts the old or new part of the diff of a file around a specific line number
+func GetFileDiffCutAroundLine(
+	repo *Repository, startCommit, endCommit, treePath string,
+	line int64, old bool, numbersOfLine int,
+) (_ string, retErr error) {
+	diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, startCommit, endCommit, RawDiffNormal, treePath)
+	if err != nil {
+		return "", err
+	}
+	defer func() {
+		err := diffFinish()
+		if retErr == nil {
+			retErr = err // only return command's error if no previous error
+		}
+	}()
+	return CutDiffAroundLine(diffOutput, line, old, numbersOfLine)
+}
+
+// getRepoRawDiffForFile returns an io.Reader for the diff results of file in given commit ID
+// and a "finish" function to wait for the git command and clean up resources after reading is done.
+func getRepoRawDiffForFile(ctx context.Context, repo *Repository, startCommit, endCommit string, diffType RawDiffType, file string) (io.Reader, func() gitcmd.RunStdError, error) {
+	commit, err := repo.GetCommit(endCommit)
+	if err != nil {
+		return nil, nil, err
+	}
 	var files []string
 	if len(file) > 0 {
 		files = append(files, file)
 	}
 
-	cmd := gitcmd.NewCommand()
+	cmd := gitcmd.NewCommand().WithDir(repo.Path)
 	switch diffType {
 	case RawDiffNormal:
 		if len(startCommit) != 0 {
@@ -53,7 +84,7 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff
 		} else {
 			c, err := commit.Parent(0)
 			if err != nil {
-				return err
+				return nil, nil, err
 			}
 			cmd.AddArguments("diff").
 				AddOptionFormat("--find-renames=%s", setting.Git.DiffRenameSimilarityThreshold).
@@ -68,18 +99,25 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff
 		} else {
 			c, err := commit.Parent(0)
 			if err != nil {
-				return err
+				return nil, nil, err
 			}
 			query := fmt.Sprintf("%s...%s", endCommit, c.ID.String())
 			cmd.AddArguments("format-patch", "--no-signature", "--stdout").AddDynamicArguments(query).AddDashesAndList(files...)
 		}
 	default:
-		return fmt.Errorf("invalid diffType: %s", diffType)
+		return nil, nil, util.NewInvalidArgumentErrorf("invalid diff type: %s", diffType)
 	}
 
-	return cmd.WithDir(repo.Path).
-		WithStdoutCopy(writer).
-		RunWithStderr(repo.Ctx)
+	stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe()
+	err = cmd.StartWithStderr(ctx)
+	if err != nil {
+		stdoutReaderClose()
+		return nil, nil, err
+	}
+	return stdoutReader, func() gitcmd.RunStdError {
+		stdoutReaderClose()
+		return cmd.WaitWithStderr()
+	}, nil
 }
 
 // ParseDiffHunkString parse the diff hunk content and return

options/locale/locale_en-US.json

@@ -2663,7 +2663,7 @@
   "repo.branch.new_branch_from": "Create new branch from \"%s\"",
   "repo.branch.renamed": "Branch %s was renamed to %s.",
   "repo.branch.rename_default_or_protected_branch_error": "Only admins can rename default or protected branches.",
-  "repo.branch.rename_protected_branch_failed": "This branch is protected by glob-based protection rules.",
+  "repo.branch.rename_protected_branch_failed": "Failed to rename branch due to branch protection rules.",
   "repo.branch.commits_divergence_from": "Commit divergence: %[1]d behind and %[2]d ahead of %[3]s",
   "repo.branch.commits_no_divergence": "The same as branch %[1]s",
   "repo.tag.create_tag": "Create tag %s",

routers/api/v1/repo/branch.go

@@ -515,7 +515,7 @@ func RenameBranch(ctx *context.APIContext) {
 		case repo_model.IsErrUserDoesNotHaveAccessToRepo(err):
 			ctx.APIError(http.StatusForbidden, "User must be a repo or site admin to rename default or protected branches.")
 		case errors.Is(err, git_model.ErrBranchIsProtected):
-			ctx.APIError(http.StatusForbidden, "Branch is protected by glob-based protection rules.")
+			ctx.APIError(http.StatusForbidden, "Failed to rename branch due to branch protection rules.")
 		default:
 			ctx.APIErrorInternal(err)
 		}

routers/web/repo/editor_cherry_pick.go

@@ -67,7 +67,7 @@ func CherryPickPost(ctx *context.Context) {
 		if parsed.form.Revert {
 			err = gitrepo.GetReverseRawDiff(ctx, ctx.Repo.Repository, fromCommitID, buf)
 		} else {
-			err = git.GetRawDiff(ctx.Repo.GitRepo, fromCommitID, "patch", buf)
+			err = git.GetRawDiff(ctx.Repo.GitRepo, fromCommitID, git.RawDiffPatch, buf)
 		}
 		if err == nil {
 			opts.Content = buf.String()

services/actions/job_emitter.go

@@ -114,7 +114,7 @@ func checkJobsByRunID(ctx context.Context, runID int64) error {
 			}
 		}
 		if runUpdated {
-			NotifyWorkflowRunStatusUpdateWithReload(ctx, jobs[0])
+			NotifyWorkflowRunStatusUpdateWithReload(ctx, js[0])
 		}
 	}
 	return nil

services/migrations/gitea_uploader.go

@@ -895,21 +895,10 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base
 			// SECURITY: The TreePath must be cleaned! use relative path
 			comment.TreePath = util.PathJoinRel(comment.TreePath)
 
-			var patch string
-			reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock
-			defer func() {
-				_ = reader.Close()
-				_ = writer.Close()
-			}()
-			go func(comment *base.ReviewComment) {
-				if err := git.GetRepoRawDiffForFile(g.gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, comment.TreePath, writer); err != nil {
-					// We should ignore the error since the commit maybe removed when force push to the pull request
-					log.Warn("GetRepoRawDiffForFile failed when migrating [%s, %s, %s, %s]: %v", g.gitRepo.Path, pr.MergeBase, headCommitID, comment.TreePath, err)
-				}
-				_ = writer.Close()
-			}(comment)
-
-			patch, _ = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
+			patch, _ := git.GetFileDiffCutAroundLine(
+				g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath,
+				int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines,
+			)
 
 			if comment.CreatedAt.IsZero() {
 				comment.CreatedAt = review.CreatedAt

services/pull/review.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io"
 	"strings"
 
 	"code.gitea.io/gitea/models/db"
@@ -274,22 +273,12 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
 		if len(commitID) == 0 {
 			commitID = headCommitID
 		}
-		reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock
-		defer func() {
-			_ = reader.Close()
-			_ = writer.Close()
-		}()
-		go func() {
-			if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, writer); err != nil {
-				_ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, headCommitID, treePath, err))
-				return
-			}
-			_ = writer.Close()
-		}()
 
-		patch, err = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines)
+		patch, err = git.GetFileDiffCutAroundLine(
+			gitRepo, pr.MergeBase, headCommitID, treePath,
+			int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines,
+		)
 		if err != nil {
-			log.Error("Error whilst generating patch: %v", err)
 			return nil, err
 		}
 

services/repository/branch.go

@@ -442,6 +442,15 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, doer *user_m
 		}
 	}
 
+	// We also need to check if "to" matches with a protected branch rule.
+	rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, repo.ID, to)
+	if err != nil {
+		return "", err
+	}
+	if rule != nil && !rule.CanUserPush(ctx, doer) {
+		return "", git_model.ErrBranchIsProtected
+	}
+
 	if err := git_model.RenameBranch(ctx, repo, from, to, func(ctx context.Context, isDefault bool) error {
 		err2 := gitrepo.RenameBranch(ctx, repo, from, to)
 		if err2 != nil {

tests/integration/api_branch_test.go

@@ -237,7 +237,40 @@ func TestAPIRenameBranch(t *testing.T) {
 			MakeRequest(t, req, http.StatusCreated)
 
 			resp := testAPIRenameBranch(t, "user2", "user2", "repo1", from, "new-branch-name", http.StatusForbidden)
-			assert.Contains(t, resp.Body.String(), "Branch is protected by glob-based protection rules.")
+			assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.")
+		})
+		t.Run("RenameBranchToMatchProtectionRulesWithAllowedUser", func(t *testing.T) {
+			// allow an admin (the owner in this case) to rename a regular branch to one that matches a branch protection rule
+			repoName := "repo1"
+			ownerName := "user2"
+			from := "regular-branch-1"
+			ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository)
+			testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated)
+
+			// NOTE: The protected/** branch protection rule was created in a previous test, with push enabled.
+			testAPIRenameBranch(t, ownerName, ownerName, repoName, from, "protected/2", http.StatusNoContent)
+		})
+		t.Run("RenameBranchToMatchProtectionRulesWithUnauthorizedUser", func(t *testing.T) {
+			// don't allow renaming a regular branch to a protected branch if the doer is not in the push whitelist
+			repoName := "repo1"
+			ownerName := "user2"
+			pushWhitelist := []string{ownerName}
+			token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository)
+			req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/api/v1/repos/%s/%s/branch_protections", ownerName, repoName),
+				&api.BranchProtection{
+					RuleName:               "owner-protected/**",
+					PushWhitelistUsernames: pushWhitelist,
+				}).AddTokenAuth(token)
+			MakeRequest(t, req, http.StatusCreated)
+
+			from := "regular-branch-2"
+			ctx := NewAPITestContext(t, ownerName, repoName, auth_model.AccessTokenScopeWriteRepository)
+			testAPICreateBranch(t, ctx.Session, ownerName, repoName, "", from, http.StatusCreated)
+
+			unprivilegedUser := "user40"
+			resp := testAPIRenameBranch(t, unprivilegedUser, ownerName, repoName, from, "owner-protected/1", http.StatusForbidden)
+
+			assert.Contains(t, resp.Body.String(), "Failed to rename branch due to branch protection rules.")
 		})
 		t.Run("RenameBranchNormalScenario", func(t *testing.T) {
 			testAPIRenameBranch(t, "user2", "user2", "repo1", "branch2", "new-branch-name", http.StatusNoContent)