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

Checkout for PR is not simulating a merge with HEAD of main #696

Open
grzegorzkrukowski opened this issue Feb 14, 2022 · 2 comments
Open

Comments

@grzegorzkrukowski
Copy link

grzegorzkrukowski commented Feb 14, 2022

I am running into some issues, because checkout action is doing a merge of PR into wrong SHA.

When running checkout on this repository:
https://github.com/raycast/extensions

You can see unwanted behaviour here:
https://github.com/raycast/extensions/runs/5185777181?check_suite_focus=true#step:2:486

It merged the HEAD of PULLREQUEST with some random commit from main - cd113f35.
It looks like this visually:

Red arrow is how the merge is done now, green is how I would expect it to work.

CleanShot 2022-02-14 at 18 26 51@2x

What I think should be happening here is that PR should be done between 4706f01c76ef11c4220e9f81e99e13cc58e80642 and de0735da937fb75489043e4ed89e5d96a6b4dbeb.

This is maybe because this PR is coming from a forked repository.

@millerick
Copy link

We've observed similar when rerunning jobs. It uses the same merge commit that was present when the job was originally run, as opposed to a more up to date merge commit.

@polarathene
Copy link

polarathene commented Jun 28, 2022

Check the github context ${{ github.event.pull_request.base.sha }} in future, this should be the commit SHA of the latest commit on the base branch (eg main) when the PR is opened, not the commit the PR originally branched from.

The default checkout by actions/checkout has a test merge commit when the PR is updated (though it seems to always be against the latest commit of the base branch, not the mentioned commit SHA from the github context object).

EDIT: I'm mistaken, as @millerick pointed out, this is not the case with workflow job re-runs. It seems that the test merge commit (github.sha on github.ref?) is a cached value like the base SHA; it is probably updated at the same time when the PR branch history is.


Types of merging for branches (unrelated to action)

Merge Commit - History of PR commits retained (distinguishable from other commits in branch)

When it comes to merging the PR, my understanding is that a merge commit itself is empty and the history of your commits on the base branch can be spread out based on the date of each commit (I did this to retain history over the years of our GIthub Wiki when migrating it's commits into the main repo).

So it'd make no difference where the merge commit itself was placed if your associated commits from the PR will always be in the same place. Only if there was a conflict from newer commits, would that need to be resolved, and thus newer commits in the PR history would have addressed that (bumping up the merge commit itself).

Rebase - Merge by rebasing commits (no history interleaving, no merge commit, not distinguishable as it's own subset of commits in the branch)

If you do not want the merge commit in your history, and want your PR commits added on to the top of the base branch head, then you need to change the merge option for your PRs from a "merge commit" to "rebase".

"Rebase" will rebase the PR history on top of the base branch history, no merge commit will be added. The extra line in your screenshot showing all those commits belonging together would be lost, and it'd appear like above all on the blue line (no distinctive grouping of the PR commit history, where the commits started and ended, just commits).

Squash - Merge by squashing all commits into 1 (with reference to PR page)

If neither of those sound appealing, you may prefer the "squash commit" type of merge. This is my preference. It will combine all the PR commits into a single commit which is added to the top of your base branch.

The commit has the full diff and by default the commit title should include a reference link to your PR (useful for git blame / traceability). The PR on Github remains accessible, and the individual commit history it originally had, even if the contributor chooses to delete their branch.

With the squash commit, you generally get a cleaner commit history, but git blame is less useful if the PRs are large (especially if you can't clone the PR branch because it was deleted).


Action behaviour

We've observed similar when rerunning jobs. It uses the same merge commit that was present when the job was originally run, as opposed to a more up to date merge commit.

Just tried that and was surprised that happened. (UPDATE: It seems like the merge commit is cached, possibly to make re-runs more deterministic)

So...

  • The base SHA is the latest commit of the base branch when the PR was opened, or when a merge commit of the base branch into the PR branch occurs.
  • The test merge commit will be against the latest commit of the base branch (at the time of the last push to the PR branch)? Thus not in sync with what the base SHA references. UPDATE: It seems like the context for each commit is cached (the merge commit has the same SHA as what ${{ github.sha }} context provides).
  • The merge commit of the PR branch into the base branch for @grzegorzkrukowski is a normal merge commit as expected. The date of the merge commit matches the date of the last commit in the PR, while the view is showing all commits belonging to that merge commit (if viewing linear history instead, the older commits will appear spread out and interleaved through other commits in that main base branch).

UPDATE: I tested this with another commit pushed to the PR branch too and re-running the workflow job from the previous commit. The job continued to use the outdated base branch HEAD commit from when that commit was pushed (thus cached, not generating a new/different merge commit).

I also noticed that other Github context was also cached for that commit at it's time. Such as the number of commits the PR has. That caused my workflow to fail as it lacked the fetch-depth needed (old commit count context, but fetching that amount from the latest commit history from the head branch ref). The context for the head.sha remained unchanged, but I had fetched with ref: ${{ github.event.pull_request.head.ref }} which is probably why the job was not deterministic (cached context, but fetching by ref is non-deterministic).

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

No branches or pull requests

3 participants