Skip to content
Open
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
3 changes: 1 addition & 2 deletions crates/lib/src/api/client/repositories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,7 @@ mod tests {
let mock_pre_push = server.mock("POST", &path[..]).create_async().await;

// Branch and commit id we're pushing
let branch =
repositories::branches::get_by_name(&local_repo, DEFAULT_BRANCH_NAME)?.unwrap();
let branch = repositories::branches::get_by_name(&local_repo, DEFAULT_BRANCH_NAME)?;
let commit_id = branch.commit_id.clone();

api::client::repositories::pre_push(&remote_repo, &branch, &commit_id).await?;
Expand Down
3 changes: 1 addition & 2 deletions crates/lib/src/core/v_latest/branches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,7 @@ pub async fn checkout(
from_commit: &Option<Commit>,
) -> Result<(), OxenError> {
log::debug!("checkout {branch_name}");
let branch = repositories::branches::get_by_name(repo, branch_name)?
.ok_or_else(|| OxenError::local_branch_not_found(branch_name))?;
let branch = repositories::branches::get_by_name(repo, branch_name)?;

let commit = repositories::commits::get_by_id(repo, &branch.commit_id)?
.ok_or_else(|| OxenError::commit_id_does_not_exist(&branch.commit_id))?;
Expand Down
1 change: 0 additions & 1 deletion crates/lib/src/core/v_latest/commits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ fn get_commit_by_ref<S: AsRef<str> + Clone>(
fn get_commit_by_branch(repo: &LocalRepository, branch_name: &str) -> Option<Commit> {
repositories::branches::get_by_name(repo, branch_name)
.ok()
.flatten()
.and_then(|branch| get_by_id(repo, &branch.commit_id).ok().flatten())
}

Expand Down
3 changes: 1 addition & 2 deletions crates/lib/src/core/v_latest/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ pub async fn merge(
) -> Result<Option<Commit>, OxenError> {
let branch_name = branch_name.as_ref();

let merge_branch = repositories::branches::get_by_name(repo, branch_name)?
.ok_or_else(|| OxenError::local_branch_not_found(branch_name))?;
let merge_branch = repositories::branches::get_by_name(repo, branch_name)?;

let base_commit = repositories::commits::head_commit(repo)?;
let merge_commit = get_commit_or_head(repo, Some(merge_branch.commit_id.clone()))?;
Expand Down
4 changes: 1 addition & 3 deletions crates/lib/src/core/v_latest/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ pub async fn push_remote_branch(
// start a timer
let start = std::time::Instant::now();

let Some(local_branch) = repositories::branches::get_by_name(repo, &opts.branch)? else {
return Err(OxenError::local_branch_not_found(&opts.branch));
};
let local_branch = repositories::branches::get_by_name(repo, &opts.branch)?;

println!(
"🐂 oxen push {} {} -> {}",
Expand Down
25 changes: 9 additions & 16 deletions crates/lib/src/core/v_latest/workspaces/commit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,16 @@ pub async fn commit(
let repo = &workspace.base_repo;
let commit = &workspace.commit;

let mut branch = repositories::branches::get_by_name(repo, branch_name)?;
let branch = match repositories::branches::get_by_name(repo, branch_name) {
Ok(branch) => branch,
Err(OxenError::BranchNotFound(_)) => {
log::debug!("commit creating branch: {branch_name}");
repositories::branches::create(repo, branch_name, &commit.id)?
}
Err(e) => return Err(e),
};
log::debug!("commit looking up branch: {:#?}", &branch);

if branch.is_none() {
log::debug!("commit creating branch: {branch_name}");
branch = Some(repositories::branches::create(
repo,
branch_name,
&commit.id,
)?);
}

let branch = branch.unwrap();

let staged_db_path = util::fs::oxen_hidden_dir(&workspace.workspace_repo.path).join(STAGED_DIR);

log::debug!("workspaces::commit staged db path: {staged_db_path:?}");
Expand Down Expand Up @@ -108,10 +104,7 @@ pub fn mergeability(
branch_name: impl AsRef<str>,
) -> Result<Mergeable, OxenError> {
let branch_name = branch_name.as_ref();
let Some(branch) = repositories::branches::get_by_name(&workspace.base_repo, branch_name)?
else {
return Err(OxenError::BranchNotFound(branch_name.into()));
};
let branch = repositories::branches::get_by_name(&workspace.base_repo, branch_name)?;

let base = &workspace.commit;
let Some(head) = repositories::commits::get_by_id(&workspace.base_repo, &branch.commit_id)?
Expand Down
28 changes: 11 additions & 17 deletions crates/lib/src/repositories/branches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ pub fn list_with_commits(repo: &LocalRepository) -> Result<Vec<(Branch, Commit)>
with_ref_manager(repo, |manager| manager.list_branches_with_commits())
}

/// Get a branch by name
pub fn get_by_name(repo: &LocalRepository, name: &str) -> Result<Option<Branch>, OxenError> {
with_ref_manager(repo, |manager| manager.get_branch_by_name(name))
/// Get a branch by name, returning an error if it doesn't exist
pub fn get_by_name(repo: &LocalRepository, name: &str) -> Result<Branch, OxenError> {
with_ref_manager(repo, |manager| manager.get_branch_by_name(name))?
.ok_or_else(|| OxenError::local_branch_not_found(name))
}

/// Get branch by name or fall back the current
Expand All @@ -34,10 +35,7 @@ pub fn get_by_name_or_current(
) -> Result<Branch, OxenError> {
if let Some(branch_name) = branch_name {
let branch_name = branch_name.as_ref();
match repositories::branches::get_by_name(repo, branch_name)? {
Some(branch) => Ok(branch),
None => Err(OxenError::local_branch_not_found(branch_name)),
}
repositories::branches::get_by_name(repo, branch_name)
} else {
match repositories::branches::current_branch(repo)? {
Some(branch) => Ok(branch),
Expand All @@ -55,11 +53,8 @@ pub fn get_commit_id(repo: &LocalRepository, name: &str) -> Result<Option<String
}

/// Check if a branch exists
pub fn exists(repo: &LocalRepository, name: &str) -> Result<bool, OxenError> {
match get_by_name(repo, name)? {
Some(_) => Ok(true),
None => Ok(false),
}
pub fn exists(repo: &LocalRepository, name: &str) -> bool {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I could also see this being:

match get_by_name(repo,name) {
  Ok(_) => Ok(true),
  Err(OxenError::BranchNotFound(_)) => Ok(false),
  error => error,
}

And keeping it as Result<bool, OxenError> and adding the following to the docstring:

/// Check if a branch exists. Returns Ok(false) if the branch doesn't exist. Returns Err() if there was some other problem accessing the local repository.

get_by_name(repo, name).is_ok()
}
Comment on lines +56 to 58
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check usages of branches::exists to see if callers handle errors
rg -n -C3 'branches::exists'

Repository: Oxen-AI/Oxen

Length of output: 730


🏁 Script executed:

# Find get_by_name implementation
rg -A15 'fn get_by_name' crates/lib/src/repositories/branches.rs | head -40

Repository: Oxen-AI/Oxen

Length of output: 922


🏁 Script executed:

# Also check OxenError types to understand BranchNotFound
rg -B2 -A2 'BranchNotFound' crates/lib/src/ --type rust | head -30

Repository: Oxen-AI/Oxen

Length of output: 1971


exists() silently treats lookup errors as "not found".

The implementation returns false for any error from get_by_name(), including I/O or database failures, not just BranchNotFound. While this matches the single usage pattern (as a simple boolean guard in checkout.rs:23), it does mask underlying errors.

However, the current design appears intentional: the API is a simple existence check, and callers that need the actual branch follow up with get_by_name()? for error handling. The suggested alternative of returning Result<bool, OxenError> would require updating the existing caller's usage pattern. If preserving error distinction is important, consider documenting the error-suppression behavior or slightly refactoring the checkout logic to propagate lookup errors more explicitly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/lib/src/repositories/branches.rs` around lines 56 - 58, The current
exists(repo, name) function swallows all errors from get_by_name(repo,
name).is_ok(), treating I/O/db failures the same as BranchNotFound; fix by
either (A) changing exists signature to return Result<bool, OxenError> and
propagate get_by_name(repo, name)? so callers (e.g., the checkout logic that
calls exists) can handle real errors, or (B) keep the bool signature but
explicitly match the error from get_by_name and return false only for the
BranchNotFound variant while propagating or returning an error for other cases
(i.e., call get_by_name and match Err(BranchNotFound) => false, Err(other) =>
propagate/log/return error). Update the caller in checkout to use the new
Result<bool, OxenError> or handle the propagated error accordingly.


/// Get the current branch
Expand Down Expand Up @@ -270,8 +265,7 @@ pub fn list_entry_versions_on_branch(
branch_name: &str,
path: &Path,
) -> Result<Vec<(Commit, CommitEntry)>, OxenError> {
let branch = repositories::branches::get_by_name(local_repo, branch_name)?
.ok_or_else(|| OxenError::local_branch_not_found(branch_name))?;
let branch = repositories::branches::get_by_name(local_repo, branch_name)?;
log::debug!(
"get branch commits for branch {:?} -> {}",
branch.name,
Expand Down Expand Up @@ -348,7 +342,7 @@ mod tests {

let commit_3 = repositories::commit(&repo, "adding test file 2")?;

let _branch = repositories::branches::get_by_name(&repo, DEFAULT_BRANCH_NAME)?.unwrap();
let _branch = repositories::branches::get_by_name(&repo, DEFAULT_BRANCH_NAME)?;

let file_versions =
repositories::branches::list_entry_versions_on_branch(&repo, "main", &file_path)?;
Expand Down Expand Up @@ -434,8 +428,8 @@ mod tests {
repositories::add(&repo, &repo.path).await?;
let commit_6 = repositories::commit(&repo, "adding test file 6")?;

let _main = repositories::branches::get_by_name(&repo, DEFAULT_BRANCH_NAME)?.unwrap();
let _branch = repositories::branches::get_by_name(&repo, "test_branch")?.unwrap();
let _main = repositories::branches::get_by_name(&repo, DEFAULT_BRANCH_NAME)?;
let _branch = repositories::branches::get_by_name(&repo, "test_branch")?;
let main_versions =
repositories::branches::list_entry_versions_on_branch(&repo, "main", &file_path)?;

Expand Down
6 changes: 3 additions & 3 deletions crates/lib/src/repositories/checkout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ pub async fn checkout(
) -> Result<Option<Branch>, OxenError> {
let value = value.as_ref();
log::debug!("--- CHECKOUT START {value} ----");
if repositories::branches::exists(repo, value)? {
if repositories::branches::exists(repo, value) {
if repositories::branches::is_checked_out(repo, value) {
println!("Already on branch {value}");
return repositories::branches::get_by_name(repo, value);
return Ok(Some(repositories::branches::get_by_name(repo, value)?));
}

println!("Checkout branch: {value}");
Expand All @@ -37,7 +37,7 @@ pub async fn checkout(
repositories::branches::checkout_subtrees_to_commit(repo, &commit, &subtree_paths, depth)
.await?;
repositories::branches::set_head(repo, value)?;
repositories::branches::get_by_name(repo, value)
Ok(Some(repositories::branches::get_by_name(repo, value)?))
} else {
// If we are already on the commit, do nothing
if repositories::branches::is_checked_out(repo, value) {
Expand Down
4 changes: 2 additions & 2 deletions crates/lib/src/repositories/entries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub fn list_directory_w_version(
MinOxenVersion::V0_10_0 => panic!("v0.10.0 no longer supported"),
_ => {
let revision_str = revision.as_ref().to_string();
let branch = repositories::branches::get_by_name(repo, &revision_str)?;
let branch = repositories::branches::get_by_name(repo, &revision_str).ok();
let commit = repositories::revisions::get(repo, &revision_str)?;
let parsed_resource = ParsedResource {
path: directory.as_ref().to_path_buf(),
Expand Down Expand Up @@ -140,7 +140,7 @@ pub fn list_directory_w_workspace_depth(
revision_str.clone()
};

let branch = repositories::branches::get_by_name(repo, &revision_str)?;
let branch = repositories::branches::get_by_name(repo, &revision_str).ok();
let commit = repositories::revisions::get(repo, &revision_str)?;
let parsed_resource = ParsedResource {
path: directory.as_ref().to_path_buf(),
Expand Down
3 changes: 1 addition & 2 deletions crates/lib/src/repositories/load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ pub async fn load(
let repo = LocalRepository::from_dir(&dest_path)?;

println!("🐂 Unpacking files to working directory {dest_path:?}");
let branch = repositories::branches::get_by_name(&repo, DEFAULT_BRANCH_NAME)?
.ok_or_else(|| OxenError::local_branch_not_found(DEFAULT_BRANCH_NAME))?;
let branch = repositories::branches::get_by_name(&repo, DEFAULT_BRANCH_NAME)?;
let commit = repositories::commits::get_by_id(&repo, &branch.commit_id)?
.ok_or_else(|| OxenError::commit_id_does_not_exist(&branch.commit_id))?;
repositories::branches::set_working_repo_to_commit(&repo, &commit, &None).await?;
Expand Down
9 changes: 4 additions & 5 deletions crates/lib/src/repositories/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,9 +655,8 @@ mod tests {
assert!(result.is_some());

// But now there should be conflicts when trying to merge in the human branch
let base_branch = repositories::branches::get_by_name(&repo, &og_branch.name)?.unwrap();
let merge_branch =
repositories::branches::get_by_name(&repo, human_branch_name)?.unwrap();
let base_branch = repositories::branches::get_by_name(&repo, &og_branch.name)?;
let merge_branch = repositories::branches::get_by_name(&repo, human_branch_name)?;

// Check if there are conflicts
let has_conflicts =
Expand Down Expand Up @@ -979,8 +978,8 @@ mod tests {
repositories::commit(&repo, "Adding fish to labels.txt file")?;

// 4. merge main onto new branch (re-fetch branches to get current commit IDs)
let og_branch = repositories::branches::get_by_name(&repo, &og_branch.name)?.unwrap();
let new_branch = repositories::branches::get_by_name(&repo, new_branch_name)?.unwrap();
let og_branch = repositories::branches::get_by_name(&repo, &og_branch.name)?;
let new_branch = repositories::branches::get_by_name(&repo, new_branch_name)?;
let merge_result =
repositories::merge::merge_into_base(&repo, &og_branch, &new_branch).await;

Expand Down
15 changes: 4 additions & 11 deletions crates/lib/src/repositories/revisions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,10 @@ pub fn get(repo: &LocalRepository, revision: impl AsRef<str>) -> Result<Option<C
return Ok(Some(commit));
}

if repositories::branches::exists(repo, revision)? {
log::debug!("revision is a branch: {revision}");
let branch = repositories::branches::get_by_name(repo, revision)?;
let branch = branch.ok_or_else(|| OxenError::local_branch_not_found(revision))?;
let commit = repositories::commits::get_by_id(repo, &branch.commit_id)?;
Ok(commit)
} else {
log::debug!("revision is a commit id: {revision}");
let commit = repositories::commits::get_by_id(repo, revision)?;
Ok(commit)
}
let commit_id = repositories::branches::get_commit_id(repo, revision)?
.unwrap_or_else(|| revision.to_string());
let commit = repositories::commits::get_by_id(repo, &commit_id)?;
Ok(commit)
}

/// Get the version file path from a commit id
Expand Down
3 changes: 1 addition & 2 deletions crates/oxen-py/src/py_repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,7 @@ impl PyRepo {
let branch = if delete {
repositories::branches::delete(&repo, name)
} else {
repositories::branches::get_by_name(&repo, name)?
.ok_or_else(|| OxenError::local_branch_not_found(name))
repositories::branches::get_by_name(&repo, name)
};
Ok(PyBranch::from(branch?))
}
Expand Down
34 changes: 16 additions & 18 deletions crates/server/src/controllers/branches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,7 @@ pub async fn show(req: HttpRequest) -> actix_web::Result<HttpResponse, OxenHttpE
let repository = get_repo(&app_data.path, namespace, name)?;

log::debug!("show branch {branch_name:?}");
let branch = repositories::branches::get_by_name(&repository, &branch_name)?
.ok_or_else(|| OxenError::remote_branch_not_found(&branch_name))?;
let branch = repositories::branches::get_by_name(&repository, &branch_name)?;
log::debug!("show branch found {branch:?}");

let view = BranchResponse {
Expand Down Expand Up @@ -170,18 +169,20 @@ fn create_from_branch(
repo: &LocalRepository,
data: &BranchNewFromBranchName,
) -> Result<HttpResponse, OxenHttpError> {
let maybe_new_branch: Option<liboxen::model::Branch> =
repositories::branches::get_by_name(repo, &data.new_name)?;
if let Some(branch) = maybe_new_branch {
let view = BranchResponse {
status: StatusMessage::resource_found(),
branch,
};
return Ok(HttpResponse::Ok().json(view));
match repositories::branches::get_by_name(repo, &data.new_name) {
Ok(branch) => {
let view = BranchResponse {
status: StatusMessage::resource_found(),
branch,
};
return Ok(HttpResponse::Ok().json(view));
}
Err(OxenError::BranchNotFound(_)) => {} // branch doesn't exist yet, continue
Err(e) => return Err(e.into()),
}

let from_branch = repositories::branches::get_by_name(repo, &data.from_name)?
.ok_or(OxenHttpError::NotFound)?;
let from_branch = repositories::branches::get_by_name(repo, &data.from_name)
.map_err(|_| OxenHttpError::NotFound)?;

let new_branch = repositories::branches::create(repo, &data.new_name, from_branch.commit_id)?;

Expand Down Expand Up @@ -226,8 +227,7 @@ pub async fn delete(req: HttpRequest) -> actix_web::Result<HttpResponse, OxenHtt
let branch_name = path_param(&req, "branch_name")?;
let repository = get_repo(&app_data.path, namespace, name)?;

let branch = repositories::branches::get_by_name(&repository, &branch_name)?
.ok_or_else(|| OxenError::remote_branch_not_found(&branch_name))?;
let branch = repositories::branches::get_by_name(&repository, &branch_name)?;

repositories::branches::force_delete(&repository, &branch.name)?;
Ok(HttpResponse::Ok().json(BranchResponse {
Expand Down Expand Up @@ -315,8 +315,7 @@ pub async fn maybe_create_merge(
let name = path_param(&req, "repo_name")?;
let repository = get_repo(&app_data.path, namespace, name)?;
let branch_name = path_param(&req, "branch_name")?;
let branch = repositories::branches::get_by_name(&repository, &branch_name)?
.ok_or_else(|| OxenError::remote_branch_not_found(&branch_name))?;
let branch = repositories::branches::get_by_name(&repository, &branch_name)?;

let data: Result<BranchRemoteMerge, serde_json::Error> = serde_json::from_str(&body);
let data = data.map_err(|err| OxenHttpError::BadRequest(format!("{err:?}").into()))?;
Expand Down Expand Up @@ -386,8 +385,7 @@ pub async fn list_entry_versions(

// Get branch
let repo = get_repo(&app_data.path, namespace.clone(), &repo_name)?;
let branch = repositories::branches::get_by_name(&repo, &branch_name)?
.ok_or_else(|| OxenError::remote_branch_not_found(&branch_name))?;
let branch = repositories::branches::get_by_name(&repo, &branch_name)?;

let path = PathBuf::from(path_param(&req, "path")?);
let repo = get_repo(&app_data.path, namespace, &repo_name)?;
Expand Down
18 changes: 12 additions & 6 deletions crates/server/src/controllers/workspaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ pub async fn get_or_create(
};

// Try to get the branch, or create it if the repo is empty
let branch = match repositories::branches::get_by_name(&repo, &data.branch_name)? {
Some(branch) => branch,
None => {
let branch = match repositories::branches::get_by_name(&repo, &data.branch_name) {
Ok(branch) => branch,
Err(OxenError::BranchNotFound(_)) => {
// Branch doesn't exist - check if repo is empty
if repositories::commits::head_commit_maybe(&repo)?.is_some() {
// Repo has commits but branch doesn't exist - this is an error
Expand Down Expand Up @@ -95,8 +95,8 @@ pub async fn get_or_create(

// Now get the newly created branch
repositories::branches::get_by_name(&repo, &data.branch_name)?
.ok_or_else(|| OxenError::basic_str("Failed to create initial branch"))?
}
Err(e) => return Err(e.into()),
};

// Return workspace if it already exists
Expand Down Expand Up @@ -401,8 +401,14 @@ pub async fn commit(req: HttpRequest, body: String) -> Result<HttpResponse, Oxen
.json(StatusMessageDescription::workspace_not_found(workspace_id)));
};

let Some(branch) = repositories::branches::get_by_name(&repo, &branch_name)? else {
return Ok(HttpResponse::NotFound().json(StatusMessageDescription::not_found(branch_name)));
let branch = match repositories::branches::get_by_name(&repo, &branch_name) {
Ok(branch) => branch,
Err(OxenError::BranchNotFound(_)) => {
return Ok(
HttpResponse::NotFound().json(StatusMessageDescription::not_found(branch_name))
);
}
Err(e) => return Err(e.into()),
};

match repositories::workspaces::commit(&workspace, &data, &branch_name).await {
Expand Down
3 changes: 1 addition & 2 deletions crates/server/src/controllers/workspaces/data_frames.rs
Original file line number Diff line number Diff line change
Expand Up @@ -488,8 +488,7 @@ pub async fn get_by_branch(
let page_size = query.page_size.unwrap_or(constants::DEFAULT_PAGE_SIZE);

// Staged dataframes must be on a branch.
let branch = repositories::branches::get_by_name(&repo, branch_name)?
.ok_or_else(|| OxenError::remote_branch_not_found(branch_name))?;
let branch = repositories::branches::get_by_name(&repo, branch_name)?;

let commit = repositories::commits::get_by_id(&repo, &branch.commit_id)?
.ok_or_else(|| OxenError::resource_not_found(&branch.commit_id))?;
Expand Down
7 changes: 5 additions & 2 deletions crates/server/src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,11 @@ pub fn resolve_revision(
}

pub fn resolve_branch(repo: &LocalRepository, name: &str) -> Result<Option<Branch>, OxenError> {
// Lookup branch name
repositories::branches::get_by_name(repo, name)
match repositories::branches::get_by_name(repo, name) {
Ok(branch) => Ok(Some(branch)),
Err(OxenError::BranchNotFound(_)) => Ok(None),
Err(e) => Err(e),
}
}

fn user_cli_is_out_of_date(user_agent: &str) -> bool {
Expand Down
Loading