From f0bb2b414bc688a5779789b047afe62f7995f93a Mon Sep 17 00:00:00 2001 From: Osher De Paz Date: Thu, 19 Jun 2025 19:10:50 +0300 Subject: [PATCH] fix: Allow main branch invocation with merge strategy (#5617) Signed-off-by: Osher De Paz Co-authored-by: Marcus Bastian Co-authored-by: PePe Amengual <2208324+jamengual@users.noreply.github.com> --- server/events/vcs/instrumented_client.go | 6 +++ server/events/working_dir.go | 13 ++++--- server/events/working_dir_test.go | 47 ++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/server/events/vcs/instrumented_client.go b/server/events/vcs/instrumented_client.go index d8553c4bc..1fd7b8e43 100644 --- a/server/events/vcs/instrumented_client.go +++ b/server/events/vcs/instrumented_client.go @@ -206,6 +206,12 @@ func (c *InstrumentedClient) PullIsMergeable(logger logging.SimpleLogging, repo } func (c *InstrumentedClient) UpdateStatus(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error { + // If the plan isn't coming from a pull request, + // don't attempt to update the status. + if pull.Num == 0 { + return nil + } + scope := c.StatsScope.SubScope("update_status") scope = SetGitScopeTags(scope, repo.FullName, pull.Num) diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 870eca941..db949a8fa 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -77,9 +77,9 @@ type FileWorkspace struct { // TestingOverrideBaseCloneURL can be used during testing to override the // URL of the base repo to be cloned. If it's empty then we clone normally. TestingOverrideBaseCloneURL string - // GithubAppEnabled is true if we should fetch the ref "pull/PR_NUMBER/head" - // from the "origin" remote. If this is false, we fetch "+refs/heads/$HEAD_BRANCH" - // from the "head" remote. + // GithubAppEnabled is true and a PR number is supplied, we should fetch + // the ref "pull/PR_NUMBER/head" from the "origin" remote. If this is false, + // we fetch "+refs/heads/$HEAD_BRANCH" from the "head" remote. GithubAppEnabled bool // use the global setting without overriding GpgNoSigningEnabled bool @@ -109,12 +109,13 @@ func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo logger.Debug("clone directory '%s' already exists, checking if it's at the right commit", cloneDir) // We use git rev-parse to see if our repo is at the right commit. - // If just checking out the pull request branch, we can use HEAD. + // If just checking out the pull request branch or if there is no + // pull request (API triggered with a custom git ref), we can use HEAD. // If doing a merge, then HEAD won't be at the pull request's HEAD // because we'll already have performed a merge. Instead, we'll check // HEAD^2 since that will be the commit before our merge. pullHead := "HEAD" - if w.CheckoutMerge { + if w.CheckoutMerge && c.pr.Num > 0 { pullHead = "HEAD^2" } revParseCmd := exec.Command("git", "rev-parse", pullHead) // #nosec @@ -351,7 +352,7 @@ func (w *FileWorkspace) wrappedGit(logger logging.SimpleLogging, c wrappedGitCon func (w *FileWorkspace) mergeToBaseBranch(logger logging.SimpleLogging, c wrappedGitContext) error { fetchRef := fmt.Sprintf("+refs/heads/%s:", c.pr.HeadBranch) fetchRemote := "head" - if w.GithubAppEnabled { + if w.GithubAppEnabled && c.pr.Num > 0 { fetchRef = fmt.Sprintf("pull/%d/head:", c.pr.Num) fetchRemote = "origin" } diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 26c82823b..9fd85487e 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -56,6 +56,53 @@ func TestClone_NoneExisting(t *testing.T) { Equals(t, expCommit, actCommit) } +// Test running on main branch with merge strategy +func TestClone_MainBranchWithMergeStrategy(t *testing.T) { + // Initialize the git repo. + repoDir := initRepo(t) + expCommit := runCmd(t, repoDir, "git", "rev-parse", "main") + + dataDir := t.TempDir() + + logger := logging.NewNoopLogger(t) + + overrideURL := fmt.Sprintf("file://%s", repoDir) + wd := &events.FileWorkspace{ + DataDir: dataDir, + CheckoutMerge: true, + TestingOverrideHeadCloneURL: overrideURL, + TestingOverrideBaseCloneURL: overrideURL, + GpgNoSigningEnabled: true, + } + + _, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + Num: 0, + BaseRepo: models.Repo{}, + HeadBranch: "branch", + BaseBranch: "main", + }, "default") + Ok(t, err) + + // Create a file that we can use to check if the repo was recloned. + runCmd(t, dataDir, "touch", "repos/0/default/proof") + + // re-clone to make sure we don't try to merge main into itself + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + BaseRepo: models.Repo{}, + HeadBranch: "branch", + BaseBranch: "main", + }, "default") + Ok(t, err) + + // Use rev-parse to verify at correct commit. + actCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD") + Equals(t, expCommit, actCommit) + + // Check that our proof file is still there, proving that we didn't reclone. + _, err = os.Stat(filepath.Join(cloneDir, "proof")) + Ok(t, err) +} + // Test that if we don't have any existing files, we check out the repo // successfully when we're using the merge method. func TestClone_CheckoutMergeNoneExisting(t *testing.T) {