Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid using short commit IDs #1236

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Avoid using short commit IDs #1236

merged 3 commits into from
Nov 27, 2023

Conversation

camertron
Copy link
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
Copy link

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

🦋 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
Copy link

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

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
Copy link

Juice10 commented Nov 7, 2023

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
Copy link
Member

Andarist commented Nov 7, 2023

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
Copy link
Contributor Author

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

@Juice10
Copy link

Juice10 commented Nov 9, 2023

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

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

@Andarist
Copy link
Member

Andarist commented Nov 9, 2023

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

@camertron
Copy link
Contributor Author

Ok, done!

@Juice10
Copy link

Juice10 commented Nov 10, 2023

Ok, done!

Awesome, thanks @camertron!

@Andarist
Copy link
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
1 check passed
@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
@Hdoc1509 Hdoc1509 mentioned this pull request Oct 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Juice10 Juice10 Juice10 approved these changes

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