Skip to content
Merged
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
2 changes: 1 addition & 1 deletion src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2525,7 +2525,7 @@ struct LogsResponse {
data: Vec<LogEntry>,
}

/// VCS information for mobile app uploads
/// VCS information for build app uploads
#[derive(Debug)]
pub struct VcsInfo<'a> {
pub head_sha: Option<&'a str>,
Expand Down
43 changes: 34 additions & 9 deletions src/commands/build/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::utils::fs::TempDir;
use crate::utils::fs::TempFile;
use crate::utils::progress::ProgressBar;
use crate::utils::vcs::{
self, get_provider_from_remote, get_repo_from_remote, git_repo_remote_url,
self, get_provider_from_remote, get_repo_from_remote, git_repo_head_ref, git_repo_remote_url,
};

pub fn make_command(command: Command) -> Command {
Expand Down Expand Up @@ -106,12 +106,13 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {

let cached_remote = config.get_cached_vcs_remote();
// Try to open the git repository and find the remote, but handle errors gracefully.
let (vcs_provider, head_repo_name) = {
let (vcs_provider, head_repo_name, head_ref) = {
// Try to open the repo and get the remote URL, but don't fail if not in a repo.
let repo = git2::Repository::open_from_env().ok();
let remote_url = repo.and_then(|repo| git_repo_remote_url(&repo, &cached_remote).ok());
let repo_ref = repo.as_ref();
let remote_url = repo_ref.and_then(|repo| git_repo_remote_url(repo, &cached_remote).ok());

let vcs_provider: Option<Cow<'_, str>> = matches
let vcs_provider = matches
.get_one("vcs_provider")
.map(String::as_str)
.map(Cow::Borrowed)
Expand All @@ -122,7 +123,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
.map(Cow::Owned)
});

let head_repo_name: Option<Cow<'_, str>> = matches
let head_repo_name = matches
.get_one("head_repo_name")
.map(String::as_str)
.map(Cow::Borrowed)
Expand All @@ -133,13 +134,37 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
.map(Cow::Owned)
});

(vcs_provider, head_repo_name)
let head_ref = matches
.get_one("head_ref")
.map(String::as_str)
.map(Cow::Borrowed)
.or_else(|| {
// Try to get the current ref from the VCS if not provided
// Note: git_repo_head_ref will return an error for detached HEAD states,
// which the error handling converts to None - this prevents sending "HEAD" as a branch name
// In that case, the user will need to provide a valid branch name.
Copy link
Member

Choose a reason for hiding this comment

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

[question] Does this imply that the head_ref is required? From what I can tell right now, we don't validate its presence locally, so if it is required by the server, we would likely get a 4xx response when attempting the upload. If that is the case, we should likely return an error here, rather than swallow the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not required, it is optional on the backend so no response failure if not provided.

repo_ref
.and_then(|r| match git_repo_head_ref(r) {
Ok(ref_name) => {
debug!("Found current branch reference: {}", ref_name);
Some(ref_name)
}
Err(e) => {
debug!(
"No valid branch reference found (likely detached HEAD): {}",
e
);
None
}
})
.map(Cow::Owned)
});

(vcs_provider, head_repo_name, head_ref)
};

let base_repo_name = matches.get_one("base_repo_name").map(String::as_str);

let base_sha = matches.get_one("base_sha").map(String::as_str);
let head_ref = matches.get_one("head_ref").map(String::as_str);
let base_ref = matches.get_one("base_ref").map(String::as_str);
let pr_number = matches.get_one::<u32>("pr_number");

Expand Down Expand Up @@ -203,7 +228,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
vcs_provider: vcs_provider.as_deref(),
head_repo_name: head_repo_name.as_deref(),
base_repo_name,
head_ref,
head_ref: head_ref.as_deref(),
base_ref,
pr_number,
};
Expand Down
56 changes: 56 additions & 0 deletions src/utils/vcs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,23 @@ pub fn git_repo_remote_url(
.ok_or_else(|| git2::Error::from_str("No remote URL found"))
}

pub fn git_repo_head_ref(repo: &git2::Repository) -> Result<String> {
let head = repo.head()?;

// Only return a reference name if we're not in a detached HEAD state
// In detached HEAD state, head.shorthand() returns "HEAD" which is not a valid branch name
if head.is_branch() {
head.shorthand()
.map(|s| s.to_owned())
.ok_or_else(|| anyhow::anyhow!("No HEAD reference found"))
} else {
// In detached HEAD state, return an error to indicate no valid branch reference
Err(anyhow::anyhow!(
"HEAD is detached - no branch reference available"
))
}
}

fn find_reference_url(repo: &str, repos: &[Repo]) -> Result<Option<String>> {
let mut non_git = false;
for configured_repo in repos {
Expand Down Expand Up @@ -881,6 +898,14 @@ fn git_initialize_repo() -> TempDir {
.wait()
.expect("Failed to wait on `git init`.");

Command::new("git")
.args(["branch", "-M", "main"])
.current_dir(&dir)
.spawn()
.expect("Failed to execute `git branch`.")
.wait()
.expect("Failed to wait on `git branch`.");

Command::new("git")
.args(["config", "--local", "user.name", "test"])
.current_dir(&dir)
Expand Down Expand Up @@ -1188,3 +1213,34 @@ fn test_generate_patch_ignore_missing() {
".*.timestamp" => "[timestamp]"
});
}

#[test]
fn test_git_repo_head_ref() {
let dir = git_initialize_repo();

// Create initial commit
git_create_commit(
dir.path(),
"foo.js",
b"console.log(\"Hello, world!\");",
"\"initial commit\"",
);

let repo = git2::Repository::open(dir.path()).expect("Failed");

// Test on a branch (should succeed)
let head_ref = git_repo_head_ref(&repo).expect("Should get branch reference");
assert_eq!(head_ref, "main");

// Test in detached HEAD state (should fail)
let head_commit = repo.head().unwrap().target().unwrap();
repo.set_head_detached(head_commit)
.expect("Failed to detach HEAD");

let head_ref_result = git_repo_head_ref(&repo);
assert!(head_ref_result.is_err());
assert_eq!(
head_ref_result.unwrap_err().to_string(),
"HEAD is detached - no branch reference available"
);
}