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.

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

Clear remote ref with auto merge #219

Merged
merged 10 commits into from
Feb 23, 2024
Merged

Clear remote ref with auto merge #219

merged 10 commits into from
Feb 23, 2024

Conversation

lpusok
Copy link
Contributor

@lpusok lpusok commented Feb 16, 2024

Checklist

  • I've read and followed the Contribution Guidelines
  • step.yml and README.md is updated with the changes (if needed)

Version

Requires a PATCH version update

Context

When the git checkout directory is persisted between runs, we can encounter the following;

$ git "fetch" "--jobs=10" "--depth=1" "--no-tags" "origin" "refs/pull/7/merge:refs/remotes/pull/7/merge"
From github.com:bitrise-io/my-repo
! [rejected] refs/pull/2313/merge -> pull/2313/merge (non-fast-forward)
This is caused by the remote merge and head branches being "force-pushed" by GitHub.
To solve it we remove merge and head branch refs.
$ git update-ref -d refs/remotes/pull/7/merge

Changes

Investigation details

Decisions

@lpusok lpusok marked this pull request as ready for review February 19, 2024 08:55
@lpusok lpusok requested a review from ofalvai February 19, 2024 09:00
@@ -162,3 +162,10 @@ func (g *Git) SparseCheckoutSet(opts ...string) *command.Model {
args = append(args, opts...)
return g.command(args...)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How did this update if there is no go.sum change? Is the vendor folder consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with correct vendor updates.

@@ -40,8 +40,15 @@ func (c checkoutPRMergeRef) do(gitCmd git.Git, fetchOpts fetchOptions, fallback
// https://git-scm.com/book/en/v2/Git-Internals-The-Refspec
refSpec := fmt.Sprintf("%s:%s", c.remoteMergeRef(), c.localMergeRef())

// delete remote branch ref
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining why we need this? It's not trivial and no one will remember in a year

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment.

// $ git update-ref -d refs/remotes/pull/7/merge
err := deleteRef(gitCmd, c.remoteMergeRef())
if err != nil {
return fmt.Errorf("failed to delte ref: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The doc says that it first verifies that the ref exists before deleting it.. Which means we can't just run it all the time, it's going to bubble up the error if there is nothing to clean up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested on macos/linux, the git command does not fail.

@lpusok lpusok requested a review from ofalvai February 22, 2024 15:42
@lpusok lpusok merged commit 51ad770 into master Feb 23, 2024
1 check passed
@lpusok lpusok deleted the BE-1288 branch February 23, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants