feat(build): Auto-detect base_ref from git merge-base #2720

runningcode · 2025-08-28T17:06:19Z

Auto-detect base reference for build uploads using git merge-base.

Finds the merge-base commit SHA between HEAD and the remote tracking branch (refs/remotes/{remote}/HEAD), eliminating the need to manually specify --base-sha for most workflows.

In the future, we will add more checks for common CI providers to automatically get this value as well.

src/commands/build/upload.rs

src/utils/vcs.rs

szokeasaurusrex · 2025-08-29T10:51:57Z

src/commands/build/upload.rs

+                            debug!("Error getting base branch reference: {}", e);
+                            None


Why do we swallow this error without propagating it to the user? Seems like potentially undesired behavior, as if something is broken, the user will not be made aware, unless they have debug logging turned on (which is unlikely in most use cases).

This is a good question. I would expect this to fail if the user isn't using git or has some non default branching strategy.
The question is how do we want to handle this.

Throw an error at the CLI level.

Show some error message after they've uploaded in the UI if the git information is missing.

I don't have a strong preference for 1 or 2, it just how we want the user experience to be. Throwing an error here might be frustrating if trying to get this working initially but once the integration is working throwing an error would be the easiest way to know that a change broke the integration.

Actually I found a better way to do this without iterating through the branches. Just updated the logic.

szokeasaurusrex

I have a couple questions about the implementation, plus a suggestion for how to simplify the code a bit

src/utils/vcs.rs

szokeasaurusrex · 2025-08-29T13:29:08Z

src/utils/vcs.rs

+            let mut remote = repo.find_remote(remote_name)?;
+            remote.connect(git2::Direction::Fetch)?;
+            let default_branch_buf = remote.default_branch()?;
+            let default_branch = default_branch_buf.as_str().unwrap();


ChatGPT seems to think not, but the fact that the value returned by the git2 library (which is just Rust bindings for libgit2) is not a string and only performs a fallible conversion to str makes me think that we should perhaps be cautious. There is probably a reason why git2 is implemented this way; perhaps in some cases on some platforms a non-UTF-8 value is possible. It does not really cost anything to handle the error gracefully, without panicking, so let's do that!

src/utils/vcs.rs

runningcode · 2025-09-03T11:34:04Z

Thanks for the suggestions @szokeasaurusrex ! I applied them.

szokeasaurusrex

Please see my reply to the Cursor comment

src/utils/vcs.rs

runningcode · 2025-09-03T15:45:51Z

@szokeasaurusrex I couldn’t reply directly to your comment but that’s a good catch. I’m mostly relying on AI here.
It seems that git2/libgit2 doesn't provide a direct way to query "which branches point to this specific commit”.

Alternatively: we could call out to a shell with git branch --points-at?

szokeasaurusrex · 2025-09-03T15:53:39Z

Let's avoid calling git directly, as adding a dependency on git is potentially a breaking change.

Idk how git works internally, but maybe it's possible that git itself only links from branches to commits, and not the other way around. In which case, it would make no difference that we are manually iterating.

So I'd say let's stick with how we have the code now. It would likely be preferable to write as an iterator chain instead of a loop though (I can also perform the rewrite for you)

runningcode · 2025-09-04T07:59:43Z

Sounds good! I’ve fixed the tests and used an iterator chain.

runningcode · 2025-09-08T07:23:17Z

@szokeasaurusrex I’ve simplified the code here. It just tries to get the remote tracking branch as origin/HEADand then finds the merge base with HEAD. This is what we previously did at Emerge tools.

Cursor correctly comments that this won’t work in all cases. We will log a warning when it doesn’t work.
In future PRs, I will add support for common CI providers to calculate the merge base using environment variables on CI, for example GITHUB_BASE_REF on github.

szokeasaurusrex

This is looking much better! After these changes, though, it looks like we can make some small simplifications of the function signatures

szokeasaurusrex · 2025-09-05T09:21:54Z

src/utils/vcs.rs

