Skip to content

Avoid using short commit IDs - #1236

Merged
Andarist merged 3 commits into
changesets:mainfrom
camertron:no_short_shas
Nov 27, 2023
Merged

Avoid using short commit IDs#1236
Andarist merged 3 commits into
changesets:mainfrom
camertron:no_short_shas

Conversation

@camertron

@camertron camertron commented Sep 22, 2023

Copy link
Copy Markdown
Contributor

My team and I ran into a wild issue with changesets today that has prompted me to open this PR.

Setup

We're using GitHub Actions and saw the following output in one of our release builds:

The following error was encountered while generating changelog entries
We have escaped applying the changesets, and no files should have been affected
🦋  error TypeError: Cannot read properties of null (reading 'author')
🦋  error     at Object.getInfo (/home/runner/work/primitives/primitives/node_modules/@changesets/get-github-info/dist/get-github-info.cjs.dev.js:235:12)
🦋  error     at processTicksAndRejections (node:internal/process/task_queues:96:5)
🦋  error     at async /home/runner/work/primitives/primitives/node_modules/@changesets/changelog-github/dist/changelog-github.cjs.dev.js:122:13
🦋  error     at async Object.getReleaseLine (/home/runner/work/primitives/primitives/node_modules/@changesets/changelog-github/dist/changelog-github.cjs.dev.js:99:19)
🦋  error     at async Promise.all (index 0)
🦋  error     at async generateChangesForVersionTypeMarkdown (/home/runner/work/primitives/primitives/node_modules/@changesets/apply-release-plan/dist/apply-release-plan.cjs.dev.js:192:22)
🦋  error     at async getChangelogEntry (/home/runner/work/primitives/primitives/node_modules/@changesets/apply-release-plan/dist/apply-release-plan.cjs.dev.js:247:109)
🦋  error     at async /home/runner/work/primitives/primitives/node_modules/@changesets/apply-release-plan/dist/apply-release-plan.cjs.dev.js:420:[21](https://github.com/primer/primitives/actions/runs/6267066703/job/17044280307#step:8:22)
🦋  error     at async Promise.all (index 0)
🦋  error     at async Object.applyReleasePlan [as default] (/home/runner/work/primitives/primitives/node_modules/@changesets/apply-release-plan/dist/apply-release-plan.cjs.dev.js:[31](https://github.com/primer/primitives/actions/runs/6267066703/job/17044280307#step:8:32)1:31)

Looks like it's trying to access the author property of null. After looking at the changesets code, it appears the error happens when processing the result of executing a GraphQL query via the GitHub API. After sticking some console.log statements inside the changesets code, I was able to print out the query:

query {
    a0: repository(owner: "primer" name: "primitives") {
        a51dbba8: object(expression: "51dbba8") {
            ... on Commit {
                commitUrl
                associatedPullRequests(first: 50) {
                    nodes {
                        number
                        url
                        mergedAt
                        author {
                            login
                            url
                        }
                    }
                }
                author {
                    user {
                        login
                        url
                    }
                }
            }
        }
        a2565a2d: object(expression: "2565a2d") {
            ... on Commit {
                commitUrl
                associatedPullRequests(first: 50) {
                    nodes {
                        number
                        url
                        mergedAt
                        author {
                            login
                            url
                        }
                    }
                }
                author {
                    user {
                        login
                        url
                    }
                }
            }
        }
    }
}

You should be able to copy/paste this query into GitHub's GraphQL explorer and see the following response (modified for brevity):

{
  "data": {
    "a0": {
      "a51dbba8": null,
      "a2565a2d": {
        "commitUrl": "https://github.com/primer/primitives/commit/2565a2d30dbca13b73e21064036910e269385770",
        "associatedPullRequests": {
          "nodes": [
            { ... }
          ]
        },
        "author": {
          "user": { ... }
        }
      }
    }
  }
}

Notice the full and complete response for commit 2565a2d but a null value for commit 51dbba8.

Diagnosis

So what the heck? Running git rev-parse 51dbba8 returns the full commit ID of 51dbba881eea1cd1a2d0c601023c9d4b8357b64d, which definitely exists.

My only idea, although very unlikely, was a hash collision.

$> git cat-file -t 51dbba8
error: short object ID 51dbba8 is ambiguous
hint: The candidates are:
hint:   51dbba881 commit 2023-09-21 - update diffStat tokens (#728)
hint:   51dbba865 tree
fatal: Not a valid object name 51dbba8

Bingo! There is a hash collision! 51dbba8 is a prefix shared by both a commit and a tree object.

Using the full SHA (or even just one additional digit) in the GraphQL query returns non-null data for both the commits in question. Problem solved.

Solution

Unless there's a really good reason not to, I think changesets should use full commit IDs everywhere. Since it's a popular project with lots of users, it was only a matter of time before a collision issue like this occurred.

After code spelunking for a few hours, I wasn't able to identify precisely the right part of the code to modify, so I just removed all the instances of short: true being passed into the various git functions. Please let me know if a more nuanced change is necessary.

@changeset-bot

changeset-bot Bot commented Sep 22, 2023 •
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a907e78

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@changesets/apply-release-plan Minor
@changesets/cli Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci

codesandbox-ci Bot commented Sep 22, 2023 •
edited
Loading

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a907e78:

Sandbox Source
Vanilla Configuration

@camertron camertron mentioned this pull request Sep 22, 2023
Closed
9 tasks
@Juice10

Juice10 commented Nov 7, 2023

Copy link
Copy Markdown

Great find @camertron! We've been having exactly the same issue on https://github.com/rrweb-io/rrweb and (thanks to CodeSandbox) I've tested this PR and confirmed it fixes the issue.

@Andarist

Andarist commented Nov 7, 2023

Copy link
Copy Markdown
Member

I think that the long hashes won't be as pleasant as the shorter ones in the CHANGELOGs so perhaps we could truncate them here, here, here, here and here

@camertron

camertron commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

@Andarist ok makes sense 👍 Should I make those changes in this PR?

@Juice10

Juice10 commented Nov 9, 2023

Copy link
Copy Markdown

@Andarist ok makes sense 👍 Should I make those changes in this PR?

I think that makes sense. Could you do it @camertron ?

@Andarist

Andarist commented Nov 9, 2023

Copy link
Copy Markdown
Member

Yeah, it would be cool to do it as part of this PR.

@camertron

camertron commented Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Ok, done!

@Juice10

Juice10 commented Nov 10, 2023

Copy link
Copy Markdown

Ok, done!

Awesome, thanks @camertron!

@Andarist

Andarist commented Nov 15, 2023

Copy link
Copy Markdown
Member

Thanks for making the requested changes. I'll try to land this soon. It would be great to land #1085 first but I'm not sure if I will have time to finish this PR right now so I might decide just to validate this PR locally.

@Andarist
Andarist merged commit dfd4cca into changesets:main Nov 27, 2023
@github-actions github-actions Bot mentioned this pull request Jul 5, 2023
Merged
@camertron
camertron deleted the no_short_shas branch November 27, 2023 18:26
@Andarist Andarist mentioned this pull request Apr 15, 2025
Closed
@camertron camertron mentioned this pull request May 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@Juice10 Juice10 Juice10 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@camertron @Juice10 @Andarist