Fix Actions pull_request.paths being triggered incorrectly by rebase (#36045) (#36054)

Backport #36045

Partially fix #34710

The bug described in #34710 can be divided into two parts: `push.paths`
and `pull_request.paths`. This PR fixes the issue related to
`pull_request.paths`. The root cause is that the check for whether the
workflow can be triggered happens **before** updating the PR’s merge
base. This causes the file-change detection to use the old merge base.
Therefore, we need to update the merge base first and then check whether
the workflow can be triggered.

models/pull/review_state.go

@@ -73,18 +73,18 @@ func GetReviewState(ctx context.Context, userID, pullID int64, commitSHA string)
 
 // UpdateReviewState updates the given review inside the database, regardless of whether it existed before or not
 // The given map of files with their viewed state will be merged with the previous review, if present
-func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) error {
+func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) (*ReviewState, error) {
 	log.Trace("Updating review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, updatedFiles)
 
 	review, exists, err := GetReviewState(ctx, userID, pullID, commitSHA)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	if exists {
 		review.UpdatedFiles = mergeFiles(review.UpdatedFiles, updatedFiles)
 	} else if previousReview, err := getNewestReviewStateApartFrom(ctx, userID, pullID, commitSHA); err != nil {
-		return err
+		return nil, err
 
 		// Overwrite the viewed files of the previous review if present
 	} else if previousReview != nil {
@@ -98,11 +98,11 @@ func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA stri
 	if !exists {
 		log.Trace("Inserting new review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, review.UpdatedFiles)
 		_, err := engine.Insert(review)
-		return err
+		return nil, err
 	}
 	log.Trace("Updating already existing review with ID %d (user %d, repo %d, commit %s) with the updated files %v.", review.ID, userID, pullID, commitSHA, review.UpdatedFiles)
-	_, err = engine.ID(review.ID).Update(&ReviewState{UpdatedFiles: review.UpdatedFiles})
-	return err
+	_, err = engine.ID(review.ID).Cols("updated_files").Update(review)
+	return review, err
 }
 
 // mergeFiles merges the given maps of files with their viewing state into one map.

routers/api/packages/container/container.go

@@ -290,8 +290,8 @@ func PostBlobsUploads(ctx *context.Context) {
 				Creator: ctx.Doer,
 			},
 		); err != nil {
-			switch err {
-			case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize:
+			switch {
+			case errors.Is(err, packages_service.ErrQuotaTotalCount), errors.Is(err, packages_service.ErrQuotaTypeSize), errors.Is(err, packages_service.ErrQuotaTotalSize):
 				apiError(ctx, http.StatusForbidden, err)
 			default:
 				apiError(ctx, http.StatusInternalServerError, err)
@@ -439,8 +439,8 @@ func PutBlobsUpload(ctx *context.Context) {
 			Creator: ctx.Doer,
 		},
 	); err != nil {
-		switch err {
-		case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize:
+		switch {
+		case errors.Is(err, packages_service.ErrQuotaTotalCount), errors.Is(err, packages_service.ErrQuotaTypeSize), errors.Is(err, packages_service.ErrQuotaTotalSize):
 			apiError(ctx, http.StatusForbidden, err)
 		default:
 			apiError(ctx, http.StatusInternalServerError, err)
@@ -592,13 +592,10 @@ func PutManifest(ctx *context.Context) {
 			apiErrorDefined(ctx, namedError)
 		} else if errors.Is(err, container_model.ErrContainerBlobNotExist) {
 			apiErrorDefined(ctx, errBlobUnknown)
+		} else if errors.Is(err, packages_service.ErrQuotaTotalCount) || errors.Is(err, packages_service.ErrQuotaTypeSize) || errors.Is(err, packages_service.ErrQuotaTotalSize) {
+			apiError(ctx, http.StatusForbidden, err)
 		} else {
-			switch err {
-			case packages_service.ErrQuotaTotalCount, packages_service.ErrQuotaTypeSize, packages_service.ErrQuotaTotalSize:
-				apiError(ctx, http.StatusForbidden, err)
-			default:
-				apiError(ctx, http.StatusInternalServerError, err)
-			}
+			apiError(ctx, http.StatusInternalServerError, err)
 		}
 		return
 	}

routers/api/packages/container/manifest.go

@@ -82,9 +82,11 @@ type processManifestTxRet struct {
 }
 
 func handleCreateManifestResult(ctx context.Context, err error, mci *manifestCreationInfo, contentStore *packages_module.ContentStore, txRet *processManifestTxRet) (string, error) {
-	if err != nil && txRet.created && txRet.pb != nil {
-		if err := contentStore.Delete(packages_module.BlobHash256Key(txRet.pb.HashSHA256)); err != nil {
-			log.Error("Error deleting package blob from content store: %v", err)
+	if err != nil {
+		if txRet.created && txRet.pb != nil {
+			if err := contentStore.Delete(packages_module.BlobHash256Key(txRet.pb.HashSHA256)); err != nil {
+				log.Error("Error deleting package blob from content store: %v", err)
+			}
 		}
 		return "", err
 	}
@@ -198,14 +200,14 @@ func processOciImageIndex(ctx context.Context, mci *manifestCreationInfo, buf *p
 				if errors.Is(err, container_model.ErrContainerBlobNotExist) {
 					return errManifestBlobUnknown
 				}
-				return err
+				return fmt.Errorf("GetContainerBlob: %w", err)
 			}
 
 			size, err := packages_model.CalculateFileSize(ctx, &packages_model.PackageFileSearchOptions{
 				VersionID: pfd.File.VersionID,
 			})
 			if err != nil {
-				return err
+				return fmt.Errorf("CalculateFileSize: %w", err)
 			}
 
 			metadata.Manifests = append(metadata.Manifests, &container_module.Manifest{
@@ -217,7 +219,7 @@ func processOciImageIndex(ctx context.Context, mci *manifestCreationInfo, buf *p
 
 		pv, err := createPackageAndVersion(ctx, mci, metadata)
 		if err != nil {
-			return err
+			return fmt.Errorf("createPackageAndVersion: %w", err)
 		}
 
 		txRet.pv = pv
@@ -240,7 +242,7 @@ func createPackageAndVersion(ctx context.Context, mci *manifestCreationInfo, met
 	if p, err = packages_model.TryInsertPackage(ctx, p); err != nil {
 		if !errors.Is(err, packages_model.ErrDuplicatePackage) {
 			log.Error("Error inserting package: %v", err)
-			return nil, err
+			return nil, fmt.Errorf("TryInsertPackage: %w", err)
 		}
 		created = false
 	}
@@ -248,7 +250,7 @@ func createPackageAndVersion(ctx context.Context, mci *manifestCreationInfo, met
 	if created {
 		if _, err := packages_model.InsertProperty(ctx, packages_model.PropertyTypePackage, p.ID, container_module.PropertyRepository, strings.ToLower(mci.Owner.LowerName+"/"+mci.Image)); err != nil {
 			log.Error("Error setting package property: %v", err)
-			return nil, err
+			return nil, fmt.Errorf("InsertProperty(PropertyRepository): %w", err)
 		}
 	}
 
@@ -256,7 +258,7 @@ func createPackageAndVersion(ctx context.Context, mci *manifestCreationInfo, met
 
 	metadataJSON, err := json.Marshal(metadata)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("json.Marshal(metadata): %w", err)
 	}
 
 	// "docker buildx imagetools create" multi-arch operations:
@@ -276,43 +278,43 @@ func createPackageAndVersion(ctx context.Context, mci *manifestCreationInfo, met
 	pv, err := packages_model.GetOrInsertVersion(ctx, _pv)
 	if err != nil {
 		if !errors.Is(err, packages_model.ErrDuplicatePackageVersion) {
-			log.Error("Error inserting package: %v", err)
-			return nil, err
+			log.Error("Error GetOrInsertVersion (first try) package: %v", err)
+			return nil, fmt.Errorf("GetOrInsertVersion: first try: %w", err)
 		}
 		if err = packages_service.DeletePackageVersionAndReferences(ctx, pv); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("DeletePackageVersionAndReferences: %w", err)
 		}
 		// keep download count on overwriting
 		_pv.DownloadCount = pv.DownloadCount
 		pv, err = packages_model.GetOrInsertVersion(ctx, _pv)
 		if err != nil {
 			if !errors.Is(err, packages_model.ErrDuplicatePackageVersion) {
-				log.Error("Error inserting package: %v", err)
-				return nil, err
+				log.Error("Error GetOrInsertVersion (second try) package: %v", err)
+				return nil, fmt.Errorf("GetOrInsertVersion: second try: %w", err)
 			}
 		}
 	}
 
 	if err := packages_service.CheckCountQuotaExceeded(ctx, mci.Creator, mci.Owner); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("CheckCountQuotaExceeded: %w", err)
 	}
 
 	if mci.IsTagged {
 		if err = packages_model.InsertOrUpdateProperty(ctx, packages_model.PropertyTypeVersion, pv.ID, container_module.PropertyManifestTagged, ""); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("InsertOrUpdateProperty(ManifestTagged): %w", err)
 		}
 	} else {
 		if err = packages_model.DeletePropertiesByName(ctx, packages_model.PropertyTypeVersion, pv.ID, container_module.PropertyManifestTagged); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("DeletePropertiesByName(ManifestTagged): %w", err)
 		}
 	}
 
 	if err = packages_model.DeletePropertiesByName(ctx, packages_model.PropertyTypeVersion, pv.ID, container_module.PropertyManifestReference); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("DeletePropertiesByName(ManifestReference): %w", err)
 	}
 	for _, manifest := range metadata.Manifests {
 		if _, err = packages_model.InsertProperty(ctx, packages_model.PropertyTypeVersion, pv.ID, container_module.PropertyManifestReference, manifest.Digest); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("InsertProperty(ManifestReference): %w", err)
 		}
 	}
 

routers/web/repo/pull_review.go

@@ -331,7 +331,7 @@ func UpdateViewedFiles(ctx *context.Context) {
 		updatedFiles[file] = state
 	}
 
-	if err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil {
+	if _, err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil {
 		ctx.ServerError("UpdateReview", err)
 	}
 }

services/gitdiff/gitdiff.go

@@ -1344,15 +1344,17 @@ outer:
 		}
 	}
 
-	// Explicitly store files that have changed in the database, if any is present at all.
-	// This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed.
-	// On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed.
 	if len(filesChangedSinceLastDiff) > 0 {
-		err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff)
+		// Explicitly store files that have changed in the database, if any is present at all.
+		// This has the benefit that the "Has Changed" attribute will be present as long as the user does not explicitly mark this file as viewed, so it will even survive a page reload after marking another file as viewed.
+		// On the other hand, this means that even if a commit reverting an unseen change is committed, the file will still be seen as changed.
+		updatedReview, err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff)
 		if err != nil {
 			log.Warn("Could not update review for user %d, pull %d, commit %s and the changed files %v: %v", review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff, err)
 			return nil, err
 		}
+		// Update the local review to reflect the changes immediately
+		review = updatedReview
 	}
 
 	return review, nil

services/mailer/incoming/incoming.go

@@ -6,6 +6,7 @@ package incoming
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"fmt"
 	net_mail "net/mail"
 	"regexp"
@@ -221,7 +222,7 @@ loop:
 			err := func() error {
 				r := msg.GetBody(section)
 				if r == nil {
-					return fmt.Errorf("could not get body from message: %w", err)
+					return errors.New("could not get body from message")
 				}
 
 				env, err := enmime.ReadEnvelope(r)

services/pull/pull.go

@@ -425,10 +425,16 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) {
 				for _, pr := range prs {
 					objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName)
 					if opts.NewCommitID != "" && opts.NewCommitID != objectFormat.EmptyObjectID().String() {
-						changed, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID)
+						changed, newMergeBase, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID)
 						if err != nil {
 							log.Error("checkIfPRContentChanged: %v", err)
 						}
+						if newMergeBase != "" && pr.MergeBase != newMergeBase {
+							pr.MergeBase = newMergeBase
+							if err := pr.UpdateColsIfNotMerged(ctx, "merge_base"); err != nil {
+								log.Error("Update merge base for %-v: %v", pr, err)
+							}
+						}
 						if changed {
 							// Mark old reviews as stale if diff to mergebase has changed
 							if err := issues_model.MarkReviewsAsStale(ctx, pr.IssueID); err != nil {
@@ -502,30 +508,30 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) {
 
 // checkIfPRContentChanged checks if diff to target branch has changed by push
 // A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged
-func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) {
+func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, mergeBase string, err error) {
 	prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
 	if err != nil {
 		log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err)
-		return false, err
+		return false, "", err
 	}
 	defer cancel()
 
 	tmpRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath)
 	if err != nil {
-		return false, fmt.Errorf("OpenRepository: %w", err)
+		return false, "", fmt.Errorf("OpenRepository: %w", err)
 	}
 	defer tmpRepo.Close()
 
 	// Find the merge-base
-	_, base, err := tmpRepo.GetMergeBase("", "base", "tracking")
+	mergeBase, _, err = tmpRepo.GetMergeBase("", "base", "tracking")
 	if err != nil {
-		return false, fmt.Errorf("GetMergeBase: %w", err)
+		return false, "", fmt.Errorf("GetMergeBase: %w", err)
 	}
 
-	cmd := gitcmd.NewCommand("diff", "--name-only", "-z").AddDynamicArguments(newCommitID, oldCommitID, base)
+	cmd := gitcmd.NewCommand("diff", "--name-only", "-z").AddDynamicArguments(newCommitID, oldCommitID, mergeBase)
 	stdoutReader, stdoutWriter, err := os.Pipe()
 	if err != nil {
-		return false, fmt.Errorf("unable to open pipe for to run diff: %w", err)
+		return false, mergeBase, fmt.Errorf("unable to open pipe for to run diff: %w", err)
 	}
 
 	stderr := new(bytes.Buffer)
@@ -542,19 +548,19 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
 		},
 	}); err != nil {
 		if err == util.ErrNotEmpty {
-			return true, nil
+			return true, mergeBase, nil
 		}
 		err = gitcmd.ConcatenateError(err, stderr.String())
 
 		log.Error("Unable to run diff on %s %s %s in tempRepo for PR[%d]%s/%s...%s/%s: Error: %v",
-			newCommitID, oldCommitID, base,
+			newCommitID, oldCommitID, mergeBase,
 			pr.ID, pr.BaseRepo.FullName(), pr.BaseBranch, pr.HeadRepo.FullName(), pr.HeadBranch,
 			err)
 
-		return false, fmt.Errorf("Unable to run git diff --name-only -z %s %s %s: %w", newCommitID, oldCommitID, base, err)
+		return false, mergeBase, fmt.Errorf("Unable to run git diff --name-only -z %s %s %s: %w", newCommitID, oldCommitID, mergeBase, err)
 	}
 
-	return false, nil
+	return false, mergeBase, nil
 }
 
 // PushToBaseRepo pushes commits from branches of head repository to

services/pull/review.go

@@ -309,7 +309,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos
 		if headCommitID == commitID {
 			stale = false
 		} else {
-			stale, err = checkIfPRContentChanged(ctx, pr, commitID, headCommitID)
+			stale, _, err = checkIfPRContentChanged(ctx, pr, commitID, headCommitID)
 			if err != nil {
 				return nil, nil, err
 			}

services/wiki/wiki.go

@@ -137,7 +137,7 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model
 	if hasDefaultBranch {
 		if err := gitRepo.ReadTreeToIndex("HEAD"); err != nil {
 			log.Error("Unable to read HEAD tree to index in: %s %v", basePath, err)
-			return fmt.Errorf("fnable to read HEAD tree to index in: %s %w", basePath, err)
+			return fmt.Errorf("unable to read HEAD tree to index in: %s %w", basePath, err)
 		}
 	}
 

templates/base/head_navbar.tmpl

@@ -120,6 +120,13 @@
 						{{svg "octicon-question"}}
 						{{ctx.Locale.Tr "help"}}
 					</a>
+					{{if .IsAdmin}}
+					<div class="divider"></div>
+					<a class="{{if .PageIsAdmin}}active {{end}}item" href="{{AppSubUrl}}/-/admin">
+						{{svg "octicon-server"}}
+						{{ctx.Locale.Tr "admin_panel"}}
+					</a>
+					{{end}}
 					<div class="divider"></div>
 					<a class="item link-action" href data-url="{{AppSubUrl}}/user/logout">
 						{{svg "octicon-sign-out"}}

tests/integration/actions_trigger_test.go

@@ -31,6 +31,7 @@ import (
 	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
+	webhook_module "code.gitea.io/gitea/modules/webhook"
 	issue_service "code.gitea.io/gitea/services/issue"
 	pull_service "code.gitea.io/gitea/services/pull"
 	release_service "code.gitea.io/gitea/services/release"
@@ -1604,3 +1605,56 @@ jobs:
 		assert.NotNil(t, run)
 	})
 }
