Apply clone depth to each checkout strategy (#137)

* Apply clone depth to all checkout strategies. * Update PRDiffFileParams handling. * Separate go-tests and wire in step.yml lint for the ci workflow. * Apply reset unshallow fallback strategy to PR checkouts. * Fix bitrise.yml. * Do not apply fetch fallback in case of branch checkout.
bitrise-steplib · Mar 11, 2021 · 47b323a · 47b323a
1 parent 6b85fbc
commit 47b323a
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 80 deletions.
diff --git a/bitrise.yml b/bitrise.yml
@@ -3,19 +3,19 @@ default_step_lib_source: https://github.com/bitrise-io/bitrise-steplib.git
 
 app:
   envs:
-    - STEP_ID_IN_STEPLIB: git-clone
-    - GIT_REPOSITORY_URL: https://github.com/bitrise-io/steps-git-clone.git
-    - opts:
-        is_expand: false
-      EVAL_SCRIPT: |-
-        echo "GIT_CLONE_COMMIT_HASH: ${GIT_CLONE_COMMIT_HASH}"
-        echo "GIT_CLONE_COMMIT_MESSAGE_SUBJECT: ${GIT_CLONE_COMMIT_MESSAGE_SUBJECT}"
-        echo "GIT_CLONE_COMMIT_MESSAGE_BODY: ${GIT_CLONE_COMMIT_MESSAGE_BODY}"
-        echo "GIT_CLONE_COMMIT_COUNT: ${GIT_CLONE_COMMIT_COUNT}"
-        echo "GIT_CLONE_COMMIT_AUTHOR_NAME: ${GIT_CLONE_COMMIT_AUTHOR_NAME}"
-        echo "GIT_CLONE_COMMIT_AUTHOR_EMAIL: ${GIT_CLONE_COMMIT_AUTHOR_EMAIL}"
-        echo "GIT_CLONE_COMMIT_COMMITER_NAME: ${GIT_CLONE_COMMIT_COMMITER_NAME}"
-        echo "GIT_CLONE_COMMIT_COMMITER_EMAIL: ${GIT_CLONE_COMMIT_COMMITER_EMAIL}"
+  - EVAL_SCRIPT: |-
+      echo "GIT_CLONE_COMMIT_HASH: ${GIT_CLONE_COMMIT_HASH}"
+      echo "GIT_CLONE_COMMIT_MESSAGE_SUBJECT: ${GIT_CLONE_COMMIT_MESSAGE_SUBJECT}"
+      echo "GIT_CLONE_COMMIT_MESSAGE_BODY: ${GIT_CLONE_COMMIT_MESSAGE_BODY}"
+      echo "GIT_CLONE_COMMIT_COUNT: ${GIT_CLONE_COMMIT_COUNT}"
+      echo "GIT_CLONE_COMMIT_AUTHOR_NAME: ${GIT_CLONE_COMMIT_AUTHOR_NAME}"
+      echo "GIT_CLONE_COMMIT_AUTHOR_EMAIL: ${GIT_CLONE_COMMIT_AUTHOR_EMAIL}"
+      echo "GIT_CLONE_COMMIT_COMMITER_NAME: ${GIT_CLONE_COMMIT_COMMITER_NAME}"
+      echo "GIT_CLONE_COMMIT_COMMITER_EMAIL: ${GIT_CLONE_COMMIT_COMMITER_EMAIL}"
+    opts:
+      is_expand: false
+  # define these envs in your .bitrise.secrets.yml
+  - SSH_RSA_PRIVATE_KEY: $SSH_RSA_PRIVATE_KEY
 
 workflows:
   test:
@@ -32,13 +32,12 @@ workflows:
     envs:
     - CLONE_INTO_DIR: .
     - GIT_REPOSITORY_URL: https://github.com/bitrise-io/git-clone-test.git
+    before_run:
+    - audit-this-step
+    - go-tests
     steps:
     - activate-ssh-key:
         run_if: true
-    - go-list:
-    - golint:
-    - errcheck:
-    - go-test:
     after_run:
     - _test_error
     - _test_submodule
@@ -54,13 +53,20 @@ workflows:
     - _test_checkout_pull_request_with_depth
     - _test_unshallow
     - _test_checkout_different_dir
-    - _test_manual_merge_ignore_depth
+    - _test_manual_merge_unshallow
     - _test_commit_logs
     - _test_hosted_git_notfork
     - _test_unrelated_histories
     - _test_hosted_git_ssh_prefix
     - test_diff_file
 
+  go-tests:
+    steps:
+    - go-list:
+    - golint:
+    - errcheck:
+    - go-test:
+
   _test_submodule:
     before_run:
     - _create_tmpdir
@@ -406,7 +412,7 @@ workflows:
         inputs:
         - dir_to_check: $CLONE_INTO_DIR
 
-  _test_manual_merge_ignore_depth:
+  _test_manual_merge_unshallow:
     before_run:
     - _create_tmpdir
     steps:
@@ -686,8 +692,6 @@ workflows:
         inputs:
         - path: $STEP_TMPDIR
 
-  # ----------------------------------------------------------------
-  # --- workflow to Share this step into a Step Library
   audit-this-step:
     steps:
     - script:

diff --git a/gitclone/checkout.go b/gitclone/checkout.go
@@ -160,9 +160,14 @@ func createCheckoutStrategy(checkoutMethod CheckoutMethod, cfg Config, patch pat
 				return nil, fmt.Errorf("merging PR (automatic) failed, there is no Pull Request branch and could not download diff file: %v", err)
 			}
 
+			params, err := NewPRDiffFileParams(cfg.BranchDest)
+			if err != nil {
+				return nil, err
+			}
+
 			return checkoutPRDiffFile{
-				baseBranch: cfg.BranchDest,
-				patchFile:  patchFile,
+				params:    *params,
+				patchFile: patchFile,
 			}, nil
 		}
 	case CheckoutForkPRManualMergeMethod:
