Skip to content

Commit

Permalink
Implement fallback to manual merge upon failing on apply (#139)
Browse files Browse the repository at this point in the history
* 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.

* Manual merge params added PRDiff

* Set manual merge params to PRDiff param

* Fallback implemented for E2E testing

* Fetch and merge param data holder type created for fetchAndMerge

* Fallback logic extacted and simplified:
:

* Tests implemented:

* TODO comments removed from tests

* Commit added to diff testing WF

* Review fixes

* Unwanted &s removed

* Another & removed

* Leftover & removed

* Merge issues fixed

* Commit hash replaced in bitrise.yml

* Formatting issue fixed

* Formatting issue really fixed

Co-authored-by: Krisztián Gödrei <krisztian.godrei@bitrise.io>
istvankovacs-bitrise and godrei authored Mar 12, 2021
1 parent 9ccf039 commit 1dda0a4
Showing 11 changed files with 165 additions and 22 deletions.
2 changes: 1 addition & 1 deletion bitrise.yml
Original file line number Diff line number Diff line change
@@ -635,7 +635,7 @@ workflows:
- pull_request_merge_branch: ""
- branch_dest: "master"
- branch: "master"
- commit: ""
- commit: "c6810e6"
- tag: ""
- clone_depth: "1"
- manual_merge: "no"
16 changes: 15 additions & 1 deletion gitclone/checkout.go
Original file line number Diff line number Diff line change
@@ -156,7 +156,12 @@ 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)
prManualMergeParam, forkPRManualMergeParam, err := createManualMergeParams(cfg)
if err != nil {
return nil, err
}

params, err := NewPRDiffFileParams(cfg.BranchDest, prManualMergeParam, forkPRManualMergeParam)
if err != nil {
return nil, err
}
@@ -228,3 +233,12 @@ func selectFallbacks(checkoutStrategy CheckoutMethod, fetchOpts fetchOptions) fa
return nil
}
}

func createManualMergeParams(cfg Config) (prManualMergeParam *PRManualMergeParams, forkPRManualMergeParam *ForkPRManualMergeParams, err error) {
if isFork(cfg.RepositoryURL, cfg.PRRepositoryURL) {
forkPRManualMergeParam, err = NewForkPRManualMergeParams(cfg.Branch, cfg.PRRepositoryURL, cfg.BranchDest)
} else {
prManualMergeParam, err = NewPRManualMergeParams(cfg.Branch, cfg.Commit, cfg.BranchDest)
}
return
}
22 changes: 21 additions & 1 deletion gitclone/checkout_helper.go
Original file line number Diff line number Diff line change
@@ -10,6 +10,17 @@ import (
"github.com/bitrise-io/go-utils/log"
)

type fetchParams struct {
branch string
remote string
options fetchOptions
}

type mergeParams struct {
arg string
fallback fallbackRetry
}

