Release of 1.21.10 (#30073)

The main reason for this release is to fix a bug #29997

CHANGELOG.md

@@ -4,6 +4,16 @@ This changelog goes through all the changes that have been made in each release
 without substantial changes to our git log; to see the highlights of what has
 been added to each release, please refer to the [blog](https://blog.gitea.com).
 
+## [1.21.10](https://github.com/go-gitea/gitea/releases/tag/1.21.10) - 2024-03-25
+
+* BUGFIXES
+  * Fix Add/Remove WIP on pull request title failure (#29999) (#30066)
+  * Fix misuse of `TxContext` (#30061) (#30062)
+  * Respect DEFAULT_ORG_MEMBER_VISIBLE setting when adding creator to org (#30013) (#30035)
+  * Escape paths for find file correctly (#30026) (#30031)
+  * Remove duplicate option in admin screen and now-unused translation keys (#28492) (#30024)
+  * Fix manual merge form and 404 page templates (#30000)
+
 ## [1.21.9](https://github.com/go-gitea/gitea/releases/tag/1.21.9) - 2024-03-21
 
 * PERFORMANCE

models/db/context.go

@@ -120,6 +120,16 @@ func (c *halfCommitter) Close() error {
 
 // TxContext represents a transaction Context,
 // it will reuse the existing transaction in the parent context or create a new one.
+// Some tips to use:
+//
+//	1 It's always recommended to use `WithTx` in new code instead of `TxContext`, since `WithTx` will handle the transaction automatically.
+//	2. To maintain the old code which uses `TxContext`:
+//	  a. Always call `Close()` before returning regardless of whether `Commit()` has been called.
+//	  b. Always call `Commit()` before returning if there are no errors, even if the code did not change any data.
+//	  c. Remember the `Committer` will be a halfCommitter when a transaction is being reused.
+//	     So calling `Commit()` will do nothing, but calling `Close()` without calling `Commit()` will rollback the transaction.
+//	     And all operations submitted by the caller stack will be rollbacked as well, not only the operations in the current function.
+//	  d. It doesn't mean rollback is forbidden, but always do it only when there is an error, and you do want to rollback.
 func TxContext(parentCtx context.Context) (*Context, Committer, error) {
 	if sess, ok := inTransaction(parentCtx); ok {
 		return newContext(parentCtx, sess, true), &halfCommitter{committer: sess}, nil

models/issues/review.go

@@ -621,7 +621,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo
 
 	// skip it when reviewer hase been request to review
 	if review != nil && review.Type == ReviewTypeRequest {
-		return nil, nil
+		return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction.
 	}
 
 	// if the reviewer is an official reviewer,

models/organization/org.go

@@ -319,8 +319,9 @@ func CreateOrganization(org *Organization, owner *user_model.User) (err error) {
 
 	// Add initial creator to organization and owner team.
 	if err = db.Insert(ctx, &OrgUser{
-		UID:   owner.ID,
-		OrgID: org.ID,
+		UID:      owner.ID,
+		OrgID:    org.ID,
+		IsPublic: setting.Service.DefaultOrgMemberVisible,
 	}); err != nil {
 		return fmt.Errorf("insert org-user relation: %w", err)
 	}

options/locale/locale_en-US.ini

@@ -3078,7 +3078,6 @@ config.enable_openid_signin = Enable OpenID Sign-In
 config.show_registration_button = Show Register Button
 config.require_sign_in_view = Require Sign-In to View Pages
 config.mail_notify = Enable Email Notifications
-config.disable_key_size_check = Disable Minimum Key Size Check
 config.enable_captcha = Enable CAPTCHA
 config.active_code_lives = Active Code Lives
 config.reset_password_code_lives = Recover Account Code Expiry Time

routers/web/repo/find.go

@@ -8,6 +8,7 @@ import (
 
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/util"
 )
 
 const (
@@ -17,7 +18,7 @@ const (
 // FindFiles render the page to find repository files
 func FindFiles(ctx *context.Context) {
 	path := ctx.Params("*")
-	ctx.Data["TreeLink"] = ctx.Repo.RepoLink + "/src/" + path
-	ctx.Data["DataLink"] = ctx.Repo.RepoLink + "/tree-list/" + path
+	ctx.Data["TreeLink"] = ctx.Repo.RepoLink + "/src/" + util.PathEscapeSegments(path)
+	ctx.Data["DataLink"] = ctx.Repo.RepoLink + "/tree-list/" + util.PathEscapeSegments(path)
 	ctx.HTML(http.StatusOK, tplFindFiles)
 }

services/issue/issue.go

@@ -16,6 +16,7 @@ import (
 	system_model "code.gitea.io/gitea/models/system"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/storage"
 	notify_service "code.gitea.io/gitea/services/notify"
 )
@@ -70,25 +71,19 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode
 		return err
 	}
 
-	var reviewNotifers []*ReviewRequestNotifier
-
-	if err := db.WithTx(ctx, func(ctx context.Context) error {
-		if err := issues_model.ChangeIssueTitle(ctx, issue, doer, oldTitle); err != nil {
-			return err
-		}
-
-		if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) {
-			var err error
-			reviewNotifers, err = PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest)
-			if err != nil {
-				return err
-			}
-		}
-		return nil
-	}); err != nil {
+	if err := issues_model.ChangeIssueTitle(ctx, issue, doer, oldTitle); err != nil {
 		return err
 	}
 
+	var reviewNotifers []*ReviewRequestNotifier
+	if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) {
+		var err error
+		reviewNotifers, err = PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest)
+		if err != nil {
+			log.Error("PullRequestCodeOwnersReview: %v", err)
+		}
+	}
+
 	notify_service.IssueChangeTitle(ctx, doer, issue, oldTitle)
 	ReviewRequestNotify(ctx, issue, issue.Poster, reviewNotifers)
 

services/issue/pull.go

@@ -39,7 +39,7 @@ type ReviewRequestNotifier struct {
 	ReviewTeam *org_model.Team
 }
 
-func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) {
+func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) {
 	files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}
 
 	if pr.IsWorkInProgress(ctx) {
@@ -89,7 +89,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue,
 
 	// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
 	// between the merge base and the head commit but not the base branch and the head commit
-	changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.HeadCommitID)
+	changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitRefName())
 	if err != nil {
 		return nil, err
 	}
@@ -111,9 +111,13 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue,
 
 	notifiers := make([]*ReviewRequestNotifier, 0, len(uniqUsers)+len(uniqTeams))
 
+	if err := issue.LoadPoster(ctx); err != nil {
+		return nil, err
+	}
+
 	for _, u := range uniqUsers {
-		if u.ID != pull.Poster.ID {
-			comment, err := issues_model.AddReviewRequest(ctx, pull, u, pull.Poster)
+		if u.ID != issue.Poster.ID {
+			comment, err := issues_model.AddReviewRequest(ctx, issue, u, issue.Poster)
 			if err != nil {
 				log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
 				return nil, err
@@ -121,12 +125,12 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue,
 			notifiers = append(notifiers, &ReviewRequestNotifier{
 				Comment: comment,
 				IsAdd:   true,
-				Reviwer: pull.Poster,
+				Reviwer: u,
 			})
 		}
 	}
 	for _, t := range uniqTeams {
-		comment, err := issues_model.AddTeamReviewRequest(ctx, pull, t, pull.Poster)
+		comment, err := issues_model.AddTeamReviewRequest(ctx, issue, t, issue.Poster)
 		if err != nil {
 			log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
 			return nil, err

templates/admin/config.tmpl

@@ -151,8 +151,6 @@
 				<dd>{{if .Service.RequireSignInView}}{{svg "octicon-check"}}{{else}}{{svg "octicon-x"}}{{end}}</dd>
 				<dt>{{ctx.Locale.Tr "admin.config.mail_notify"}}</dt>
 				<dd>{{if .Service.EnableNotifyMail}}{{svg "octicon-check"}}{{else}}{{svg "octicon-x"}}{{end}}</dd>
-				<dt>{{ctx.Locale.Tr "admin.config.disable_key_size_check"}}</dt>
-				<dd>{{if .SSH.MinimumKeySizeCheck}}{{svg "octicon-check"}}{{else}}{{svg "octicon-x"}}{{end}}</dd>
 				<dt>{{ctx.Locale.Tr "admin.config.enable_captcha"}}</dt>
 				<dd>{{if .Service.EnableCaptcha}}{{svg "octicon-check"}}{{else}}{{svg "octicon-x"}}{{end}}</dd>
 				<dt>{{ctx.Locale.Tr "admin.config.default_keep_email_private"}}</dt>

templates/repo/issue/view_content/pull.tmpl

@@ -360,7 +360,7 @@
 			{{if and $.StillCanManualMerge (not $showGeneralMergeForm)}}
 				<div class="divider"></div>
 				<div class="ui form">
-					<form action="{{.Link}}/merge" method="post">
+					<form class="form-fetch-action" action="{{.Link}}/merge" method="post">
 						{{.CsrfTokenHtml}}
 						<div class="field">
 							<input type="text" name="merge_commit_id" placeholder="{{ctx.Locale.Tr "repo.pulls.merge_commit_id"}}">

templates/status/404.tmpl

@@ -1,14 +1,10 @@
 {{template "base/head" .}}
-<div role="main" aria-label="{{.Title}}" class="page-content ui container center gt-w-screen {{if .IsRepo}}repository{{end}}">
+<div role="main" aria-label="{{.Title}}" class="page-content {{if .IsRepo}}repository{{end}}">
 	{{if .IsRepo}}{{template "repo/header" .}}{{end}}
 	<div class="ui container center">
-		<p style="margin-top: 100px"><img src="{{AssetUrlPrefix}}/img/404.png" alt="404"></p>
+		<img style="margin: 50px 0; max-width: 80vw" src="{{AssetUrlPrefix}}/img/404.png" alt="404">
 		<p>{{if .NotFoundPrompt}}{{.NotFoundPrompt}}{{else}}{{ctx.Locale.Tr "error404" | Safe}}{{end}}</p>
 		{{if .NotFoundGoBackURL}}<a class="ui button green" href="{{.NotFoundGoBackURL}}">{{ctx.Locale.Tr "go_back"}}</a>{{end}}
-
-		<div class="divider"></div>
-		<br>
-		{{if .ShowFooterVersion}}<p>{{ctx.Locale.Tr "admin.config.app_ver"}}: {{AppVer}}</p>{{end}}
 	</div>
 </div>
 {{template "base/footer" .}}

tests/integration/pull_review_test.go

@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/test"
+	issue_service "code.gitea.io/gitea/services/issue"
 	repo_service "code.gitea.io/gitea/services/repository"
 	files_service "code.gitea.io/gitea/services/repository/files"
 	"code.gitea.io/gitea/tests"
@@ -85,8 +86,21 @@ func TestPullView_CodeOwner(t *testing.T) {
 			session := loginUser(t, "user2")
 			createPullRequest(t, session, "user2", "test_codeowner", repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request")
 
-			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"})
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadRepoID: repo.ID, HeadBranch: "codeowner-basebranch"})
 			unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
+			assert.NoError(t, pr.LoadIssue(db.DefaultContext))
+
+			err := issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request")
+			assert.NoError(t, err)
+			prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
+			assert.NoError(t, prUpdated1.LoadIssue(db.DefaultContext))
+			assert.EqualValues(t, "[WIP] Test Pull Request", prUpdated1.Issue.Title)
+
+			err = issue_service.ChangeTitle(db.DefaultContext, prUpdated1.Issue, user2, "Test Pull Request2")
+			assert.NoError(t, err)
+			prUpdated2 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
+			assert.NoError(t, prUpdated2.LoadIssue(db.DefaultContext))
+			assert.EqualValues(t, "Test Pull Request2", prUpdated2.Issue.Title)
 		})
 
 		// change the default branch CODEOWNERS file to change README.md's codeowner

web_src/js/markup/anchors.js

@@ -1,29 +1,30 @@
 import {svg} from '../svg.js';
 
+const addPrefix = (str) => `user-content-${str}`;
+const removePrefix = (str) => str.replace(/^user-content-/, '');
+const hasPrefix = (str) => str.startsWith('user-content-');
+
 // scroll to anchor while respecting the `user-content` prefix that exists on the target
-function scrollToAnchor(encodedId, initial) {
-  // abort if the browser has already scrolled to another anchor during page load
-  if (!encodedId || (initial && document.querySelector(':target'))) return;
+function scrollToAnchor(encodedId) {
+  if (!encodedId) return;
   const id = decodeURIComponent(encodedId);
-  let el = document.getElementById(`user-content-${id}`);
+  const prefixedId = addPrefix(id);
+  let el = document.getElementById(prefixedId);
 
   // check for matching user-generated `a[name]`
   if (!el) {
-    const nameAnchors = document.getElementsByName(`user-content-${id}`);
+    const nameAnchors = document.getElementsByName(prefixedId);
     if (nameAnchors.length) {
       el = nameAnchors[0];
     }
   }
 
   // compat for links with old 'user-content-' prefixed hashes
-  if (!el && id.startsWith('user-content-')) {
-    const el = document.getElementById(id);
-    if (el) el.scrollIntoView();
+  if (!el && hasPrefix(id)) {
+    return document.getElementById(id)?.scrollIntoView();
   }
 
-  if (el) {
-    el.scrollIntoView();
-  }
+  el?.scrollIntoView();
 }
 
 export function initMarkupAnchors() {
@@ -32,11 +33,10 @@ export function initMarkupAnchors() {
 
   for (const markupEl of markupEls) {
     // create link icons for markup headings, the resulting link href will remove `user-content-`
-    for (const heading of markupEl.querySelectorAll(`:is(h1, h2, h3, h4, h5, h6`)) {
-      const originalId = heading.id.replace(/^user-content-/, '');
+    for (const heading of markupEl.querySelectorAll('h1, h2, h3, h4, h5, h6')) {
       const a = document.createElement('a');
       a.classList.add('anchor');
-      a.setAttribute('href', `#${encodeURIComponent(originalId)}`);
+      a.setAttribute('href', `#${encodeURIComponent(removePrefix(heading.id))}`);
       a.innerHTML = svg('octicon-link');
       heading.prepend(a);
     }
@@ -45,8 +45,7 @@ export function initMarkupAnchors() {
     for (const a of markupEl.querySelectorAll('a[href^="#"]')) {
       const href = a.getAttribute('href');
       if (!href.startsWith('#user-content-')) continue;
-      const originalId = href.replace(/^#user-content-/, '');
-      a.setAttribute('href', `#${originalId}`);
+      a.setAttribute('href', `#${removePrefix(href.substring(1))}`);
     }
 
     // add `user-content-` prefix to user-generated `a[name]` link targets
@@ -54,15 +53,18 @@ export function initMarkupAnchors() {
     for (const a of markupEl.querySelectorAll('a[name]')) {
       const name = a.getAttribute('name');
       if (!name) continue;
-      a.setAttribute('name', `user-content-${a.name}`);
+      a.setAttribute('name', addPrefix(a.name));
     }
 
     for (const a of markupEl.querySelectorAll('a[href^="#"]')) {
       a.addEventListener('click', (e) => {
-        scrollToAnchor(e.currentTarget.getAttribute('href')?.substring(1), false);
+        scrollToAnchor(e.currentTarget.getAttribute('href')?.substring(1));
       });
     }
   }
 
-  scrollToAnchor(window.location.hash.substring(1), true);
+  // scroll to anchor unless the browser has already scrolled somewhere during page load
+  if (!document.querySelector(':target')) {
+    scrollToAnchor(window.location.hash?.substring(1));
+  }
 }