Description
Context
Currently the create_release_backmerge_pull_request
action assumes that the repository it is working on uses the "Merge Commit" strategy, i.e. that pull requests create a merge commit (--no-ff
).
This assumption mostly affects the part of the action where it's checking if a PR is actually necessary to create or not, as for this it tests if the list of commits the PR would contain is empty or not. In a repo that uses the "Merge Commit" strategy, if it's empty that means that the release branch have already been merged into the target branch; and in those cases Github would error if we made an API call to create a PR where there would be no commit at all.
But this logic breaks if the repository used the "Squash commit" strategy when creating PRs, as this strategy creates a new commit—whose content is the same as the compound content of all the commits in the PR—on the target branch. This means that the commits in the release
branch will never be merged into / reachable from the target branch, and the commit list would never be empty, and the backmerge PR will always be created, even if there were no new commit added to it since the last backmerge.
Use Cases
We should consider two key concepts when trying to determine if a backmerge is necessary:
- If the list of commits between the head and target branch is empty (such case would be refused by the GitHub API anyway)
- If the PR diff is empty (which can happen even if the commit-list is not empty, see cases 2 and 3 below)
Note though that in practice in Squash Merge stategy, the commit list will never be empty, since last squash created a different commit sha on the target branch.
There are thus multiple scenarios where this could impact the logic
- The release branch did not have any new commit since the last backmerge.
- This means not only an empty diff, but also an empty commit list
- In that case we don't want a backmerge PR to be created.
- The release branch had new commits, but those new commits were also cherry-picked on the target branch already
- In that case the diff would be empty, but the commit list wouldn't
- For those cases, we probably still want to create a backmerge PR for repos using the Merge Commit strategy, even if it wouldn't change the resulting code, mostly to keep the git graph consistent and making it clear just from the graph that everything that landed in the
release
branch got back-propagated - But for repos using Squash Strategy, it would probably not make sense / be necessary to create a backmerge PR in those cases
- The release branch had new commits, but those commits were also reverted soon after on the release branch (i.e. we pushed both a commit and its revert)
- In this case the diff would be empty, but the commit list wouldn't
- So same as for 2, we probably still want a backmerge PR for repos using Merge Commit, but not for repos using Squash
Also note that there are some repos (e.g. WPAndroid iinm) that don't enforce a specific merge strategy, leading to some developers choosing to squash when they merge their PR, while some others choosing to create a merge commit. So in those repos there's a mix of both squash and merge, and the behavior we want to apply for create_release_backmerge_pull_request
will not depend on a single strategy set in the repo's settings, but on the team's preferences when it comes to if creating a backmerge PR with empty diff is wanted (for consistency with git graph) or not (deemed useless)
All those have also been discussed internally here: p1741031781258789/1741030520.481599-slack-C0211NP7Y5U
Solution
We shall add a new skip_if_empty
parameter (ConfigItem
) to the create_release_backmerge_pull_request
action that can take either :commit_list
or :diff
as possible values.
- If set to
:commit_list
, the PR would only be created if the list of commits to be included in the PR is not empty, even if the diff is empty (because we're in case 2 or 3 above) - If set to
:diff
, the PR would only be created if the three-dots diff is not empty, i.e. the "File Changes" tab of the PR would not be empty. This will thus skip the PR even if the commit list is non-empty but the diff is empty, as could be the case for cases 2 or 3 above.
We'll probably want :commit_list
to be the default value for this new parameter, also because that's the behavior the action has been using so far so keeping it the default would prevent introducing a breaking change in behavior.
Alternatives Considered
We already brainstormed with @iangmaia about alternative names for this new parameter.
Since this is a two-state parameter, it was tempting to try and make this parameter be a Boolean
instead of one accepting a Symbol. But trying to come up with a boolean-like parameter name that would be explicit enough ends up being tricky and risk either making the name of the parameter extra verbose so it can be clear enough, or make it ambiguous and unclear.
Besides, in addition of making things clearer, having this parameter be a Symbol
opens up the possibility to extend the supported strategies later. For example we could imagine differentiating the case of empty-two-dots-diff vs empty-three-dots-diff (I can't see an application for this right now, but who knows…), or skip the backmerge if the only diff would be on specific files but not others…
We don't have a need for such a fine-grained condition yet, but at least keeping it a Symbol
makes those possibilities open if we even needed them.