feat(build): Auto-detect base_ref from git merge-base #2720
Conversation
runningcode
force-pushed
the
feat/build-upload-auto-base-ref
branch
from
e3c6ea9 to
cca503f
Compare
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.
runningcode
force-pushed
the
feat/build-upload-auto-base-ref
branch
from
cca503f to
1a0e73b
Compare
src/commands/build/upload.rs
Outdated
| debug!("Error getting base branch reference: {}", e); | ||
| None |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
runningcode
force-pushed
the
feat/build-upload-auto-base-ref
branch
from
357b35f to
1f1d297
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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(); |
There was a problem hiding this comment.
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!
| 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(); |
There was a problem hiding this comment.
This is how you can handle the error gracefully
| let default_branch = default_branch_buf.as_str().unwrap(); | |
| let default_branch = default_branch_buf | |
| .as_str() | |
| .ok_or_else(|| anyhow::anyhow!("Default branch contains invalid UTF-8"))?; |
| .strip_prefix("refs/heads/") | ||
| .unwrap_or(default_branch); | ||
| let remote_branch = format!("refs/remotes/{remote_name}/{branch_name}"); | ||
| repo.find_reference(&remote_branch) |
There was a problem hiding this comment.
As the return type of this closure will now be an anyhow::Error, we need a small refactor here, to convert to the new error type
| repo.find_reference(&remote_branch) | |
| Ok(repo.find_reference(&remote_branch)?) |
| let remote_branch = format!("refs/remotes/{remote_name}/{branch_name}"); | ||
| repo.find_reference(&remote_branch) | ||
| }) | ||
| .map_err(|e| { |
There was a problem hiding this comment.
And, we also need a type annotation here, as the return value of the previous closure can no longer be inferred
| .map_err(|e| { | |
| .map_err(|e: Error| { |
- 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 <[email protected]>
Add automatic detection of base reference for build uploads by finding
the merge-base between current HEAD and remote tracking branch.
This works by:
default_branch)This mirrors the existing head_ref auto-detection behavior.