diff --git a/.deadcode-out b/.deadcode-out index f79416ebf3..9ff8db8c17 100644 --- a/.deadcode-out +++ b/.deadcode-out @@ -108,6 +108,7 @@ forgejo.org/modules/git AddChangesWithArgs CommitChanges CommitChangesWithArgs + IsErrMoreThanOne SetUpdateHook openRepositoryWithDefaultContext ToEntryMode diff --git a/cmd/hook.go b/cmd/hook.go index 7378dc21ad..aabf08851b 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -295,35 +295,7 @@ Forgejo or set your environment appropriately.`, "") // runHookUpdate process the update hook: https://git-scm.com/docs/githooks#update func runHookUpdate(ctx context.Context, c *cli.Command) error { - // Now if we're an internal don't do anything else - if isInternal, _ := strconv.ParseBool(os.Getenv(repo_module.EnvIsInternal)); isInternal { - return nil - } - - ctx, cancel := installSignals(ctx) - defer cancel() - - if c.NArg() != 3 { - return nil - } - args := c.Args().Slice() - - // The arguments given to the hook are in order: reference name, old commit ID and new commit ID. - refFullName := git.RefName(args[0]) - newCommitID := args[2] - - // Only process pull references. - if !refFullName.IsPull() { - return nil - } - - // Empty new commit ID means deletion. - if git.IsEmptyCommitID(newCommitID, nil) { - return fail(ctx, fmt.Sprintf("The deletion of %s is skipped as it's an internal reference.", refFullName), "") - } - - // If the new comment isn't empty it means modification. - return fail(ctx, fmt.Sprintf("The modification of %s is skipped as it's an internal reference.", refFullName), "") + return nil } func runHookPostReceive(ctx context.Context, c *cli.Command) error { diff --git a/cmd/hook_test.go b/cmd/hook_test.go index 82ed392fb8..19e10455cb 100644 --- a/cmd/hook_test.go +++ b/cmd/hook_test.go @@ -161,42 +161,3 @@ func TestDelayWriter(t *testing.T) { require.Empty(t, out) }) } - -func TestRunHookUpdate(t *testing.T) { - app := cli.Command{} - app.Commands = []*cli.Command{subcmdHookUpdate()} - - t.Run("Removal of internal reference", func(t *testing.T) { - defer test.MockVariableValue(&cli.OsExiter, func(code int) {})() - defer test.MockVariableValue(&setting.IsProd, false)() - finish := captureOutput(t, os.Stderr) - - err := app.Run(t.Context(), []string{"./forgejo", "update", "refs/pull/1/head", "0a51ae26bc73c47e2f754560c40904cf14ed51a9", "0000000000000000000000000000000000000000"}) - out := finish() - require.Error(t, err) - - assert.Contains(t, out, "The deletion of refs/pull/1/head is skipped as it's an internal reference.") - }) - - t.Run("Update of internal reference", func(t *testing.T) { - defer test.MockVariableValue(&cli.OsExiter, func(code int) {})() - defer test.MockVariableValue(&setting.IsProd, false)() - finish := captureOutput(t, os.Stderr) - - err := app.Run(t.Context(), []string{"./forgejo", "update", "refs/pull/1/head", "0a51ae26bc73c47e2f754560c40904cf14ed51a9", "0000000000000000000000000000000000000001"}) - out := finish() - require.Error(t, err) - - assert.Contains(t, out, "The modification of refs/pull/1/head is skipped as it's an internal reference.") - }) - - t.Run("Removal of branch", func(t *testing.T) { - err := app.Run(t.Context(), []string{"./forgejo", "update", "refs/head/main", "0a51ae26bc73c47e2f754560c40904cf14ed51a9", "0000000000000000000000000000000000000000"}) - require.NoError(t, err) - }) - - t.Run("Not enough arguments", func(t *testing.T) { - err := app.Run(t.Context(), []string{"./forgejo", "update"}) - require.NoError(t, err) - }) -} diff --git a/modules/git/fetch.go b/modules/git/fetch.go new file mode 100644 index 0000000000..886ca9ca60 --- /dev/null +++ b/modules/git/fetch.go @@ -0,0 +1,51 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later +package git + +import ( + "errors" + "fmt" + "strings" +) + +var ErrRemoteRefNotFound = errors.New("unable to find remote ref") + +// Fetch executes git-fetch(1) for the repository, it will fetch the refspec +// from the remote into the repository. +// +// Valid remote URLs are denoted in section GIT URLS of git-fetch(1). +// +// The return value, on success, is the object ID of the remote reference. If +// no local reference is given in `refspec` then do not assume it's available in +// the `FETCH_HEAD` reference, it might have been overwritten by the time you +// read or use it. +func (repo *Repository) Fetch(remoteURL, refspec string) (string, error) { + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return "", err + } + + stdout, stderr, err := NewCommand(repo.Ctx, "fetch", "--porcelain", "--end-of-options").AddDynamicArguments(remoteURL, refspec).RunStdString(&RunOpts{Dir: repo.Path}) + if err != nil { + if strings.HasPrefix(stderr, "fatal: couldn't find remote ref ") { + return "", ErrRemoteRefNotFound + } + return "", err + } + + localReference, _, ok := strings.Cut(refspec, ":") + if !ok { + localReference = "FETCH_HEAD" + } + + // The porcelain format, per section OUTPUT of git-fetch(1), is expected to be: + // \n + // flag is one character. + if expectedLen := 1 + 1 + objectFormat.FullLength() + 1 + objectFormat.FullLength() + 1 + len(localReference) + 1; len(stdout) != expectedLen { + return "", fmt.Errorf("output of git-fetch(1) is unexpected, we expected it to be %d bytes but it is %d bytes. stdout: %s", expectedLen, len(stdout), stdout) + } + + // Extract the new objectID. + newObjectID := stdout[1+1+objectFormat.FullLength()+1 : 1+1+objectFormat.FullLength()+1+objectFormat.FullLength()] + return newObjectID, nil +} diff --git a/modules/git/fetch_test.go b/modules/git/fetch_test.go new file mode 100644 index 0000000000..95a1fa387d --- /dev/null +++ b/modules/git/fetch_test.go @@ -0,0 +1,105 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later +package git + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFetch(t *testing.T) { + t.Run("SHA1", func(t *testing.T) { + dstDir := t.TempDir() + + require.NoError(t, Clone(t.Context(), filepath.Join(testReposDir, "repo1_bare"), dstDir, CloneRepoOptions{})) + + repo, err := OpenRepository(t.Context(), dstDir) + require.NoError(t, err) + defer repo.Close() + + t.Run("Reference", func(t *testing.T) { + otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "language_stats_repo")) + require.NoError(t, err) + + fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/master") + require.NoError(t, err) + assert.Equal(t, "95d3505f2db273e40be79f84416051ae85e9ea0d", fetchedCommitID) + + c, err := repo.getCommit(MustIDFromString(fetchedCommitID)) + require.NoError(t, err) + assert.NotNil(t, c) + }) + + t.Run("CommitID", func(t *testing.T) { + otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame")) + require.NoError(t, err) + + fetchedCommitID, err := repo.Fetch(otherRepoPath, "45fb6cbc12f970b04eacd5cd4165edd11c8d7376") + require.NoError(t, err) + assert.Equal(t, "45fb6cbc12f970b04eacd5cd4165edd11c8d7376", fetchedCommitID) + + c, err := repo.getCommit(MustIDFromString(fetchedCommitID)) + require.NoError(t, err) + assert.NotNil(t, c) + }) + + t.Run("Invalid reference", func(t *testing.T) { + otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame")) + require.NoError(t, err) + + fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/does-not-exist") + require.ErrorIs(t, err, ErrRemoteRefNotFound) + assert.Empty(t, fetchedCommitID) + }) + }) + + t.Run("SHA256", func(t *testing.T) { + skipIfSHA256NotSupported(t) + + dstDir := t.TempDir() + + require.NoError(t, Clone(t.Context(), filepath.Join(testReposDir, "repo1_bare_sha256"), dstDir, CloneRepoOptions{})) + + repo, err := OpenRepository(t.Context(), dstDir) + require.NoError(t, err) + defer repo.Close() + + t.Run("Reference", func(t *testing.T) { + otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame_sha256")) + require.NoError(t, err) + + fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/main") + require.NoError(t, err) + assert.Equal(t, "e2f5660e15159082902960af0ed74fc144921d2b0c80e069361853b3ece29ba3", fetchedCommitID) + + c, err := repo.getCommit(MustIDFromString(fetchedCommitID)) + require.NoError(t, err) + assert.NotNil(t, c) + }) + + t.Run("CommitID", func(t *testing.T) { + otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_merge_sha256")) + require.NoError(t, err) + + fetchedCommitID, err := repo.Fetch(otherRepoPath, "d2e5609f630dd8db500f5298d05d16def282412e3e66ed68cc7d0833b29129a1") + require.NoError(t, err) + assert.Equal(t, "d2e5609f630dd8db500f5298d05d16def282412e3e66ed68cc7d0833b29129a1", fetchedCommitID) + + c, err := repo.getCommit(MustIDFromString(fetchedCommitID)) + require.NoError(t, err) + assert.NotNil(t, c) + }) + + t.Run("Invalid reference", func(t *testing.T) { + otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame_sha256")) + require.NoError(t, err) + + fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/does-not-exist") + require.ErrorIs(t, err, ErrRemoteRefNotFound) + assert.Empty(t, fetchedCommitID) + }) + }) +} diff --git a/modules/git/git.go b/modules/git/git.go index 851b090b53..e3ffa39514 100644 --- a/modules/git/git.go +++ b/modules/git/git.go @@ -303,6 +303,10 @@ func syncGitConfig() (err error) { } } + if err = configSet("receive.hideRefs", "refs/pull/"); err != nil { + return err + } + if !setting.Git.DisablePartialClone { if err = configSet("uploadpack.allowfilter", "true"); err != nil { return err diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index f445ce4a1a..29f269a511 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -41,7 +41,6 @@ import ( "forgejo.org/modules/markup" "forgejo.org/modules/markup/markdown" "forgejo.org/modules/optional" - repo_module "forgejo.org/modules/repository" "forgejo.org/modules/setting" api "forgejo.org/modules/structs" "forgejo.org/modules/templates" @@ -3167,13 +3166,7 @@ func NewComment(ctx *context.Context) { if prHeadCommitID != headBranchCommitID { // force push to base repo - err := git.Push(ctx, pull.HeadRepo.RepoPath(), git.PushOptions{ - Remote: pull.BaseRepo.RepoPath(), - Branch: pull.HeadBranch + ":" + prHeadRef, - Force: true, - Env: repo_module.InternalPushingEnvironment(pull.Issue.Poster, pull.BaseRepo), - }) - if err != nil { + if err := pull_service.PushToBaseRepo(ctx, pull); err != nil { ctx.ServerError("force push error", err) return } diff --git a/services/pull/comment.go b/services/pull/comment.go index 25542daa9f..0869e0c745 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -79,6 +79,10 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss return nil, err } + if err := pr.LoadIssue(ctx); err != nil { + return nil, err + } + ops.Issue = pr.Issue dataJSON, err := json.Marshal(data) diff --git a/services/pull/pull.go b/services/pull/pull.go index 6a6be319c4..640523db30 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -25,7 +25,6 @@ import ( "forgejo.org/modules/json" "forgejo.org/modules/log" "forgejo.org/modules/process" - repo_module "forgejo.org/modules/repository" "forgejo.org/modules/setting" "forgejo.org/modules/sync" app_context "forgejo.org/services/context" @@ -430,10 +429,6 @@ func ValidatePullRequest(ctx context.Context, pr *issues_model.PullRequest, newC // 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) { log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitRefName()) if err := pr.LoadHeadRepo(ctx); err != nil { @@ -448,44 +443,18 @@ func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, pre } baseRepoPath := pr.BaseRepo.RepoPath() - if err = pr.LoadIssue(ctx); err != nil { - return fmt.Errorf("unable to load issue %d for pr %d: %w", pr.IssueID, pr.ID, err) + baseRepo, err := git.OpenRepository(ctx, baseRepoPath) + if err != nil { + return 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) + defer baseRepo.Close() + + fetchedCommitID, err := baseRepo.Fetch("file://"+headRepoPath, git.BranchPrefix+pr.HeadBranch) + if err != nil { + return err } - gitRefName := pr.GetGitRefName() - - if err := git.Push(ctx, headRepoPath, git.PushOptions{ - Remote: baseRepoPath, - Branch: prefixHeadBranch + pr.HeadBranch + ":" + gitRefName, - 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 git.IsErrPushRejected(err) { - rejectErr := err.(*git.ErrPushRejected) - 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) - } - - return nil + return baseRepo.SetReference(pr.GetGitRefName(), fetchedCommitID) } // UpdateRef update refs/pull/id/head directly for agit flow pull request diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index 1f7097a7dd..8717dc0ad7 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -778,13 +778,11 @@ func doInternalReferences(ctx *APITestContext, dstPath string) func(t *testing.T _, stdErr, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments(fmt.Sprintf(":refs/pull/%d/head", pr1.Index)).RunStdString(&git.RunOpts{Dir: dstPath}) require.Error(t, gitErr) - assert.Contains(t, stdErr, fmt.Sprintf("remote: Forgejo: The deletion of refs/pull/%d/head is skipped as it's an internal reference.", pr1.Index)) - assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] refs/pull/%d/head (hook declined)", pr1.Index)) + assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] refs/pull/%d/head (deny deleting a hidden ref)", pr1.Index)) _, stdErr, gitErr = git.NewCommand(git.DefaultContext, "push", "origin", "--force").AddDynamicArguments(fmt.Sprintf("HEAD~1:refs/pull/%d/head", pr1.Index)).RunStdString(&git.RunOpts{Dir: dstPath}) require.Error(t, gitErr) - assert.Contains(t, stdErr, fmt.Sprintf("remote: Forgejo: The modification of refs/pull/%d/head is skipped as it's an internal reference.", pr1.Index)) - assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] HEAD~1 -> refs/pull/%d/head (hook declined)", pr1.Index)) + assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] HEAD~1 -> refs/pull/%d/head (deny updating a hidden ref)", pr1.Index)) } }