What range of commits are looked at?

[michaelgwelch]

We have a private repo so I can't share it with you. But we have two active branches going. Every Friday (or Monday) I merge from v4 to v5. Four weeks in a row this merge is failed due to the commitlint-github-action.

When I look at the details I see old commit messages that are not included in the current PR that are failing.

I would expect it to just be examining the handful of commits that are from v4 that are new to v5 and not older commit messages that were committed prior to our use of commitlint.

What data could I provide to help debug this issue?

this is our job definition (run on pull request):

jobs:
  lint:
    name: Lint Commit Messages
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          fetch-depth: 0
      - uses: wagoid/commitlint-github-action@v4

[wagoid]

Hi @michaelgwelch! Basically we run a command similar to this to get all commit messages of a PR: git log --first-parent [fromHash]^1..[toHash].
In theory, this should be enough to be sure that old commit messages are not included. We would need to investigate your use case to understand what are the changes needed to the final git log command in order to fix it.

If you could create a repository that reproduces this scenario, it would be helpful so that I or someone else can investigate the issue 👍

[michaelgwelch]

@wagoid thanks for your response. I'll see what I can do.

[michaelgwelch]

@wagoid It does indeed bring in "old commits" and now that I've used git log to see it maybe I can explain (and then create a new repo with the issue).

So we have two branches v4 and v5 which over a week or two may diverge. (Fixes go into v4 and new features being added to v5). Once a week or so I merge from v4 into v5.

When I run git log --first-parent v4ish...v5ish (after doing such a merge) it includes commits from v5 that have already been reviewed. It doesn't just include the new commits from v4.

I'll work on putting together a test repo.

[wagoid]

@michaelgwelch got it, thanks for setting up the repo!

Another thing you can try to fix this quicker is to rebase one branch into the other instead of merging. Maybe this is not possible on your use case, but leaving the suggestion here in case it helps.

[michaelgwelch]

@wagoid Yeah, we can't rebase as both v4 and v5 are "released". So we can't be changing history on v5 to incorporate v4 changes into it.

Sorry I didn't get to this sooner with the repo. Putting it on my TODO for today.

[hicksjacobp]

I believe for us, our solution is to exclude the target. Following the example from @michaelgwelch above, instead of git log --first-parent v4ish...v5ish, this would be more appropriate: git log --first-parent v4ish...v5ish ^v5ish (exclude everything reachable from the target branch).

I'm not sure if this change is applicable to all consumers of this action...mainly those that use feature/topic branches for development which I believe your action was originally intending for @wagoid. Off hand and without diving deeper git log or testing it, I don't know if adding ^target to always be used would have any negative effects.

Maybe this action can support a more advanced configuration option for additional log arguments?

[michaelgwelch]

I've finally reproduced it and @hicksjacobp seems to have the solution.

If you look at this PR you'll see only 2 commits.

What's in this PR? This simulates our periodic need to merge from v1 into v2. Where v1 and v2 have both changed since the last merge. So I've created a branch based off of v2 called mergeBranch. I then did a git merge v1. I then pushed mergeBranch and created a PR from it targeting v2.

So the PR includes a change to v1 plus the merge commit.

If I do a git log [from]^1...[to] --first-parent --oneline on mergeBranch this is what I get:

> git log --first-parent c92923^1...3994bd --oneline
3994bda (HEAD -> mergeBranch, origin/mergeBranch) Merge branch 'v1' into mergeBranch
d34d780 (origin/v2, v2) v2: file5
c032725 Merge branch 'v1' into mergeBranch
e74ec71 v2: file4
aa7bd0a v2: file3

