fix(vcs): Handle non-github workflow in find_base_sha #2897

chromy · 2025-10-30T15:52:06Z

find_head_sha supported both Github workflow contexts and 'raw git'
contexts however the matching function find_base_sha did not handle
raw git contexts.

Thi updates find_base_sha to return the equivalent of:
git merge-base HEAD origin/HEAD. For:

     o---o---o---origin/main
    /
---1---o---o---o---foo

We return the SHA of 1.

szokeasaurusrex · 2025-10-31T10:44:07Z

src/utils/vcs.rs

-        .map_err(Error::from)
-        .and_then(|event_path| std::fs::read_to_string(event_path).map_err(Error::from))
-        .context("Failed to read GitHub event path")?;
+pub fn find_base_sha(remote_name: &str) -> Result<Option<String>> {


m: As we no longer ever return Ok(None) from this function, we should simplify the signature to return Result<String>.

Suggested change

pub fn find_base_sha(remote_name: &str) -> Result<Option<String>> {

pub fn find_base_sha(remote_name: &str) -> Result<String> {

The intention is that if the merge-base could not be found we return None. If that is not what the Rust code:

let remote_commit = remote_ref.peel_to_commit()?; let merge_base_oid = repo.merge_base(head_commit.id(), remote_commit.id())?;

does could you suggest the best way to make it do that?

No, that is not what this code does. The ? will propagate any Err returned by these functions, causing find_base_sha to return Err(<whatever error from the method>).

Will add my suggestion for how to do this below

szokeasaurusrex

Looks good for the most part, did notice a few improvements (and a typo), however.

Note: I am using the LOGAF scale in my comments

src/utils/vcs.rs

szokeasaurusrex · 2025-10-31T10:54:57Z

src/utils/vcs.rs

+
+    let repo = git2::Repository::open_from_env()?;
+
+    let head_commit = repo.head()?.peel_to_commit()?;


l: Context for better error message:

Suggested change

let head_commit = repo.head()?.peel_to_commit()?;

let head_commit = repo.head()

.and_then(|h| h.peel_to_commit())

.context("Could not find head commit")?;

@chromy are you also wanting to return None if this call fails?

No I think this one should probably Error. It's legitimate that there might not be a merge-base for two commits, you can get it if the history is separate e.g.

$ git commit -m'foo' --allow-empty [main (root-commit) fa819e8] foo $ git commit -m'bar' --allow-empty [main c35d2ee] bar $ git checkout --orphan baz Switched to a new branch 'baz' $ git commit -m'baz' --allow-empty [baz (root-commit) 27134ca] baz $ git merge-base main baz $ git merge-base HEAD baz 27134ca5b445526335d7a8d955790fac1e5d3690

This one:

$ git merge-base main baz

returns nothing because there is no common commit in the history of the main branch and the orphan baz branch.

However I don't think there can be a legitimate case where HEAD doesn't point at a commit. I think that will only happen if the repo is in quite a corrupt state.

szokeasaurusrex · 2025-10-31T10:56:03Z

src/utils/vcs.rs

+    let remote_commit = remote_ref.peel_to_commit()?;
+    let merge_base_oid = repo.merge_base(head_commit.id(), remote_commit.id())?;


l: Would also add context for these

As above: the intention is that if the merge-base could not be found we return None.

I don't think there is value in adding context to peel_to_commit()

szokeasaurusrex · 2025-10-31T11:00:05Z

CHANGELOG.md


+### Fixes
+
+- Fix auto filled git metadata when using the `build upload` subcommand in non-github workflow contexts ([#2897](https://github.com/getsentry/sentry-cli/pull/2897))


m: Can you please add some more detail to this changelog entry/make it more user-friendly? E.g. by being more specific of what metadata we can now add in non-GH workflow contexts? I have been putting more effort into creating human-readable changelog entries the past few releases.

l: Also, I would consider this more of a new feature than a fix; can we put it in a ### New Features section? (and if we do, please move the section above deprecations)

Maybe this works:

Suggested change

### Fixes

- Fix auto filled git metadata when using the `build upload` subcommand in non-github workflow contexts ([#2897](https://github.com/getsentry/sentry-cli/pull/2897))

### New Features

- `build upload` now supports automatic git base commit detection in local development and all CI environments, not just GitHub Actions ([#2897](https://github.com/getsentry/sentry-cli/pull/2897))

I've been more specific in the message. I don't think it's a feature. This was intended behaviour since this was introduced (as seen in the head code and also in the calling code, that it did not work previously is a bug.

…a_from_event

@szokeasaurusrex

`find_head_sha` supported both Github workflow contexts and 'raw git' contexts however the matching function `find_base_sha` did not handle raw git contexts. Thi updates `find_base_sha` to return the equivalent of: `git merge-base HEAD origin/HEAD`. For: ``` o---o---o---origin/main / ---1---o---o---o---foo ``` We return the SHA of 1. Apply suggestion from @szokeasaurusrex Co-authored-by: Daniel Szoke <[email protected]> Update src/utils/vcs.rs Co-authored-by: Daniel Szoke <[email protected]>

szokeasaurusrex · 2025-11-03T09:48:49Z

src/utils/vcs.rs

-        .map_err(Error::from)
-        .and_then(|event_path| std::fs::read_to_string(event_path).map_err(Error::from))
-        .context("Failed to read GitHub event path")?;
+pub fn find_base_sha(remote_name: &str) -> Result<Option<String>> {


No, that is not what this code does. The ? will propagate any Err returned by these functions, causing find_base_sha to return Err(<whatever error from the method>).

Will add my suggestion for how to do this below

src/utils/vcs.rs

szokeasaurusrex · 2025-11-03T09:59:59Z

src/utils/vcs.rs

+
+    let repo = git2::Repository::open_from_env()?;
+
+    let head_commit = repo.head()?.peel_to_commit()?;


@chromy are you also wanting to return None if this call fails?

CHANGELOG.md

Co-authored-by: Daniel Szoke <[email protected]>

chromy requested review from a team and szokeasaurusrex as code owners October 30, 2025 15:52

chromy marked this pull request as draft October 30, 2025 15:53

chromy changed the base branch from master to chromy/2025-10-29-fix-base-part-1 October 30, 2025 16:49

chromy force-pushed the chromy/2025-10-29-fix-base-part-2 branch from 4433ce5 to 4f1cba4 Compare October 30, 2025 16:53

chromy marked this pull request as ready for review October 31, 2025 09:48

chromy mentioned this pull request Oct 31, 2025

fix(vcs): Fix base_ref / base_sha for non-github actions #2888

Closed

szokeasaurusrex reviewed Oct 31, 2025

View reviewed changes

chromy force-pushed the chromy/2025-10-29-fix-base-part-1 branch from e4ec720 to cf24ed0 Compare October 31, 2025 11:23

Base automatically changed from chromy/2025-10-29-fix-base-part-1 to master October 31, 2025 11:29

This comment was marked as outdated.

Sign in to view

ref(vcs): Make extract_pr_base_sha_from_event like extract_pr_head_sh…

4fee058

…a_from_event

chromy force-pushed the chromy/2025-10-29-fix-base-part-2 branch from 3827998 to 64bbabb Compare October 31, 2025 11:47

chromy force-pushed the chromy/2025-10-29-fix-base-part-2 branch from 64bbabb to 629a6a8 Compare October 31, 2025 12:07

szokeasaurusrex reviewed Nov 3, 2025

View reviewed changes

chromy and others added 2 commits November 3, 2025 11:35

Update CHANGELOG.md

7a383a6

Co-authored-by: Daniel Szoke <[email protected]>

Update src/utils/vcs.rs

5bbf531

Co-authored-by: Daniel Szoke <[email protected]>

szokeasaurusrex approved these changes Nov 3, 2025

View reviewed changes

chromy merged commit 1c7fcdc into master Nov 3, 2025
27 checks passed

chromy deleted the chromy/2025-10-29-fix-base-part-2 branch November 3, 2025 13:06

BrewTestBot mentioned this pull request Nov 6, 2025

sentry-cli 2.58.0 Homebrew/homebrew-core#253203

Merged

github-actions bot mentioned this pull request Nov 9, 2025

Update Sentry CLI from 2.57.0 to 2.58.0 ioki-mobility/SentryProguardGradlePlugin#247

Closed

	pub fn find_base_sha(remote_name: &str) -> Result<Option<String>> {
	pub fn find_base_sha(remote_name: &str) -> Result<String> {


		let repo = git2::Repository::open_from_env()?;

		let head_commit = repo.head()?.peel_to_commit()?;

		let remote_commit = remote_ref.peel_to_commit()?;
		let merge_base_oid = repo.merge_base(head_commit.id(), remote_commit.id())?;


		### Fixes

		- Fix auto filled git metadata when using the `build upload` subcommand in non-github workflow contexts ([#2897](https://github.com/getsentry/sentry-cli/pull/2897))

Uh oh!

fix(vcs): Handle non-github workflow in find_base_sha #2897

fix(vcs): Handle non-github workflow in find_base_sha #2897

Uh oh!

Conversation

chromy commented Oct 30, 2025

Uh oh!

szokeasaurusrex Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

szokeasaurusrex Oct 31, 2025 •

edited

Loading