Detect squash and rebase merge methods #399
Conversation
Signed-off-by: Joshua Schmid <jaiks@posteo.de>
jschmid1
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
4dc6132
to
3954941
Compare
|
Thanks @jschmid1 this looks very useful. I'll review it in the coming days 🙇 |
There was a problem hiding this comment.
Amazing contribution @jschmid1 🚀
👍 I really like the idea of checking whether a commit is associated with the pull request. I wasn't aware this API existed.
❌ Currently, this pull request breaks behavior for some scenarios like merging with a merge commit (see comment below). We'll need to improve the merge method detection.
🔧 As this logic requires multiple additional requests to GH's API which could lower performance and lead to 429 Too Many Requests; and because it changes behavior for squashed pull requests, I'd like to place this functionality behind a feature flag. I was thinking about something like features: ['cherrypick_squashed_commit'], experimental: ['detect_merge_method'], or something similar that can be expected and removed easily without being considered a breaking change.
🙇 Let me know if you need any assistance.
Signed-off-by: Joshua Schmid <jaiks@posteo.de>
jschmid1
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
f626f14
to
3e74f93
Compare
jschmid1
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
2 times, most recently
from
edf37b4
to
24f9501
Compare
jschmid1
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
24f9501
to
b0c7519
Compare
|
Thanks for the changes @jschmid1. I'll try my best to give a review of your changes again soon. In the meantime, we still need to hide this behind a feature flag. Would you be willing to add that to the PR as well? |
Signed-off-by: Joshua Schmid <jaiks@posteo.de>
jschmid1
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
f458a24
to
11f890c
Compare
korthout
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
ea686db
to
2d809b5
Compare
|
Today, I spent some more time on this. Rather than a back-and-forth, I chose to commit some changes instead. I hope that's okay with you. First, I replaced the I also believe that I found a way to get the rebased commits in a performant way. I still need to do some testing on this, but the approach looks promising. @jschmid1 Please have a look at the changes. I tried to explain them in the respective commits. I'm curious to hear your thoughts about them. 🙇 |
|
I've performed some e2e tests with experimental
@jschmid1 If you're okay with the current approach, I'll merge and release this. |
|
Looks good. Thanks for taking care! |
The experimental input allows more flexibility when adding new experimental features, as well as the ability to remove them again without breaking things. For unknown specified experimental properties, the action can log warnings. The input property also clarifies to users that this concerns an experimental feature, meaning that bugs may be present or that the feature breaks user space when enabled. The spread operator can be used quite simply to set some defaults and overwrite them.
The merge commit sha was requested in the mergeStrategy method to determine the merge strategy, and when the strategy is squashed, it would retrieve the same sha again. We can avoid one of these calls to the GH API by requesting this value early. We need anyways to continue determining the method strategy. This way, we can also reuse the value for the rebased strategy when we implement a different picking approach there. Note that the merge_commit_sha can be either a string or null, but when we've determined that it's a squashed merge, we know that the merge_commit_sha is set. So we can use the ! (non-null assertion operator) to instruct typescript that it's really just a string at this point.
I believe this functionality makes most sense to users. If a rebase is detected they expect the rebased commits to be cherry-picked rather than the commits from the pull request. To find these commits, we can use git log over a range of commits. Starting from the merge_commit_sha (which is the commit that the base branch was updated to) we want to cherry-pick n number of commits. Let's say there are 3 commits on the base branch after merging the pull request: * abc * def * ghi * .. We can find these three commits by traversing all commits from abc excluding any commits reachable from the parent of ghi. In other words we can do `git log ^ghi^ abc`. Note that we don't know the sha of ghi nor its parent, but we know how to reach it. We can find the parent of ghi as abc~3. This means we can do: `git log ^abc~3 abc` for which the shorthand notation is `git log abc~3..abc`. Note that the number 3 can be replaced by the number of commits on the pull request. To do this, we must ensure that these commits are known locally. So we must fetch these commits (and +1 in case we're in a shallowly cloned repo).
In order to cherry-pick the merge_commit_sha it must be available. I think that missing this was the reason for the increased fetch of the target. But it should not be necessary. So I've reverted that change.
When the user specifies unknown properties in the experimental input, then we should inform them about it. It does not have to stop the action from working, but a logged warning makes sense.
When a commit is fetched directly by sha, it is only tracked by FETCH_HEAD. However, FETCH_HEAD gets overwritten in subsequent calls. To ensure we store the fetched commit, we must specify a refspec in the form of `+src:dst`. If <dst> is not an empty string, an attempt is made to update the local ref that matches it. To find a unique ref to use as dst we can use the src sha. It may be that this requires --force but I think we should try without first.
When finding the shas in a range, the resulting output appears to have doublequotes surrounding each of the shas. We must remove these to make them usable.
In order to skip merge commits, we check for merge commits among the commits to cherry pick. Now that those have changed due to squash/rebase merge methods, we should check the correct commits. Note, it's a bit useless to check these commits for merge commits, but we can optimize it later. I'd rather be strict about what we're checking. This does still fix a case where a merge commit exists on the pull request that was rebased into a non-merge commit.
korthout
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
89cc862
to
e6ef2ab
Compare
The commits are looked up for a range using log, which will automatically start with the latest commit first. However, when cherry-picking we want to start with the oldest commit first so we can build on top of the commits. Otherwise, we'd easily run into conflicts.
korthout
force-pushed
the
feat/respect-squash-and-rebase-merges
branch
from
e6ef2ab
to
1e95a33
Compare
korthout
changed the title
korthout
changed the title
|
I find this is a great feature, thanks for adding them! I think this should be the default behavior (in next major versions), assuming the source PR backports originate from has been merged. Referring to a commit (from which cherry-picked) that would never exist in the repo is almost meaningless. |
|
@wookayin It's great to hear you like the feature. I wanted to keep this an experimental feature for some time in case some bugs are discovered. But already, several teams have enabled it in different usages and so far, so good. I think we can promote the feature in two steps:
I intend to take both steps eventually, but I prefer giving such features time to be battle-tested. I did something similar with the shallow clone and specific-depth fetching changes. By the way, I do not intend to break this assumption, but it may become configurable if users need to backport unmerged pulls. Do you see any issues with that?
Thanks again for sharing your input, it's greatly appreciated 🙇 |
@korthout Github allows multiple merge methods.
In cases where the merger used "Rebase and Merge" we can use the commits associated with the PR. (Not the commits that will be applied on the
headbranch once the PR is merged).When using "Squash and Merge" we should use the single squashed commit that ends up on the
headto accurately reflect any changes in the commit message and to ensure an accurate history between branches.This patch tries to implement this behavior by looking at the parent of the
merge_commit_shathat is populated when the PR is merged. If the parent is also associated to the PR (utilisingoctokit.rest.repos.listPullRequestsAssociatedWithCommitto check this) we can assume that the "rebase and merge" strategy was used as a squash would only produce a single commit and the parent would not be associated with the PR.If the parent is not associated to the PR, we can use the newly generated
merge_commit_shawhich is the squashed commit applied on thehead.