fix(releases): handle partial SHAs correctly in commit resolution

[srest2021]

Fixes commit resolution failures that caused "Unable to fetch commits" errors in Sentry when using partial SHAs with sentry-cli releases set-commits.

• Partial SHAs like f915d32 are now passed to the API without padding, allowing single partial SHAs and ranges of partial SHAs to be successfully associated with a release.
• Ensures SHAs are >=64 chars long, same as the backend (no validation for minimum length or hexademical).
• Drop support for specifying commits via branch names (an undocumented feature that relied on the repo being checked out locally).

Fixes #2064
Fixes CLI-55
Fixes REPLAY-413

Before (commit associations with partial SHAs fail, as denoted by the lack of profile pic for the top 2 releases where I used partial SHAs):

After (commit associations with partial SHAs now work; note the profile pics for the top 2 releases are now present):

[linear]

REPLAY-413 [Manually Associating Commits] Partial SHA Fails To Resolve

[szokeasaurusrex]

Hey @srest2021, thanks for this PR!

While I agree completely that the current lack of support for full commit hashes is indeed unfortunate and should be addressed, this proposed change has a fundamental flaw: while Sentry CLI will try to resolve the partial commit hashes to a full commit hash in the currently checked-out repository, any repository in the user's Sentry project can be passed in the --commit argument.

For example, let's say I am running set-commits like so, from within the getsentry/sentry-cli repository:

sentry-cli set-commits my-release --commit "getsentry/sentry@01234...12345"

Importantly, note that we passed getsentry/sentry in the --commit argument, not getsentry/sentry-cli.

Let's say there are two commits in the Sentry CLI repo with hashes starting with 01234 and 12345. With this PR, we will resolve to those Sentry CLI hashes, even though the command is meant to be doing something on the getsentry/sentry repo.

My recommendation

I think we likely should fully rethink how this command works. The current way of specifying commits, including needing to provide the full hash, and also the fact that the repo must be explicitly specified in the --commit argument by following the format <repo>@<full from hash>..<full to hash>, is quite clunky, and probably should be improved significantly.

I would be happy to discuss ideas for reworking the whole command line interface for this command to improve usability with you and your team.

For now, until we have decided on an improved interface, I think it might be preferable just to validate that the SHAs are full SHAs, and throw an error if not.

Please let me know what you think 🙏

[ryan953]

$0.02 opinion here:

It does seem kind of clunky that people have to specify the full <repo>@<from>..<to> syntax to make things work. I think there should be two possibilities here for making things easier for people:

1. Validating that a full SHA is passed in could work.
   The set-commits command only seems to work with git repos, so we know what length to expect from a SHA. Because it can reference some other repo we won't actually be able to validate that the SHA exists, so it would just mostly be an augmented length/formatting check afaik, yes?

2. Based on the Issue #43274 it also seems like the API does accept partials SHA's.
   The workaround that suggests people call the API directly explicitly uses partial SHA's, and any git command would work fine too. So if that's the case why are we changing the data that the user gave us at all? If they passed in a partial commit SHA can we simply forward that into the API and not have to reformat the SHA into something else?

Note that: a partial SHA in git is not a fixed length. It must is only long enough to disambiguate a commit within the repo.

[srest2021]

So if that's the case why are we changing the data that the user gave us at all? If they passed in a partial commit SHA can we simply forward that into the API and not have to reformat the SHA into something else?

I'll look into this today and see if it's possible to just preserve the original partial SHA

[srest2021]

Using the original unpadded partial SHAs in the API call worked for me! What do you think @szokeasaurusrex ?

[szokeasaurusrex]

Using the original unpadded SHAs in the API request worked for me, and I believe this is the correct approach.

I agree with you that using the original unpadded SHAs is the correct high-level approach. That said, I believe we can implement this change in a far simpler and more robust manner.

Recommendation 🚀

Where to change 🗺️

Rather than modifying vcs.rs, I’d change the code path handling --commit args so the current code never runs. This means reverting the changes in this PR and instead updating this section of set_commits.rs.

Right now, that code calls find_heads, which assumes the repo is checked out locally—a flawed assumption that causes many issues.

The fix 💡

Instead of calling find_heads, we should the commit specs directly and turn them into Ref structs. Optionally we can validate they’re valid hex SHAs.