+    let remote_branch_name = format!("refs/remotes/{remote_name}/HEAD");
+    let remote_ref = repo
+        .find_reference(&remote_branch_name)
+        .or_else(|_| -> Result<git2::Reference, anyhow::Error> {


Result here is anyhow::Result (imported above), as it is in most parts of Sentry CLI. With anyhow::Result, the error value is implicitly anyhow::Error when not provided. Using Result<git2::Reference> is therefore simpler here.

However, in this case, the entire type annotation is unnecessary, as the compiler can infer it:

Suggested change

.or_else(|_| -> Result<git2::Reference, anyhow::Error> {

.or_else(|_| {

szokeasaurusrex · 2025-09-05T09:24:47Z

src/utils/vcs.rs

+            let remote_branch = format!("refs/remotes/{remote_name}/{branch_name}");
+            Ok(repo.find_reference(&remote_branch)?)
+        })
+        .map_err(|e: anyhow::Error| {


Seems we don't need this annotation anymore, either 😅

Suggested change

.map_err(|e: anyhow::Error| {

.map_err(|e| {

szokeasaurusrex · 2025-09-08T10:05:02Z

src/utils/vcs.rs

+    repo: &git2::Repository,
+    head_commit: &git2::Commit,
+    remote_ref: &git2::Reference,
+) -> Result<Option<String>> {


From what I can tell, we are never returning Ok(None), so I think we can make this function return Result<String> instead

Assuming I am correct, we should also be able to make git_repo_base_ref return a Result<String>

szokeasaurusrex · 2025-09-08T10:07:50Z

@runningcode please update the PR description to reflect the new behavior, e.g. that we are now no longer looking for the default_branch. I think this may be why Cursor is complaining (or at least, one of the reasons why)

Add automatic detection of base reference for build uploads by finding the merge-base between current HEAD and remote tracking branch. This works by: - Finding the remote tracking branch (origin/HEAD, main, master, develop) - Calculating git merge-base between HEAD and remote branch - Returning the branch name pointing to merge-base or commit SHA This mirrors the existing head_ref auto-detection behavior.

- Add debug logging when base_ref is found - Use info level logging when base_ref cannot be detected for CI visibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Fix UTF-8 error handling with proper anyhow::Error conversion - Add explicit type annotations for closure error handling - Use anyhow::Error consistently instead of git2::Error for better error messages - Implement cleaner error propagation as suggested in review 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Updated logging for base ref detection failures from info to warning level to make these important failure cases more visible to users. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

The new base ref auto-detection feature logs warnings when it cannot find a local branch that points to the merge-base. Updated integration tests to expect these warning messages in their output. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

The CI environment gets SSL/TLS errors when trying to connect to git remotes, resulting in "Could not detect base branch reference: ..." messages instead of the simpler local messages. Updated test expectations to use flexible patterns that match both local and CI error scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

… loop Replace the explicit for loop with a more idiomatic iterator chain using find_map(). This addresses the PR review comment about simplifying the branching code with method chaining. The new implementation: - Uses find_map() to search and transform in one step - Eliminates the explicit for loop and early returns - Maintains the same functionality with cleaner, more idiomatic Rust 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

…anch name Previously the base ref detection would only work if a local branch pointed to the exact merge-base commit. This implementation now returns the merge-base commit SHA directly, enabling automatic base reference detection in all cases where a merge-base can be determined. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

Remove the network fallback that queries remote default branch when origin/HEAD is not set locally. This eliminates SSL/TLS errors that occur when libgit2 lacks proper SSL support. Users without origin/HEAD set up will get a clear error message and can fix it with: git remote set-head origin --auto 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

The git_repo_base_ref and find_merge_base_ref functions never return Ok(None), so simplified return types and removed unreachable code paths. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

runningcode · 2025-09-08T13:21:54Z

Updated PR description!

szokeasaurusrex

Much simpler now, thank you 🚀

runningcode force-pushed the feat/build-upload-auto-base-ref branch 2 times, most recently from cca503f to 1a0e73b Compare August 28, 2025 17:23

runningcode requested a review from rbro112 August 28, 2025 17:25

rbro112 reviewed Aug 28, 2025

View reviewed changes

src/commands/build/upload.rs Outdated Show resolved Hide resolved

rbro112 reviewed Aug 28, 2025

View reviewed changes

src/utils/vcs.rs Outdated Show resolved Hide resolved

runningcode marked this pull request as ready for review August 29, 2025 10:15

runningcode requested review from a team and szokeasaurusrex as code owners August 29, 2025 10:15

szokeasaurusrex reviewed Aug 29, 2025

View reviewed changes

src/utils/vcs.rs Outdated Show resolved Hide resolved

src/utils/vcs.rs Outdated Show resolved Hide resolved