Fix open pull request against the wrong branch (#421)

commands/commands.go

@@ -17,10 +17,10 @@ type FrogbotCommand interface {
 	Run(config utils.RepoAggregator, client vcsclient.VcsClient) error
 }
 
-func Exec(command FrogbotCommand, name string) (err error) {
+func Exec(command FrogbotCommand, commandName string) (err error) {
 	// Get frogbotUtils that contains the config, server, and VCS client
 	log.Info("Frogbot version:", utils.FrogbotVersion)
-	frogbotUtils, err := utils.GetFrogbotDetails()
+	frogbotUtils, err := utils.GetFrogbotDetails(commandName)
 	if err != nil {
 		return err
 	}
@@ -47,25 +47,25 @@ func Exec(command FrogbotCommand, name string) (err error) {
 
 	// Send a usage report
 	usageReportSent := make(chan error)
-	go utils.ReportUsage(name, frogbotUtils.ServerDetails, usageReportSent)
+	go utils.ReportUsage(commandName, frogbotUtils.ServerDetails, usageReportSent)
 
 	// Invoke the command interface
-	log.Info(fmt.Sprintf("Running Frogbot %q command", name))
-	err = command.Run(frogbotUtils.Repositories, frogbotUtils.Client)
+	log.Info(fmt.Sprintf("Running Frogbot %q command", commandName))
+	err = command.Run(frogbotUtils.Repositories, frogbotUtils.GitClient)
 
 	// Wait for a signal, letting us know that the usage reporting is done.
 	<-usageReportSent
 
 	if err == nil {
-		log.Info(fmt.Sprintf("Frogbot %q command finished successfully", name))
+		log.Info(fmt.Sprintf("Frogbot %q command finished successfully", commandName))
 	}
 	return err
 }
 
 func GetCommands() []*clitool.Command {
 	return []*clitool.Command{
 		{
-			Name:    "scan-pull-request",
+			Name:    utils.ScanPullRequest,
 			Aliases: []string{"spr"},
 			Usage:   "Scans a pull request with JFrog Xray for security vulnerabilities.",
 			Action: func(ctx *clitool.Context) error {
@@ -74,7 +74,7 @@ func GetCommands() []*clitool.Command {
 			Flags: []clitool.Flag{},
 		},
 		{
-			Name:    "create-fix-pull-requests",
+			Name:    utils.CreateFixPullRequests,
 			Aliases: []string{"cfpr"},
 			Usage:   "Scan the current branch and create pull requests with fixes if needed",
 			Action: func(ctx *clitool.Context) error {
@@ -83,7 +83,7 @@ func GetCommands() []*clitool.Command {
 			Flags: []clitool.Flag{},
 		},
 		{
-			Name:    "scan-pull-requests",
+			Name:    utils.ScanPullRequests,
 			Aliases: []string{"sprs"},
 			Usage:   "Scans all the open pull requests within a single or multiple repositories with JFrog Xray for security vulnerabilities",
 			Action: func(ctx *clitool.Context) error {
@@ -92,7 +92,7 @@ func GetCommands() []*clitool.Command {
 			Flags: []clitool.Flag{},
 		},
 		{
-			Name:    "scan-and-fix-repos",
+			Name:    utils.ScanAndFixRepos,
 			Aliases: []string{"safr"},
 			Usage:   "Scan single or multiple repositories and create pull requests with fixes if any security vulnerabilities are found",
 			Action: func(ctx *clitool.Context) error {

commands/createfixpullrequests.go

@@ -43,26 +43,29 @@ type CreateFixPullRequestsCmd struct {
 	handlers map[coreutils.Technology]packagehandlers.PackageHandler
 }
 
-func (cfp *CreateFixPullRequestsCmd) Run(repoAggregator utils.RepoAggregator, client vcsclient.VcsClient) error {
-	if err := utils.ValidateSingleRepoConfiguration(&repoAggregator); err != nil {
+func (cfp *CreateFixPullRequestsCmd) Run(repoAggregator utils.RepoAggregator, client vcsclient.VcsClient) (err error) {
+	if err = utils.ValidateSingleRepoConfiguration(&repoAggregator); err != nil {
 		return err
 	}
 	repository := repoAggregator[0]
 	for _, branch := range repository.Branches {
-		err := cfp.scanAndFixRepository(&repository, branch, client)
-		if err != nil {
-			return err
+		if err = cfp.scanAndFixRepository(&repository, branch, client); err != nil {
+			return
 		}
 	}
-	return nil
+	return
 }
 
 func (cfp *CreateFixPullRequestsCmd) scanAndFixRepository(repository *utils.Repository, branch string, client vcsclient.VcsClient) (err error) {
-	cfp.baseWd, err = os.Getwd()
-	if err != nil {
+	if cfp.baseWd, err = os.Getwd(); err != nil {
+		return
+	}
+	if err = cfp.setCommandPrerequisites(repository, branch, client); err != nil {
 		return
 	}
-	cfp.setCommandPrerequisites(repository, branch, client)
+	if err = cfp.gitManager.Checkout(branch); err != nil {
+		return fmt.Errorf("failed to checkout to %s branch before scanning. The following error has been received:\n%s", branch, err.Error())
+	}
 	for i := range repository.Projects {
 		cfp.details.Project = &repository.Projects[i]
 		cfp.projectTech = ""
@@ -73,7 +76,7 @@ func (cfp *CreateFixPullRequestsCmd) scanAndFixRepository(repository *utils.Repo
 	return
 }
 
-func (cfp *CreateFixPullRequestsCmd) setCommandPrerequisites(repository *utils.Repository, branch string, client vcsclient.VcsClient) {
+func (cfp *CreateFixPullRequestsCmd) setCommandPrerequisites(repository *utils.Repository, branch string, client vcsclient.VcsClient) (err error) {
 	cfp.details = utils.NewScanDetails(client, &repository.Server, &repository.Git).
 		SetXrayGraphScanParams(repository.Watches, repository.JFrogProjectKey).
 		SetFailOnInstallationErrors(*repository.FailOnSecurityIssues).
@@ -82,12 +85,14 @@ func (cfp *CreateFixPullRequestsCmd) setCommandPrerequisites(repository *utils.R
 		SetMinSeverity(repository.MinSeverity)
 	cfp.aggregateFixes = repository.Git.AggregateFixes
 	cfp.OutputWriter = utils.GetCompatibleOutputWriter(repository.GitProvider)
+	cfp.gitManager, err = utils.NewGitManager(cfp.dryRun, cfp.dryRunRepoPath, ".", "origin", cfp.details.Token, cfp.details.Username, cfp.details.Git)
+	return
 }
 
 func (cfp *CreateFixPullRequestsCmd) scanAndFixProject(repository *utils.Repository) error {
 	var fixNeeded bool
 	// A map that contains the full project paths as a keys
-	// The value is a map of vulnerable package names -> the details of the vulnerable packages.x
+	// The value is a map of vulnerable package names -> the details of the vulnerable packages.
 	// That means we have a map of all the vulnerabilities that were found in a specific folder, along with their full details.
 	vulnerabilitiesByPathMap := make(map[string]map[string]*utils.VulnerabilityDetails)
 	projectFullPathWorkingDirs := getFullPathWorkingDirs(cfp.details.Project.WorkingDirs, cfp.baseWd)
@@ -147,12 +152,6 @@ func (cfp *CreateFixPullRequestsCmd) getVulnerabilitiesMap(scanResults *xrayutil
 }
 
 func (cfp *CreateFixPullRequestsCmd) fixVulnerablePackages(vulnerabilitiesByWdMap map[string]map[string]*utils.VulnerabilityDetails) (err error) {
-	if cfp.gitManager == nil {
-		cfp.gitManager, err = utils.NewGitManager(cfp.dryRun, cfp.dryRunRepoPath, ".", "origin", cfp.details.Token, cfp.details.Username, cfp.details.Git)
-		if err != nil {
-			return
-		}
-	}
 	clonedRepoDir, restoreBaseDir, err := cfp.cloneRepository()
 	if err != nil {
 		return
@@ -186,12 +185,12 @@ func (cfp *CreateFixPullRequestsCmd) fixProjectVulnerabilities(fullProjectPath s
 
 	// 'CD' into the relevant working directory
 	if projectWorkingDir != "" {
-		restoreDir, err := utils.Chdir(projectWorkingDir)
-		if err != nil {
-			return err
+		var restoreDirFunc func() error
+		if restoreDirFunc, err = utils.Chdir(projectWorkingDir); err != nil {
+			return
 		}
 		defer func() {
-			err = errors.Join(err, restoreDir())
+			err = errors.Join(err, restoreDirFunc())
 		}()
 	}
 
@@ -203,7 +202,7 @@ func (cfp *CreateFixPullRequestsCmd) fixProjectVulnerabilities(fullProjectPath s
 
 		// After fixing the current vulnerability, checkout to the base branch to start fixing the next vulnerability
 		log.Debug("Running git checkout to base branch:", cfp.details.Branch())
-		if e := cfp.gitManager.CheckoutLocalBranch(cfp.details.Branch()); e != nil {
+		if e := cfp.gitManager.Checkout(cfp.details.Branch()); e != nil {
 			err = errors.Join(err, cfp.handleUpdatePackageErrors(e))
 			return
 		}
@@ -545,7 +544,7 @@ func (cfp *CreateFixPullRequestsCmd) aggregateFixAndOpenPullRequest(vulnerabilit
 		}
 	}
 	log.Info("-----------------------------------------------------------------")
-	if e := cfp.gitManager.CheckoutLocalBranch(cfp.details.Branch()); e != nil {
+	if e := cfp.gitManager.Checkout(cfp.details.Branch()); e != nil {
 		err = errors.Join(err, e)
 	}
 	return

commands/createfixpullrequests_test.go

@@ -91,7 +91,7 @@ func TestCreateFixPullRequestsCmd_Run(t *testing.T) {
 			repoName:               "aggregate-multi-dir",
 			testDir:                "createfixpullrequests/aggregate-multi-dir",
 			expectedBranches:       []string{"frogbot-update-npm-dependencies"},
-			expectedDiff:           []string{"diff --git a/npm1/package.json b/npm1/package.json\nindex ae09978..286211d 100644\n--- a/npm1/package.json\n+++ b/npm1/package.json\n@@ -9,8 +9,8 @@\n   \"author\": \"\",\n   \"license\": \"ISC\",\n   \"dependencies\": {\n-    \"uuid\": \"^9.0.0\",\n-    \"minimatch\":\"3.0.2\",\n-    \"mpath\": \"0.7.0\"\n+    \"minimatch\": \"^3.0.5\",\n+    \"mpath\": \"^0.8.4\",\n+    \"uuid\": \"^9.0.0\"\n   }\n-}\n\\ No newline at end of file\n+}\ndiff --git a/npm2/package.json b/npm2/package.json\nindex be180a6..14b5c7a 100644\n--- a/npm2/package.json\n+++ b/npm2/package.json\n@@ -1,5 +1,5 @@\n {\n   \"dependencies\": {\n-    \"minimist\": \"^1.2.5\"\n+    \"minimist\": \"^1.2.6\"\n   }\n }\n"},
+			expectedDiff:           []string{"diff --git a/npm1/package.json b/npm1/package.json\nindex ae09978..286211d 100644\n--- a/npm1/package.json\n+++ b/npm1/package.json\n@@ -9,8 +9,8 @@\n   \"author\": \"\",\n   \"license\": \"ISC\",\n   \"dependencies\": {\n-    \"uuid\": \"^9.0.0\",\n-    \"minimatch\":\"3.0.2\",\n-    \"mpath\": \"0.7.0\"\n+    \"minimatch\": \"^3.0.5\",\n+    \"mpath\": \"^0.8.4\",\n+    \"uuid\": \"^9.0.0\"\n   }\n-}\n\\ No newline at end of file\n+}\ndiff --git a/npm2/package.json b/npm2/package.json\nindex ff94a18..14b5c7a 100644\n--- a/npm2/package.json\n+++ b/npm2/package.json\n@@ -1,5 +1,5 @@\n {\n   \"dependencies\": {\n-    \"minimist\": \"1.2.5\"\n+    \"minimist\": \"^1.2.6\"\n   }\n }\n"},
 			packageDescriptorPaths: []string{"npm1/package.json", "npm2/package.json"},
 			aggregateFixes:         true,
 			configPath:             "testdata/createfixpullrequests/aggregate-multi-dir/.frogbot/frogbot-config.yml",
@@ -143,7 +143,7 @@ func TestCreateFixPullRequestsCmd_Run(t *testing.T) {
 			var port string
 			server := httptest.NewServer(createHttpHandler(t, &port, test.repoName))
 			port = server.URL[strings.LastIndex(server.URL, ":")+1:]
-			gitTestParams := utils.GitClientInfo{
+			gitTestParams := utils.Git{
 				GitProvider: vcsutils.GitHub,
 				VcsInfo: vcsclient.VcsInfo{
 					Token:       "123456",
@@ -243,7 +243,7 @@ pr body
 				server.Close()
 			}()
 			port = server.URL[strings.LastIndex(server.URL, ":")+1:]
-			gitTestParams := &utils.GitClientInfo{
+			gitTestParams := &utils.Git{
 				GitProvider: vcsutils.GitHub,
 				VcsInfo: vcsclient.VcsInfo{
 					Token:       "123456",

commands/scanandfixrepos_test.go

@@ -37,7 +37,7 @@ func TestScanAndFixRepos(t *testing.T) {
 	defer server.Close()
 	port = server.URL[strings.LastIndex(server.URL, ":")+1:]
 
-	gitTestParams := utils.GitClientInfo{
+	gitTestParams := utils.Git{
 		GitProvider: vcsutils.GitHub,
 		RepoOwner:   "jfrog",
 		VcsInfo: vcsclient.VcsInfo{

commands/scanpullrequest_test.go

@@ -598,8 +598,8 @@ func TestVerifyGitHubFrogbotEnvironmentNoReviewers(t *testing.T) {
 
 func TestVerifyGitHubFrogbotEnvironmentOnPrem(t *testing.T) {
 	repoConfig := &utils.Repository{
-		Params: utils.Params{Git: utils.Git{GitClientInfo: utils.GitClientInfo{
-			VcsInfo: vcsclient.VcsInfo{APIEndpoint: "https://acme.vcs.io"}}},
+		Params: utils.Params{Git: utils.Git{
+			VcsInfo: vcsclient.VcsInfo{APIEndpoint: "https://acme.vcs.io"}},
 		},
 	}
 
@@ -609,7 +609,7 @@ func TestVerifyGitHubFrogbotEnvironmentOnPrem(t *testing.T) {
 }
 
 func prepareConfigAndClient(t *testing.T, configPath string, server *httptest.Server, serverParams coreconfig.ServerDetails) (utils.RepoAggregator, vcsclient.VcsClient) {
-	gitTestParams := &utils.GitClientInfo{
+	gitTestParams := &utils.Git{
 		GitProvider: vcsutils.GitHub,
 		RepoOwner:   "jfrog",
 		VcsInfo: vcsclient.VcsInfo{
@@ -839,7 +839,8 @@ func TestDeletePreviousPullRequestMessages(t *testing.T) {
 	repository := &utils.Repository{
 		Params: utils.Params{
 			Git: utils.Git{
-				GitClientInfo: utils.GitClientInfo{RepoName: "repo", RepoOwner: "owner"},
+				RepoName:      "repo",
+				RepoOwner:     "owner",
 				PullRequestID: 17,
 			},
 		},

commands/scanpullrequests_test.go

@@ -20,11 +20,9 @@ var gitParams = &utils.Repository{
 	OutputWriter: &utils.SimplifiedOutput{},
 	Params: utils.Params{
 		Git: utils.Git{
-			GitClientInfo: utils.GitClientInfo{
-				RepoOwner: "repo-owner",
-				Branches:  []string{"master"},
-				RepoName:  "repo-name",
-			},
+			RepoOwner: "repo-owner",
+			Branches:  []string{"master"},
+			RepoName:  "repo-name",
 		},
 	},
 }

commands/testdata/createfixpullrequests/aggregate-multi-dir/git/COMMIT_EDITMSG

@@ -1 +1 @@
-fix tests
+init

commands/testdata/createfixpullrequests/aggregate-multi-dir/git/logs/HEAD

@@ -1 +1 @@
-0000000000000000000000000000000000000000 20cfb0cbabf22f1c12d626eec6e6dd834f4e43d1 Omer Zidkoni <[email protected]> 1689852711 +0300	commit (initial): fix tests
+0000000000000000000000000000000000000000 59fbea5efff5cfe6a005f33aaadbf21d58dc4af8 Omer Zidkoni <[email protected]> 1691578013 +0300	commit (initial): init

commands/testdata/createfixpullrequests/aggregate-multi-dir/git/logs/refs/heads/main

@@ -1 +1 @@
-0000000000000000000000000000000000000000 20cfb0cbabf22f1c12d626eec6e6dd834f4e43d1 Omer Zidkoni <[email protected]> 1689852711 +0300	commit (initial): fix tests
+0000000000000000000000000000000000000000 59fbea5efff5cfe6a005f33aaadbf21d58dc4af8 Omer Zidkoni <[email protected]> 1691578013 +0300	commit (initial): init

commands/testdata/createfixpullrequests/aggregate-multi-dir/git/objects/59/fbea5efff5cfe6a005f33aaadbf21d58dc4af8

@@ -0,0 +1,2 @@
+x
��A� E]s�ٛ�)��x���	EC'!����+�|?y�'n��ٞF�(O��b,��2{k�P�ő�9`"a���wX[�p�ۇ�
+�n���yIܮ�h��Hp֤��U��H����u��d5

commands/testdata/createfixpullrequests/aggregate-multi-dir/git/refs/heads/main

@@ -1 +1 @@
-20cfb0cbabf22f1c12d626eec6e6dd834f4e43d1
+59fbea5efff5cfe6a005f33aaadbf21d58dc4af8

commands/utils/git.go

@@ -80,13 +80,12 @@ func NewGitManager(dryRun bool, clonedRepoPath, projectPath, remoteName, token,
 	return &GitManager{repository: repository, dryRunRepoPath: clonedRepoPath, remoteName: remoteName, auth: basicAuth, dryRun: dryRun, customTemplates: templates, git: g}, nil
 }
 
-func (gm *GitManager) CheckoutLocalBranch(branchName string) error {
-	err := gm.createBranchAndCheckout(branchName, false)
-	if err != nil {
-		err = fmt.Errorf("'git checkout %s' failed with error: %s", branchName, err.Error())
+func (gm *GitManager) Checkout(branchName string) error {
+	log.Debug("Running git checkout to branch:", branchName)
+	if err := gm.createBranchAndCheckout(branchName, false); err != nil {
+		return fmt.Errorf("'git checkout %s' failed with error: %s", branchName, err.Error())
 	}
-	log.Debug("Running git checkout to local branch:", branchName)
-	return err
+	return nil
 }
 
 func (gm *GitManager) Clone(destinationPath, branchName string) error {
@@ -164,6 +163,14 @@ func (gm *GitManager) createBranchAndCheckout(branchName string, create bool) er
 	return worktree.Checkout(checkoutConfig)
 }
 
+func getCurrentBranch(repository *git.Repository) (string, error) {
+	head, err := repository.Head()
+	if err != nil {
+		return "", err
+	}
+	return head.Name().Short(), nil
+}
+
 func (gm *GitManager) AddAllAndCommit(commitMessage string) error {
 	log.Debug("Running git add all and commit...")
 	err := gm.addAll()
@@ -388,28 +395,6 @@ func (gm *GitManager) generateHTTPSCloneUrl() (url string, err error) {
 	}
 }
 
-func (gm *GitManager) CheckoutRemoteBranch(branchName string) error {
-	var checkoutConfig *git.CheckoutOptions
-	if gm.dryRun {
-		// On dry runs we mimic remote as local branches.
-		checkoutConfig = &git.CheckoutOptions{
-			Branch: plumbing.NewBranchReferenceName(branchName),
-			Force:  true,
-		}
-	} else {
-		checkoutConfig = &git.CheckoutOptions{
-			Branch: plumbing.NewRemoteReferenceName(gm.remoteName, branchName),
-			Force:  true,
-		}
-	}
-	log.Debug("Running git checkout to remote branch:", branchName)
-	worktree, err := gm.repository.Worktree()
-	if err != nil {
-		return err
-	}
-	return worktree.Checkout(checkoutConfig)
-}
-
 func toBasicAuth(token, username string) *githttp.BasicAuth {
 	// The username can be anything except for an empty string
 	if username == "" {
@@ -429,6 +414,14 @@ func getFullBranchName(branchName string) plumbing.ReferenceName {
 	return plumbing.NewBranchReferenceName(plumbing.ReferenceName(branchName).Short())
 }
 
+func GetBranchFromDotGit() (string, error) {
+	currentRepo, err := git.PlainOpen(".")
+	if err != nil {
+		return "", errors.New("unable to retrieve the branch to scan, as the .git folder was not found in the current working directory. The error that was received: " + err.Error())
+	}
+	return getCurrentBranch(currentRepo)
+}
+
 func loadCustomTemplates(commitMessageTemplate, branchNameTemplate, pullRequestTitleTemplate string) (CustomTemplates, error) {
 	template := CustomTemplates{
 		commitMessageTemplate:    commitMessageTemplate,