Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- Deprecated the `upload-proguard` subcommand's `--platform` flag ([#2863](https://github.com/getsentry/sentry-cli/pull/2863)). This flag appears to have been a no-op for some time, so we will remove it in the next major.

### Fixes

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

## 2.57.0

### New Features
Expand Down
2 changes: 1 addition & 1 deletion src/commands/build/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
.map(String::as_str)
.map(Cow::Borrowed)
.or_else(|| {
vcs::find_base_sha()
vcs::find_base_sha(&cached_remote)
.inspect_err(|e| debug!("Error finding base SHA: {e}"))
.ok()
.flatten()
Expand Down
101 changes: 57 additions & 44 deletions src/utils/vcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::fmt;
use std::path::PathBuf;

use anyhow::{bail, format_err, Context as _, Error, Result};
use anyhow::{bail, format_err, Error, Result};
use chrono::{DateTime, FixedOffset, TimeZone as _};
use git2::{Commit, Repository, Time};
use if_chain::if_chain;
Expand Down Expand Up @@ -272,30 +272,27 @@ pub fn git_repo_head_ref(repo: &git2::Repository) -> Result<String> {
}

pub fn git_repo_base_ref(repo: &git2::Repository, remote_name: &str) -> Result<String> {
// Get the current HEAD commit
let head_commit = repo.head()?.peel_to_commit()?;

// Try to find the remote tracking branch
let remote_branch_name = format!("refs/remotes/{remote_name}/HEAD");
let remote_ref = repo.find_reference(&remote_branch_name).map_err(|e| {
anyhow::anyhow!("Could not find remote tracking branch for {remote_name}: {e}")
})?;

find_merge_base_ref(repo, &head_commit, &remote_ref)
}

fn find_merge_base_ref(
repo: &git2::Repository,
head_commit: &git2::Commit,
remote_ref: &git2::Reference,
) -> Result<String> {
let remote_commit = remote_ref.peel_to_commit()?;
let merge_base_oid = repo.merge_base(head_commit.id(), remote_commit.id())?;

// Return the merge-base commit SHA as the base reference
let merge_base_sha = merge_base_oid.to_string();
debug!("Found merge-base commit as base reference: {merge_base_sha}");
Ok(merge_base_sha)
let name = remote_ref
.resolve()?
.shorthand()
.ok_or(anyhow::anyhow!(
"Could not find remote tracking branch for {remote_name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shorthand() returns None if the shorthand is not valid UTF-8.

Suggested change
"Could not find remote tracking branch for {remote_name}"
"Remote branch name is not valid UTF-8"

))?
.to_owned();

let expected_prefix = format!("{remote_name}/");
if let Some(branch_name) = name.strip_prefix(&expected_prefix) {
Ok(branch_name.to_owned())
} else {
Err(anyhow::anyhow!(
"Remote branch name '{name}' does not start with expected prefix '{expected_prefix}'"
))
}
}

/// Like git_repo_base_repo_name but preserves the original case of the repository name.
Expand Down Expand Up @@ -569,13 +566,31 @@ pub fn find_head_sha() -> Result<String> {
Ok(head.id().to_string())
}

pub fn find_base_sha() -> Result<Option<String>> {
let github_event = std::env::var("GITHUB_EVENT_PATH")
.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>> {
if let Some(pr_base_sha) = std::env::var("GITHUB_EVENT_PATH")
.ok()
.and_then(|event_path| std::fs::read_to_string(event_path).ok())
.and_then(|content| extract_pr_base_sha_from_event(&content))
{
debug!("Using GitHub Actions PR base SHA from event payload: {pr_base_sha}");
return Ok(Some(pr_base_sha));
}

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

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

extract_pr_base_sha_from_event(&github_event)
// Try to find the remote tracking branch
let remote_branch_name = format!("refs/remotes/{remote_name}/HEAD");
let remote_ref = repo.find_reference(&remote_branch_name).map_err(|e| {
anyhow::anyhow!("Could not find remote tracking branch for {remote_name}: {e}")
})?;

let remote_commit = remote_ref.peel_to_commit()?;
let merge_base_oid = repo.merge_base(head_commit.id(), remote_commit.id())?;
let merge_base_sha = merge_base_oid.to_string();
debug!("Found merge-base commit as base reference: {merge_base_sha}");
Ok(Some(merge_base_sha))
}

/// Extracts the PR head SHA from GitHub Actions event payload JSON.
Expand All @@ -595,15 +610,19 @@ fn extract_pr_head_sha_from_event(json_content: &str) -> Option<String> {
}

/// Extracts the PR base SHA from GitHub Actions event payload JSON.
/// Returns Ok(None) if not a PR event or if SHA cannot be extracted.
/// Returns an error if we cannot parse the JSON.
fn extract_pr_base_sha_from_event(json_content: &str) -> Result<Option<String>> {
let v: Value = serde_json::from_str(json_content)
.context("Failed to parse GitHub event payload as JSON")?;
/// Returns None if not a PR event or if SHA cannot be extracted.
fn extract_pr_base_sha_from_event(json_content: &str) -> Option<String> {
let v: Value = match serde_json::from_str(json_content) {
Ok(v) => v,
Err(_) => {
debug!("Failed to parse GitHub event payload as JSON");
return None;
}
};

Ok(v.pointer("/pull_request/base/sha")
v.pointer("/pull_request/base/sha")
.and_then(|s| s.as_str())
.map(|s| s.to_owned()))
.map(|s| s.to_owned())
}
Comment on lines 612 to 626
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason for this function to change. If we want to debug-log the error message rather than error, the calling code should change how it handles the error returned from this function.

Suggested change
/// Extracts the PR base SHA from GitHub Actions event payload JSON.
/// Returns Ok(None) if not a PR event or if SHA cannot be extracted.
/// Returns an error if we cannot parse the JSON.
fn extract_pr_base_sha_from_event(json_content: &str) -> Result<Option<String>> {
let v: Value = serde_json::from_str(json_content)
.context("Failed to parse GitHub event payload as JSON")?;
/// Returns None if not a PR event or if SHA cannot be extracted.
fn extract_pr_base_sha_from_event(json_content: &str) -> Option<String> {
let v: Value = match serde_json::from_str(json_content) {
Ok(v) => v,
Err(_) => {
debug!("Failed to parse GitHub event payload as JSON");
return None;
}
};
Ok(v.pointer("/pull_request/base/sha")
v.pointer("/pull_request/base/sha")
.and_then(|s| s.as_str())
.map(|s| s.to_owned()))
.map(|s| s.to_owned())
}
/// Extracts the PR base SHA from GitHub Actions event payload JSON.
/// Returns Ok(None) if not a PR event or if SHA cannot be extracted.
/// Returns an error if we cannot parse the JSON.
fn extract_pr_base_sha_from_event(json_content: &str) -> Result<Option<String>> {
let v: Value = serde_json::from_str(json_content)
.context("Failed to parse GitHub event payload as JSON")?;
Ok(v.pointer("/pull_request/base/sha")
.and_then(|s| s.as_str())
.map(|s| s.to_owned()))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, there are two functions:

  1. extract_pr_head_sha_from_event
  2. extract_pr_base_sha_from_event

They ought to match exactly except for which SHA they extract.
Previously they didn't have the same implementation or API Result<Option<String>> vs. Option<String>. It does not make sense for one to handle invalid json and not the other.

Since invalid json from the Github API is (hopefully) rare I don't think it makes sense to push handling the case on to callers (making all calling code more complicated)*. Better options would be:

  • crashing
  • returning None and logging

I would generally lean towards crashing but since extract_pr_head_sha_from_event already had that behavior I matched that.

*There is actually only one real caller so I was tempted to inline these and simplify the code (c.f. http://number-none.com/blow/john_carmack_on_inlined_code.html) but since it has some nice tests I left it.

I have split this into it's own PR here: #2893


/// Given commit specs, repos and remote_name this returns a list of head
Expand Down Expand Up @@ -1705,7 +1724,7 @@ mod tests {
.to_string();

assert_eq!(
extract_pr_base_sha_from_event(&pr_json).unwrap(),
extract_pr_base_sha_from_event(&pr_json),
Some("55e6bc8c264ce95164314275d805f477650c440d".to_owned())
);

Expand All @@ -1719,10 +1738,7 @@ mod tests {
})
.to_string();

assert_eq!(extract_pr_base_sha_from_event(&push_json).unwrap(), None);

// Test with malformed JSON
assert!(extract_pr_base_sha_from_event("invalid json {").is_err());
assert_eq!(extract_pr_base_sha_from_event(&push_json), None);

// Test with missing base SHA
let incomplete_json = r#"{
Expand All @@ -1733,10 +1749,7 @@ mod tests {
}
}"#;

assert_eq!(
extract_pr_base_sha_from_event(incomplete_json).unwrap(),
None
);
assert_eq!(extract_pr_base_sha_from_event(incomplete_json), None);
}

#[test]
Expand Down Expand Up @@ -1765,15 +1778,15 @@ mod tests {
fs::write(&event_file, pr_json).expect("Failed to write event file");
std::env::set_var("GITHUB_EVENT_PATH", event_file.to_str().unwrap());

let result = find_base_sha();
let result = find_base_sha("origin");
assert_eq!(
result.unwrap().unwrap(),
"55e6bc8c264ce95164314275d805f477650c440d"
);

// Test without GITHUB_EVENT_PATH
std::env::remove_var("GITHUB_EVENT_PATH");
let result = find_base_sha();
let result = find_base_sha("origin");
assert!(result.is_err());
}
}