googleapis/release-please

Chronological commit sorting means that merged PRs can be ignored

ivomurrell opened this issue · 4 comments

ivomurrell commented

Environment details

  • OS: macOS 12.4
  • Node.js version: 16.14.0
  • npm version: 8.5.4
  • release-please version: 13.19.3

Steps to reproduce

  1. Make commits to branch foo and create a PR
  2. Make commits to branch bar and create a PR
  3. Merge bar's PR and then the release-please PR created thereafter
  4. Merge foo into main (without rebasing first!)
  5. Observe that the commits from foo are ignored by release-please

This happens because the commits are pulled from GitHub's API in chronological order (i.e., using git log --date-order) rather than topological order (i.e., using git log --topo-order.) This means that the order of commits is foo, bar, bar merge commit, release-please, release-please merge commit, foo merge commit. release-please will only consider commits since its last release, so will slice away any commits before release-please merge commit, leaving only foo merge commit but none of the commits from foo! Topological sorting would instead produce the desired order of bar, bar merge commit, release-please, release-please merge commit, foo, foo merge commit, as it traverses the all the child commits before showing newer commits.

chingor13 commented

Hmm, we are using GraphQL to fetch the commit history and its docs say:

The linear commit history starting from (and including) this commit, in the same order as `git log`

There do not appear to be any options for sorting order. The REST commits#list API also does not appear to have any topological sorting option.

I know this is not a solution, but this is one of the main reasons we avoid using merge commits. We enforce squash/merge to keep a simpler history and you can attach multiple commit messages with the conventional commit spec.

SurferJeffAtGoogle commented

To implement this feature request, I think release please would have to clone the repo.

m-orchard commented

May I suggest at least updating the documentation to remove or amend the suggestion that merge commits are supported - https://github.com/googleapis/release-please#whats-a-release-pr ? I think it's misleading given this is quite a significant issue in terms of generating the correct version number and changelog.

joshorr commented

I've found a couple of instances of my merge commits being ignored. Once when I went back to the change log in the past to see what changed a few releases back I discovered that release-please missed a few commits.

And just now, I merged in a PR and it did not find any releasable-commits (even though there were some).

In both cases it was due to this issue. This finally makes sense to me now, based on what I saw in the log and how release-please only considers commits in chronological order.

It sounds like release-please indeed does not support merge commits? 😞 Are there any work arounds for this? Or do I have to use use a different PR merge style, such as squash or rebase?

Clone Repo Thoughts

As to the must clone the repo.... I use release-please at a point where I've already cloned/checked-out the repo; so it could get the info for 'free' from the already cloned repo (I run unit tests and then run release-please).

Perhaps you could make and already-cloned repo a requirement to support merge-commits? It could be an optional feature (where you tell the release-please that you already have cloned the repo).... I use the release-please-action though... so perhaps this is more of a feature-request type of situation for the action?