Since these Refs feed into the API request, Sentry itself will resolve the commits—whether full or partial SHAs.

Considerations 🧐

This would drop support for specifying commits via branch names. But that feature was undocumented and never worked correctly, as it relied on the repo being checked out locally. So, I think it is reasonable to make this change without a major, as it is essentially a bug fix. With this change, we should clarify that only Git SHAs (full or partial) are supported.

Future work 🔮

I think this is a good first step for improving the set-commits command.

If you all would like, I would be happy to work with your team to rethink the entire release management experience in Sentry CLI, in a way which would address current customer pain points. There is a lot we could still improve.

---

Thanks again for raising this and identifying the root solution! I’m happy to help with implementation if needed 🙏

[linear]

CLI-55 Error when not using full SHA in set-commits

[srest2021]

@szokeasaurusrex With these changes, will we support commands with only a single partial SHA, like so?

sentry-cli releases set-commits --commit srest2021/test-sentry-app@6b06f0f "test-release" --log-level=debug

Sending this payload to the API doesn't work, so I just wanted to ask if this is expected due to the fact that we can't use the locally checked-out repo anymore:

{"refs":[{"repository":"srest2021/test-sentry-app","commit":"6b06f0f","previousCommit":null}]}

[szokeasaurusrex]

@srest2021 I'm not sure I completely understand your question. I'd expect that we should still be able to set both SHAs. If we cannot, that's a problem (either in CLI or in the backend).

Would be happy to take a closer look at this next week, though 🙂

[szokeasaurusrex]

Sending this payload to the API doesn't work, so I just wanted to ask if this is expected due to the fact that we can't use the locally checked-out repo anymore:

{"refs":[{"repository":"srest2021/test-sentry-app","commit":"6b06f0f","previousCommit":null}]}

@srest2021, ah I think I misread before. You are saying that the API call did not work with a previousCommit being blank?

If that is the case, I think the previousCommit is required when you are making the first release for a project. Afterwards, I believe it is optional, since we can get the commit from the previous release. I don't think the changes here should have any affect on that.

Could this explain the behavior you observed? What exactly went wrong for you?

[srest2021]

Hey @szokeasaurusrex sorry for the late update! I'm still trying to figure out what's going on with the API calls not working. I've got a bunch of different test cases that I wanted to use to compare the old vs. new partial behavior, and I'm getting inconsistent results 😭

Example: trying to associate a single full SHA with a release:

sentry-cli releases new "og-single-full4" --project=javascript-react --log-level=debug --org my-org-slug

sentry-cli releases set-commits --commit srest2021/test-sentry-app@6b06f0ff81cab161e670554caff1e765d171dba5 "og-single-full4" --log-level=debug --org my-org-slug

These commands--using the original sentry-cli--worked for me perfectly the first time I tried them and haven't worked since (other results include either associating all 12 commits inside my test repo, or associating 0 commits). This inconsistency in the API (probably due to order of release creation like you mentioned) has made it a bit tricky to clarify the end goal and confirm whether the changes are working or not. But, from what I've observed so far, partial SHAs don't work at all in the old code, and a range of partial SHAs works in this PR. That's why I asked if it's possible for the API to successfully handle a single partial SHA. But, it sounds to me that if we're passing the repo name and a single partial SHA to the API, it should be possible for the API to resolve this.

I'll switch to soft validation and keep working on this--will keep you updated as soon as I make progress! Thanks again for helping out with this issue

Update: Yes I believe the problem I was encountering was due to the first vs. non-first release distinction! Sending a single partial SHA now works in this PR as long as it's not the first release.

[szokeasaurusrex]

Update: Yes I believe the problem I was encountering was due to the first vs. non-first release distinction! Sending a single partial SHA now works in this PR as long as it's not the first release.

Awesome, glad to hear it @srest2021! So, just to confirm, is everything working as expected now? Is this PR ready for me to review?

[srest2021]

Yep should be all good @szokeasaurusrex !

[szokeasaurusrex]

Added a couple small suggestions. But, overall, this is looking pretty good! Thanks for your work on this

[szokeasaurusrex]

Getting closer, I have some minor suggestions still

[szokeasaurusrex]

Looks good, please apply the minor suggestion I added prior to merging! Thanks again for improving this 🥔