You can see that it includes "old" commits that are already in v2. (I know this by the naming convention I used for each commit. Every explicit commit to v2 has v2 in the summary.

Note: what is actually found by commitlint-github-action also includes these old commits.

However, if I include ^v2 on the command line as suggested by @hicksjacobp those are excluded.

> git log --first-parent c929234238abf0540e3666514ec1e6715d5cec6c^1...3994bd ^v2 --oneline
3994bda (HEAD -> mergeBranch, origin/mergeBranch) Merge branch 'v1' into mergeBranch

I'm working on a patch now that I've reproduced and understand what is going on.

I'd like to find more documentation on git log <revision-range> to understand why it's looking for commits outside of the range.

[michaelgwelch]

UPDATE there's another issue here as well. Notice that the new commit from the v1 branch is missing in both list of commits that I generated in my previous comment.

The actual solution is to use ^v2 BUT NOT --first-parent. It appears the correct invocation is

> git log  c92923^1...3994bd ^v2 --oneline
3994bda (origin/mergeBranch) Merge branch 'v1' into mergeBranch
c929234 (origin/v1, v1) v1: file6

I'm curious, why not just use the list of commits returned by the pull request itself. This code here appears to already have the commits that need linting:

commitlint-github-action/src/action.js

Lines 57 to 61 in 481aff4

	const { data: commits } = await octokit.rest.pulls.listCommits({
	owner,
	repo,
	pull_number: number,
	})

Then just use git show sha1 sha2 sha3 ... --oneline -s to get the messages.

That way you can just forget about --first-parent and my proposed excludeBranchTarget

Or alternatively, if sticking with git log:

git log GITHUB_ENV_BASE_REF...GETHUB_ENV_HEAD_REF

Which I believe is exactly what the PR itself is doing:

Another special notation is "<commit1>…​<commit2>" which is useful for merges. The resulting set of commits is the symmetric difference between the two operands. The following two commands are equivalent:

[michaelgwelch]

I'm working on fix and tests. I see the mocking you have in your test scripts and think I can create a test scenario that reproduces our issue and then tests the fix.

[wagoid]

hi @michaelgwelch! TBH I don't remember anymore why I don't just use the result from github API. Maybe at the time I thought commit names were not in the response. I like the idea of just getting the commit messages from GitHub API, would reduce a lot of code and fix the discrepancies that we've seen in this issue 🚀

[michaelgwelch]

@wagoid I didn't even think of that. Yes assuming the octokit method listCommits returns a collection with the commit messages in them, you are all set.

And it appears it does. Here's the response from pull #660

List Commits Response

[
  {
    "sha": "9fa60b5335e8f0c91ff36a791a1f0e0a653c6382",
    "node_id": "C_kwDODKX2iNoAKDlmYTYwYjUzMzVlOGYwYzkxZmYzNmE3OTFhMWYwZTBhNjUzYzYzODI",
    "commit": {
      "author": {
        "name": "dependabot[bot]",
        "email": "49699333+dependabot[bot]@users.noreply.github.com",
        "date": "2023-01-30T02:12:21Z"
      },
      "committer": {
        "name": "GitHub",
        "email": "noreply@github.com",
        "date": "2023-01-30T02:12:21Z"
      },
      "message": "chore(deps-dev): bump babel-jest from 28.1.1 to 29.4.1\n\nBumps [babel-jest](https://github.com/facebook/jest/tree/HEAD/packages/babel-jest) from 28.1.1 to 29.4.1.\n- [Release notes](https://github.com/facebook/jest/releases)\n- [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md)\n- [Commits](https://github.com/facebook/jest/commits/v29.4.1/packages/babel-jest)\n\n---\nupdated-dependencies:\n- dependency-name: babel-jest\n  dependency-type: direct:development\n  update-type: version-update:semver-major\n...\n\nSigned-off-by: dependabot[bot] <support@github.com>",
      "tree": {
        "sha": "e0fc4aedc7407b5ae0aa9799514e8a88acfafefd",
        "url": "https://api.github.com/repos/wagoid/commitlint-github-action/git/trees/e0fc4aedc7407b5ae0aa9799514e8a88acfafefd"
      },
      "url": "https://api.github.com/repos/wagoid/commitlint-github-action/git/commits/9fa60b5335e8f0c91ff36a791a1f0e0a653c6382",
      "comment_count": 0,
      "verification": {
        "verified": true,
        "reason": "valid",
        "signature": "-----BEGIN PGP SIGNATURE-----\n\nwsBcBAABCAAQBQJj1yeFCRBK7hj4Ov3rIwAAmBUIAErDY/DqRSxA6qMZYeo+msrn\nbXAmgs3Ec2OvcyfgVrXmHEiVe8UbcktWMDPh2fOymQAer8tvmDBRTY8JuIuD0HK4\nDhcYwOQkmTvRN8WFzzfr+BvxQpCrRQ49aFpO+fW05oGk7GlZf1BZAcqZtF7Mq38t\nIwZkPmu1j+IZ2HvFEetTgK8tzOcRAa9CH8LYVe8CVKG2Ji26dFnp1Dfour4oqz06\n+/VG2FdzmIb0vu4GJMf9USNUDRYGoI6T+rfH2Ayn9ZwNGxduZP+N663/U7AP8JZ5\nD9cXdRbgATMFF+T3wpoOM2MDBIAotbj/7w1OoBtEKj8jFP1C6rfipc3K410KXZ4=\n=cWpA\n-----END PGP SIGNATURE-----\n",
        "payload": "tree e0fc4aedc7407b5ae0aa9799514e8a88acfafefd\nparent 481aff4de4d0de6d28d05533d4230d298ea3377d\nauthor dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> 1675044741 +0000\ncommitter GitHub <noreply@github.com> 1675044741 +0000\n\nchore(deps-dev): bump babel-jest from 28.1.1 to 29.4.1\n\nBumps [babel-jest](https://github.com/facebook/jest/tree/HEAD/packages/babel-jest) from 28.1.1 to 29.4.1.\n- [Release notes](https://github.com/facebook/jest/releases)\n- [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md)\n- [Commits](https://github.com/facebook/jest/commits/v29.4.1/packages/babel-jest)\n\n---\nupdated-dependencies:\n- dependency-name: babel-jest\n  dependency-type: direct:development\n  update-type: version-update:semver-major\n...\n\nSigned-off-by: dependabot[bot] <support@github.com>"
      }
    },
    "url": "https://api.github.com/repos/wagoid/commitlint-github-action/commits/9fa60b5335e8f0c91ff36a791a1f0e0a653c6382",
    "html_url": "https://github.com/wagoid/commitlint-github-action/commit/9fa60b5335e8f0c91ff36a791a1f0e0a653c6382",
    "comments_url": "https://api.github.com/repos/wagoid/commitlint-github-action/commits/9fa60b5335e8f0c91ff36a791a1f0e0a653c6382/comments",
    "author": {
      "login": "dependabot[bot]",
      "id": 49699333,
      "node_id": "MDM6Qm90NDk2OTkzMzM=",
      "avatar_url": "https://avatars.githubusercontent.com/in/29110?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/dependabot%5Bbot%5D",
      "html_url": "https://github.com/apps/dependabot",
      "followers_url": "https://api.github.com/users/dependabot%5Bbot%5D/followers",
      "following_url": "https://api.github.com/users/dependabot%5Bbot%5D/following{/other_user}",
      "gists_url": "https://api.github.com/users/dependabot%5Bbot%5D/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/dependabot%5Bbot%5D/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/dependabot%5Bbot%5D/subscriptions",
      "organizations_url": "https://api.github.com/users/dependabot%5Bbot%5D/orgs",
      "repos_url": "https://api.github.com/users/dependabot%5Bbot%5D/repos",
      "events_url": "https://api.github.com/users/dependabot%5Bbot%5D/events{/privacy}",
      "received_events_url": "https://api.github.com/users/dependabot%5Bbot%5D/received_events",
      "type": "Bot",
      "site_admin": false
    },
    "committer": {
      "login": "web-flow",
      "id": 19864447,
      "node_id": "MDQ6VXNlcjE5ODY0NDQ3",
      "avatar_url": "https://avatars.githubusercontent.com/u/19864447?v=4",
      "gravatar_id": "",
      "url": "https://api.github.com/users/web-flow",
      "html_url": "https://github.com/web-flow",
      "followers_url": "https://api.github.com/users/web-flow/followers",
      "following_url": "https://api.github.com/users/web-flow/following{/other_user}",
      "gists_url": "https://api.github.com/users/web-flow/gists{/gist_id}",
      "starred_url": "https://api.github.com/users/web-flow/starred{/owner}{/repo}",
      "subscriptions_url": "https://api.github.com/users/web-flow/subscriptions",
      "organizations_url": "https://api.github.com/users/web-flow/orgs",
      "repos_url": "https://api.github.com/users/web-flow/repos",
      "events_url": "https://api.github.com/users/web-flow/events{/privacy}",
      "received_events_url": "https://api.github.com/users/web-flow/received_events",
      "type": "User",
      "site_admin": false
    },
    "parents": [
      {
        "sha": "481aff4de4d0de6d28d05533d4230d298ea3377d",
        "url": "https://api.github.com/repos/wagoid/commitlint-github-action/commits/481aff4de4d0de6d28d05533d4230d298ea3377d",
        "html_url": "https://github.com/wagoid/commitlint-github-action/commit/481aff4de4d0de6d28d05533d4230d298ea3377d"
      }
    ]
  }
]

[michaelgwelch]

FYI, in prepping for my tasks I ran your tests. Then I randomly commented things out and they still passed.

For example, I commented out the code that sets firstParent:

commitlint-github-action/src/action.js

Lines 77 to 79 in 481aff4

	if (getInput('firstParent') === 'true') {
	options.firstParent = true
	}

And the tests still pass even though it appears you have tests that were meant to cover this.

[wagoid]

Nice catch! Thanks for identifying this, will have a look later

[tfwright]

I'm also having this problem after merging a commit with an invalid message. Now all merge prs from that branch to upstream branches are failing, even though the PR does not include the invalid commit. Since it's history, I just want to ignore it and only continue to validate new commits.

[heylookltsme]

Hello! Any updates on refactoring this action to use the github API response instead of checking out the full history of a repo and running git log?

I'm working in an enormous repository where checking out the whole shebang takes upwards of 30 minutes (🙀 ), so that's just not tenable for me. 😞

[wagoid]

Hey everyone! Sorry for the delay on this, I've just released a new major version that changes the behavior to use github API. You don't need fetch-depth: 0 anymore 💪

[wagoid]

Correction: there was an issue with the release, it got released as a patch. Anyway it should be fine, there's only firstParent input that I removed, shouldn't affect most users.