Skip to content

Conversation

@halms
Copy link
Contributor

@halms halms commented Mar 18, 2024

Context: Slack

The pull_request target creates a (fake) merge commit that fails the branch linearity check.

This change tries to detect if the commit at the tip is a merge commit against the target branch.
If that is the case, then it ignores it.

Tried it a bit locally and appears to work, but no guarantees for it 😅

+ a small change to make it nicer to use as a direct shell script

@halms halms force-pushed the chore/ignore-merge-at-tip branch from c61731f to 0402474 Compare March 18, 2024 20:03
@halms halms marked this pull request as ready for review March 18, 2024 20:05
@halms halms requested a review from yannrouillard March 19, 2024 09:40
@halms halms force-pushed the chore/ignore-merge-at-tip branch from 0402474 to efbabe3 Compare March 19, 2024 09:41
# Detection of (PR) merge commits at tip of HEAD:
target_sha=$(git rev-parse origin/${TARGET_BRANCH})
# Get SHA of parent commit
merge_target=$(git log --merges --oneline --parents --no-abbrev-commit HEAD^..HEAD | cut -d" " -f2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that if the last commit is a merge commit and has the head of the target branch as the left child, you will ignore it (consider it's the GitHub generated one) and check for merge commit in the below commits.

It's not clear to me if this approach works if someone did the same thing in his/her branch that GitHub CI does, i.e. he/she merged the main branch into its development branch, which we also wouldn't want in case of linearity check.

I tried locally and the check worked, but it was because the origin/main commit was the right child of the merge commit and not the left child.

Are you sure testing always the left child will achieve the expected results?
Unfortunately, I was not able to test it in a real PR, I had some difficulties reproducing the issue.

BTW I thought initially to use something more GitHub specific, using for instance the environment variable exported.
The GITHUB_SHA contains the tip commit (which will be the merge commit if one was created)
and GITHUB_HEAD_REF contains the source branch name (and is only defined in PR context)

If the 2 don't point to the same thing, we are probably in a GitHub merge commit situation.
Or simpler, if GITHUB_HEAD_REF is non-empty, we do git rev-parse ${GITHUB_HEAD_REF} to get the tip commit to use.

BTW I found this page informative about the process: https://www.kenmuse.com/blog/the-many-shas-of-a-github-pull-request/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I think it will work because locally the left child is the target of the merge (ours) and the right side what is merged into the target (theirs).
A local merge commit will merge main into the current branch, so it will end up on the right.
Whereas GitHub create a new branch from the base commit and merge the pull request branch into it so the origin/main commit will be on the left.

I still think using GITHUB_HEAD_REF would be simpler but as you want :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took me a while (and needed some calm time) to wholly follow through your points.
I have to say, I did not think at all about any GH-specific variables, only about the git history itself. But what you suggested makes a lot of sense.

So the case you were concerned about is if we have a merge commit from main into the dev branch (what we don't want to allow) at the tip of the history.
But this is detected correctly because it has the left and right parent commits inversed.

In any case, yes, I think using the GITHUB_HEAD_REF is a good idea here. And ignoring it if it's empty.
We could even compare SHAs of rev-parse $GITHUB_HEAD_REF and HEAD^ and if they match assume we are in a GH merge commit scenario and start the history check from that SHA?

Copy link
Contributor

@yannrouillard yannrouillard left a comment

Choose a reason for hiding this comment

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

I think we can simplify the check for GitHub case and be more sure it works in all cases.
See my comment.

@halms halms force-pushed the chore/ignore-merge-at-tip branch 7 times, most recently from d87c44f to 758be91 Compare August 15, 2025 06:57
@halms halms force-pushed the chore/ignore-merge-at-tip branch from 758be91 to cbfbd4a Compare August 15, 2025 07:16
@halms
Copy link
Contributor Author

halms commented Aug 18, 2025

I recently stumbled across this and picked it up again, building on top of GITHUB_HEAD_REF.
Took a bit of trial and error to have both happy case and case to capture work both locally and on GH (in PR context).

Aimed to not have to fetch full git history, but only the shallow parts that are needed.

Tested runs in a temp branch in another repo: happy case, merge caught.

@halms halms requested a review from yannrouillard August 18, 2025 11:25
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.

3 participants