Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force-checkout remote branch to fix unrelated histories #228

Merged
merged 7 commits into from
Jan 6, 2025
Merged

Force-checkout remote branch to fix unrelated histories #228

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51a2cdd
Force-checkout remote branch to resolve unrelated histories
ofalvai Dec 11, 2024
ca25547
Test fixes
ofalvai Dec 11, 2024
a7455e5
Use Println()
ofalvai Dec 11, 2024
5bb21d6
*
ofalvai Dec 13, 2024
1c64132
*
ofalvai Dec 13, 2024
85cdf57
*
ofalvai Dec 17, 2024
ae21614
go-utils update
ofalvai Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 10 additions & 20 deletions gitclone/checkout_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,31 +99,21 @@ func checkoutWithCustomRetry(gitCmd git.Git, arg string, retry fallbackRetry) er
return nil
}

func fetchInitialBranch(gitCmd git.Git, remote string, branchRef string, fetchTraits fetchOptions) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard 😵‍💫

func forceCheckoutRemoteBranch(gitCmd git.Git, remote string, branchRef string, fetchTraits fetchOptions) error {
branch := strings.TrimPrefix(branchRef, refsHeadsPrefix)
if err := fetch(gitCmd, remote, branchRef, fetchTraits); err != nil {
wErr := fmt.Errorf("failed to fetch branch (%s): %w", branchRef, err)
wErr := fmt.Errorf("fetch branch %s: %w", branchRef, err)
return fmt.Errorf("%v: %w", wErr, errors.New("please make sure the branch still exists"))
}

if err := checkoutWithCustomRetry(gitCmd, branch, nil); err != nil {
return handleCheckoutError(
Copy link
Contributor Author

@ofalvai ofalvai Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom retry here adds nicer error message (a list of existing branch names), but if a branch doesn't exist, I think it fails in the fetch step above.
I don't have a strong opinion on this, but I don't like this abstraction and I can easily say goodbye to this.
I was also thinking about adding -B to this checkout wrapper, but it only works in the context of checking out branches, while the wrapper is used for tags and commits as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have vague memories about this error handling, I think the project scanner uses it. Probably the returned branches are propagated to the UI.
I suggest checking if it is in use by the scanner, or leaving it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Unfortunately" I found references to it even in the frontend code, so this is still used: https://github.com/bitrise-io/bitrise-website/blob/2815eaa08f3d1d31bd782535dffa56c69a549dae/app/javascript/pages/AddNewApp/stages/ProjectConfig/ProjectConfig.types.ts#L11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted handleCheckoutError so the above should keep working.
checkoutWithCustomRetry is still removed, but because it was called with a nil retry function, it wasn't doing anything interesting.

listBranches(gitCmd),
checkoutFailedTag,
err,
"Checkout has failed",
branch,
)
}

// Update branch: 'git fetch' followed by a 'git merge' is the same as 'git pull'.

remoteBranch := fmt.Sprintf("%s/%s", remote, branch)
if err := runner.Run(gitCmd.Merge(remoteBranch)); err != nil {
return newStepError(
"update_branch_failed",
fmt.Errorf("updating branch (%s) failed: %w", branch, err),
"Updating branch failed",
)
// -B: create the branch if it doesn't exist, reset if it does
// The latter is important in persistent environments because shallow-fetching only fetches 1 commit,
// so the next run would see unrelated histories after shallow-fetching another single commit.
out, err := runner.RunForOutput(command.New("git", "checkout", "-B", branch, remoteBranch))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use gitCmd.Checkout("-B", branch, remoteBranch) instead of a raw command? The reason is that gitCmd is initialised with a workadir, the command here only works if pwd = repo root.

(gitCmd.Checkout accepts a single argumentum, we might update to a variadic function)

if err != nil {
fmt.Printf(out)
return fmt.Errorf("checkout remote-tracking branch %s: %w", remoteBranch, err)
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion gitclone/checkout_method_pr_manual.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type checkoutPRManualMerge struct {
func (c checkoutPRManualMerge) do(gitCmd git.Git, fetchOptions fetchOptions, fallback fallbackRetry) error {
// Fetch and checkout destinations branch
destBranchRef := refsHeadsPrefix + c.params.DestinationBranch
if err := fetchInitialBranch(gitCmd, originRemoteName, destBranchRef, fetchOptions); err != nil {
if err := forceCheckoutRemoteBranch(gitCmd, originRemoteName, destBranchRef, fetchOptions); err != nil {
return fmt.Errorf("failed to fetch base branch: %w", err)
}

Expand Down
2 changes: 1 addition & 1 deletion gitclone/checkout_method_simple.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ type checkoutBranch struct {
}

func (c checkoutBranch) do(gitCmd git.Git, fetchOptions fetchOptions, _ fallbackRetry) error {
if err := fetchInitialBranch(gitCmd, originRemoteName, c.localRef(), fetchOptions); err != nil {
if err := forceCheckoutRemoteBranch(gitCmd, originRemoteName, c.localRef(), fetchOptions); err != nil {
return err
}

Expand Down
16 changes: 6 additions & 10 deletions gitclone/gitclone_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ func Test_checkoutState(t *testing.T) {
},
wantCmds: []string{
`git "fetch" "--jobs=10" "--depth=1" "--no-tags" "--no-recurse-submodules" "origin" "refs/heads/hcnarb"`,
`git "checkout" "hcnarb"`,
`git "merge" "origin/hcnarb"`,
`git "checkout" "-B" "hcnarb" "origin/hcnarb"`,
},
},
{
Expand Down Expand Up @@ -204,7 +203,7 @@ func Test_checkoutState(t *testing.T) {
`git "fetch" "--jobs=10"`,
`git "branch" "-r"`,
},
wantErr: fmt.Errorf("failed to fetch base branch: failed to fetch branch (refs/heads/master): dummy_cmd_error: please make sure the branch still exists"),
wantErr: fmt.Errorf("failed to fetch base branch: fetch branch refs/heads/master: dummy_cmd_error: please make sure the branch still exists"),
},
{
name: "PR - fork - no merge ref - diff file available",
Expand Down Expand Up @@ -245,8 +244,7 @@ func Test_checkoutState(t *testing.T) {
`git "checkout" "master"`,
`git "apply" "--index" "diff_path"`,
`git "fetch" "--jobs=10" "--depth=1" "--no-tags" "--no-recurse-submodules" "origin" "refs/heads/master"`,
`git "checkout" "master"`,
`git "merge" "origin/master"`,
`git "checkout" "-B" "master" "origin/master"`,
`git "log" "-1" "--format=%H"`,
`git "fetch" "--jobs=10" "--depth=1" "--no-tags" "--no-recurse-submodules" "origin" "refs/heads/test/commit-messages"`,
`git "merge" "76a934ae"`,
Expand All @@ -273,8 +271,7 @@ func Test_checkoutState(t *testing.T) {
`git "checkout" "master"`,
`git "apply" "--index" "diff_path"`,
`git "fetch" "--jobs=10" "--depth=1" "--no-tags" "--no-recurse-submodules" "origin" "refs/heads/master"`,
`git "checkout" "master"`,
`git "merge" "origin/master"`,
`git "checkout" "-B" "master" "origin/master"`,
`git "log" "-1" "--format=%H"`,
`git "remote" "add" "fork" "git@github.com:bitrise-io/other-repo.git"`,
`git "fetch" "--jobs=10" "--depth=1" "--no-tags" "--no-recurse-submodules" "fork" "refs/heads/test/commit-messages"`,
Expand Down Expand Up @@ -375,7 +372,7 @@ func Test_checkoutState(t *testing.T) {
},
wantErr: newStepErrorWithBranchRecommendations(
fetchFailedTag,
fmt.Errorf("failed to fetch branch (refs/heads/fake): %w: please make sure the branch still exists", errors.New(rawCmdError)),
fmt.Errorf("fetch branch refs/heads/fake: %w: please make sure the branch still exists", errors.New(rawCmdError)),
"Fetching repository has failed",
"fake",
[]string{"master"},
Expand Down Expand Up @@ -466,8 +463,7 @@ func Test_checkoutState(t *testing.T) {
},
wantCmds: []string{
`git "fetch" "--jobs=10" "--depth=1" "--filter=tree:0" "--no-tags" "--no-recurse-submodules" "origin" "refs/heads/hcnarb"`,
`git "checkout" "hcnarb"`,
`git "merge" "origin/hcnarb"`,
`git "checkout" "-B" "hcnarb" "origin/hcnarb"`,
},
},
{
Expand Down