+
+func TestPullRequestWithPathsRebase(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		session := loginUser(t, user2.Name)
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
+
+		repoName := "actions-pr-paths-rebase"
+		apiRepo := createActionsTestRepo(t, token, repoName, false)
+		repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: apiRepo.ID})
+		apiCtx := NewAPITestContext(t, "user2", repoName, auth_model.AccessTokenScopeWriteRepository)
+		runner := newMockRunner()
+		runner.registerAsRepoRunner(t, "user2", repoName, "mock-runner", []string{"ubuntu-latest"}, false)
+
+		// init files and dirs
+		testCreateFile(t, session, "user2", repoName, repo.DefaultBranch, "", "dir1/dir1.txt", "1")
+		testCreateFile(t, session, "user2", repoName, repo.DefaultBranch, "", "dir2/dir2.txt", "2")
+		wfFileContent := `name: ci
+on: 
+  pull_request:
+    paths:
+      - 'dir1/**'
+jobs:
+  ci-job:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'ci'
+`
+		testCreateFile(t, session, "user2", repoName, repo.DefaultBranch, "", ".gitea/workflows/ci.yml", wfFileContent)
+
+		// create a PR to modify "dir1/dir1.txt", the workflow will be triggered
+		testEditFileToNewBranch(t, session, "user2", repoName, repo.DefaultBranch, "update-dir1", "dir1/dir1.txt", "11")
+		_, err := doAPICreatePullRequest(apiCtx, "user2", repoName, repo.DefaultBranch, "update-dir1")(t)
+		assert.NoError(t, err)
+		pr1Task := runner.fetchTask(t)
+		_, _, pr1Run := getTaskAndJobAndRunByTaskID(t, pr1Task.Id)
+		assert.Equal(t, webhook_module.HookEventPullRequest, pr1Run.Event)
+
+		// create a PR to modify "dir2/dir2.txt" then update main branch and rebase, the workflow will not be triggered
+		testEditFileToNewBranch(t, session, "user2", repoName, repo.DefaultBranch, "update-dir2", "dir2/dir2.txt", "22")
+		apiPull, err := doAPICreatePullRequest(apiCtx, "user2", repoName, repo.DefaultBranch, "update-dir2")(t)
+		runner.fetchNoTask(t)
+		assert.NoError(t, err)
+		testEditFile(t, session, "user2", repoName, repo.DefaultBranch, "dir1/dir1.txt", "11") // change the file in "dir1"
+		req := NewRequestWithValues(t, "POST",
+			fmt.Sprintf("/%s/%s/pulls/%d/update?style=rebase", "user2", repoName, apiPull.Index), // update by rebase
+			map[string]string{
+				"_csrf": GetUserCSRFToken(t, session),
+			})
+		session.MakeRequest(t, req, http.StatusSeeOther)
+		runner.fetchNoTask(t)
+	})
+}