type fetchOptions struct {
// Sets '--tags' flag
// From https://git-scm.com/docs/fetch-options/2.29.0#Documentation/fetch-options.txt---allTags:
@@ -94,7 +105,7 @@ func fetchInitialBranch(gitCmd git.Git, remote string, branchRef string, fetchTr
}

// Update branch: 'git fetch' followed by a 'git merge' is the same as 'git pull'.
remoteBranch := fmt.Sprintf("%s/%s", defaultRemoteName, branch)
remoteBranch := fmt.Sprintf("%s/%s", originRemoteName, branch)
if err := runner.Run(gitCmd.Merge(remoteBranch)); err != nil {
return newStepError(
"update_branch_failed",
@@ -123,6 +134,15 @@ func mergeWithCustomRetry(gitCmd git.Git, arg string, retry fallbackRetry) error
return nil
}

func fetchAndMerge(gitCmd git.Git, fetchParam fetchParams, mergeParam mergeParams) error {
headBranchRef := branchRefPrefix + fetchParam.branch
if err := fetch(gitCmd, fetchParam.remote, headBranchRef, fetchParam.options); err != nil {
return nil
}

return mergeWithCustomRetry(gitCmd, mergeParam.arg, mergeParam.fallback)
}

func detachHead(gitCmd git.Git) error {
if err := runner.Run(gitCmd.Checkout("--detach")); err != nil {
return newStepError(
69 changes: 64 additions & 5 deletions gitclone/checkout_method_pr_auto_diff.go
Original file line number Diff line number Diff line change
@@ -9,23 +9,32 @@ import (

"github.com/bitrise-io/go-steputils/input"
"github.com/bitrise-io/go-utils/command/git"
"github.com/bitrise-io/go-utils/log"
"github.com/bitrise-steplib/bitrise-step-export-universal-apk/filedownloader"
)

//
// PRDiffFileParams are parameters to check out a Merge/Pull Request (when a diff file is available)
type PRDiffFileParams struct {
BaseBranch string
BaseBranch string
PRManualMergeParam *PRManualMergeParams
ForkPRManualMergeParam *ForkPRManualMergeParams
}

// NewPRDiffFileParams validates and returns a new PRDiffFileParams
func NewPRDiffFileParams(baseBranch string) (*PRDiffFileParams, error) {
func NewPRDiffFileParams(
baseBranch string,
prManualMergeParam *PRManualMergeParams,
forkPRManualMergeParam *ForkPRManualMergeParams,
) (*PRDiffFileParams, error) {
if strings.TrimSpace(baseBranch) == "" {
return nil, NewParameterValidationError("PR diff file based checkout strategy can not be used: no base branch specified")
}

return &PRDiffFileParams{
BaseBranch: baseBranch,
BaseBranch: baseBranch,
PRManualMergeParam: prManualMergeParam,
ForkPRManualMergeParam: forkPRManualMergeParam,
}, nil
}

@@ -37,7 +46,7 @@ type checkoutPRDiffFile struct {

func (c checkoutPRDiffFile) do(gitCmd git.Git, fetchOptions fetchOptions, fallback fallbackRetry) error {
baseBranchRef := branchRefPrefix + c.params.BaseBranch
if err := fetch(gitCmd, defaultRemoteName, baseBranchRef, fetchOptions); err != nil {
if err := fetch(gitCmd, originRemoteName, baseBranchRef, fetchOptions); err != nil {
return err
}

@@ -46,7 +55,16 @@ func (c checkoutPRDiffFile) do(gitCmd git.Git, fetchOptions fetchOptions, fallba
}

if err := runner.Run(gitCmd.Apply(c.patchFile)); err != nil {
return fmt.Errorf("could not apply patch (%s): %v", c.patchFile, err)
log.Warnf("Could not apply patch (%s): %v", c.patchFile, err)
log.Warnf("Falling back to manual merge...")

return fallbackToManualMergeOnApplyError(
gitCmd,
c.params.PRManualMergeParam,
c.params.ForkPRManualMergeParam,
fetchOptions,
fallback,
)
}

return detachHead(gitCmd)
@@ -72,3 +90,44 @@ func (defaultPatchSource) getDiffPath(buildURL, apiToken string) (string, error)
fileProvider := input.NewFileProvider(filedownloader.New(http.DefaultClient))
return fileProvider.LocalPath(diffURL)
}

func fallbackToManualMergeOnApplyError(
gitCmd git.Git,
prManualMergeParam *PRManualMergeParams,
forkPRManualMergeParam *ForkPRManualMergeParams,
fetchOptions fetchOptions,
fallback fallbackRetry,
) error {
var fetchParam fetchParams
var mergeParam mergeParams

if prManualMergeParam != nil {
fetchParam = fetchParams{
branch: prManualMergeParam.HeadBranch,
remote: originRemoteName,
options: fetchOptions,
}

mergeParam = mergeParams{
arg: prManualMergeParam.Commit,
fallback: fallback,
}
} else if forkPRManualMergeParam != nil {
if err := runner.Run(gitCmd.RemoteAdd(forkRemoteName, forkPRManualMergeParam.HeadRepoURL)); err != nil {
return fmt.Errorf("adding remote fork repository failed (%s): %v", forkPRManualMergeParam.HeadRepoURL, err)
}

fetchParam = fetchParams{
branch: forkPRManualMergeParam.HeadBranch,
remote: forkRemoteName,
options: fetchOptions,
}

mergeParam = mergeParams{
arg: fmt.Sprintf("%s/%s", forkRemoteName, forkPRManualMergeParam.HeadBranch),
fallback: fallback,
}
}

return fetchAndMerge(gitCmd, fetchParam, mergeParam)
}
6 changes: 3 additions & 3 deletions gitclone/checkout_method_pr_auto_merge_branch.go
Original file line number Diff line number Diff line change
@@ -38,13 +38,13 @@ func (c checkoutPRMergeBranch) do(gitCmd git.Git, fetchOpts fetchOptions, fallba
// Check out initial branch (fetchInitialBranch part1)
// `git "fetch" "origin" "refs/heads/master"`
baseBranchRef := branchRefPrefix + c.params.BaseBranch
if err := fetch(gitCmd, defaultRemoteName, baseBranchRef, fetchOpts); err != nil {
if err := fetch(gitCmd, originRemoteName, baseBranchRef, fetchOpts); err != nil {
return err
}

// `git "fetch" "origin" "refs/pull/7/head:pull/7"`
headBranchRef := fetchArg(c.params.MergeBranch)
if err := fetch(gitCmd, defaultRemoteName, headBranchRef, fetchOpts); err != nil {
if err := fetch(gitCmd, originRemoteName, headBranchRef, fetchOpts); err != nil {
return err
}

@@ -54,7 +54,7 @@ func (c checkoutPRMergeBranch) do(gitCmd git.Git, fetchOpts fetchOptions, fallba
if err := checkoutWithCustomRetry(gitCmd, c.params.BaseBranch, nil); err != nil {
return err
}
remoteBaseBranch := fmt.Sprintf("%s/%s", defaultRemoteName, c.params.BaseBranch)
remoteBaseBranch := fmt.Sprintf("%s/%s", originRemoteName, c.params.BaseBranch)
if err := runner.Run(gitCmd.Merge(remoteBaseBranch)); err != nil {
return err
}
6 changes: 3 additions & 3 deletions gitclone/checkout_method_pr_manual.go
Original file line number Diff line number Diff line change
@@ -44,7 +44,7 @@ type checkoutPRManualMerge struct {
func (c checkoutPRManualMerge) do(gitCmd git.Git, fetchOptions fetchOptions, fallback fallbackRetry) error {
// Fetch and checkout base (target) branch
baseBranchRef := branchRefPrefix + c.params.BaseBranch
if err := fetchInitialBranch(gitCmd, defaultRemoteName, baseBranchRef, fetchOptions); err != nil {
if err := fetchInitialBranch(gitCmd, originRemoteName, baseBranchRef, fetchOptions); err != nil {
return err
}

@@ -56,7 +56,7 @@ func (c checkoutPRManualMerge) do(gitCmd git.Git, fetchOptions fetchOptions, fal

// Fetch and merge
headBranchRef := branchRefPrefix + c.params.HeadBranch
if err := fetch(gitCmd, defaultRemoteName, headBranchRef, fetchOptions); err != nil {
if err := fetch(gitCmd, originRemoteName, headBranchRef, fetchOptions); err != nil {
return nil
}

@@ -103,7 +103,7 @@ type checkoutForkPRManualMerge struct {
func (c checkoutForkPRManualMerge) do(gitCmd git.Git, fetchOptions fetchOptions, fallback fallbackRetry) error {
// Fetch and checkout base branch
baseBranchRef := branchRefPrefix + c.params.BaseBranch
if err := fetchInitialBranch(gitCmd, defaultRemoteName, baseBranchRef, fetchOptions); err != nil {
if err := fetchInitialBranch(gitCmd, originRemoteName, baseBranchRef, fetchOptions); err != nil {
return err
}

6 changes: 3 additions & 3 deletions gitclone/checkout_method_simple.go
Original file line number Diff line number Diff line change
@@ -44,7 +44,7 @@ func (c checkoutCommit) do(gitCmd git.Git, fetchOptions fetchOptions, fallback f
branchRefParam = branchRefPrefix + c.params.Branch
}

if err := fetch(gitCmd, defaultRemoteName, branchRefParam, fetchOptions); err != nil {
if err := fetch(gitCmd, originRemoteName, branchRefParam, fetchOptions); err != nil {
return err
}

@@ -79,7 +79,7 @@ type checkoutBranch struct {

func (c checkoutBranch) do(gitCmd git.Git, fetchOptions fetchOptions, _ fallbackRetry) error {
branchRef := branchRefPrefix + c.params.Branch
if err := fetchInitialBranch(gitCmd, defaultRemoteName, branchRef, fetchOptions); err != nil {
if err := fetchInitialBranch(gitCmd, originRemoteName, branchRef, fetchOptions); err != nil {
return err
}

@@ -116,7 +116,7 @@ func (c checkoutTag) do(gitCmd git.Git, fetchOptions fetchOptions, fallback fall
branchRefParam = branchRefPrefix + c.params.Branch
}

if err := fetch(gitCmd, defaultRemoteName, branchRefParam, fetchOptions); err != nil {
if err := fetch(gitCmd, originRemoteName, branchRefParam, fetchOptions); err != nil {
return err
}

2 changes: 1 addition & 1 deletion gitclone/git.go
Original file line number Diff line number Diff line change
@@ -157,7 +157,7 @@ func handleCheckoutError(callback getAvailableBranches, tag string, err error, s
// We were checking out a branch (not tag or commit)
if branch != "" {
branchesByRemote, branchesErr := callback()
branches := branchesByRemote[defaultRemoteName]
branches := branchesByRemote[originRemoteName]
// There was no error grabbing the available branches
// And the current branch is not present in the list
if branchesErr == nil && !sliceutil.IsStringInSlice(branch, branches) {
4 changes: 2 additions & 2 deletions gitclone/git_test.go
Original file line number Diff line number Diff line change
@@ -141,7 +141,7 @@ func Test_handleCheckoutError(t *testing.T) {
args: args{
callback: func() (map[string][]string, error) {
return map[string][]string{
defaultRemoteName: {"master", "develop"},
originRemoteName: {"master", "develop"},
}, nil
},
tag: "checkout_failed",
@@ -192,7 +192,7 @@ func Test_handleCheckoutError(t *testing.T) {
args: args{
callback: func() (map[string][]string, error) {
return map[string][]string{
defaultRemoteName: {"master", "develop", "test"},
originRemoteName: {"master", "develop", "test"},
}, nil
},
tag: "checkout_failed",
5 changes: 3 additions & 2 deletions gitclone/gitclone.go
Original file line number Diff line number Diff line change
@@ -32,8 +32,9 @@ type Config struct {

const (
trimEnding = "..."
defaultRemoteName = "origin"
originRemoteName = "origin"
updateSubmodelFailedTag = "update_submodule_failed"
forkRemoteName = "fork"
)

func printLogAndExportEnv(gitCmd git.Git, format, env string, maxEnvLength int) error {
@@ -130,7 +131,7 @@ func Execute(cfg Config) error {
)
}
if !originPresent {
if err := runner.Run(gitCmd.RemoteAdd(defaultRemoteName, cfg.RepositoryURL)); err != nil {
if err := runner.Run(gitCmd.RemoteAdd(originRemoteName, cfg.RepositoryURL)); err != nil {
return newStepError(
"add_remote_failed",
fmt.Errorf("adding remote repository failed (%s): %v", cfg.RepositoryURL, err),
49 changes: 49 additions & 0 deletions gitclone/gitclone_test.go
Original file line number Diff line number Diff line change
@@ -284,6 +284,7 @@ var testCases = [...]struct {
BranchDest: "master",
PRID: 7,
ManualMerge: false,
Commit: "76a934ae",
CloneDepth: 1,
},
patchSource: MockPatchSource{"diff_path", nil},
@@ -295,6 +296,54 @@ var testCases = [...]struct {
`git "checkout" "--detach"`,
},
},
{
name: "PR - no fork - auto merge - diff file: fallback to manual merge if unable to apply patch",
cfg: Config{
RepositoryURL: "https://github.com/bitrise-io/git-clone-test.git",
Branch: "test/commit-messages",
BranchDest: "master",
PRID: 7,
ManualMerge: false,
Commit: "76a934ae",
CloneDepth: 1,
},
patchSource: MockPatchSource{"diff_path", nil},
mockRunner: givenMockRunner().
GivenRunFailsForCommand(`git "apply" "--index" "diff_path"`, 1).
GivenRunWithRetrySucceeds().
GivenRunSucceeds(),
wantCmds: []string{
`git "fetch" "--depth=1" "origin" "refs/heads/master"`,
`git "checkout" "master"`,
`git "apply" "--index" "diff_path"`,
`git "fetch" "--depth=1" "origin" "refs/heads/test/commit-messages"`,
`git "merge" "76a934ae"`,
},
},
{
name: "PR - fork - auto merge - diff file: fallback to manual merge if unable to apply patch",
cfg: Config{
RepositoryURL: "https://github.com/bitrise-io/git-clone-test.git",
PRRepositoryURL: "git@github.com:bitrise-io/other-repo.git",
Branch: "test/commit-messages",
BranchDest: "master",
Commit: "76a934ae",
ManualMerge: true,
},
patchSource: MockPatchSource{"diff_path", nil},
mockRunner: givenMockRunner().
GivenRunFailsForCommand(`git "apply" "--index" "diff_path"`, 1).
GivenRunWithRetrySucceeds().
GivenRunSucceeds(),
wantCmds: []string{
`git "fetch" "origin" "refs/heads/master"`,
`git "checkout" "master"`,
`git "apply" "--index" "diff_path"`,
`git "remote" "add" "fork" "git@github.com:bitrise-io/other-repo.git"`,
`git "fetch" "fork" "refs/heads/test/commit-messages"`,
`git "merge" "fork/test/commit-messages"`,
},
},

// ** Errors **
{

0 comments on commit 1dda0a4

Please sign in to comment.