@@ -193,50 +198,36 @@ func createCheckoutStrategy(checkoutMethod CheckoutMethod, cfg Config, patch pat
 
 }
 
-func selectFetchOptions(checkoutStrategy CheckoutMethod, cloneDepth int, isTag bool) fetchOptions {
+func selectFetchOptions(checkoutStrategy CheckoutMethod, cloneDepth int, fetchAllTags bool) fetchOptions {
+	opts := fetchOptions{
+		depth:   cloneDepth,
+		allTags: false,
+	}
+
 	switch checkoutStrategy {
 	case CheckoutCommitMethod, CheckoutBranchMethod:
-		return fetchOptions{
-			depth:   cloneDepth,
-			allTags: isTag,
-		}
+		opts.allTags = fetchAllTags
 	case CheckoutTagMethod:
-		return fetchOptions{
-			depth:   cloneDepth,
-			allTags: true,
-		}
-	case CheckoutPRMergeBranchMethod, CheckoutPRDiffFileMethod:
-		return fetchOptions{
-			depth:   cloneDepth,
-			allTags: false,
-		}
-	case CheckoutPRManualMergeMethod, CheckoutForkPRManualMergeMethod, CheckoutNoneMethod:
-		return fetchOptions{}
+		opts.allTags = true
 	default:
-		return fetchOptions{}
 	}
+
+	return opts
 }
 
 func selectFallbacks(checkoutStrategy CheckoutMethod, fetchOpts fetchOptions) fallbackRetry {
-	switch checkoutStrategy {
-	case CheckoutCommitMethod, CheckoutTagMethod:
-		{
-			if !fetchOpts.IsFullDepth() {
-				return simpleUnshallow{}
-			}
-
-			return nil
-		}
-	case CheckoutPRMergeBranchMethod:
-		{
-			if !fetchOpts.IsFullDepth() {
-				return resetUnshallow{}
-			}
+	if fetchOpts.IsFullDepth() {
+		return nil
+	}
 
-			return nil
-		}
-	case CheckoutPRManualMergeMethod, CheckoutForkPRManualMergeMethod, CheckoutBranchMethod, CheckoutPRDiffFileMethod:
+	switch checkoutStrategy {
+	case CheckoutBranchMethod:
+		// the given branch's tip will be checked out, no need to unshallow
 		return nil
+	case CheckoutCommitMethod, CheckoutTagMethod:
+		return simpleUnshallow{}
+	case CheckoutPRMergeBranchMethod, CheckoutPRManualMergeMethod, CheckoutForkPRManualMergeMethod, CheckoutPRDiffFileMethod:
+		return resetUnshallow{}
 	default:
 		return nil
 	}

diff --git a/gitclone/checkout_method_pr_auto_diff.go b/gitclone/checkout_method_pr_auto_diff.go
@@ -18,8 +18,8 @@ type PRDiffFileParams struct {
 	BaseBranch string
 }
 
-// NewPRDiffFileParams validates and returns a new PRDiffFile
-func NewPRDiffFileParams(baseBranch string, PRID uint) (*PRDiffFileParams, error) {
+// NewPRDiffFileParams validates and returns a new PRDiffFileParams
+func NewPRDiffFileParams(baseBranch string) (*PRDiffFileParams, error) {
 	if strings.TrimSpace(baseBranch) == "" {
 		return nil, NewParameterValidationError("PR diff file based checkout strategy can not be used: no base branch specified")
 	}
@@ -31,16 +31,17 @@ func NewPRDiffFileParams(baseBranch string, PRID uint) (*PRDiffFileParams, error
 
 // checkoutPRDiffFile
 type checkoutPRDiffFile struct {
-	baseBranch, patchFile string
+	params    PRDiffFileParams
+	patchFile string
 }
 
 func (c checkoutPRDiffFile) do(gitCmd git.Git, fetchOptions fetchOptions, fallback fallbackRetry) error {
-	baseBranchRef := branchRefPrefix + c.baseBranch
+	baseBranchRef := branchRefPrefix + c.params.BaseBranch
 	if err := fetch(gitCmd, defaultRemoteName, &baseBranchRef, fetchOptions); err != nil {
 		return err
 	}
 
-	if err := checkoutWithCustomRetry(gitCmd, c.baseBranch, fallback); err != nil {
+	if err := checkoutWithCustomRetry(gitCmd, c.params.BaseBranch, fallback); err != nil {
 		return err
 	}
 

diff --git a/gitclone/checkout_method_pr_auto_merge_branch.go b/gitclone/checkout_method_pr_auto_merge_branch.go
@@ -44,7 +44,7 @@ func (c checkoutPRMergeBranch) do(gitCmd git.Git, fetchOpts fetchOptions, fallba
 
 	// `git "fetch" "origin" "refs/pull/7/head:pull/7"`
 	headBranchRef := fetchArg(c.params.MergeBranch)
-	if err := fetch(gitCmd, defaultRemoteName, &headBranchRef, fetchOptions{}); err != nil {
+	if err := fetch(gitCmd, defaultRemoteName, &headBranchRef, fetchOpts); err != nil {
 		return err
 	}
 

diff --git a/gitclone/gitclone_test.go b/gitclone/gitclone_test.go
@@ -29,10 +29,11 @@ var testCases = [...]struct {
 	{
 		name: "Checkout commit",
 		cfg: Config{
-			Commit: "76a934a",
+			Commit:     "76a934a",
+			CloneDepth: 1,
 		},
 		wantCmds: []string{
-			`git "fetch"`,
+			`git "fetch" "--depth=1"`,
 			`git "checkout" "76a934a"`,
 		},
 	},
@@ -62,21 +63,23 @@ var testCases = [...]struct {
 	{
 		name: "Checkout branch",
 		cfg: Config{
-			Branch: "hcnarb",
+			Branch:     "hcnarb",
+			CloneDepth: 1,
 		},
 		wantCmds: []string{
-			`git "fetch" "origin" "refs/heads/hcnarb"`,
+			`git "fetch" "--depth=1" "origin" "refs/heads/hcnarb"`,
 			`git "checkout" "hcnarb"`,
 			`git "merge" "origin/hcnarb"`,
 		},
 	},
 	{
 		name: "Checkout tag",
 		cfg: Config{
-			Tag: "gat",
+			Tag:        "gat",
+			CloneDepth: 1,
 		},
 		wantCmds: []string{
-			`git "fetch" "--tags"`,
+			`git "fetch" "--depth=1" "--tags"`,
 			`git "checkout" "gat"`,
 		},
 	},
@@ -128,7 +131,7 @@ var testCases = [...]struct {
 
 	// ** PRs manual merge
 	{
-		name: "PR - no fork - manual merge: branch and commit (ignore depth)",
+		name: "PR - no fork - manual merge: branch and commit",
 		cfg: Config{
 			Commit:        "76a934ae",
 			Branch:        "test/commit-messages",
@@ -139,11 +142,11 @@ var testCases = [...]struct {
 			ManualMerge:   true,
 		},
 		wantCmds: []string{
-			`git "fetch" "origin" "refs/heads/master"`,
+			`git "fetch" "--depth=1" "origin" "refs/heads/master"`,
 			`git "checkout" "master"`,     // Already on 'master'
 			`git "merge" "origin/master"`, // Already up to date.
 			`git "log" "-1" "--format=%H"`,
-			`git "fetch" "origin" "refs/heads/test/commit-messages"`,
+			`git "fetch" "--depth=1" "origin" "refs/heads/test/commit-messages"`,
 			`git "merge" "76a934ae"`,
 			`git "checkout" "--detach"`,
 		},
@@ -154,11 +157,10 @@ var testCases = [...]struct {
 			Commit:      "76a934ae",
 			Branch:      "test/commit-messages",
 			BranchDest:  "master",
-			CloneDepth:  1,
 			ManualMerge: true,
 		},
 		wantCmds: []string{
-			`git "fetch" "--depth=1"`,
+			`git "fetch"`,
 			`git "checkout" "76a934ae"`,
 		},
 	},
@@ -171,14 +173,15 @@ var testCases = [...]struct {
 			BranchDest:      "master",
 			Commit:          "76a934ae",
 			ManualMerge:     true,
+			CloneDepth:      1,
 		},
 		wantCmds: []string{
-			`git "fetch" "origin" "refs/heads/master"`,
+			`git "fetch" "--depth=1" "origin" "refs/heads/master"`,
 			`git "checkout" "master"`,
 			`git "merge" "origin/master"`,
 			`git "log" "-1" "--format=%H"`,
 			`git "remote" "add" "fork" "https://github.com/bitrise-io/other-repo.git"`,
-			`git "fetch" "fork" "refs/heads/test/commit-messages"`,
+			`git "fetch" "--depth=1" "fork" "refs/heads/test/commit-messages"`,
 			`git "merge" "fork/test/commit-messages"`,
 			`git "checkout" "--detach"`,
 		},
@@ -212,10 +215,11 @@ var testCases = [...]struct {
 		cfg: Config{
 			BranchDest:    "master",
 			PRMergeBranch: "pull/5/merge",
+			CloneDepth:    1,
 		},
 		wantCmds: []string{
-			`git "fetch" "origin" "refs/heads/master"`,
-			`git "fetch" "origin" "refs/pull/5/head:pull/5"`,
+			`git "fetch" "--depth=1" "origin" "refs/heads/master"`,
+			`git "fetch" "--depth=1" "origin" "refs/pull/5/head:pull/5"`,
 			`git "checkout" "master"`,
 			`git "merge" "origin/master"`,
 			`git "merge" "pull/5"`,
@@ -280,11 +284,12 @@ var testCases = [...]struct {
 			BranchDest:    "master",
 			PRID:          7,
 			ManualMerge:   false,
+			CloneDepth:    1,
 		},
 		patchSource: MockPatchSource{"diff_path", nil},
 		wantErr:     nil,
 		wantCmds: []string{
-			`git "fetch" "origin" "refs/heads/master"`,
+			`git "fetch" "--depth=1" "origin" "refs/heads/master"`,
 			`git "checkout" "master"`,
 			`git "apply" "--index" "diff_path"`,
 			`git "checkout" "--detach"`,
@@ -373,7 +378,7 @@ var testCases = [...]struct {
 			GivenRunWithRetrySucceeds(),
 		wantCmds: []string{
 			`git "fetch" "--depth=1" "origin" "refs/heads/master"`,
-			`git "fetch" "origin" "refs/pull/5/head:pull/5"`,
+			`git "fetch" "--depth=1" "origin" "refs/pull/5/head:pull/5"`,
 			`git "checkout" "master"`,
 			`git "merge" "origin/master"`,
 			`git "merge" "pull/5"`,