Keep local changes before moving to a new branch optimization (#637)

jfrog · Feb 15, 2024 · c5443c8 · c5443c8
1 parent 1b0e584
commit c5443c8
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 169 deletions.
diff --git a/integrationutils.go b/integrationutils.go
@@ -100,7 +100,7 @@ func createAndCheckoutIssueBranch(t *testing.T, testDetails *IntegrationTestDeta
 	err = gitManager.Clone(tmpDir, issuesBranch)
 	require.NoError(t, err)
 
-	err = gitManager.CreateBranchAndCheckout(currentIssuesBranch)
+	err = gitManager.CreateBranchAndCheckout(currentIssuesBranch, false)
 	require.NoError(t, err)
 
 	// This step is necessary because GitHub limits the number of pull requests from the same commit of the source branch

diff --git a/scanrepository/scanrepository.go b/scanrepository/scanrepository.go
@@ -311,21 +311,17 @@ func (cfp *ScanRepositoryCmd) fixSinglePackageAndCreatePR(vulnDetails *utils.Vul
 	}
 
 	workTreeIsClean, err := cfp.gitManager.IsClean()
+	if err != nil {
+		return
+	}
 	if !workTreeIsClean {
-		var removeTempDirCallback func() error
-		err, removeTempDirCallback = cfp.gitManager.CreateBranchAndCheckoutWithCopyingFilesDiff(fixBranchName)
-		defer func() {
-			err = errors.Join(err, removeTempDirCallback())
-		}()
-		if err != nil {
-			err = fmt.Errorf("failed while creating branch %s: %s", fixBranchName, err.Error())
-			return
-		}
+		// If there are local changes, such as files generated after running an 'install' command, we aim to preserve them in the new branch
+		err = cfp.gitManager.CreateBranchAndCheckout(fixBranchName, true)
 	} else {
-		if err = cfp.gitManager.CreateBranchAndCheckout(fixBranchName); err != nil {
-			err = fmt.Errorf("failed while creating branch %s: %s", fixBranchName, err.Error())
-			return
-		}
+		err = cfp.gitManager.CreateBranchAndCheckout(fixBranchName, false)
+	}
+	if err != nil {
+		return
 	}
 
 	if err = cfp.updatePackageToFixedVersion(vulnDetails); err != nil {
@@ -545,21 +541,17 @@ func (cfp *ScanRepositoryCmd) aggregateFixAndOpenPullRequest(vulnerabilitiesMap
 	log.Info("Starting aggregated dependencies fix")
 
 	workTreeIsClean, err := cfp.gitManager.IsClean()
+	if err != nil {
+		return
+	}
 	if !workTreeIsClean {
-		var removeTempDirCallback func() error
-		err, removeTempDirCallback = cfp.gitManager.CreateBranchAndCheckoutWithCopyingFilesDiff(aggregatedFixBranchName)
-		defer func() {
-			err = errors.Join(err, removeTempDirCallback())
-		}()
-		if err != nil {
-			err = fmt.Errorf("failed while creating branch %s: %s", aggregatedFixBranchName, err.Error())
-			return
-		}
+		// If there are local changes, such as files generated after running an 'install' command, we aim to preserve them in the new branch
+		err = cfp.gitManager.CreateBranchAndCheckout(aggregatedFixBranchName, true)
 	} else {
-		if err = cfp.gitManager.CreateBranchAndCheckout(aggregatedFixBranchName); err != nil {
-			err = fmt.Errorf("failed while creating branch %s: %s", aggregatedFixBranchName, err.Error())
-			return
-		}
+		err = cfp.gitManager.CreateBranchAndCheckout(aggregatedFixBranchName, false)
+	}
+	if err != nil {
+		return
 	}
 
 	// Fix all packages in the same branch if expected error accrued, log and continue.
@@ -578,6 +570,7 @@ func (cfp *ScanRepositoryCmd) aggregateFixAndOpenPullRequest(vulnerabilitiesMap
 		return
 	}
 	if !updateRequired {
+		err = errors.Join(err, cfp.gitManager.Checkout(cfp.scanDetails.BaseBranch()))
 		log.Info("The existing pull request is in sync with the latest scan, and no further updates are required.")
 		return
 	}

diff --git a/utils/git.go b/utils/git.go
@@ -146,7 +146,7 @@ func (gm *GitManager) SetDryRun(dryRun bool, dryRunRepoPath string) *GitManager
 
 func (gm *GitManager) Checkout(branchName string) error {
 	log.Debug("Running git checkout to branch:", branchName)
-	if err := gm.createBranchAndCheckout(branchName, false); err != nil {
+	if err := gm.createBranchAndCheckout(branchName, false, false); err != nil {
 		return fmt.Errorf("'git checkout %s' failed with error: %s", branchName, err.Error())
 	}
 	return nil
@@ -180,24 +180,38 @@ func (gm *GitManager) Clone(destinationPath, branchName string) error {
 	return nil
 }
 
-func (gm *GitManager) CreateBranchAndCheckout(branchName string) error {
+// Creates a new branch and switches to it.
+// If keepLocalChanges is set to true, all changes made on the current branch before switching to the new one will be transferred to the new branch.
+func (gm *GitManager) CreateBranchAndCheckout(branchName string, keepLocalChanges bool) error {
 	log.Debug("Creating branch", branchName, "...")
-	err := gm.createBranchAndCheckout(branchName, true)
+	err := gm.createBranchAndCheckout(branchName, true, keepLocalChanges)
 	if err != nil {
 		// Don't fail on dryRuns as we operate on local repositories, branch could be existing.
 		if gm.dryRun {
 			return nil
 		}
-		err = fmt.Errorf("git create and checkout failed with error: %s", err.Error())
+		if errors.Is(err, plumbing.ErrReferenceNotFound) {
+			return err
+		}
+		err = fmt.Errorf("failed upon creating/checkout branch '%s' with error: %s", branchName, err.Error())
 	}
 	return err
 }
 
-func (gm *GitManager) createBranchAndCheckout(branchName string, create bool) error {
-	checkoutConfig := &git.CheckoutOptions{
-		Create: create,
-		Branch: GetFullBranchName(branchName),
-		Force:  true,
+func (gm *GitManager) createBranchAndCheckout(branchName string, create bool, keepLocalChanges bool) error {
+	var checkoutConfig *git.CheckoutOptions
+	if keepLocalChanges {
+		checkoutConfig = &git.CheckoutOptions{
+			Create: create,
+			Branch: GetFullBranchName(branchName),
+			Keep:   true,
+		}
+	} else {
+		checkoutConfig = &git.CheckoutOptions{
+			Create: create,
+			Branch: GetFullBranchName(branchName),
+			Force:  true,
+		}
 	}
 	worktree, err := gm.localGitRepository.Worktree()
 	if err != nil {
@@ -206,30 +220,6 @@ func (gm *GitManager) createBranchAndCheckout(branchName string, create bool) er
 	return worktree.Checkout(checkoutConfig)
 }
 
-// Initiates a new branch and switches to it.
-// To carry over modifications from the current branch to the new one, we copy all FILES from the current working directory to a temporary directory.
-// Before switching to the new branch, we transfer the missing files from the temporary directory to the working directory associated with the new branch.
-// Note: Only FILES are copied, avoiding directory and configurations overrides (such as .git configurations).
-func (gm *GitManager) CreateBranchAndCheckoutWithCopyingFilesDiff(branchName string) (error, func() error) {
-	tempDirPath, err := CopyCurrentDirFilesToTempDir()
-	if err != nil {
-		return err, func() error { return nil }
-	}
-
-	removeDirCallback := func() error {
-		return fileutils.RemoveTempDir(tempDirPath)
-	}
-
-	if err = gm.CreateBranchAndCheckout(branchName); err != nil {
-		return err, removeDirCallback
-	}
-
-	// Copying all the files from the existing working directory to the new branch
-	// avoids the need for reinstalling the project in case it was previously installed.
-	err = CopyMissingFilesToCurrentWorkingDir(tempDirPath)
-	return err, removeDirCallback
-}
-
 func getCurrentBranch(repository *git.Repository) (string, error) {
 	head, err := repository.Head()
 	if err != nil {

diff --git a/utils/git_test.go b/utils/git_test.go
@@ -2,10 +2,10 @@ package utils
 
 import (
 	"errors"
+	"fmt"
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/config"
 	"github.com/go-git/go-git/v5/plumbing/object"
-	bitests "github.com/jfrog/build-info-go/tests"
 	"github.com/jfrog/froggit-go/vcsutils"
 	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
 	"github.com/jfrog/jfrog-client-go/utils/io/fileutils"
@@ -195,34 +195,71 @@ func TestGitManager_GenerateAggregatedCommitMessage(t *testing.T) {
 }
 
 func TestGitManager_Checkout(t *testing.T) {
-	tmpDir, err := fileutils.CreateTempDir()
-	assert.NoError(t, err)
-	defer func() {
-		assert.NoError(t, fileutils.RemoveTempDir(tmpDir))
-	}()
-	var restoreWd func() error
-	restoreWd, err = Chdir(tmpDir)
-	assert.NoError(t, err)
-	defer func() {
-		assert.NoError(t, restoreWd())
-	}()
-	gitManager := createFakeDotGit(t, tmpDir)
-	// Get the current branch that is set as HEAD
-	headRef, err := gitManager.localGitRepository.Head()
-	assert.NoError(t, err)
-	assert.Equal(t, headRef.Name().Short(), "master")
-	// Create 'dev' branch and checkout
-	err = gitManager.CreateBranchAndCheckout("dev")
-	assert.NoError(t, err)
-	var currBranch string
-	currBranch, err = getCurrentBranch(gitManager.localGitRepository)
-	assert.NoError(t, err)
-	assert.Equal(t, "dev", currBranch)
-	// Checkout back to 'master'
-	assert.NoError(t, gitManager.Checkout("master"))
-	currBranch, err = getCurrentBranch(gitManager.localGitRepository)
-	assert.NoError(t, err)
-	assert.Equal(t, "master", currBranch)
+	testCases := []struct {
+		withLocalChanges bool
+	}{
+		{
+			withLocalChanges: false,
+		},
+		{
+			withLocalChanges: true,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(fmt.Sprintf("test branch checkout: local changes:%t", test.withLocalChanges), func(t *testing.T) {
+			tmpDir, err := fileutils.CreateTempDir()
+			assert.NoError(t, err)
+			defer func() {
+				assert.NoError(t, fileutils.RemoveTempDir(tmpDir))
+			}()
+			var restoreWd func() error
+			restoreWd, err = Chdir(tmpDir)
+			assert.NoError(t, err)
+			defer func() {
+				assert.NoError(t, restoreWd())
+			}()
+			gitManager := createFakeDotGit(t, tmpDir)
+			// Get the current branch that is set as HEAD
+			headRef, err := gitManager.localGitRepository.Head()
+			assert.NoError(t, err)
+			assert.Equal(t, headRef.Name().Short(), "master")
+
+			if test.withLocalChanges {
+				// Create new file in master branch
+				tempFilePath := filepath.Join(tmpDir, "myFile.txt")
+				var file *os.File
+				file, err = os.Create(tempFilePath)
+				assert.NoError(t, err)
+				assert.NoError(t, file.Close())
+
+				// Create 'dev' branch and checkout
+				err = gitManager.CreateBranchAndCheckout("dev", true)
+				assert.NoError(t, err)
+
+				// Verify that temp file exist in new branch
+				var fileExists bool
+				fileExists, err = fileutils.IsFileExists(tempFilePath, false)
+				assert.NoError(t, err)
+				assert.True(t, fileExists)
+			} else {
+				// Create 'dev' branch and checkout
+				err = gitManager.CreateBranchAndCheckout("dev", false)
+				assert.NoError(t, err)
+			}
+
+			var currBranch string
+			currBranch, err = getCurrentBranch(gitManager.localGitRepository)
+			assert.NoError(t, err)
+			assert.Equal(t, "dev", currBranch)
+
+			// Checkout back to 'master'
+			assert.NoError(t, gitManager.Checkout("master"))
+			currBranch, err = getCurrentBranch(gitManager.localGitRepository)
+			assert.NoError(t, err)
+			assert.Equal(t, "master", currBranch)
+		})
+	}
 }
 
 func createFakeDotGit(t *testing.T, testPath string) *GitManager {
@@ -350,33 +387,3 @@ func TestGetAggregatedPullRequestTitle(t *testing.T) {
 		})
 	}
 }
-
-func TestCreateBranchAndCheckoutWithCopyingFilesDiff(t *testing.T) {
-	curWd, err := os.Getwd()
-	assert.NoError(t, err)
-	tempDirPath, createTempDirCallback := bitests.CreateTempDirWithCallbackAndAssert(t)
-	assert.NoError(t, os.Chdir(tempDirPath))
-	defer func() {
-		assert.NoError(t, os.Chdir(curWd))
-	}()
-
-	gitManager := createFakeDotGit(t, tempDirPath)
-	// Generating a new file that will result in an unclean working tree.
-	newFile, err := os.Create("new-file.txt")
-	assert.NoError(t, err)
-
-	var removeDirCallback func() error
-	err, removeDirCallback = gitManager.CreateBranchAndCheckoutWithCopyingFilesDiff("new-branch")
-	assert.NoError(t, err)
-	defer func() {
-		assert.NoError(t, newFile.Close())
-		assert.NoError(t, removeDirCallback())
-		createTempDirCallback()
-	}()
-
-	// Confirm that the new files exist in the new branch
-	var fileExists bool
-	fileExists, err = fileutils.IsFileExists(filepath.Join(tempDirPath, newFile.Name()), false)
-	assert.NoError(t, err)
-	assert.True(t, fileExists)
-}
diff --git a/utils/scanrepository.go b/utils/scanrepository.go